CORDA-904 - Fix evolver to work with setter instantiated classes

Backport from master

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

@@ -21,12 +21,11 @@ import kotlin.reflect.jvm.javaType
  * @property constructorArgs used to hold the properties as sent to the object's constructor. Passed in as a
  * pre populated array as properties not present on the old constructor must be initialised in the factory
  */
-class EvolutionSerializer(
+abstract class EvolutionSerializer(
         clazz: Type,
         factory: SerializerFactory,
-        private val oldReaders: Map<String, OldParam>,
+        protected val oldReaders: Map<String, OldParam>,
-        override val kotlinConstructor: KFunction<Any>?,
+        override val kotlinConstructor: KFunction<Any>?) : ObjectSerializer(clazz, factory) {
-        private val constructorArgs: Array<Any?>) : ObjectSerializer(clazz, factory) {
 
     // explicitly set as empty to indicate it's unused by this type of serializer
     override val propertySerializers = PropertySerializersEvolution()
@@ -35,12 +34,11 @@ class EvolutionSerializer(
      * Represents a parameter as would be passed to the constructor of the class as it was
      * when it was serialised and NOT how that class appears now
      *
-     * @param type The jvm type of the parameter
      * @param resultsIndex index into the constructor argument list where the read property
      * should be placed
      * @param property object to read the actual property value
      */
-    data class OldParam(val type: Type, var resultsIndex: Int, val property: PropertySerializer) {
+    data class OldParam(var resultsIndex: Int, val property: PropertySerializer) {
         fun readProperty(obj: Any?, schemas: SerializationSchemas, input: DeserializationInput, new: Array<Any?>) =
                 property.readProperty(obj, schemas, input).apply {
                     if(resultsIndex >= 0) {
@@ -64,7 +62,7 @@ class EvolutionSerializer(
             val clazz: Class<*> = type.asClass()!!
             if (!isConcrete(clazz)) return null
 
-            val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.type) }
+            val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.property.resolvedType) }
 
             var maxConstructorVersion = Integer.MIN_VALUE
             var constructor: KFunction<Any>? = null
@@ -82,6 +80,36 @@ class EvolutionSerializer(
             return constructor ?: constructorForDeserialization(type)
         }
 
+        private fun makeWithConstructor(
+                new: ObjectSerializer,
+                factory: SerializerFactory,
+                constructor: KFunction<Any>,
+                readersAsSerialized: Map<String, OldParam>): AMQPSerializer<Any> {
+            val constructorArgs = arrayOfNulls<Any?>(constructor.parameters.size)
+
+            constructor.parameters.withIndex().forEach {
+                readersAsSerialized.get(it.value.name!!)?.apply {
+                    this.resultsIndex = it.index
+                } ?: if (!it.value.type.isMarkedNullable) {
+                    throw NotSerializableException(
+                            "New parameter ${it.value.name} is mandatory, should be nullable for evolution to worK")
+                }
+            }
+            return EvolutionSerializerViaConstructor (new.type, factory, readersAsSerialized, constructor, constructorArgs)
+        }
+
+        private fun makeWithSetters(
+                new: ObjectSerializer,
+                factory: SerializerFactory,
+                constructor: KFunction<Any>,
+                readersAsSerialized: Map<String, OldParam>,
+                classProperties: Map<String, PropertyDescriptor>): AMQPSerializer<Any> {
+            val setters = propertiesForSerializationFromSetters(classProperties,
+                    new.type,
+                    factory).associateBy({ it.getter.name }, { it })
+            return EvolutionSerializerViaSetters (new.type, factory, readersAsSerialized, constructor, setters)
+        }
+
         /**
          * Build a serialization object for deserialization only of objects serialised
          * as different versions of a class.
@@ -96,45 +124,43 @@ class EvolutionSerializer(
         fun make(old: CompositeType, new: ObjectSerializer,
                  factory: SerializerFactory): AMQPSerializer<Any> {
 
-            val readersAsSerialized = linkedMapOf<String, OldParam>(
+            // The order in which the properties were serialised is important and must be preserved
-                    *(old.fields.map {
+            val readersAsSerialized = LinkedHashMap<String, OldParam>()
-                        val returnType =  try {
+            old.fields.forEach {
-                            it.getTypeAsClass(factory.classloader)
+                readersAsSerialized[it.name] = try {
+                    OldParam(-1, PropertySerializer.make(it.name, EvolutionPropertyReader(),
+                            it.getTypeAsClass(factory.classloader), factory))
                 } catch (e: ClassNotFoundException) {
                     throw NotSerializableException(e.message)
                 }
-
+            }
-                        it.name to OldParam(
-                                returnType,
-                                -1,
-                                PropertySerializer.make(
-                                        it.name, PublicPropertyReader(null), returnType, factory))
-                    }.toTypedArray())
-            )
 
             val constructor = getEvolverConstructor(new.type, readersAsSerialized) ?:
                     throw NotSerializableException(
                             "Attempt to deserialize an interface: ${new.type}. Serialized form is invalid.")
 
-            val constructorArgs = arrayOfNulls<Any?>(constructor.parameters.size)
+            val classProperties = new.type.asClass()?.propertyDescriptors() ?: emptyMap()
 
-            constructor.parameters.withIndex().forEach {
+            return if (classProperties.isNotEmpty() && constructor.parameters.isEmpty()) {
-                readersAsSerialized.get(it.value.name!!)?.apply {
+                makeWithSetters(new, factory, constructor, readersAsSerialized, classProperties)
-                    this.resultsIndex = it.index
+            }
-                } ?: if (!it.value.type.isMarkedNullable) {
+            else {
-                    throw NotSerializableException(
+                makeWithConstructor(new, factory, constructor, readersAsSerialized)
-                            "New parameter ${it.value.name} is mandatory, should be nullable for evolution to worK")
+            }
         }
     }
 
-            return EvolutionSerializer(new.type, factory, readersAsSerialized, constructor, constructorArgs)
+    override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput, debugIndent: Int) {
-        }
-    }
-
-    override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput, offset: Int) {
         throw UnsupportedOperationException("It should be impossible to write an evolution serializer")
     }
+}
 
+class EvolutionSerializerViaConstructor(
+        clazz: Type,
+        factory: SerializerFactory,
+        oldReaders: Map<String, EvolutionSerializer.OldParam>,
+        kotlinConstructor: KFunction<Any>?,
+        private val constructorArgs: Array<Any?>) : EvolutionSerializer (clazz, factory, oldReaders, kotlinConstructor) {
     /**
      * Unlike a normal [readObject] call where we simply apply the parameter deserialisers
      * to the object list of values we need to map that list, which is ordered per the
@@ -151,7 +177,36 @@ class EvolutionSerializer(
         oldReaders.values.zip(obj).map { it.first.readProperty(it.second, schemas, input, constructorArgs) }
 
         return javaConstructor?.newInstance(*(constructorArgs)) ?:
-                throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.")
+                throw NotSerializableException(
+                        "Attempt to deserialize an interface: $clazz. Serialized form is invalid.")
+    }
+}
+
+/**
+ * Specific instance of an [EvolutionSerializer] where the properties of the object are set via calling
+ * named setter functions on the instantiated object.
+ */
+class EvolutionSerializerViaSetters(
+        clazz: Type,
+        factory: SerializerFactory,
+        oldReaders: Map<String, EvolutionSerializer.OldParam>,
+        kotlinConstructor: KFunction<Any>?,
+        private val setters: Map<String, PropertyAccessor>) : EvolutionSerializer (clazz, factory, oldReaders, kotlinConstructor) {
+
+    override fun readObject(obj: Any, schemas: SerializationSchemas, input: DeserializationInput): Any {
+        if (obj !is List<*>) throw NotSerializableException("Body of described type is unexpected $obj")
+
+        val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException (
+                "Failed to instantiate instance of object $clazz")
+
+        // *must* read all the parameters in the order they were serialized
+        oldReaders.values.zip(obj).forEach {
+            // if that property still exists on the new object then set it
+            it.first.property.readProperty(it.second, schemas, input).apply {
+                setters[it.first.property.name]?.set(instance, this)
+            }
+        }
+        return instance
     }
 }
 

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

@@ -14,6 +14,9 @@ abstract class PropertyReader {
     abstract fun isNullable(): Boolean
 }
 
+/**
+ * Accessor for those properties of a class that have defined getter functions.
+ */
 class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() {
     init {
         readMethod?.isAccessible = true
@@ -43,6 +46,10 @@ class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() {
     override fun isNullable(): Boolean = readMethod?.returnsNullable() ?: false
 }
 
+/**
+ * Accessor for those properties of a class that do not have defined getter functions. In which case
+ * we used reflection to remove the unreadable status from that property whilst it's accessed.
+ */
 class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader() {
     init {
         loggerFor<PropertySerializer>().warn("Create property Serializer for private property '${field.name}' not "
@@ -68,6 +75,20 @@ class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader
     }
 }
 
+/**
+ * Special instance of a [PropertyReader] for use only by [EvolutionSerializer]s to make
+ * it explicit that no properties are ever actually read from an object as the evolution
+ * serializer should only be accessing the already serialized form.
+ */
+class EvolutionPropertyReader : PropertyReader() {
+    override fun read(obj: Any?): Any? {
+        throw UnsupportedOperationException("It should be impossible for an evolution serializer to "
+                + "be reading from an object")
+    }
+
+    override fun isNullable() = true
+}
+
 /**
  * Represents a generic interface to a serializable property of an object.
  *

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

@@ -253,7 +253,7 @@ internal fun <T : Any> propertiesForSerializationFromConstructor(
  * If we determine a class has a constructor that takes no parameters then check for pairs of getters / setters
  * and use those
  */
-private fun propertiesForSerializationFromSetters(
+fun propertiesForSerializationFromSetters(
         properties: Map<String, PropertyDescriptor>,
         type: Type,
         factory: SerializerFactory): List<PropertyAccessor> {

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

@@ -538,4 +538,45 @@ class EvolvabilityTests {
         File(URI("$localPath/$resource")).writeBytes( signedAndSerialized.bytes)
     }
 
+    @Suppress("UNCHECKED_CAST")
+    @Test
+    fun getterSetterEvolver1() {
+        val resource = "EvolvabilityTests.getterSetterEvolver1"
+        val sf = testDefaultFactory()
+
+        //
+        // Class as it was serialised
+        //
+        // data class C(var c: Int, var d: Int, var b: Int, var e: Int, var a: Int) {
+        //     // This will force the serialization engine to use getter / setter
+        //     // instantiation for the object rather than construction
+        //     @ConstructorForDeserialization
+        //     @Suppress("UNUSED")
+        //     constructor() : this(0, 0, 0, 0, 0)
+        // }
+        //
+        // File(URI("$localPath/$resource")).writeBytes(SerializationOutput(sf).serialize(C(3,4,2,5,1)).bytes)
+
+        //
+        // Class as it exists now, c has been removed
+        //
+        data class C(var d: Int, var b: Int, var e: Int, var a: Int) {
+            // This will force the serialization engine to use getter / setter
+            // instantiation for the object rather than construction
+            @ConstructorForDeserialization
+            @Suppress("UNUSED")
+            constructor() : this(0, 0, 0, 0)
+        }
+
+        val path = EvolvabilityTests::class.java.getResource(resource)
+        val f = File(path.toURI())
+
+        val sc2 = f.readBytes()
+        val deserializedC = DeserializationInput(sf).deserialize(SerializedBytes<C>(sc2))
+
+        assertEquals(1, deserializedC.a)
+        assertEquals(2, deserializedC.b)
+        assertEquals(4, deserializedC.d)
+        assertEquals(5, deserializedC.e)
+    }
 }