Review Comments

Refactor small bits of the serialiser, move things out of the read
methods and into the make factory companion object

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

@@ -6,6 +6,6 @@ package net.corda.core.serialization
  */
 @Target(AnnotationTarget.CONSTRUCTOR)
 @Retention(AnnotationRetention.RUNTIME)
-annotation class CordaSerializerConstructor(val version: Int)
+annotation class EvolvedSerializerConstructor(val version: Int)
 
 

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

@@ -1,6 +1,6 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
-import net.corda.core.serialization.CordaSerializerConstructor
+import net.corda.core.serialization.EvolvedSerializerConstructor
 import net.corda.nodeapi.internal.serialization.carpenter.getTypeAsClass
 import org.apache.qpid.proton.codec.Data
 import java.lang.reflect.Type
@@ -16,7 +16,8 @@ import kotlin.reflect.jvm.javaType
 class EvolutionSerializer(
         clazz: Type,
         factory: SerializerFactory,
-        val oldParams : Map<String, oldParam>,
+        val oldParams : Map<String, OldParam>,
+        val newParams : Map<String, Boolean>,
         override val kotlinConstructor: KFunction<Any>?) : ObjectSerializer (clazz, factory) {
 
     // explicitly null this out as we won't be using this list
@@ -31,7 +32,10 @@ class EvolutionSerializer(
      * order may have been changed and we need to know where into the list to look
      * @param property object to read the actual property value
      */
-    data class oldParam (val type: Type, val idx: Int, val property: PropertySerializer)
+    data class OldParam (val type: Type, val idx: Int, val property: PropertySerializer) {
+        fun readProperty(paramValues: List<*>, schema: Schema, input: DeserializationInput) =
+                property.readProperty(paramValues[idx], schema, input)
+    }
 
     companion object {
         /**
@@ -53,7 +57,7 @@ class EvolutionSerializer(
             var maxConstructorVersion = Integer.MIN_VALUE
             var constructor: KFunction<Any>? = null
             clazz.kotlin.constructors.forEach {
-                val version = it.findAnnotation<CordaSerializerConstructor>()?.version ?: Integer.MIN_VALUE
+                val version = it.findAnnotation<EvolvedSerializerConstructor>()?.version ?: Integer.MIN_VALUE
                 if (oldArgumentSet.containsAll(it.parameters.map { v -> Pair(v.name, v.type.javaType) }) &&
                         version > maxConstructorVersion) {
                     constructor = it
@@ -67,7 +71,7 @@ class EvolutionSerializer(
         }
 
         /**
-         * Build a serialization object for deserialisation only of objects serislaised
+         * Build a serialization object for deserialisation only of objects serialised
          * as different versions of a class
          *
          * @param old is an object holding the schema that represents the object
@@ -86,15 +90,17 @@ class EvolutionSerializer(
                     throw NotSerializableException(
                             "Attempt to deserialize an interface: new.type. Serialized form is invalid.")
 
-            val oldArgs = mutableMapOf<String, oldParam>()
+            val oldArgs = mutableMapOf<String, OldParam>()
             var idx = 0
             old.fields.forEach {
                 val returnType = it.getTypeAsClass(factory.classloader)
-                oldArgs[it.name] = oldParam(
+                oldArgs[it.name] = OldParam(
                         returnType, idx++, PropertySerializer.make(it.name, null, returnType, factory))
             }
 
-            return EvolutionSerializer(new.type, factory, oldArgs, constructor)
+            val newArgs = constructor.parameters.associateBy({ it.name!! }, {it.type.isMarkedNullable})
+
+            return EvolutionSerializer(new.type, factory, oldArgs, newArgs, constructor)
         }
     }
 
@@ -112,17 +118,14 @@ class EvolutionSerializer(
     override fun readObject(obj: Any, schema: Schema, input: DeserializationInput): Any {
         if (obj !is List<*>) throw NotSerializableException("Body of described type is unexpected $obj")
 
-        val newArgs = kotlinConstructor?.parameters?.associateBy({ it.name!! }, {it.type.isMarkedNullable}) ?:
+        return construct(newParams.map {
-            throw NotSerializableException ("Bad Constructor selected for object $obj")
-
-        return construct(newArgs.map {
             val param = oldParams[it.key]
             if (param == null && !it.value) {
                 throw NotSerializableException(
                         "New parameter ${it.key} is mandatory, should be nullable for evolution to worK")
             }
 
-            param?.property?.readProperty(obj[param.idx], schema, input)
+            param?.readProperty(obj, schema, input)
         })
     }
 }

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

@@ -15,6 +15,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS
     override val type: Type get() = clazz
     open internal val propertySerializers: Collection<PropertySerializer>
     open val kotlinConstructor = constructorForDeserialization(clazz)
+    val javaConstructor by lazy { kotlinConstructor?.javaConstructor }
 
     init {
         propertySerializers = propertiesForSerialization(kotlinConstructor, clazz, factory)
@@ -71,9 +72,10 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS
 
 
     fun construct(properties: List<Any?>): Any {
-        val javaConstructor = kotlinConstructor?.javaConstructor ?:
+        if (javaConstructor == null) {
             throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.")
+        }
 
-        return javaConstructor.newInstance(*properties.toTypedArray())
+        return javaConstructor!!.newInstance(*properties.toTypedArray())
     }
 }

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

@@ -191,8 +191,8 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
                 val serialiser = processSchemaEntry(typeNotation)
 
                 // if we just successfully built a serialiser for the type but the type fingerprint
-                // doesn't match that of the serialised object then we are dealing with an older
+                // doesn't match that of the serialised object then we are dealing with  different
-                // instance of the class, as such we need to build an evolverSerilaiser
+                // instance of the class, as such we need to build an EvolutionSerialiser
                 if (serialiser.typeDescriptor != typeNotation.descriptor.name) {
                     getEvolutionSerializer(typeNotation, serialiser as ObjectSerializer)
                 }

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

@@ -1,13 +1,20 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
 import net.corda.core.serialization.SerializedBytes
-import net.corda.core.serialization.CordaSerializerConstructor
+import net.corda.core.serialization.EvolvedSerializerConstructor
 
 import org.junit.Test
 import java.io.File
 import java.io.NotSerializableException
 import kotlin.test.assertEquals
 
+// To regenerate any of the binary test files do the following
+//
+//  1. Uncomment the code where the original form of the class is defined in the test
+//  2. Comment out the rest of the test
+//  3. Run the test
+//  4. Using the printed path copy that file to the resources directory
+//  5. Comment back out the generation code and uncomment the actual test
 class EvolvabilityTests {
 
     @Test
@@ -104,7 +111,7 @@ class EvolvabilityTests {
 
         // Expected to throw as we can't construct the new type as it contains a newly
         // added parameter that isn't optional, i.e. not nullable and there isn't
-        // a compiler that takes the old parameters
+        // a constructor that takes the old parameters
         DeserializationInput(sf).deserialize(SerializedBytes<C>(sc2))
     }
 
@@ -180,7 +187,7 @@ class EvolvabilityTests {
 
         @Suppress("UNUSED")
         data class CC (val a: Int, val b: String) {
-            @CordaSerializerConstructor(1)
+            @EvolvedSerializerConstructor(1)
             constructor (a: Int) : this (a, "hello")
         }
 
@@ -239,7 +246,7 @@ class EvolvabilityTests {
         data class CC (val a: Int, val b: Int, val c: String, val d: String) {
             // ensure none of the original parameters align with the initial
             // construction order
-            @CordaSerializerConstructor(1)
+            @EvolvedSerializerConstructor(1)
             constructor (c: String, a: Int, b: Int) : this (a, b, c, "wibble")
         }
 
@@ -275,7 +282,7 @@ class EvolvabilityTests {
             // ensure none of the original parameters align with the initial
             // construction order
             @Suppress("UNUSED")
-            @CordaSerializerConstructor(1)
+            @EvolvedSerializerConstructor(1)
             constructor (c: String, a: Int) : this (a, c, "wibble")
         }
 
@@ -317,11 +324,11 @@ class EvolvabilityTests {
 
         @Suppress("UNUSED")
         data class C (val e: Int, val c: Int, val b: Int, val a: Int, val d: Int) {
-            @CordaSerializerConstructor(1)
+            @EvolvedSerializerConstructor(1)
             constructor (b: Int, a: Int) : this (-1, -1, b, a, -1)
-            @CordaSerializerConstructor(2)
+            @EvolvedSerializerConstructor(2)
             constructor (a: Int, c: Int, b: Int) : this (-1, c, b, a, -1)
-            @CordaSerializerConstructor(3)
+            @EvolvedSerializerConstructor(3)
             constructor (a: Int, b: Int, c: Int, d: Int) : this (-1, c, b, a, d)
         }
 
@@ -411,13 +418,13 @@ class EvolvabilityTests {
 
         @Suppress("UNUSED")
         data class C (val b: Int, val c: Int, val d: Int, val e: Int, val f: Int, val g: Int) {
-            @CordaSerializerConstructor(1)
+            @EvolvedSerializerConstructor(1)
             constructor (b: Int, c: Int) : this (b, c, -1, -1, -1, -1)
-            @CordaSerializerConstructor(2)
+            @EvolvedSerializerConstructor(2)
             constructor (b: Int, c: Int, d: Int) : this (b, c, d, -1, -1, -1)
-            @CordaSerializerConstructor(3)
+            @EvolvedSerializerConstructor(3)
             constructor (b: Int, c: Int, d: Int, e: Int) : this (b, c, d, e, -1, -1)
-            @CordaSerializerConstructor(4)
+            @EvolvedSerializerConstructor(4)
             constructor (b: Int, c: Int, d: Int, e: Int, f: Int) : this (b, c, d, e, f, -1)
         }