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

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/EvolutionSerializer.kt

@@ -3,6 +3,9 @@ package net.corda.serialization.internal.amqp
 import net.corda.core.internal.isConcreteClass
 import net.corda.core.serialization.DeprecatedConstructorForDeserialization
 import net.corda.core.serialization.SerializationContext
+import net.corda.core.utilities.contextLogger
+import net.corda.core.utilities.debug
+import net.corda.core.utilities.loggerFor
 import net.corda.serialization.internal.carpenter.getTypeAsClass
 import org.apache.qpid.proton.codec.Data
 import java.io.NotSerializableException
@@ -48,9 +51,15 @@ abstract class EvolutionSerializer(
                 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
@@ -66,22 +75,37 @@ abstract class EvolutionSerializer(
 
             if (!clazz.isConcreteClass) 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(

serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolvabilityTests.kt

@@ -8,10 +8,7 @@ import net.corda.core.node.NotaryInfo
 import net.corda.core.serialization.ConstructorForDeserialization
 import net.corda.core.serialization.DeprecatedConstructorForDeserialization
 import net.corda.core.serialization.SerializedBytes
-import net.corda.serialization.internal.amqp.testutils.TestSerializationOutput
-import net.corda.serialization.internal.amqp.testutils.deserialize
-import net.corda.serialization.internal.amqp.testutils.serialize
-import net.corda.serialization.internal.amqp.testutils.testDefaultFactory
+import net.corda.serialization.internal.amqp.testutils.*
 import net.corda.testing.common.internal.ProjectStructure.projectRootDir
 import net.corda.testing.core.DUMMY_NOTARY_NAME
 import net.corda.testing.core.TestIdentity
@@ -22,6 +19,7 @@ import java.io.NotSerializableException
 import java.net.URI
 import java.time.Instant
 import kotlin.test.assertEquals
+import net.corda.serialization.internal.amqp.custom.InstantSerializer
 
 // To regenerate any of the binary test files do the following
 //
@@ -202,6 +200,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() {
@@ -479,7 +557,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.serialization.internal.amqp.custom.InstantSerializer(sf))
         sf.register(net.corda.serialization.internal.amqp.custom.PublicKeySerializer)
@@ -515,7 +593,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.serialization.internal.amqp.custom.InstantSerializer(sf))