CORDA-553 - Review Comments

core/src/main/kotlin/net/corda/core/serialization/CordaSerializationTransformEnumDefault.kt

@@ -1,14 +1,89 @@
 package net.corda.core.serialization
 
 /**
+ * This annotation is used to mark an enumerated type as having had multiple members added, It acts
+ * as a container annotation for instances of [CordaSerializationTransformEnumDefault], each of which
+ * details individual additions.
  *
+ * @property value an array of [CordaSerializationTransformEnumDefault].
+ *
+ * NOTE: Order is important, new values should always be added before any others
+ *
+ * ```
+ *  // initial implementation
+ *  enum class ExampleEnum {
+ *    A, B, C
+ *  }
+ *
+ *  // First alteration
+ *  @CordaSerializationTransformEnumDefaults(
+ *      CordaSerializationTransformEnumDefault("D", "C"))
+ *  enum class ExampleEnum {
+ *    A, B, C, D
+ *  }
+ *
+ *  // Second alteration, new transform is placed at the head of the list
+ *  @CordaSerializationTransformEnumDefaults(
+ *      CordaSerializationTransformEnumDefault("E", "C"),
+ *      CordaSerializationTransformEnumDefault("D", "C"))
+ *  enum class ExampleEnum {
+ *    A, B, C, D, E
+ *  }
+ * ```
+ *
+ * IMPORTANT - Once added (and in production) do NOT remove old annotations. See documentation for
+ * more discussion on this point!.
  */
 @Target(AnnotationTarget.CLASS)
 @Retention(AnnotationRetention.RUNTIME)
 annotation class CordaSerializationTransformEnumDefaults(vararg val value: CordaSerializationTransformEnumDefault)
 
 /**
+ * This annotation is used to mark an enumerated type as having had a new constant appended to it. For
+ * each additional constant added a new annotation should be appended to the class. If more than one
+ * is required the wrapper annotation [CordaSerializationTransformEnumDefaults] should be used to
+ * encapsulate them
  *
+ * @property new [String] equivalent of the value of the new constant
+ * @property old [String] equivalent of the value of the existing constant that deserialisers should
+ * favour when de-serialising a value they have no corresponding value for
+ *
+ * For Example
+ *
+ * Enum before modification:
+ * ```
+ *  enum class ExampleEnum {
+ *    A, B, C
+ *  }
+ * ```
+ *
+ * Assuming at some point a new constant is added it is required we have some mechanism by which to tell
+ * nodes with an older version of the class on their Class Path what to do if they attempt to deserialize
+ * an example of the class with that new value
+ *
+ * ```
+ *  @CordaSerializationTransformEnumDefault("D", "C")
+ *  enum class ExampleEnum {
+ *    A, B, C, D
+ *  }
+ * ```
+ *
+ * So, on deserialisation treat any instance of the enum that is encoded as D as C
+ *
+ * Adding a second new constant requires the wrapper annotation [CordaSerializationTransformEnumDefaults]
+ *
+ * ```
+ *  @CordaSerializationTransformEnumDefaults(
+ *      CordaSerializationTransformEnumDefault("E", "D"),
+ *      CordaSerializationTransformEnumDefault("D", "C"))
+ *  enum class ExampleEnum {
+ *    A, B, C, D, E
+ *  }
+ * ```
+ *
+ * It's fine to assign the second new value a default that may not be present in all versions as in this
+ * case it will work down the transform hierarchy until it finds a value it can apply, in this case it would
+ * try E -> D -> C (when E -> D fails)
  */
 @Target(AnnotationTarget.CLASS)
 @Retention(AnnotationRetention.RUNTIME)

core/src/main/kotlin/net/corda/core/serialization/CordaSerializationTransformRename.kt

@@ -1,13 +1,31 @@
 package net.corda.core.serialization
 
 /**
+ * This annotation is used to mark a class as having had multiple elements renamed as a container annotation for
+ * instances of [CordaSerializationTransformRename], each of which details an individual rename.
  *
+ * @property value an array of [CordaSerializationTransformRename]
+ *
+ * NOTE: Order is important, new values should always be added before existing
+ *
+ * IMPORTANT - Once added (and in production) do NOT remove old annotations. See documentation for
+ * more discussion on this point!.
  */
 @Target(AnnotationTarget.CLASS)
 @Retention(AnnotationRetention.RUNTIME)
 annotation class CordaSerializationTransformRenames(vararg val value: CordaSerializationTransformRename)
 
