CORDA-1530 - Generics break default evolver (#3232)

* CORDA-1530 - Generics break default  evolver

When selecting an annotated constructor for evolving a type make sure
we treat generics in the same manner we did when serialized. Effectively
throw away the template information and treat lists as lists and maps
as maps

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

@@ -2,6 +2,8 @@ package net.corda.nodeapi.internal.serialization.amqp
 
 import net.corda.core.serialization.DeprecatedConstructorForDeserialization
 import net.corda.nodeapi.internal.serialization.carpenter.getTypeAsClass
+import net.corda.core.utilities.contextLogger
+import net.corda.core.utilities.debug
 import org.apache.qpid.proton.codec.Data
 import java.lang.reflect.Type
 import java.io.NotSerializableException
@@ -16,10 +18,8 @@ import kotlin.reflect.jvm.javaType
  *
  * @property oldReaders A linked map representing the properties of the object as they were serialized. Note
  * this may contain properties that are no longer needed by the class. These *must* be read however to ensure
- * any refferenced objects in the object stream are captured properly
- * @property kotlinConstructor
- * @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
+ * any referenced objects in the object stream are captured properly
+ * @property kotlinConstructor reference to the constructor used to instantiate an instance of the class.
  */
 abstract class EvolutionSerializer(
         clazz: Type,
@@ -39,15 +39,21 @@ abstract class EvolutionSerializer(
      * @param property object to read the actual property value
      */
     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) {
-                        new[resultsIndex] = this
-                    }
-                }
+        fun readProperty(obj: Any?, schemas: SerializationSchemas, input: DeserializationInput,
+                         new: Array<Any?>
+        ) = property.readProperty(obj, schemas, input).apply {
+            if(resultsIndex >= 0) {
+                new[resultsIndex] = this
+            }
+        }
+        override fun toString(): String {
+            return "resultsIndex = $resultsIndex property = ${property.name}"
+        }
     }
 
     companion object {
+        val logger = contextLogger()
+
         /**
          * Unlike the generic deserialization case where we need to locate the primary constructor
          * for the object (or our best guess) in the case of an object whose structure has changed
@@ -63,22 +69,37 @@ abstract class EvolutionSerializer(
 
             if (!isConcrete(clazz)) return null
 
-            val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.property.resolvedType) }
-
+            val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.property.resolvedType.asClass()) }
             var maxConstructorVersion = Integer.MIN_VALUE
             var constructor: KFunction<Any>? = null
+
             clazz.kotlin.constructors.forEach {
                 val version = it.findAnnotation<DeprecatedConstructorForDeserialization>()?.version ?: Integer.MIN_VALUE
-                if (oldArgumentSet.containsAll(it.parameters.map { v -> Pair(v.name, v.type.javaType) }) &&
-                        version > maxConstructorVersion) {
+
+                if (version > maxConstructorVersion &&
+                        oldArgumentSet.containsAll(it.parameters.map { v -> Pair(v.name, v.type.javaType.asClass()) })
+                ) {
                     constructor = it
                     maxConstructorVersion = version
+
+                    with(logger) {
+                        info("Select annotated constructor version=$version nparams=${it.parameters.size}")
+                        debug{"  params=${it.parameters}"}
+                    }
+                } else if (version != Integer.MIN_VALUE){
+                    with(logger) {
+                        info("Ignore annotated constructor version=$version nparams=${it.parameters.size}")
+                        debug{"  params=${it.parameters}"}
+                    }
                 }
             }
 
             // if we didn't get an exact match revert to existing behaviour, if the new parameters
             // are not mandatory (i.e. nullable) things are fine
-            return constructor ?: constructorForDeserialization(type)
+            return constructor ?: run {
+                logger.info("Failed to find annotated historic constructor")
+                constructorForDeserialization(type)
+            }
         }
 
         private fun makeWithConstructor(

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

@@ -18,6 +18,7 @@ import java.io.NotSerializableException
 import java.net.URI
 import java.time.Instant
 import kotlin.test.assertEquals
+import net.corda.nodeapi.internal.serialization.amqp.custom.InstantSerializer
 
 // To regenerate any of the binary test files do the following
 //
@@ -38,8 +39,8 @@ class EvolvabilityTests {
         val sf = testDefaultFactory()
         val resource = "EvolvabilityTests.simpleOrderSwapSameType"
 
-        val A = 1
-        val B = 2
+        val a = 1
+        val b = 2
 
         // Original version of the class for the serialised version of this class
         // data class C (val a: Int, val b: Int)
@@ -54,8 +55,8 @@ class EvolvabilityTests {
         val sc2 = f.readBytes()
         val deserializedC = DeserializationInput(sf).deserialize(SerializedBytes<C>(sc2))
 
-        assertEquals(A, deserializedC.a)
-        assertEquals(B, deserializedC.b)
+        assertEquals(a, deserializedC.a)
+        assertEquals(b, deserializedC.b)
     }
 
     @Test
@@ -206,6 +207,86 @@ class EvolvabilityTests {
         assertEquals("hello", deserializedCC.b)
     }
 
+    @Test
+    fun addMandatoryFieldWithAltConstructorForceReorder() {
+        val sf = testDefaultFactory()
+        val z = 30
+        val y = 20
+        val resource = "EvolvabilityTests.addMandatoryFieldWithAltConstructorForceReorder"
+
+        // Original version of the class as it was serialised
+        // data class CC(val z: Int, val y: Int)
+        // File(URI("$localPath/$resource")).writeBytes(SerializationOutput(sf).serialize(CC(z, y)).bytes)
+
+        @Suppress("UNUSED")
+        data class CC(val z: Int, val y: Int, val a: String) {
+            @DeprecatedConstructorForDeserialization(1)
+            constructor (z: Int, y: Int) : this(z, y, "10")
+        }
+
+        val url = EvolvabilityTests::class.java.getResource(resource)
+        val deserializedCC = DeserializationInput(sf).deserialize(SerializedBytes<CC>(url.readBytes()))
+
+        assertEquals("10", deserializedCC.a)
+        assertEquals(y, deserializedCC.y)
+        assertEquals(z, deserializedCC.z)
+    }
+
+    @Test
+    fun moreComplexNonNullWithReorder() {
+        val resource = "${javaClass.simpleName}.${testName()}"
+
+        data class NetworkParametersExample(
+                val minimumPlatformVersion: Int,
+                val notaries: List<String>,
+                val maxMessageSize: Int,
+                val maxTransactionSize: Int,
+                val modifiedTime: Instant,
+                val epoch: Int,
+                val whitelistedContractImplementations: Map<String, List<Int>>,
+                /* to regenerate test class, comment out this element */
+                val eventHorizon: Int
+        ) {
+            // when regenerating test class this won't be required
+            @DeprecatedConstructorForDeserialization(1)
+            @Suppress("UNUSED")
+            constructor (
+                    minimumPlatformVersion: Int,
+                    notaries: List<String>,
+                    maxMessageSize: Int,
+                    maxTransactionSize: Int,
+                    modifiedTime: Instant,
+                    epoch: Int,
+                    whitelistedContractImplementations: Map<String, List<Int>>
+            ) : this(minimumPlatformVersion,
+                    notaries,
+                    maxMessageSize,
+                    maxTransactionSize,
+                    modifiedTime,
+                    epoch,
+                    whitelistedContractImplementations,
+                    Int.MAX_VALUE)
+        }
+
+        val factory = testDefaultFactory().apply {
+            register(InstantSerializer(this))
+        }
+
+        // Uncomment to regenerate test case
+        // File(URI("$localPath/$resource")).writeBytes(SerializationOutput(factory).serialize(
+        //         NetworkParametersExample(
+        //                 10,
+        //                 listOf("Notary1", "Notary2"),
+        //                 100,
+        //                 10,
+        //                 Instant.now(),
+        //                 9,
+        //                 mapOf("A" to listOf(1, 2, 3), "B" to listOf (4, 5, 6)))).bytes)
+
+        val url = EvolvabilityTests::class.java.getResource(resource)
+        DeserializationInput(factory).deserialize(SerializedBytes<NetworkParametersExample>(url.readBytes()))
+    }
+
     @Test(expected = NotSerializableException::class)
     @Suppress("UNUSED")
     fun addMandatoryFieldWithAltConstructorUnAnnotated() {
@@ -487,7 +568,7 @@ class EvolvabilityTests {
     //
     @Test
     @Ignore("Test fails after moving NetworkParameters and NotaryInfo into core from node-api")
-    fun readBrokenNetworkParameters(){
+    fun readBrokenNetworkParameters() {
         val sf = testDefaultFactory()
         sf.register(net.corda.nodeapi.internal.serialization.amqp.custom.InstantSerializer(sf))
         sf.register(net.corda.nodeapi.internal.serialization.amqp.custom.PublicKeySerializer)
@@ -524,7 +605,7 @@ class EvolvabilityTests {
         val resource = "networkParams.<corda version>.<commit sha>"
         val DUMMY_NOTARY = TestIdentity(DUMMY_NOTARY_NAME, 20).party
         val networkParameters = NetworkParameters(
-                3, listOf(NotaryInfo(DUMMY_NOTARY, false)),1000, 1000, Instant.EPOCH, 1 , emptyMap())
+                3, listOf(NotaryInfo(DUMMY_NOTARY, false)), 1000, 1000, Instant.EPOCH, 1, emptyMap())
 
         val sf = testDefaultFactory()
         sf.register(net.corda.nodeapi.internal.serialization.amqp.custom.InstantSerializer(sf))