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

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<String, Int>, 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<Any>): (Array<Any?>) -> Any {
+
+    override fun invoke(parameters: Array<Any?>): 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<String, LocalPropertyInformation>): () -> 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<String, LocalPropertyInformation>): 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<String, LocalPropertyInformation>, 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<LocalPropertyInformation.GetterSetterProperty>()
-            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<String, LocalPropertyInformation>, 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<LocalPropertyInformation.GetterSetterProperty>): 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<SetterCaller?>): 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<Any>(parameterIndices.size)
+    private val params = arrayOfNulls<Any>(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<String, LocalPropertyInformation>, providedProperties: List<String>): () -> 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<String, LocalPropertyInformation>, remoteProperties: List<String>): () -> 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)
+            }
         }
     }
 

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

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)

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<Any>,
+        val observedMethod: Constructor<Any>,
         val parameters: List<LocalConstructorParameterInformation>) {
     val hasParameters: Boolean get() = parameters.isNotEmpty()
 }

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

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<CC>(sc2))
+
+        assertEquals("hello", deserializedCC.b)
+        assertEquals(2, deserializedCC.d)
+        assertEquals("hello sailor", deserializedCC.e)
+    }
+
     @Suppress("UNUSED_VARIABLE")
     @Test
     fun addAndRemoveParameters() {