From cf9fbc715d18d4b840acc117e0dd28986870e6e0 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Thu, 24 Aug 2017 11:21:30 +0100 Subject: [PATCH] Review Comments Refactor small bits of the serialiser, move things out of the read methods and into the make factory companion object --- ...tor.kt => EvolvedSerializerConstructor.kt} | 2 +- .../serialization/amqp/EvolutionSerializer.kt | 29 +++++++++-------- .../serialization/amqp/ObjectSerializer.kt | 8 +++-- .../serialization/amqp/SerializerFactory.kt | 4 +-- .../serialization/amqp/EvolvabilityTests.kt | 31 ++++++++++++------- 5 files changed, 43 insertions(+), 31 deletions(-) rename core/src/main/kotlin/net/corda/core/serialization/{CordaSerializerConstructor.kt => EvolvedSerializerConstructor.kt} (82%) diff --git a/core/src/main/kotlin/net/corda/core/serialization/CordaSerializerConstructor.kt b/core/src/main/kotlin/net/corda/core/serialization/EvolvedSerializerConstructor.kt similarity index 82% rename from core/src/main/kotlin/net/corda/core/serialization/CordaSerializerConstructor.kt rename to core/src/main/kotlin/net/corda/core/serialization/EvolvedSerializerConstructor.kt index a84b46741e..07978d1840 100644 --- a/core/src/main/kotlin/net/corda/core/serialization/CordaSerializerConstructor.kt +++ b/core/src/main/kotlin/net/corda/core/serialization/EvolvedSerializerConstructor.kt @@ -6,6 +6,6 @@ package net.corda.core.serialization */ @Target(AnnotationTarget.CONSTRUCTOR) @Retention(AnnotationRetention.RUNTIME) -annotation class CordaSerializerConstructor(val version: Int) +annotation class EvolvedSerializerConstructor(val version: Int) 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 fe77f9540a..98c51fbdf7 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 @@ -1,6 +1,6 @@ package net.corda.nodeapi.internal.serialization.amqp -import net.corda.core.serialization.CordaSerializerConstructor +import net.corda.core.serialization.EvolvedSerializerConstructor import net.corda.nodeapi.internal.serialization.carpenter.getTypeAsClass import org.apache.qpid.proton.codec.Data import java.lang.reflect.Type @@ -16,7 +16,8 @@ import kotlin.reflect.jvm.javaType class EvolutionSerializer( clazz: Type, factory: SerializerFactory, - val oldParams : Map, + val oldParams : Map, + val newParams : Map, override val kotlinConstructor: KFunction?) : ObjectSerializer (clazz, factory) { // explicitly null this out as we won't be using this list @@ -31,7 +32,10 @@ class EvolutionSerializer( * order may have been changed and we need to know where into the list to look * @param property object to read the actual property value */ - data class oldParam (val type: Type, val idx: Int, val property: PropertySerializer) + data class OldParam (val type: Type, val idx: Int, val property: PropertySerializer) { + fun readProperty(paramValues: List<*>, schema: Schema, input: DeserializationInput) = + property.readProperty(paramValues[idx], schema, input) + } companion object { /** @@ -53,7 +57,7 @@ class EvolutionSerializer( var maxConstructorVersion = Integer.MIN_VALUE var constructor: KFunction? = null clazz.kotlin.constructors.forEach { - val version = it.findAnnotation()?.version ?: Integer.MIN_VALUE + val version = it.findAnnotation()?.version ?: Integer.MIN_VALUE if (oldArgumentSet.containsAll(it.parameters.map { v -> Pair(v.name, v.type.javaType) }) && version > maxConstructorVersion) { constructor = it @@ -67,7 +71,7 @@ class EvolutionSerializer( } /** - * Build a serialization object for deserialisation only of objects serislaised + * Build a serialization object for deserialisation only of objects serialised * as different versions of a class * * @param old is an object holding the schema that represents the object @@ -86,15 +90,17 @@ class EvolutionSerializer( throw NotSerializableException( "Attempt to deserialize an interface: new.type. Serialized form is invalid.") - val oldArgs = mutableMapOf() + val oldArgs = mutableMapOf() var idx = 0 old.fields.forEach { val returnType = it.getTypeAsClass(factory.classloader) - oldArgs[it.name] = oldParam( + oldArgs[it.name] = OldParam( returnType, idx++, PropertySerializer.make(it.name, null, returnType, factory)) } - return EvolutionSerializer(new.type, factory, oldArgs, constructor) + val newArgs = constructor.parameters.associateBy({ it.name!! }, {it.type.isMarkedNullable}) + + return EvolutionSerializer(new.type, factory, oldArgs, newArgs, constructor) } } @@ -112,17 +118,14 @@ class EvolutionSerializer( override fun readObject(obj: Any, schema: Schema, input: DeserializationInput): Any { if (obj !is List<*>) throw NotSerializableException("Body of described type is unexpected $obj") - val newArgs = kotlinConstructor?.parameters?.associateBy({ it.name!! }, {it.type.isMarkedNullable}) ?: - throw NotSerializableException ("Bad Constructor selected for object $obj") - - return construct(newArgs.map { + return construct(newParams.map { val param = oldParams[it.key] if (param == null && !it.value) { throw NotSerializableException( "New parameter ${it.key} is mandatory, should be nullable for evolution to worK") } - param?.property?.readProperty(obj[param.idx], schema, input) + param?.readProperty(obj, schema, input) }) } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt index 120d315f52..da29d36e45 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt @@ -15,6 +15,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS override val type: Type get() = clazz open internal val propertySerializers: Collection open val kotlinConstructor = constructorForDeserialization(clazz) + val javaConstructor by lazy { kotlinConstructor?.javaConstructor } init { propertySerializers = propertiesForSerialization(kotlinConstructor, clazz, factory) @@ -71,9 +72,10 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS fun construct(properties: List): Any { - val javaConstructor = kotlinConstructor?.javaConstructor ?: + if (javaConstructor == null) { throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.") + } - return javaConstructor.newInstance(*properties.toTypedArray()) + return javaConstructor!!.newInstance(*properties.toTypedArray()) } -} +} \ No newline at end of file diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt index 6d3f8dcbf9..c0b8b19c3f 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt @@ -191,8 +191,8 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) { val serialiser = processSchemaEntry(typeNotation) // if we just successfully built a serialiser for the type but the type fingerprint - // doesn't match that of the serialised object then we are dealing with an older - // instance of the class, as such we need to build an evolverSerilaiser + // doesn't match that of the serialised object then we are dealing with different + // instance of the class, as such we need to build an EvolutionSerialiser if (serialiser.typeDescriptor != typeNotation.descriptor.name) { getEvolutionSerializer(typeNotation, serialiser as ObjectSerializer) } 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 a11e969821..2fb47af7db 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 @@ -1,13 +1,20 @@ package net.corda.nodeapi.internal.serialization.amqp import net.corda.core.serialization.SerializedBytes -import net.corda.core.serialization.CordaSerializerConstructor +import net.corda.core.serialization.EvolvedSerializerConstructor import org.junit.Test import java.io.File import java.io.NotSerializableException import kotlin.test.assertEquals +// To regenerate any of the binary test files do the following +// +// 1. Uncomment the code where the original form of the class is defined in the test +// 2. Comment out the rest of the test +// 3. Run the test +// 4. Using the printed path copy that file to the resources directory +// 5. Comment back out the generation code and uncomment the actual test class EvolvabilityTests { @Test @@ -104,7 +111,7 @@ class EvolvabilityTests { // Expected to throw as we can't construct the new type as it contains a newly // added parameter that isn't optional, i.e. not nullable and there isn't - // a compiler that takes the old parameters + // a constructor that takes the old parameters DeserializationInput(sf).deserialize(SerializedBytes(sc2)) } @@ -180,7 +187,7 @@ class EvolvabilityTests { @Suppress("UNUSED") data class CC (val a: Int, val b: String) { - @CordaSerializerConstructor(1) + @EvolvedSerializerConstructor(1) constructor (a: Int) : this (a, "hello") } @@ -239,7 +246,7 @@ class EvolvabilityTests { data class CC (val a: Int, val b: Int, val c: String, val d: String) { // ensure none of the original parameters align with the initial // construction order - @CordaSerializerConstructor(1) + @EvolvedSerializerConstructor(1) constructor (c: String, a: Int, b: Int) : this (a, b, c, "wibble") } @@ -275,7 +282,7 @@ class EvolvabilityTests { // ensure none of the original parameters align with the initial // construction order @Suppress("UNUSED") - @CordaSerializerConstructor(1) + @EvolvedSerializerConstructor(1) constructor (c: String, a: Int) : this (a, c, "wibble") } @@ -317,11 +324,11 @@ class EvolvabilityTests { @Suppress("UNUSED") data class C (val e: Int, val c: Int, val b: Int, val a: Int, val d: Int) { - @CordaSerializerConstructor(1) + @EvolvedSerializerConstructor(1) constructor (b: Int, a: Int) : this (-1, -1, b, a, -1) - @CordaSerializerConstructor(2) + @EvolvedSerializerConstructor(2) constructor (a: Int, c: Int, b: Int) : this (-1, c, b, a, -1) - @CordaSerializerConstructor(3) + @EvolvedSerializerConstructor(3) constructor (a: Int, b: Int, c: Int, d: Int) : this (-1, c, b, a, d) } @@ -411,13 +418,13 @@ class EvolvabilityTests { @Suppress("UNUSED") data class C (val b: Int, val c: Int, val d: Int, val e: Int, val f: Int, val g: Int) { - @CordaSerializerConstructor(1) + @EvolvedSerializerConstructor(1) constructor (b: Int, c: Int) : this (b, c, -1, -1, -1, -1) - @CordaSerializerConstructor(2) + @EvolvedSerializerConstructor(2) constructor (b: Int, c: Int, d: Int) : this (b, c, d, -1, -1, -1) - @CordaSerializerConstructor(3) + @EvolvedSerializerConstructor(3) constructor (b: Int, c: Int, d: Int, e: Int) : this (b, c, d, e, -1, -1) - @CordaSerializerConstructor(4) + @EvolvedSerializerConstructor(4) constructor (b: Int, c: Int, d: Int, e: Int, f: Int) : this (b, c, d, e, f, -1) }