+// TODO When we have class renaming update the docs
 /**
+ * This annotation is used to mark a class has having had a property element. It is used by the
+ * AMQP deserialiser to allow instances with different versions of the class on their Class Path
+ * to successfully deserialize the object
+ *
+ * NOTE: Renaming of the class itself is not be done with this annotation. For class renaming
+ * see ???
+ *
+ * @property to [String] representation of the properties new name
+ * @property from [String] representation of the properties old new
  *
  */
 @Target(AnnotationTarget.CLASS)

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPDescriptorRegistry.kt

@@ -10,10 +10,17 @@ import org.apache.qpid.proton.amqp.UnsignedLong
  * Repeated here for brevity:
  *   50530 - R3 - Mike Hearn - mike&r3.com
  */
-const val DESCRIPTOR_TOP_32BITS: Long = 0xc5620000
+const val DESCRIPTOR_TOP_32BITS: Long = 0xc562 shl(32 + 16)
 
+/**
+ * AMQP desriptor ID's for our custom types.
+ *
+ * NEVER DELETE OR CHANGE THE ID ASSOCIATED WITH A TYPE
+ *
+ * these are encoded as part of a serialised blob and doing so would render us unable to
+ * de-serialise that blob!!!
+ */
 enum class AMQPDescriptorRegistry(val id: Long) {
-
     ENVELOPE(1),
     SCHEMA(2),
     OBJECT_DESCRIPTOR(3),

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Envelope.kt

@@ -18,6 +18,14 @@ data class Envelope(val obj: Any?, val schema: Schema, val transformsSchema: Tra
         val DESCRIPTOR = AMQPDescriptorRegistry.ENVELOPE.amqpDescriptor
         val DESCRIPTOR_OBJECT = Descriptor(null, DESCRIPTOR)
 
+        // described list should either be two or three elements long
+        private const val ENVELOPE_WITHOUT_TRANSFORMS = 2
+        private const val ENVELOPE_WITH_TRANSFORMS = 3
+
+        private const val BLOB_IDX = 0
+        private const val SCHEMA_IDX = 1
+        private const val TRANSFORMS_SCHEMA_IDX = 2
+
         fun get(data: Data): Envelope {
             val describedType = data.`object` as DescribedType
             if (describedType.descriptor != DESCRIPTOR) {
@@ -27,28 +35,29 @@ data class Envelope(val obj: Any?, val schema: Schema, val transformsSchema: Tra
 
             // We need to cope with objects serialised without the transforms header element in the
             // envelope
-            val transformSchema : Any? = when (list.size) {
+            val transformSchema: Any? = when (list.size) {
-                2 -> null
+                ENVELOPE_WITHOUT_TRANSFORMS -> null
-                3 -> list[2]
+                ENVELOPE_WITH_TRANSFORMS -> list[TRANSFORMS_SCHEMA_IDX]
                 else -> throw NotSerializableException("Malformed list, bad length of ${list.size} (should be 2 or 3)")
             }
 
-            return newInstance(listOf(list[0], Schema.get(list[1]!!), TransformsSchema.newInstance(transformSchema)))
+            return newInstance(listOf(list[BLOB_IDX], Schema.get(list[SCHEMA_IDX]!!),
+                    TransformsSchema.newInstance(transformSchema)))
         }
 
-        // This seperation of functions is needed as this will be the entry point for the default
+        // This separation of functions is needed as this will be the entry point for the default
-        // AMQP decoder if one is used (see the unit tests)
+        // AMQP decoder if one is used (see the unit tests).
         override fun newInstance(described: Any?): Envelope {
             val list = described as? List<*> ?: throw IllegalStateException("Was expecting a list")
 
             // We need to cope with objects serialised without the transforms header element in the
             // envelope
             val transformSchema = when (list.size) {
-                2 -> TransformsSchema.newInstance(null)
+                ENVELOPE_WITHOUT_TRANSFORMS -> TransformsSchema.newInstance(null)
-                3 -> list[2] as TransformsSchema
+                ENVELOPE_WITH_TRANSFORMS -> list[TRANSFORMS_SCHEMA_IDX] as TransformsSchema
                 else -> throw NotSerializableException("Malformed list, bad length of ${list.size} (should be 2 or 3)")
             }
-            return Envelope(list[0], list[1] as Schema, transformSchema)
+            return Envelope(list[BLOB_IDX], list[SCHEMA_IDX] as Schema, transformSchema)
         }
 
         override fun getTypeClass(): Class<*> = Envelope::class.java

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt

@@ -17,11 +17,11 @@ import java.util.*
 import net.corda.nodeapi.internal.serialization.carpenter.Field as CarpenterField
 import net.corda.nodeapi.internal.serialization.carpenter.Schema as CarpenterSchema
 
+const val DESCRIPTOR_DOMAIN: String = "net.corda"
 
 // "corda" + majorVersionByte + minorVersionMSB + minorVersionLSB
 val AmqpHeaderV1_0: OpaqueBytes = OpaqueBytes("corda\u0001\u0000\u0000".toByteArray())
 
-
 /**
  * This and the classes below are OO representations of the AMQP XML schema described in the specification. Their
  * [toString] representations generate the associated XML form.

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt

@@ -34,6 +34,8 @@ open class SerializerFactory(val whitelist: ClassWhitelist, cl: ClassLoader) {
     private val serializersByType = ConcurrentHashMap<Type, AMQPSerializer<Any>>()
     private val serializersByDescriptor = ConcurrentHashMap<Any, AMQPSerializer<Any>>()
     private val customSerializers = CopyOnWriteArrayList<CustomSerializer<out Any>>()
+    val transformsCache = ConcurrentHashMap<String, EnumMap<TransformTypes, MutableList<Transform>>>()
+
     open val classCarpenter = ClassCarpenter(cl, whitelist)
     val classloader: ClassLoader
         get() = classCarpenter.classloader

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SupportedTransforms.kt

@@ -1,19 +1,19 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
-import net.corda.core.serialization.CordaSerializationTransformEnumDefaults
 import net.corda.core.serialization.CordaSerializationTransformEnumDefault
-import net.corda.core.serialization.CordaSerializationTransformRenames
+import net.corda.core.serialization.CordaSerializationTransformEnumDefaults
 import net.corda.core.serialization.CordaSerializationTransformRename
+import net.corda.core.serialization.CordaSerializationTransformRenames
 
 /**
- * Utility class that defines an instance of a transform we support
+ * Utility class that defines an instance of a transform we support.
  *
- * @property type The transform annotation
+ * @property type The transform annotation.
  * @property enum Maps the annotaiton onto a transform type, we expect there are multiple annotations that
- * would map to a single transform type
+ * would map to a single transform type.
- * @property f Anonymous function that should return a list of Annotations encapsualted by the parent annotation
+ * @property getAnnotations Anonymous function that should return a list of Annotations encapsualted by the parent annotation
  * that reference the transform. Notionally this allows the code that extracts transforms to work on single instances
- * of a transform or a meta list of them
+ * of a transform or a meta list of them.
  */
 data class SupportedTransform(
         val type: Class<out Annotation>,
@@ -37,7 +37,7 @@ private val wrapperExtract = { x: Annotation ->
 private val singleExtract = { x: Annotation -> listOf(x) }
 
 /**
- * Utility list of all transforms we support that simplifies our generator
+ * Utility list of all transforms we support that simplifies our generation code
  *
  * NOTE: We have to support single instances of the transform annotations as well as the wrapping annotation
  * when many instances are repeated

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TansformTypes.kt

@@ -1,10 +1,9 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
-import org.apache.qpid.proton.amqp.DescribedType
-
 import net.corda.core.serialization.CordaSerializationTransformEnumDefault
 import net.corda.core.serialization.CordaSerializationTransformEnumDefaults
 import net.corda.core.serialization.CordaSerializationTransformRename
+import org.apache.qpid.proton.amqp.DescribedType
 import org.apache.qpid.proton.codec.DescribedTypeConstructor
 import java.io.NotSerializableException
 

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformsSchema.kt

@@ -1,11 +1,11 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
-import java.util.*
 import net.corda.core.serialization.CordaSerializationTransformEnumDefault
 import net.corda.core.serialization.CordaSerializationTransformRename
 import org.apache.qpid.proton.amqp.DescribedType
 import org.apache.qpid.proton.codec.DescribedTypeConstructor
 import java.io.NotSerializableException
+import java.util.*
 
 // NOTE: We are effectively going to replicate the annotations, we need to do this because
 // we can't instantiate instances of those annotation classes and this code needs to
@@ -14,7 +14,7 @@ import java.io.NotSerializableException
  * Base class for representations of specific types of transforms as applied to a type within the
  * Corda serialisation framework
  */
-sealed class Transform : DescribedType {
+abstract class Transform : DescribedType {
     companion object : DescribedTypeConstructor<Transform> {
         val DESCRIPTOR = AMQPDescriptorRegistry.TRANSFORM_ELEMENT.amqpDescriptor
 
@@ -92,10 +92,10 @@ class EnumDefaultSchemeTransform(val old: String, val new: String) : Transform()
     override fun getDescribed(): Any = listOf(name, old, new)
     override fun params() = "old=${old.esc()} new=${new.esc()}"
 
-    override fun equals(other: Any?): Boolean {
+    override fun equals(other: Any?) = (
-        val o = other as? EnumDefaultSchemeTransform ?: return super.equals(other)
+            (other is EnumDefaultSchemeTransform && other.new == new && other.old == old) || super.equals(other))
-        return o.new == new && o.old == old
+
-    }
+    override fun hashCode() = (17 * new.hashCode()) + old.hashCode()
 
     override val name: String get() = typeName
 }
@@ -130,17 +130,21 @@ class RenameSchemaTransform(val from: String, val to: String) : Transform() {
 
     override fun params() = "from=${from.esc()} to=${to.esc()}"
 
-    override fun equals(other: Any?): Boolean {
+    override fun equals(other: Any?) = (
-        val o = other as? RenameSchemaTransform ?: return super.equals(other)
+            (other is RenameSchemaTransform && other.from == from && other.to == to) || super.equals(other))
-        return o.from == from && o.to == to
+
-    }
+    override fun hashCode() = (11 * from.hashCode()) + to.hashCode()
 
     override val name: String get() = typeName
 }
 
 
 /**
- * @property types is a list of serialised types that have transforms, each list element is a
+ * Represents the set of all transforms that can be a applied to all classes represented as part of
+ * an AMQP schema. It forms a part of the AMQP envelope alongside the [Schema] and the serialized bytes
+ *
+ * @property types maps class names to a map of transformation types. In turn those transformation types
+ * are each a list of instances o that transform.
  */
 data class TransformsSchema(val types: Map<String, EnumMap<TransformTypes, MutableList<Transform>>>) : DescribedType {
     companion object : DescribedTypeConstructor<TransformsSchema> {
@@ -158,31 +162,38 @@ data class TransformsSchema(val types: Map<String, EnumMap<TransformTypes, Mutab
             val rtn = mutableMapOf<String, EnumMap<TransformTypes, MutableList<Transform>>>()
 
             schema.types.forEach { type ->
-                val clazz = try {
+                sf.transformsCache.computeIfAbsent(type.name) {
-                    sf.classloader.loadClass(type.name)
+                    val transforms = EnumMap<TransformTypes, MutableList<Transform>>(TransformTypes::class.java)
-                } catch (e: ClassNotFoundException) {
+                    try {
-                    return@forEach
+                        val clazz = sf.classloader.loadClass(type.name)
-                }
 
-                supportedTransforms.forEach { transform ->
+                        supportedTransforms.forEach { transform ->
-                    clazz.getAnnotation(transform.type)?.let { list ->
+                            clazz.getAnnotation(transform.type)?.let { list ->
-                        transform.getAnnotations(list).forEach {
+                                transform.getAnnotations(list).forEach { annotation ->
-                            val t = transform.enum.build(it)
+                                    val t = transform.enum.build(annotation)
 
-                            val m = rtn.computeIfAbsent(type.name) {
+                                    // we're explicitly rejecting repeated annotations, whilst it's fine and we'd just
-                                EnumMap<TransformTypes, MutableList<Transform>>(TransformTypes::class.java)
+                                    // ignore them it feels like a good thing to alert the user to since this is
+                                    // more than likely a typo in their code so best make it an actual error
+                                    if (transforms.computeIfAbsent(transform.enum) { mutableListOf() }
+                                            .filter { t == it }.isNotEmpty()) {
+                                        throw NotSerializableException(
+                                                "Repeated unique transformation annotation of type ${t.name}")
+                                    }
+
+                                    transforms[transform.enum]!!.add(t)
+                                }
                             }
-
-                            // we're explicitly rejecting repeated annotations, whilst it's fine and we'd just
-                            // ignore them it feels like a good thing to alert the user to since this is
-                            // more than likely a typo in their code so best make it an actual error
-                            if (m.computeIfAbsent(transform.enum) { mutableListOf() }.filter { it == t }.isNotEmpty()) {
-                                throw NotSerializableException(
-                                        "Repeated unique transformation annotation of type ${t.name}")
-                            }
-
-                            m[transform.enum]!!.add(t)
                         }
+                    } catch (_: ClassNotFoundException) {
+                        // if we can't load the class we'll end up caching an empty list which is fine as that
+                        // list, on lookup, won't be included in the schema because it's empty
+                    }
+
+                    transforms
+                }.apply {
+                    if (isNotEmpty()) {
+                        rtn[type.name] = this
                     }
                 }
             }
@@ -198,7 +209,6 @@ data class TransformsSchema(val types: Map<String, EnumMap<TransformTypes, Mutab
          */
         override fun newInstance(described: Any?): TransformsSchema {
             val rtn = mutableMapOf<String, EnumMap<TransformTypes, MutableList<Transform>>>()
-
             val describedType = described as? DescribedType ?: return TransformsSchema(rtn)
 
             if (describedType.descriptor != DESCRIPTOR) {
@@ -208,7 +218,6 @@ data class TransformsSchema(val types: Map<String, EnumMap<TransformTypes, Mutab
             val map = describedType.described as? Map<*, *> ?:
                     throw NotSerializableException("Transform schema must be encoded as a map")
 
-
             map.forEach { type ->
                 val fingerprint = type.key as? String ?:
                         throw NotSerializableException("Fingerprint must be encoded as a string")
@@ -238,6 +247,7 @@ data class TransformsSchema(val types: Map<String, EnumMap<TransformTypes, Mutab
     override fun toString(): String {
         data class Indent(val indent: String) {
             @Suppress("UNUSED") constructor(i: Indent) : this("  ${i.indent}")
+
             override fun toString() = indent
         }
 

node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaSerializationOutputTests.java

@@ -186,6 +186,8 @@ public class JavaSerializationOutputTests {
         decoder.register(CompositeType.Companion.getDESCRIPTOR(), CompositeType.Companion);
         decoder.register(Choice.Companion.getDESCRIPTOR(), Choice.Companion);
         decoder.register(RestrictedType.Companion.getDESCRIPTOR(), RestrictedType.Companion);
+        decoder.register(Transform.Companion.getDESCRIPTOR(), Transform.Companion);
+        decoder.register(TransformsSchema.Companion.getDESCRIPTOR(), TransformsSchema.Companion);
 
         new EncoderImpl(decoder);
         decoder.setByteBuffer(ByteBuffer.wrap(bytes.getBytes(), 8, bytes.getSize() - 8));

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolvabilityTests.kt

@@ -377,4 +377,31 @@ class EnumEvolvabilityTests {
         assertEquals(2, e2S[TransformTypes.EnumDefault]!!.size)
         assertEquals(1, e3S[TransformTypes.EnumDefault]!!.size)
     }
+
+    @Test
+    fun testCache() {
+        data class C2(val annotatedEnum: AnnotatedEnumOnce)
+        data class C1(val annotatedEnum: AnnotatedEnumOnce)
+
+        val sf = testDefaultFactory()
+
+        assertEquals(0, sf.transformsCache.size)
+
+        val sb1 = TestSerializationOutput(VERBOSE, sf).serializeAndReturnSchema(C1(AnnotatedEnumOnce.D))
+
+        assertEquals(2, sf.transformsCache.size)
+        assertTrue(sf.transformsCache.containsKey(C1::class.java.name))
+        assertTrue(sf.transformsCache.containsKey(AnnotatedEnumOnce::class.java.name))
+
+        val sb2 = TestSerializationOutput(VERBOSE, sf).serializeAndReturnSchema(C2(AnnotatedEnumOnce.D))
+
+        assertEquals(3, sf.transformsCache.size)
+        assertTrue(sf.transformsCache.containsKey(C1::class.java.name))
+        assertTrue(sf.transformsCache.containsKey(C2::class.java.name))
+        assertTrue(sf.transformsCache.containsKey(AnnotatedEnumOnce::class.java.name))
+
+        assertEquals (sb1.transformsSchema.types[AnnotatedEnumOnce::class.java.name],
+                sb2.transformsSchema.types[AnnotatedEnumOnce::class.java.name])
+
+    }
 }