From 488f11e2e607d2b3b5238d8d28b471d9f50d0947 Mon Sep 17 00:00:00 2001 From: Dominic Fox <40790090+distributedleetravis@users.noreply.github.com> Date: Thu, 29 Nov 2018 15:54:30 +0000 Subject: [PATCH] CORDA-2263 evolve types with calculated properties (#4314) * CORDA-2263 evolve types with calculated properties * Push handling of computed properties into builders * Set isAccessible to true prior to calling * Type information captures Java constructor, not Kotlin wrapper --- .../internal/amqp/ObjectBuilder.kt | 200 +++++++++++++----- .../internal/amqp/ObjectSerializer.kt | 4 +- .../amqp/custom/ThrowableSerializer.kt | 2 +- .../internal/model/LocalTypeInformation.kt | 2 +- .../model/LocalTypeInformationBuilder.kt | 16 +- .../internal/amqp/EvolvabilityTests.kt | 29 +++ ...sts.removeParameterWithCalculatedParameter | Bin 0 -> 455 bytes 7 files changed, 192 insertions(+), 61 deletions(-) create mode 100644 serialization/src/test/resources/net/corda/serialization/internal/amqp/EvolvabilityTests.removeParameterWithCalculatedParameter 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 2b32e5482b..01db0ab66b 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 @@ -2,100 +2,202 @@ package net.corda.serialization.internal.amqp import net.corda.serialization.internal.model.* import java.io.NotSerializableException +import java.lang.reflect.Constructor +import java.lang.reflect.InvocationTargetException +import java.lang.reflect.Method +private const val IGNORE_COMPUTED = -1 + +/** + * Creates a new [ObjectBuilder] ready to be populated with deserialized values so that it can create an object instance. + * + * @property propertySlots The slot indices of the properties written by the provided [ObjectBuilder], by property name. + * @param provider The thunk that provides a new, empty [ObjectBuilder] + */ +data class ObjectBuilderProvider(val propertySlots: Map, private val provider: () -> ObjectBuilder) + : () -> ObjectBuilder by provider + +/** + * Wraps the operation of calling a constructor, with helpful exception handling. + */ +private class ConstructorCaller(private val javaConstructor: Constructor): (Array) -> Any { + + override fun invoke(parameters: Array): Any = + try { + javaConstructor.newInstance(*parameters) + } catch (e: InvocationTargetException) { + throw NotSerializableException( + "Constructor for ${javaConstructor.declaringClass} (isAccessible=${javaConstructor.isAccessible}) " + + "failed when called with parameters ${parameters.toList()}: ${e.cause!!.message}") + } catch (e: IllegalAccessException) { + throw NotSerializableException( + "Constructor for ${javaConstructor.declaringClass} (isAccessible=${javaConstructor.isAccessible}) " + + "not accessible: ${e.message}") + } +} + +/** + * Wraps the operation of calling a setter, with helpful exception handling. + */ +private class SetterCaller(val setter: Method): (Any, Any?) -> Unit { + override fun invoke(target: Any, value: Any?) { + try { + setter.invoke(target, value) + } catch (e: InvocationTargetException) { + throw NotSerializableException( + "Setter ${setter.declaringClass}.${setter.name} (isAccessible=${setter.isAccessible} " + + "failed when called with parameter $value: ${e.cause!!.message}") + } catch (e: IllegalAccessException) { + throw NotSerializableException( + "Setter ${setter.declaringClass}.${setter.name} (isAccessible=${setter.isAccessible} " + + "not accessible: ${e.message}") + } + } +} + +/** + * Initialises, configures and creates a new object by receiving values into indexed slots. + */ interface ObjectBuilder { companion object { - fun makeProvider(typeInformation: LocalTypeInformation.Composable): () -> ObjectBuilder = + /** + * Create an [ObjectBuilderProvider] for the given [LocalTypeInformation.Composable]. + */ + fun makeProvider(typeInformation: LocalTypeInformation.Composable): ObjectBuilderProvider = makeProvider(typeInformation.typeIdentifier, typeInformation.constructor, typeInformation.properties) - fun makeProvider(typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation, properties: Map): () -> ObjectBuilder { - val nonCalculatedProperties = properties.asSequence() - .filterNot { (name, property) -> property.isCalculated } - .sortedBy { (name, _) -> name } - .map { (_, property) -> property } - .toList() + /** + * Create an [ObjectBuilderProvider] for the given type, constructor and set of properties. + * + * The [EvolutionObjectBuilder] uses this to create [ObjectBuilderProvider]s for objects initialised via an + * evolution constructor (i.e. a constructor annotated with [DeprecatedConstructorForDeserialization]). + */ + fun makeProvider(typeIdentifier: TypeIdentifier, + constructor: LocalConstructorInformation, + properties: Map): ObjectBuilderProvider = + if (constructor.hasParameters) makeConstructorBasedProvider(properties, typeIdentifier, constructor) + else makeGetterSetterProvider(properties, typeIdentifier, constructor) - val propertyIndices = nonCalculatedProperties.mapNotNull { - when(it) { - is LocalPropertyInformation.ConstructorPairedProperty -> it.constructorSlot.parameterIndex - is LocalPropertyInformation.PrivateConstructorPairedProperty -> it.constructorSlot.parameterIndex - else -> null + private fun makeConstructorBasedProvider(properties: Map, typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation): ObjectBuilderProvider { + val constructorIndices = properties.mapValues { (name, property) -> + when (property) { + is LocalPropertyInformation.ConstructorPairedProperty -> property.constructorSlot.parameterIndex + is LocalPropertyInformation.PrivateConstructorPairedProperty -> property.constructorSlot.parameterIndex + is LocalPropertyInformation.CalculatedProperty -> IGNORE_COMPUTED + else -> throw NotSerializableException( + "Type ${typeIdentifier.prettyPrint(false)} has constructor arguments, " + + "but property $name is not constructor-paired" + ) } - }.toIntArray() - - if (propertyIndices.isNotEmpty()) { - if (propertyIndices.size != nonCalculatedProperties.size) { - throw NotSerializableException( - "Some but not all properties of ${typeIdentifier.prettyPrint(false)} " + - "are constructor-based") - } - return { ConstructorBasedObjectBuilder(constructor, propertyIndices) } } - val getterSetter = nonCalculatedProperties.filterIsInstance() - return { SetterBasedObjectBuilder(constructor, getterSetter) } + val propertySlots = constructorIndices.keys.mapIndexed { slot, name -> name to slot }.toMap() + + return ObjectBuilderProvider(propertySlots) { + ConstructorBasedObjectBuilder(ConstructorCaller(constructor.observedMethod), constructorIndices.values.toIntArray()) + } + } + + private fun makeGetterSetterProvider(properties: Map, typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation): ObjectBuilderProvider { + val setters = properties.mapValues { (name, property) -> + when (property) { + is LocalPropertyInformation.GetterSetterProperty -> SetterCaller(property.observedSetter) + is LocalPropertyInformation.CalculatedProperty -> null + else -> throw NotSerializableException( + "Type ${typeIdentifier.prettyPrint(false)} has no constructor arguments, " + + "but property $name is constructor-paired" + ) + } + } + + val propertySlots = setters.keys.mapIndexed { slot, name -> name to slot }.toMap() + + return ObjectBuilderProvider(propertySlots) { + SetterBasedObjectBuilder(ConstructorCaller(constructor.observedMethod), setters.values.toList()) + } } } + /** + * Begin building the object. + */ fun initialize() + + /** + * Populate one of the builder's slots with a value. + */ fun populate(slot: Int, value: Any?) + + /** + * Return the completed, configured with the values in the builder's slots, + */ fun build(): Any } -class SetterBasedObjectBuilder( - val constructor: LocalConstructorInformation, - val properties: List): ObjectBuilder { +/** + * An [ObjectBuilder] which builds an object by calling its default no-argument constructor to obtain an instance, + * and calling a setter method for each value populated into one of its slots. + */ +private class SetterBasedObjectBuilder( + private val constructor: ConstructorCaller, + private val setters: List): ObjectBuilder { private lateinit var target: Any override fun initialize() { - target = constructor.observedMethod.call() + target = constructor.invoke(emptyArray()) } override fun populate(slot: Int, value: Any?) { - properties[slot].observedSetter.invoke(target, value) + setters[slot]?.invoke(target, value) } override fun build(): Any = target } -class ConstructorBasedObjectBuilder( - val constructor: LocalConstructorInformation, - val parameterIndices: IntArray): ObjectBuilder { +/** + * An [ObjectBuilder] which builds an object by collecting the values populated into its slots into a parameter array, + * and calling a constructor with those parameters to obtain the configured object instance. + */ +private class ConstructorBasedObjectBuilder( + private val constructor: ConstructorCaller, + private val parameterIndices: IntArray): ObjectBuilder { - private val params = arrayOfNulls(parameterIndices.size) + private val params = arrayOfNulls(parameterIndices.count { it != IGNORE_COMPUTED }) override fun initialize() {} override fun populate(slot: Int, value: Any?) { - if (slot >= parameterIndices.size) { - assert(false) - } val parameterIndex = parameterIndices[slot] - if (parameterIndex >= params.size) { - assert(false) - } - params[parameterIndex] = value + if (parameterIndex != IGNORE_COMPUTED) params[parameterIndex] = value } - override fun build(): Any = constructor.observedMethod.call(*params) + override fun build(): Any = constructor.invoke(params) } -class EvolutionObjectBuilder(private val localBuilder: ObjectBuilder, val slotAssignments: IntArray): ObjectBuilder { +/** + * An [ObjectBuilder] that wraps an underlying [ObjectBuilder], routing the property values assigned to its slots to the + * matching slots in the underlying builder, and discarding values for which the underlying builder has no slot. + */ +class EvolutionObjectBuilder(private val localBuilder: ObjectBuilder, private val slotAssignments: IntArray): ObjectBuilder { companion object { - fun makeProvider(typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation, localProperties: Map, providedProperties: List): () -> ObjectBuilder { + /** + * Construct an [EvolutionObjectBuilder] for the specified type, constructor and properties, mapping the list of + * properties defined in the remote type into the matching slots on the local type's [ObjectBuilder], and discarding + * any for which there is no matching slot. + */ + fun makeProvider(typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation, localProperties: Map, remoteProperties: List): () -> ObjectBuilder { val localBuilderProvider = ObjectBuilder.makeProvider(typeIdentifier, constructor, localProperties) - val localPropertyIndices = localProperties.asSequence() - .filter { (_, property) -> !property.isCalculated } - .mapIndexed { slot, (name, _) -> name to slot } - .toMap() - val reroutedIndices = providedProperties.map { propertyName -> localPropertyIndices[propertyName] ?: -1 } - .toIntArray() + val reroutedIndices = remoteProperties.map { propertyName -> + localBuilderProvider.propertySlots[propertyName] ?: -1 + }.toIntArray() - return { EvolutionObjectBuilder(localBuilderProvider(), reroutedIndices) } + return { + EvolutionObjectBuilder(localBuilderProvider(), reroutedIndices) + } } } diff --git a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectSerializer.kt b/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectSerializer.kt index 2e17a594c9..ddf2ed1855 100644 --- a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectSerializer.kt +++ b/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectSerializer.kt @@ -138,9 +138,7 @@ class ComposableObjectReader( obj.asSequence().zip(propertySerializers.values.asSequence()) // Read _all_ properties from the stream .map { (item, property) -> property to property.readProperty(item, schemas, input, context) } - // Throw away any calculated properties - .filter { (property, _) -> !property.isCalculated } - // Write the rest into the builder + // Write them into the builder (computed properties will be thrown away) .forEachIndexed { slot, (_, propertyValue) -> builder.populate(slot, propertyValue) } return builder.build() } diff --git a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/custom/ThrowableSerializer.kt b/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/custom/ThrowableSerializer.kt index ba93143cd4..02eaa4e732 100644 --- a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/custom/ThrowableSerializer.kt +++ b/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/custom/ThrowableSerializer.kt @@ -68,7 +68,7 @@ class ThrowableSerializer(factory: LocalSerializerFactory) : CustomSerializer.Pr proxy.additionalProperties[parameter.name] ?: proxy.additionalProperties[parameter.name.capitalize()] } - val throwable = constructor.observedMethod.call(*params.toTypedArray()) + val throwable = constructor.observedMethod.newInstance(*params.toTypedArray()) (throwable as CordaThrowable).apply { if (this.javaClass.name != proxy.exceptionClass) this.originalExceptionClassName = proxy.exceptionClass this.setMessage(proxy.message) diff --git a/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformation.kt b/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformation.kt index 0d4c555269..d6c94853be 100644 --- a/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformation.kt +++ b/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformation.kt @@ -326,7 +326,7 @@ sealed class LocalTypeInformation { * Represents information about a constructor. */ data class LocalConstructorInformation( - val observedMethod: KFunction, + val observedMethod: Constructor, val parameters: List) { val hasParameters: Boolean get() = parameters.isNotEmpty() } diff --git a/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformationBuilder.kt b/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformationBuilder.kt index 986742a7d7..adf3a7dd35 100644 --- a/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformationBuilder.kt +++ b/serialization/src/main/kotlin/net/corda/serialization/internal/model/LocalTypeInformationBuilder.kt @@ -370,13 +370,15 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup, if (observedConstructor.javaConstructor?.parameters?.getOrNull(0)?.name == "this$0") throw NotSerializableException("Type '${type.typeName} has synthetic fields and is likely a nested inner class.") - return LocalConstructorInformation(observedConstructor, observedConstructor.parameters.map { - val parameterType = it.type.javaType - LocalConstructorParameterInformation( - it.name ?: throw IllegalStateException("Unnamed parameter in constructor $observedConstructor"), - resolveAndBuild(parameterType), - parameterType.asClass().isPrimitive || !it.type.isMarkedNullable) - }) + return LocalConstructorInformation( + observedConstructor.javaConstructor!!.apply { isAccessible = true }, + observedConstructor.parameters.map { + val parameterType = it.type.javaType + LocalConstructorParameterInformation( + it.name ?: throw IllegalStateException("Unnamed parameter in constructor $observedConstructor"), + resolveAndBuild(parameterType), + parameterType.asClass().isPrimitive || !it.type.isMarkedNullable) + }) } } diff --git a/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolvabilityTests.kt b/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolvabilityTests.kt index ac35801b27..23b8b3e10b 100644 --- a/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolvabilityTests.kt +++ b/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolvabilityTests.kt @@ -7,6 +7,7 @@ import net.corda.core.node.NetworkParameters import net.corda.core.node.NotaryInfo import net.corda.core.serialization.ConstructorForDeserialization import net.corda.core.serialization.DeprecatedConstructorForDeserialization +import net.corda.core.serialization.SerializableCalculatedProperty import net.corda.core.serialization.SerializedBytes import net.corda.serialization.internal.amqp.testutils.* import net.corda.testing.common.internal.ProjectStructure.projectRootDir @@ -149,6 +150,34 @@ class EvolvabilityTests { assertEquals(D, deserializedCC.d) } + @Suppress("UNUSED_VARIABLE") + @Test + fun removeParameterWithCalculatedParameter() { + val sf = testDefaultFactory() + val resource = "EvolvabilityTests.removeParameterWithCalculatedParameter" + + // Original version of the class as it was serialised + // data class CC(val a: Int, val b: String, val c: String, val d: Int) { + // @get:SerializableCalculatedProperty + // val e: String get() = "$b $c" + // } + // File(URI("$localPath/$resource")).writeBytes(SerializationOutput(sf).serialize(CC(1, "hello", "world", 2)).bytes) + + + data class CC(val b: String, val d: Int) { + @get:SerializableCalculatedProperty + val e: String get() = "$b sailor" + } + + val url = EvolvabilityTests::class.java.getResource(resource) + val sc2 = url.readBytes() + val deserializedCC = DeserializationInput(sf).deserialize(SerializedBytes(sc2)) + + assertEquals("hello", deserializedCC.b) + assertEquals(2, deserializedCC.d) + assertEquals("hello sailor", deserializedCC.e) + } + @Suppress("UNUSED_VARIABLE") @Test fun addAndRemoveParameters() { diff --git a/serialization/src/test/resources/net/corda/serialization/internal/amqp/EvolvabilityTests.removeParameterWithCalculatedParameter b/serialization/src/test/resources/net/corda/serialization/internal/amqp/EvolvabilityTests.removeParameterWithCalculatedParameter new file mode 100644 index 0000000000000000000000000000000000000000..37c717a8a10326d34e7c5eb69839b1e7f0d75082 GIT binary patch literal 455 zcmYe!FG@*dWME)uIGO|`85kHZ0PzMOgPCEmQeJ9_UNT69RbfbQfn{i7W?G?%uS-@@ zzO$QQZe*aX?EwYW5XOb98L2rr`3qUg^NVs)LYNkEgZTsQIW`3StW?o5ZQC?z>USe)xfu3tweok3pQf5wONo7cC zaY?aCQEF~}S!zIHQDSZ?P-S>#NrrP`PI75ZVo7QWTv)}~*})ZVFY^I4CSttla9{!} zTocOy0rrKAi3^#5E^=MSXyD-Jgd`^jluKI3R$Nk)nV0VBfGL@bOA=&i3LaBav1{Z! H$jATyTwaCU literal 0 HcmV?d00001