Merge pull request #3253 from corda/serializer-backports

Serializer backports

.ci/api-current.txt

@@ -2894,7 +2894,8 @@ public final class net.corda.core.node.NetworkParameters extends java.lang.Objec
   @NotNull
   public final java.util.Map<String, java.util.List<net.corda.core.crypto.SecureHash>> getWhitelistedContractImplementations()
   public int hashCode()
-  @org.jetbrains.annotations.NotNull public String toString()
+  @NotNull
+  public String toString()
 ##
 @CordaSerializable
 public final class net.corda.core.node.NodeInfo extends java.lang.Object

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

@@ -1,5 +1,9 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
+import net.corda.core.utilities.contextLogger
+import net.corda.core.utilities.debug
+import net.corda.core.utilities.loggerFor
+import net.corda.core.utilities.trace
 import org.apache.qpid.proton.amqp.Symbol
 import org.apache.qpid.proton.codec.Data
 import java.io.NotSerializableException
@@ -10,28 +14,48 @@ import java.lang.reflect.Type
  */
 open class ArraySerializer(override val type: Type, factory: SerializerFactory) : AMQPSerializer<Any> {
     companion object {
-        fun make(type: Type, factory: SerializerFactory) = when (type) {
-            Array<Char>::class.java -> CharArraySerializer(factory)
-            else -> ArraySerializer(type, factory)
+        fun make(type: Type, factory: SerializerFactory) : AMQPSerializer<Any> {
+            contextLogger().debug { "Making array serializer, typename=${type.typeName}" }
+            return when (type) {
+                Array<Char>::class.java -> CharArraySerializer(factory)
+                else -> ArraySerializer(type, factory)
+            }
         }
     }
 
+    private val logger = loggerFor<ArraySerializer>()
+
     // because this might be an array of array of primitives (to any recursive depth) and
     // because we care that the lowest type is unboxed we can't rely on the inbuilt type
     // id to generate it properly (it will always return [[[Ljava.lang.type -> type[][][]
     // for example).
     //
-    // We *need* to retain knowledge for AMQP deserialization weather that lowest primitive
+    // We *need* to retain knowledge for AMQP deserialization whether that lowest primitive
     // was boxed or unboxed so just infer it recursively.
-    private fun calcTypeName(type: Type): String =
-            if (type.componentType().isArray()) {
-                val typeName = calcTypeName(type.componentType()); "$typeName[]"
+    private fun calcTypeName(type: Type, debugOffset : Int = 0): String {
+        logger.trace { "${"".padStart(debugOffset, ' ') }  calcTypeName - ${type.typeName}" }
+
+        return if (type.componentType().isArray()) {
+            // Special case handler for primitive byte arrays. This is needed because we can silently
+            // coerce a byte[] to our own binary type. Normally, if the component type was itself an
+            // array we'd keep walking down the chain but for byte[] stop here and use binary instead
+            val typeName =  if (SerializerFactory.isPrimitive(type.componentType())) {
+                SerializerFactory.nameForType(type.componentType())
             } else {
-                val arrayType = if (type.asClass()!!.componentType.isPrimitive) "[p]" else "[]"
-                "${type.componentType().typeName}$arrayType"
+                calcTypeName(type.componentType(), debugOffset + 4)
             }
 
-    override val typeDescriptor by lazy { Symbol.valueOf("$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}") }
+            "$typeName[]"
+        } else {
+            val arrayType = if (type.asClass()!!.componentType.isPrimitive) "[p]" else "[]"
+            "${type.componentType().typeName}$arrayType"
+        }
+    }
+
+    override val typeDescriptor by lazy {
+        Symbol.valueOf("$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}")
+    }
+
     internal val elementType: Type by lazy { type.componentType() }
     internal open val typeName by lazy { calcTypeName(type) }
 

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/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt

@@ -7,6 +7,8 @@ import net.corda.core.serialization.ClassWhitelist
 import net.corda.nodeapi.internal.serialization.carpenter.CarpenterMetaSchema
 import net.corda.nodeapi.internal.serialization.carpenter.ClassCarpenter
 import net.corda.nodeapi.internal.serialization.carpenter.MetaCarpenter
+import net.corda.core.utilities.debug
+import net.corda.core.utilities.loggerFor
 import org.apache.qpid.proton.amqp.*
 import java.io.NotSerializableException
 import java.lang.reflect.*
@@ -59,6 +61,8 @@ open class SerializerFactory(
 
     fun getTransformsCache() = transformsCache
 
+    private val logger by lazy { loggerFor<SerializerFactory>() }
+
     /**
      * Look up, and manufacture if necessary, a serializer for the given type.
      *
@@ -216,6 +220,7 @@ open class SerializerFactory(
     private fun processSchema(schemaAndDescriptor: FactorySchemaAndDescriptor, sentinel: Boolean = false) {
         val metaSchema = CarpenterMetaSchema.newInstance()
         for (typeNotation in schemaAndDescriptor.schemas.schema.types) {
+            logger.debug { "descriptor=${schemaAndDescriptor.typeDescriptor}, typeNotation=${typeNotation.name}" }
             try {
                 val serialiser = processSchemaEntry(typeNotation)
                 // if we just successfully built a serializer for the type but the type fingerprint

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

@@ -249,6 +249,14 @@ class DeserializeSimpleTypesTests {
         assertEquals(c.c[0], deserializedC.c[0])
         assertEquals(c.c[1], deserializedC.c[1])
         assertEquals(c.c[2], deserializedC.c[2])
+
+        val di = DeserializationInput(sf2)
+        val deserializedC2 = di.deserialize(serialisedC)
+
+        assertEquals(c.c.size, deserializedC2.c.size)
+        assertEquals(c.c[0], deserializedC2.c[0])
+        assertEquals(c.c[1], deserializedC2.c[1])
+        assertEquals(c.c[2], deserializedC2.c[2])
     }
 
     @Test
@@ -490,7 +498,33 @@ class DeserializeSimpleTypesTests {
         assertEquals(3, da2.a?.a?.b)
         assertEquals(2, da2.a?.a?.a?.b)
         assertEquals(1, da2.a?.a?.a?.a?.b)
-
     }
+
+    // Replicates CORDA-1545
+    @Test
+    fun arrayOfByteArray() {
+        class A(val a : Array<ByteArray>)
+
+        val ba1 = ByteArray(3)
+        ba1[0] = 0b0001; ba1[1] = 0b0101; ba1[2] = 0b1111
+
+        val ba2 = ByteArray(3)
+        ba2[0] = 0b1000; ba2[1] = 0b1100; ba2[2] = 0b1110
+
+        val a = A(arrayOf(ba1, ba2))
+
+        val serializedA = TestSerializationOutput(VERBOSE, sf1).serializeAndReturnSchema(a)
+
+        serializedA.schema.types.forEach {
+            println(it)
+        }
+
+        // This not throwing is the point of the test
+        DeserializationInput(sf1).deserialize(serializedA.obj)
+
+        // This not throwing is the point of the test
+        DeserializationInput(sf2).deserialize(serializedA.obj)
+    }
+
 }
 

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))