From ba0a94d54d5c9c2d1bac2840473e9c72cb2367a2 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Thu, 24 May 2018 18:30:45 +0100 Subject: [PATCH] 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/amqp/EvolutionSerializer.kt | 51 +++++++--- .../serialization/amqp/EvolvabilityTests.kt | 93 ++++++++++++++++-- ...ndatoryFieldWithAltConstructorForceReorder | Bin 0 -> 340 bytes ...abilityTests.moreComplexNonNullWithReorder | Bin 0 -> 1241 bytes 4 files changed, 123 insertions(+), 21 deletions(-) create mode 100644 node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.addMandatoryFieldWithAltConstructorForceReorder create mode 100644 node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.moreComplexNonNullWithReorder diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt index b2dfa16f53..0b6b517167 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt +++ b/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) = - property.readProperty(obj, schemas, input).apply { - if(resultsIndex >= 0) { - new[resultsIndex] = this - } - } + fun readProperty(obj: Any?, schemas: SerializationSchemas, input: DeserializationInput, + new: Array + ) = 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? = null + clazz.kotlin.constructors.forEach { val version = it.findAnnotation()?.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( diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.kt index 5eb6bbbab7..d4b6573c1a 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.kt +++ b/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(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(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, + val maxMessageSize: Int, + val maxTransactionSize: Int, + val modifiedTime: Instant, + val epoch: Int, + val whitelistedContractImplementations: Map>, + /* 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, + maxMessageSize: Int, + maxTransactionSize: Int, + modifiedTime: Instant, + epoch: Int, + whitelistedContractImplementations: Map> + ) : 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(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.." 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)) diff --git a/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.addMandatoryFieldWithAltConstructorForceReorder b/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.addMandatoryFieldWithAltConstructorForceReorder new file mode 100644 index 0000000000000000000000000000000000000000..b4ead2ac82f136bf78d5b87a7378ba3eeae4a584 GIT binary patch literal 340 zcmYe!FG@*dWME)uIGO|`85kHZ0I@BQ!OXB&DKE7|FBzo5%F{PEEX=9Gz_m0y(4`LNm{a1MpI2N`RGJJFcgrtIP7O*0IySXP#o5`x748J)18Pjfc*Ws>B@el=_P%us!})lEK70f1zTa^l*J^D2L)CY;2FUI!`ss``t4u(C`1H52JQ4YBv*9mA zHShTzE7pCRu59}h*Gi>hH!F2}0{Wub5X`GE(A)9cf&bR;S9noDW77aS$N)p?YZng4 zf*iVSolzMym%w@E(iy4pK8he6ET_pK*U1a!8P(jhq2Y zuw`c~#)uzgE;84$60Lt!qRnw=Hok4j915A+mWGn;NPVenubuJVGv##x2Ne`Wu!VN< z5TyVgnDUka0uE^e-K5}ufags)LomhUfNTd)G#Mv#RhtS*VSi-GtAu&DiIE4?3d);* zY@<`2RK94+D>PX%j3TKo#CLQ#GYUEoIIuQxBwj~?W7=9h7r1ZA*?t=f6etYx))^IC zRkV?`oggYk(;QJQ>T=)J1=UXDHh literal 0 HcmV?d00001