From 21985243159f7797fbd507d96819c531f7f22ee6 Mon Sep 17 00:00:00 2001 From: Joseph Zuniga-Daly <59851625+josephzunigadaly@users.noreply.github.com> Date: Wed, 19 Aug 2020 10:41:51 +0100 Subject: [PATCH] CORDA-3824: Fix property rename in AMQP object evolution (#6616) * CORDA-3824: Add unit tests * CORDA-3824: Fix property rename in AMQP object evolution * Rename deserializedException to deserializedObject * Rename test class to EvolutionObjectBuilderRenamedPropertyTests * Added descriptions of the different object evolution stages in this test * Rename file containing the serialized object * Regenerate serialized data * Add a comment explaining the commented out code. * Restrict new behaviour to EvolutionObjectBuilder and simplify the loop that builds constructor slots. --- .../internal/amqp/ObjectBuilder.kt | 20 +++- ...lutionObjectBuilderRenamedPropertyTests.kt | 90 ++++++++++++++++++ ...ionObjectBuilderRenamedPropertyTests.Step1 | Bin 0 -> 1437 bytes 3 files changed, 105 insertions(+), 5 deletions(-) create mode 100644 serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.kt create mode 100644 serialization/src/test/resources/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.Step1 diff --git a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectBuilder.kt b/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectBuilder.kt index ab1150ffff..056cfc1197 100644 --- a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectBuilder.kt +++ b/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectBuilder.kt @@ -69,7 +69,7 @@ interface ObjectBuilder { * Create an [ObjectBuilderProvider] for the given [LocalTypeInformation.Composable]. */ fun makeProvider(typeInformation: LocalTypeInformation.Composable): ObjectBuilderProvider = - makeProvider(typeInformation.typeIdentifier, typeInformation.constructor, typeInformation.properties) + makeProvider(typeInformation.typeIdentifier, typeInformation.constructor, typeInformation.properties, false) /** * Create an [ObjectBuilderProvider] for the given type, constructor and set of properties. @@ -79,11 +79,12 @@ interface ObjectBuilder { */ fun makeProvider(typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation, - properties: Map): ObjectBuilderProvider = - if (constructor.hasParameters) makeConstructorBasedProvider(properties, typeIdentifier, constructor) + properties: Map, + includeAllConstructorParameters: Boolean): ObjectBuilderProvider = + if (constructor.hasParameters) makeConstructorBasedProvider(properties, typeIdentifier, constructor, includeAllConstructorParameters) else makeGetterSetterProvider(properties, typeIdentifier, constructor) - private fun makeConstructorBasedProvider(properties: Map, typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation): ObjectBuilderProvider { + private fun makeConstructorBasedProvider(properties: Map, typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation, includeAllConstructorParameters: Boolean): ObjectBuilderProvider { val constructorIndices = properties.mapValues { (name, property) -> when (property) { is LocalPropertyInformation.ConstructorPairedProperty -> property.constructorSlot.parameterIndex @@ -94,6 +95,15 @@ interface ObjectBuilder { "but property $name is not constructor-paired" ) } + }.toMutableMap() + + if (includeAllConstructorParameters) { + // Add constructor parameters not in the list of properties + // so we can use them in object evolution + for ((parameterIndex, parameter) in constructor.parameters.withIndex()) { + // Only use the parameters not already matched to properties + constructorIndices.putIfAbsent(parameter.name, parameterIndex) + } } val propertySlots = constructorIndices.keys.mapIndexed { slot, name -> name to slot }.toMap() @@ -203,7 +213,7 @@ class EvolutionObjectBuilder(private val localBuilder: ObjectBuilder, localProperties: Map, remoteTypeInformation: RemoteTypeInformation.Composable, mustPreserveData: Boolean): () -> ObjectBuilder { - val localBuilderProvider = ObjectBuilder.makeProvider(typeIdentifier, constructor, localProperties) + val localBuilderProvider = ObjectBuilder.makeProvider(typeIdentifier, constructor, localProperties, true) val remotePropertyNames = remoteTypeInformation.properties.keys.sorted() val reroutedIndices = remotePropertyNames.map { propertyName -> diff --git a/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.kt b/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.kt new file mode 100644 index 0000000000..0e7f795839 --- /dev/null +++ b/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.kt @@ -0,0 +1,90 @@ +package net.corda.serialization.internal.amqp + +import net.corda.core.contracts.BelongsToContract +import net.corda.core.contracts.Contract +import net.corda.core.contracts.ContractState +import net.corda.core.identity.AbstractParty +import net.corda.core.serialization.DeprecatedConstructorForDeserialization +import net.corda.core.serialization.SerializedBytes +import net.corda.core.transactions.LedgerTransaction +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.writeTestResource +import org.assertj.core.api.Assertions +import org.junit.Test + +class EvolutionObjectBuilderRenamedPropertyTests +{ + private val cordappVersionTestValue = 38854445 + private val dataTestValue = "d7af8af0-c10e-45bc-a5f7-92de432be0ef" + private val xTestValue = 7568055 + private val yTestValue = 4113687 + + class TemplateContract : Contract { + override fun verify(tx: LedgerTransaction) { } + } + + /** + * Step 1 + * + * This is the original class definition in object evolution. + */ +// @BelongsToContract(TemplateContract::class) +// data class TemplateState(val cordappVersion: Int, val data: String, val x : Int?, override val participants: List = listOf()) : ContractState + + /** + * Step 2 + * + * This is an intermediate class definition in object evolution. + * The y property has been added and a constructor copies the value of x into y. x is now set to null by the constructor. + */ +// @BelongsToContract(TemplateContract::class) +// data class TemplateState(val cordappVersion: Int, val data: String, val x : Int?, val y : String?, override val participants: List = listOf()) : ContractState { +// @DeprecatedConstructorForDeserialization(1) +// constructor(cordappVersion: Int, data : String, x : Int?, participants: List) +// : this(cordappVersion, data, null, x?.toString(), participants) +// } + + /** + * Step 3 + * + * This is the final class definition in object evolution. + * The x property has been removed but the constructor that copies values of x into y still exists. We expect previous versions of this + * object to pass the value of x to the constructor when deserialized. + */ + @BelongsToContract(TemplateContract::class) + data class TemplateState(val cordappVersion: Int, val data: String, val y : String?, override val participants: List = listOf()) : ContractState { + @DeprecatedConstructorForDeserialization(1) + constructor(cordappVersion: Int, data : String, x : Int?, participants: List) : this(cordappVersion, data, x?.toString(), participants) + } + + @Test(timeout=300_000) + fun `Step 1 to Step 3`() { + + // The next two commented lines are how the serialized data is generated. To regenerate the data, uncomment these along + // with the correct version of the class and rerun the test. This will generate a new file in the project resources. + +// val step1 = TemplateState(cordappVersionTestValue, dataTestValue, xTestValue) +// saveSerializedObject(step1) + + // serialization/src/test/resources/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.Step1 + val bytes = this::class.java.getResource("EvolutionObjectBuilderRenamedPropertyTests.Step1").readBytes() + + val serializerFactory: SerializerFactory = testDefaultFactory() + val deserializedObject = DeserializationInput(serializerFactory) + .deserialize(SerializedBytes(bytes)) + + Assertions.assertThat(deserializedObject.cordappVersion).isEqualTo(cordappVersionTestValue) + Assertions.assertThat(deserializedObject.data).isEqualTo(dataTestValue) +// Assertions.assertThat(deserializedObject.x).isEqualTo(xTestValue) + Assertions.assertThat(deserializedObject.y).isEqualTo(xTestValue.toString()) + Assertions.assertThat(deserializedObject).isInstanceOf(TemplateState::class.java) + } + + /** + * Write serialized object to resources folder + */ + @Suppress("unused") + fun saveSerializedObject(obj : T) = writeTestResource(SerializationOutput(testDefaultFactory()).serialize(obj)) +} \ No newline at end of file diff --git a/serialization/src/test/resources/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.Step1 b/serialization/src/test/resources/net/corda/serialization/internal/amqp/EvolutionObjectBuilderRenamedPropertyTests.Step1 new file mode 100644 index 0000000000000000000000000000000000000000..c2a94d7f11f57bc2446a00d6cd6ec0e57d769e78 GIT binary patch literal 1437 zcmb_c%}(1u5VjMW2&qD}46cZ9mpdyuVYDyph11(e>Dy{8xw66bRuY<#> zhpOtmx87ELh+emc-unhz`xxD%)Fe?N(W;l#Xl8c4pIO@sA`9>w$G!V*u#@BX9~>us zU_qYy^2mo+GW#7*(Q!j=dHc;u(&{#Kr*rP0LwToGqvt{=r(YMzqE!X#|`MfoxZcTV)HbSIPz~L3?E6Md}4UY_zu0eaBlp!YObXLAzGdI$V79H6xT`goK|r z%!F$e6tbGgYW#ba63Ls4s}#cs0T*2Xi~?UmK8BGGTnTucP|~`AoAmQXh67EkCdjp5 zbOL?gK}(N<5JtH762>@Qdmm@AN3j*gnQrn#c0dG*KS-fKI{ou88ZOLej%({x z;a!$f%J#3c!=PCQU s4Xs0@mN