From 95f062e8ffe323b49739f5f232f3737075f9ec05 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Tue, 6 Feb 2018 12:55:49 +0000 Subject: [PATCH] CORDA-904 - Fix evolver to work with setter instantiated classses (#2463) * CORDA-904 - Make evolver work with classes that use setters * review comments * review comments * small fixs * don't include systemTest in compiler.xml --- .idea/compiler.xml | 2 +- .../serialization/amqp/EvolutionSerializer.kt | 133 +++++++++++++----- .../serialization/amqp/PropertySerializers.kt | 21 +++ .../serialization/amqp/SerializationHelper.kt | 30 ++-- .../serialization/amqp/EvolvabilityTests.kt | 43 +++++- .../EvolvabilityTests.getterSetterEvolver1 | Bin 0 -> 420 bytes 6 files changed, 177 insertions(+), 52 deletions(-) create mode 100644 node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.getterSetterEvolver1 diff --git a/.idea/compiler.xml b/.idea/compiler.xml index a33ce1e0f0..d8d9f1498a 100644 --- a/.idea/compiler.xml +++ b/.idea/compiler.xml @@ -159,4 +159,4 @@ - \ No newline at end of file + 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 78ca924deb..b2dfa16f53 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 @@ -21,12 +21,11 @@ import kotlin.reflect.jvm.javaType * @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 */ -class EvolutionSerializer( +abstract class EvolutionSerializer( clazz: Type, factory: SerializerFactory, - private val oldReaders: Map, - override val kotlinConstructor: KFunction?, - private val constructorArgs: Array) : ObjectSerializer(clazz, factory) { + protected val oldReaders: Map, + override val kotlinConstructor: KFunction?) : ObjectSerializer(clazz, factory) { // explicitly set as empty to indicate it's unused by this type of serializer override val propertySerializers = PropertySerializersEvolution() @@ -35,12 +34,11 @@ class EvolutionSerializer( * Represents a parameter as would be passed to the constructor of the class as it was * when it was serialised and NOT how that class appears now * - * @param type The jvm type of the parameter * @param resultsIndex index into the constructor argument list where the read property * should be placed * @param property object to read the actual property value */ - data class OldParam(val type: Type, var resultsIndex: Int, val property: PropertySerializer) { + 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) { @@ -62,9 +60,10 @@ class EvolutionSerializer( */ private fun getEvolverConstructor(type: Type, oldArgs: Map): KFunction? { val clazz: Class<*> = type.asClass()!! + if (!isConcrete(clazz)) return null - val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.type) } + val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.property.resolvedType) } var maxConstructorVersion = Integer.MIN_VALUE var constructor: KFunction? = null @@ -82,6 +81,36 @@ class EvolutionSerializer( return constructor ?: constructorForDeserialization(type) } + private fun makeWithConstructor( + new: ObjectSerializer, + factory: SerializerFactory, + constructor: KFunction, + readersAsSerialized: Map): AMQPSerializer { + val constructorArgs = arrayOfNulls(constructor.parameters.size) + + constructor.parameters.withIndex().forEach { + readersAsSerialized.get(it.value.name!!)?.apply { + this.resultsIndex = it.index + } ?: if (!it.value.type.isMarkedNullable) { + throw NotSerializableException( + "New parameter ${it.value.name} is mandatory, should be nullable for evolution to worK") + } + } + return EvolutionSerializerViaConstructor (new.type, factory, readersAsSerialized, constructor, constructorArgs) + } + + private fun makeWithSetters( + new: ObjectSerializer, + factory: SerializerFactory, + constructor: KFunction, + readersAsSerialized: Map, + classProperties: Map): AMQPSerializer { + val setters = propertiesForSerializationFromSetters(classProperties, + new.type, + factory).associateBy({ it.getter.name }, { it }) + return EvolutionSerializerViaSetters (new.type, factory, readersAsSerialized, constructor, setters) + } + /** * Build a serialization object for deserialization only of objects serialised * as different versions of a class. @@ -95,46 +124,43 @@ class EvolutionSerializer( */ fun make(old: CompositeType, new: ObjectSerializer, factory: SerializerFactory): AMQPSerializer { - - val readersAsSerialized = linkedMapOf( - *(old.fields.map { - val returnType = try { - it.getTypeAsClass(factory.classloader) - } catch (e: ClassNotFoundException) { - throw NotSerializableException(e.message) - } - - it.name to OldParam( - returnType, - -1, - PropertySerializer.make( - it.name, PublicPropertyReader(null), returnType, factory)) - }.toTypedArray()) - ) - - val constructor = getEvolverConstructor(new.type, readersAsSerialized) ?: - throw NotSerializableException( - "Attempt to deserialize an interface: ${new.type}. Serialized form is invalid.") - - val constructorArgs = arrayOfNulls(constructor.parameters.size) - - constructor.parameters.withIndex().forEach { - readersAsSerialized.get(it.value.name!!)?.apply { - this.resultsIndex = it.index - } ?: if (!it.value.type.isMarkedNullable) { - throw NotSerializableException( - "New parameter ${it.value.name} is mandatory, should be nullable for evolution to worK") + // The order in which the properties were serialised is important and must be preserved + val readersAsSerialized = LinkedHashMap() + old.fields.forEach { + readersAsSerialized[it.name] = try { + OldParam(-1, PropertySerializer.make(it.name, EvolutionPropertyReader(), + it.getTypeAsClass(factory.classloader), factory)) + } catch (e: ClassNotFoundException) { + throw NotSerializableException(e.message) } } - return EvolutionSerializer(new.type, factory, readersAsSerialized, constructor, constructorArgs) + // cope with the situation where a generic interface was serialised as a type, in such cases + // return the synthesised object which is, given the absence of a constructor, a no op + val constructor = getEvolverConstructor(new.type, readersAsSerialized) ?: return new + + val classProperties = new.type.asClass()?.propertyDescriptors() ?: emptyMap() + + return if (classProperties.isNotEmpty() && constructor.parameters.isEmpty()) { + makeWithSetters(new, factory, constructor, readersAsSerialized, classProperties) + } + else { + makeWithConstructor(new, factory, constructor, readersAsSerialized) + } } } - override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput, offset: Int) { + override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput, debugIndent: Int) { throw UnsupportedOperationException("It should be impossible to write an evolution serializer") } +} +class EvolutionSerializerViaConstructor( + clazz: Type, + factory: SerializerFactory, + oldReaders: Map, + kotlinConstructor: KFunction?, + private val constructorArgs: Array) : EvolutionSerializer (clazz, factory, oldReaders, kotlinConstructor) { /** * Unlike a normal [readObject] call where we simply apply the parameter deserialisers * to the object list of values we need to map that list, which is ordered per the @@ -151,7 +177,36 @@ class EvolutionSerializer( oldReaders.values.zip(obj).map { it.first.readProperty(it.second, schemas, input, constructorArgs) } return javaConstructor?.newInstance(*(constructorArgs)) ?: - throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.") + throw NotSerializableException( + "Attempt to deserialize an interface: $clazz. Serialized form is invalid.") + } +} + +/** + * Specific instance of an [EvolutionSerializer] where the properties of the object are set via calling + * named setter functions on the instantiated object. + */ +class EvolutionSerializerViaSetters( + clazz: Type, + factory: SerializerFactory, + oldReaders: Map, + kotlinConstructor: KFunction?, + private val setters: Map) : EvolutionSerializer (clazz, factory, oldReaders, kotlinConstructor) { + + override fun readObject(obj: Any, schemas: SerializationSchemas, input: DeserializationInput): Any { + if (obj !is List<*>) throw NotSerializableException("Body of described type is unexpected $obj") + + val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException ( + "Failed to instantiate instance of object $clazz") + + // *must* read all the parameters in the order they were serialized + oldReaders.values.zip(obj).forEach { + // if that property still exists on the new object then set it + it.first.property.readProperty(it.second, schemas, input).apply { + setters[it.first.property.name]?.set(instance, this) + } + } + return instance } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt index eaf0433fb3..9029dd171b 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt @@ -14,6 +14,9 @@ abstract class PropertyReader { abstract fun isNullable(): Boolean } +/** + * Accessor for those properties of a class that have defined getter functions. + */ class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() { init { readMethod?.isAccessible = true @@ -43,6 +46,10 @@ class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() { override fun isNullable(): Boolean = readMethod?.returnsNullable() ?: false } +/** + * Accessor for those properties of a class that do not have defined getter functions. In which case + * we used reflection to remove the unreadable status from that property whilst it's accessed. + */ class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader() { init { loggerFor().warn("Create property Serializer for private property '${field.name}' not " @@ -68,6 +75,20 @@ class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader } } +/** + * Special instance of a [PropertyReader] for use only by [EvolutionSerializer]s to make + * it explicit that no properties are ever actually read from an object as the evolution + * serializer should only be accessing the already serialized form. + */ +class EvolutionPropertyReader : PropertyReader() { + override fun read(obj: Any?): Any? { + throw UnsupportedOperationException("It should be impossible for an evolution serializer to " + + "be reading from an object") + } + + override fun isNullable() = true +} + /** * Represents a generic interface to a serializable property of an object. * diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt index 6763e79c9f..4ce172cc78 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt @@ -98,7 +98,6 @@ data class PropertyDescriptor(var field: Field?, var setter: Method?, var getter }.toString() constructor() : this(null, null, null, null) - constructor(field: Field?) : this(field, null, null, null) fun preferredGetter() : Method? = getter ?: iser } @@ -128,9 +127,16 @@ fun Class.propertyDescriptors(): Map { val classProperties = mutableMapOf() var clazz: Class? = this - clazz!!.declaredFields.forEach { classProperties.put(it.name, PropertyDescriptor(it)) } do { + clazz!!.declaredFields.forEach { property -> + classProperties.computeIfAbsent(property.name) { + PropertyDescriptor() + }.apply { + this.field = property + } + } + // Note: It is possible for a class to have multiple instances of a function where the types // differ. For example: // interface I { val a: T } @@ -142,7 +148,7 @@ fun Class.propertyDescriptors(): Map { // // In addition, only getters that take zero parameters and setters that take a single // parameter will be considered - clazz!!.declaredMethods?.map { func -> + clazz.declaredMethods?.map { func -> if (!Modifier.isPublic(func.modifiers)) return@map if (func.name == "getClass") return@map @@ -231,15 +237,13 @@ internal fun propertiesForSerializationFromConstructor( Pair(PublicPropertyReader(getter), returnType) } else { - try { - val field = clazz.getDeclaredField(param.value.name) - Pair(PrivatePropertyReader(field, type), field.genericType) - } catch (e: NoSuchFieldException) { + val field = classProperties[name]!!.field ?: throw NotSerializableException("No property matching constructor parameter named '$name' " + "of '$clazz'. If using Java, check that you have the -parameters option specified " + "in the Java compiler. Alternately, provide a proxy serializer " + "(SerializationCustomSerializer) if recompiling isn't an option") - } + + Pair(PrivatePropertyReader(field, type), field.genericType) } } else { throw NotSerializableException( @@ -257,12 +261,13 @@ internal fun propertiesForSerializationFromConstructor( * If we determine a class has a constructor that takes no parameters then check for pairs of getters / setters * and use those */ -private fun propertiesForSerializationFromSetters( +fun propertiesForSerializationFromSetters( properties: Map, type: Type, factory: SerializerFactory): List { return mutableListOf().apply { var idx = 0 + properties.forEach { property -> val getter: Method? = property.value.preferredGetter() val setter: Method? = property.value.setter @@ -276,7 +281,8 @@ private fun propertiesForSerializationFromSetters( val setterType = setter.parameterTypes[0]!! - if (!(TypeToken.of(property.value.field?.genericType!!).isSupertypeOf(setterType))) { + if ((property.value.field != null) && + (!(TypeToken.of(property.value.field?.genericType!!).isSupertypeOf(setterType)))) { throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + "takes parameter of type $setterType yet underlying type is " + "${property.value.field?.genericType!!}") @@ -290,7 +296,7 @@ private fun propertiesForSerializationFromSetters( } this += PropertyAccessorGetterSetter( idx++, - PropertySerializer.make(property.value.field!!.name, PublicPropertyReader(getter), + PropertySerializer.make(property.key, PublicPropertyReader(getter), resolveTypeVariables(getter.genericReturnType, type), factory), setter) } @@ -322,6 +328,8 @@ private fun propertiesForSerializationFromAbstract( return mutableListOf().apply { properties.toList().withIndex().forEach { val getter = it.value.second.getter ?: return@forEach + if (it.value.second.field == null) return@forEach + val returnType = resolveTypeVariables(getter.genericReturnType, type) this += PropertyAccessorConstructor( it.index, 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 b18abd9f82..0fce099955 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 @@ -538,4 +538,45 @@ class EvolvabilityTests { File(URI("$localPath/$resource")).writeBytes( signedAndSerialized.bytes) } -} \ No newline at end of file + @Suppress("UNCHECKED_CAST") + @Test + fun getterSetterEvolver1() { + val resource = "EvolvabilityTests.getterSetterEvolver1" + val sf = testDefaultFactory() + + // + // Class as it was serialised + // + // data class C(var c: Int, var d: Int, var b: Int, var e: Int, var a: Int) { + // // This will force the serialization engine to use getter / setter + // // instantiation for the object rather than construction + // @ConstructorForDeserialization + // @Suppress("UNUSED") + // constructor() : this(0, 0, 0, 0, 0) + // } + // + // File(URI("$localPath/$resource")).writeBytes(SerializationOutput(sf).serialize(C(3,4,2,5,1)).bytes) + + // + // Class as it exists now, c has been removed + // + data class C(var d: Int, var b: Int, var e: Int, var a: Int) { + // This will force the serialization engine to use getter / setter + // instantiation for the object rather than construction + @ConstructorForDeserialization + @Suppress("UNUSED") + constructor() : this(0, 0, 0, 0) + } + + val path = EvolvabilityTests::class.java.getResource(resource) + val f = File(path.toURI()) + + val sc2 = f.readBytes() + val deserializedC = DeserializationInput(sf).deserialize(SerializedBytes(sc2)) + + assertEquals(1, deserializedC.a) + assertEquals(2, deserializedC.b) + assertEquals(4, deserializedC.d) + assertEquals(5, deserializedC.e) + } +} diff --git a/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.getterSetterEvolver1 b/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.getterSetterEvolver1 new file mode 100644 index 0000000000000000000000000000000000000000..c544432e6d33b7a2ff606f99b0702636cf8477d0 GIT binary patch literal 420 zcmYe!FG@*dWME)uIGO|`85kHZ0C6vn!OXB&DKE7|FBzo5Dl%9ZVS%1&S$-iN_uJu zP