From 0e047d9263e482dc1cfbe87ce1b9a2ec892dd77d Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Thu, 11 Jan 2018 20:11:51 +0000 Subject: [PATCH] CORDA-914 - Deterministic property ordering for AMQP serialization --- .../amqp/CorDappCustomSerializer.kt | 4 +- .../serialization/amqp/CustomSerializer.kt | 13 +- .../serialization/amqp/EvolutionSerializer.kt | 2 +- .../serialization/amqp/ObjectSerializer.kt | 51 +++-- .../serialization/amqp/PropertySerializer.kt | 67 +------ .../serialization/amqp/PropertySerializers.kt | 179 ++++++++++++++++++ .../internal/serialization/amqp/Schema.kt | 9 +- .../serialization/amqp/SerializationHelper.kt | 134 +++++++------ .../amqp/custom/ThrowableSerializer.kt | 7 +- .../amqp/JavaPrivatePropertyTests.java | 12 +- .../amqp/PrivatePropertyTests.kt | 38 +++- .../amqp/SerializationPropertyOrdering.kt | 125 ++++++++++++ 12 files changed, 465 insertions(+), 176 deletions(-) create mode 100644 node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt create mode 100644 node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationPropertyOrdering.kt diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt index 8e5e249457..003da13268 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt @@ -70,8 +70,8 @@ class CorDappCustomSerializer( data.withDescribed(descriptor) { data.withList { - for (property in proxySerializer.propertySerializers.getters) { - property.writeProperty(proxy, this, output) + proxySerializer.propertySerializers.serializationOrder.forEach { + it.getter.writeProperty(proxy, this, output) } } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt index 647da2fd05..fc0891e93a 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt @@ -61,7 +61,14 @@ abstract class CustomSerializer : AMQPSerializer, SerializerFor { override fun isSerializerFor(clazz: Class<*>): Boolean = clazz == this.clazz override val type: Type get() = clazz override val typeDescriptor = Symbol.valueOf("$DESCRIPTOR_DOMAIN:${fingerprintForDescriptors(superClassSerializer.typeDescriptor.toString(), nameForType(clazz))}") - private val typeNotation: TypeNotation = RestrictedType(SerializerFactory.nameForType(clazz), null, emptyList(), SerializerFactory.nameForType(superClassSerializer.type), Descriptor(typeDescriptor), emptyList()) + private val typeNotation: TypeNotation = RestrictedType( + SerializerFactory.nameForType(clazz), + null, + emptyList(), + SerializerFactory.nameForType(superClassSerializer.type), + Descriptor(typeDescriptor), + emptyList()) + override fun writeClassInfo(output: SerializationOutput) { output.writeTypeNotations(typeNotation) } @@ -132,8 +139,8 @@ abstract class CustomSerializer : AMQPSerializer, SerializerFor { override fun writeDescribedObject(obj: T, data: Data, type: Type, output: SerializationOutput) { val proxy = toProxy(obj) data.withList { - for (property in proxySerializer.propertySerializers.getters) { - property.writeProperty(proxy, this, output) + proxySerializer.propertySerializers.serializationOrder.forEach { + it.getter.writeProperty(proxy, this, output) } } } 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 7335db0fa3..e3357b84d9 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 @@ -20,7 +20,7 @@ class EvolutionSerializer( override val kotlinConstructor: KFunction?) : ObjectSerializer(clazz, factory) { // explicitly set as empty to indicate it's unused by this type of serializer - override val propertySerializers = ConstructorDestructorMethods (emptyList(), emptyList()) + override val propertySerializers = PropertySerializersEvolution() /** * Represents a parameter as would be passed to the constructor of the class as it was 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 a9d099b2e3..034d4b7f93 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 @@ -11,7 +11,8 @@ import java.lang.reflect.Type import kotlin.reflect.jvm.javaConstructor /** - * Responsible for serializing and deserializing a regular object instance via a series of properties (matched with a constructor). + * Responsible for serializing and deserializing a regular object instance via a series of properties + * (matched with a constructor). */ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPSerializer { override val type: Type get() = clazz @@ -22,7 +23,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS private val logger = contextLogger() } - open internal val propertySerializers: ConstructorDestructorMethods by lazy { + open internal val propertySerializers: PropertySerializers by lazy { propertiesForSerialization(kotlinConstructor, clazz, factory) } @@ -31,17 +32,22 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS private val typeName = nameForType(clazz) override val typeDescriptor = Symbol.valueOf("$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}") - private val interfaces = interfacesForSerialization(clazz, factory) // We restrict to only those annotated or whitelisted - open internal val typeNotation: TypeNotation by lazy { CompositeType(typeName, null, generateProvides(), Descriptor(typeDescriptor), generateFields()) } + // We restrict to only those annotated or whitelisted + private val interfaces = interfacesForSerialization(clazz, factory) + + open internal val typeNotation: TypeNotation by lazy { + CompositeType(typeName, null, generateProvides(), Descriptor(typeDescriptor), generateFields()) + } override fun writeClassInfo(output: SerializationOutput) { if (output.writeTypeNotations(typeNotation)) { for (iface in interfaces) { output.requireSerializer(iface) } - for (property in propertySerializers.getters) { - property.writeClassInfo(output) + + propertySerializers.serializationOrder.forEach { property -> + property.getter.writeClassInfo(output) } } } @@ -51,8 +57,8 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS data.withDescribed(typeNotation.descriptor) { // Write list withList { - for (property in propertySerializers.getters) { - property.writeProperty(obj, this, output) + propertySerializers.serializationOrder.forEach { property -> + property.getter.writeProperty(obj, this, output) } } } @@ -63,16 +69,18 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS schemas: SerializationSchemas, input: DeserializationInput): Any = ifThrowsAppend({ clazz.typeName }) { if (obj is List<*>) { - if (obj.size > propertySerializers.getters.size) { + if (obj.size > propertySerializers.size) { throw NotSerializableException("Too many properties in described type $typeName") } - return if (propertySerializers.setters.isEmpty()) { + return if (propertySerializers.byConstructor) { readObjectBuildViaConstructor(obj, schemas, input) } else { readObjectBuildViaSetters(obj, schemas, input) } - } else throw NotSerializableException("Body of described type is unexpected $obj") + } else { + throw NotSerializableException("Body of described type is unexpected $obj") + } } private fun readObjectBuildViaConstructor( @@ -81,7 +89,11 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ logger.trace { "Calling construction based construction for ${clazz.typeName}" } - return construct(obj.zip(propertySerializers.getters).map { it.second.readProperty(it.first, schemas, input) }) + return construct (propertySerializers.serializationOrder + .zip(obj) + .map { Pair(it.first.initialPosition, it.first.getter.readProperty(it.second, schemas, input)) } + .sortedWith(compareBy({it.first})) + .map { it.second }) } private fun readObjectBuildViaSetters( @@ -93,22 +105,23 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException ( "Failed to instantiate instance of object $clazz") - // read the properties out of the serialised form + // read the properties out of the serialised form, since we're invoking the setters the order we + // do it in doesn't matter val propertiesFromBlob = obj - .zip(propertySerializers.getters) - .map { it.second.readProperty(it.first, schemas, input) } + .zip(propertySerializers.serializationOrder) + .map { it.second.getter.readProperty(it.first, schemas, input) } // one by one take a property and invoke the setter on the class - propertySerializers.setters.zip(propertiesFromBlob).forEach { - it.first?.invoke(instance, *listOf(it.second).toTypedArray()) + propertySerializers.serializationOrder.zip(propertiesFromBlob).forEach { + it.first.set(instance, it.second) } return instance } private fun generateFields(): List { - return propertySerializers.getters.map { - Field(it.name, it.type, it.requires, it.default, null, it.mandatory, false) + return propertySerializers.serializationOrder.map { + Field(it.getter.name, it.getter.type, it.getter.requires, it.getter.default, null, it.getter.mandatory, false) } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt index af7c089552..f749be0ca8 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt @@ -1,69 +1,8 @@ package net.corda.nodeapi.internal.serialization.amqp -import net.corda.core.utilities.loggerFor import org.apache.qpid.proton.amqp.Binary import org.apache.qpid.proton.codec.Data -import java.lang.reflect.Method import java.lang.reflect.Type -import java.lang.reflect.Field -import kotlin.reflect.full.memberProperties -import kotlin.reflect.jvm.javaGetter -import kotlin.reflect.jvm.kotlinProperty - -abstract class PropertyReader { - abstract fun read(obj: Any?): Any? - abstract fun isNullable(): Boolean -} - -class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() { - init { - readMethod?.isAccessible = true - } - - private fun Method.returnsNullable(): Boolean { - try { - val returnTypeString = this.declaringClass.kotlin.memberProperties.firstOrNull { it.javaGetter == this }?.returnType?.toString() ?: "?" - return returnTypeString.endsWith('?') || returnTypeString.endsWith('!') - } catch (e: kotlin.reflect.jvm.internal.KotlinReflectionInternalError) { - // This might happen for some types, e.g. kotlin.Throwable? - the root cause of the issue is: https://youtrack.jetbrains.com/issue/KT-13077 - // TODO: Revisit this when Kotlin issue is fixed. - - loggerFor().error("Unexpected internal Kotlin error", e) - return true - } - } - - override fun read(obj: Any?): Any? { - return readMethod!!.invoke(obj) - } - - override fun isNullable(): Boolean = readMethod?.returnsNullable() ?: false -} - -class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader() { - init { - loggerFor().warn("Create property Serializer for private property '${field.name}' not " - + "exposed by a getter on class '$parentType'\n" - + "\tNOTE: This behaviour will be deprecated at some point in the future and a getter required") - } - - override fun read(obj: Any?): Any? { - field.isAccessible = true - val rtn = field.get(obj) - field.isAccessible = false - return rtn - } - - override fun isNullable() = try { - field.kotlinProperty?.returnType?.isMarkedNullable ?: false - } catch (e: kotlin.reflect.jvm.internal.KotlinReflectionInternalError) { - // This might happen for some types, e.g. kotlin.Throwable? - the root cause of the issue is: https://youtrack.jetbrains.com/issue/KT-13077 - // TODO: Revisit this when Kotlin issue is fixed. - loggerFor().error("Unexpected internal Kotlin error", e) - true - } -} - /** * Base class for serialization of a property of an object. @@ -106,13 +45,13 @@ sealed class PropertySerializer(val name: String, val propertyReader: PropertyRe companion object { fun make(name: String, readMethod: PropertyReader, resolvedType: Type, factory: SerializerFactory): PropertySerializer { - if (SerializerFactory.isPrimitive(resolvedType)) { - return when (resolvedType) { + return if (SerializerFactory.isPrimitive(resolvedType)) { + when (resolvedType) { Char::class.java, Character::class.java -> AMQPCharPropertySerializer(name, readMethod) else -> AMQPPrimitivePropertySerializer(name, readMethod, resolvedType) } } else { - return DescribedTypePropertySerializer(name, readMethod, resolvedType) { factory.get(null, resolvedType) } + DescribedTypePropertySerializer(name, readMethod, resolvedType) { factory.get(null, resolvedType) } } } } 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 new file mode 100644 index 0000000000..537912a044 --- /dev/null +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt @@ -0,0 +1,179 @@ +package net.corda.nodeapi.internal.serialization.amqp + +import net.corda.core.utilities.loggerFor +import java.io.NotSerializableException +import java.lang.reflect.Method +import java.lang.reflect.Type +import kotlin.reflect.full.memberProperties +import kotlin.reflect.jvm.javaGetter +import kotlin.reflect.jvm.kotlinProperty +import java.lang.reflect.Field + +abstract class PropertyReader { + abstract fun read(obj: Any?): Any? + abstract fun isNullable(): Boolean +} + +class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() { + init { + readMethod?.isAccessible = true + } + + private fun Method.returnsNullable(): Boolean { + try { + val returnTypeString = this.declaringClass.kotlin.memberProperties.firstOrNull { + it.javaGetter == this + }?.returnType?.toString() ?: "?" + + return returnTypeString.endsWith('?') || returnTypeString.endsWith('!') + } catch (e: kotlin.reflect.jvm.internal.KotlinReflectionInternalError) { + // This might happen for some types, e.g. kotlin.Throwable? - the root cause of the issue + // is: https://youtrack.jetbrains.com/issue/KT-13077 + // TODO: Revisit this when Kotlin issue is fixed. + + loggerFor().error("Unexpected internal Kotlin error", e) + return true + } + } + + override fun read(obj: Any?): Any? { + return readMethod!!.invoke(obj) + } + + override fun isNullable(): Boolean = readMethod?.returnsNullable() ?: false +} + +class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader() { + init { + loggerFor().warn("Create property Serializer for private property '${field.name}' not " + + "exposed by a getter on class '$parentType'\n" + + "\tNOTE: This behaviour will be deprecated at some point in the future and a getter required") + } + + override fun read(obj: Any?): Any? { + field.isAccessible = true + val rtn = field.get(obj) + field.isAccessible = false + return rtn + } + + override fun isNullable() = try { + field.kotlinProperty?.returnType?.isMarkedNullable ?: false + } catch (e: kotlin.reflect.jvm.internal.KotlinReflectionInternalError) { + // This might happen for some types, e.g. kotlin.Throwable? - the root cause of the issue + // is: https://youtrack.jetbrains.com/issue/KT-13077 + // TODO: Revisit this when Kotlin issue is fixed. + loggerFor().error("Unexpected internal Kotlin error", e) + true + } +} + +/** + * Represents a generic interface to a serializable property of an object. + * + * @property initialPosition where in the constructor used for serialization the property occurs. + * @property getter a [PropertySerializer] wrapping access to the property. This will either be a + * method invocation on the getter or, if not publicly accessible, reflection based by temporally + * making the property accessible. + */ +abstract class PropertyAccessor( + val initialPosition: Int, + open val getter: PropertySerializer) { + companion object : Comparator { + override fun compare(p0: PropertyAccessor?, p1: PropertyAccessor?): Int { + return p0?.getter?.name?.compareTo(p1?.getter?.name ?: "") ?: 0 + } + } + + /** + * Override to control how the property is set on the object. + */ + abstract fun set(instance: Any, obj: Any?) + + override fun toString(): String { + return "${getter.name}($initialPosition)" + } +} + +/** + * Implementation of [PropertyAccessor] representing a property of an object that + * is serialized and deserialized via JavaBean getter and setter style methods. + */ +class PropertyAccessorGetterSetter( + initialPosition: Int, + getter: PropertySerializer, + private val setter: Method?) : PropertyAccessor(initialPosition, getter) { + /** + * Invokes the setter on the underlying object passing in the serialized value. + */ + override fun set(instance: Any, obj: Any?) { + setter?.invoke(instance, *listOf(obj).toTypedArray()) + } +} + +/** + * Implementation of [PropertyAccessor] representing a property of an object that + * is serialized via a JavaBean getter but deserialized using the constructor + * of the object the property belongs to. + */ +class PropertyAccessorConstructor( + initialPosition: Int, + override val getter: PropertySerializer) : PropertyAccessor(initialPosition, getter) { + /** + * Because the property should be being set on the obejct through the constructor any + * calls to the explicit setter should be an error. + */ + override fun set(instance: Any, obj: Any?) { + NotSerializableException ("Attempting to access a setter on an object being instantiated " + + "via its constructor.") + } +} + +/** + * Represents a collection of [PropertyAccessor]s that represent the serialized form + * of an object. + * + * @property serializationOrder a list of [PropertyAccessor]. For deterministic serialization + * should be sorted. + * @property size how many properties are being serialized. + * @property byConstructor are the properties of the class represented by this set of properties populated + * on deserialization via the object's constructor or the corresponding setter functions. Should be + * overridden and set appropriately by child types. + */ +abstract class PropertySerializers( + val serializationOrder: List) { + companion object { + fun make(serializationOrder: List) = + when (serializationOrder.firstOrNull()) { + is PropertyAccessorConstructor -> PropertySerializersConstructor(serializationOrder) + is PropertyAccessorGetterSetter -> PropertySerializersSetter(serializationOrder) + null -> PropertySerializersNoProperties() + else -> { + throw NotSerializableException ("Unknown Property Accessor type, cannot create set") + } + } + } + + val size get() = serializationOrder.size + abstract val byConstructor: Boolean +} + +class PropertySerializersNoProperties : PropertySerializers (emptyList()) { + override val byConstructor get() = true +} + +class PropertySerializersConstructor( + serializationOrder: List) : PropertySerializers(serializationOrder) { + override val byConstructor get() = true +} + +class PropertySerializersSetter( + serializationOrder: List) : PropertySerializers(serializationOrder) { + override val byConstructor get() = false +} + +class PropertySerializersEvolution : PropertySerializers(emptyList()) { + override val byConstructor get() = false +} + + diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt index e79336a731..596d04a2be 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt @@ -446,11 +446,12 @@ private fun fingerprintForObject( offset: Int = 0): Hasher { // Hash the class + properties + interfaces val name = type.asClass()?.name ?: throw NotSerializableException("Expected only Class or ParameterizedType but found $type") - propertiesForSerialization(constructorForDeserialization(type), contextType ?: type, factory).getters + propertiesForSerialization(constructorForDeserialization(type), contextType ?: type, factory) + .serializationOrder .fold(hasher.putUnencodedChars(name)) { orig, prop -> - fingerprintForType(prop.resolvedType, type, alreadySeen, orig, factory, offset+4) - .putUnencodedChars(prop.name) - .putUnencodedChars(if (prop.mandatory) NOT_NULLABLE_HASH else NULLABLE_HASH) + fingerprintForType(prop.getter.resolvedType, type, alreadySeen, orig, factory, offset+4) + .putUnencodedChars(prop.getter.name) + .putUnencodedChars(if (prop.getter.mandatory) NOT_NULLABLE_HASH else NULLABLE_HASH) } interfacesForSerialization(type, factory).map { fingerprintForType(it, type, alreadySeen, hasher, factory, offset+4) } return hasher 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 e7b21f5e38..98274bd638 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 @@ -2,7 +2,6 @@ package net.corda.nodeapi.internal.serialization.amqp import com.google.common.primitives.Primitives import com.google.common.reflect.TypeToken -import io.netty.util.internal.EmptyArrays import net.corda.core.serialization.ClassWhitelist import net.corda.core.serialization.CordaSerializable import net.corda.core.serialization.SerializationContext @@ -20,7 +19,6 @@ import kotlin.reflect.full.findAnnotation import kotlin.reflect.full.primaryConstructor import kotlin.reflect.jvm.isAccessible import kotlin.reflect.jvm.javaType -import kotlin.reflect.jvm.kotlinProperty /** * Annotation indicating a constructor to be used to reconstruct instances of a class during deserialization. @@ -29,10 +27,6 @@ import kotlin.reflect.jvm.kotlinProperty @Retention(AnnotationRetention.RUNTIME) annotation class ConstructorForDeserialization -data class ConstructorDestructorMethods( - val getters: Collection, - val setters: Collection) - /** * Code for finding the constructor we will use for deserialization. * @@ -74,17 +68,22 @@ internal fun constructorForDeserialization(type: Type): KFunction? { * Note, you will need any Java classes to be compiled with the `-parameters` option to ensure constructor parameters have * names accessible via reflection. */ -internal fun propertiesForSerialization(kotlinConstructor: KFunction?, type: Type, factory: SerializerFactory): ConstructorDestructorMethods { - val clazz = type.asClass()!! - return if (kotlinConstructor != null) propertiesForSerializationFromConstructor(kotlinConstructor, type, factory) else propertiesForSerializationFromAbstract(clazz, type, factory) -} +internal fun propertiesForSerialization( + kotlinConstructor: KFunction?, + type: Type, + factory: SerializerFactory) = PropertySerializers.make ( + if (kotlinConstructor != null) { + propertiesForSerializationFromConstructor(kotlinConstructor, type, factory) + } else { + propertiesForSerializationFromAbstract(type.asClass()!!, type, factory) + }.sortedWith(PropertyAccessor)) fun isConcrete(clazz: Class<*>): Boolean = !(clazz.isInterface || Modifier.isAbstract(clazz.modifiers)) -private fun propertiesForSerializationFromConstructor( +internal fun propertiesForSerializationFromConstructor( kotlinConstructor: KFunction, type: Type, - factory: SerializerFactory): ConstructorDestructorMethods { + factory: SerializerFactory): List { val clazz = (kotlinConstructor.returnType.classifier as KClass<*>).javaObjectType // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans. val properties = Introspector.getBeanInfo(clazz).propertyDescriptors @@ -96,39 +95,45 @@ private fun propertiesForSerializationFromConstructor( return propertiesForSerializationFromSetters(properties, type, factory) } - val rc: MutableList = ArrayList(kotlinConstructor.parameters.size) + return mutableListOf().apply { + kotlinConstructor.parameters.withIndex().forEach { param -> + val name = param.value.name ?: throw NotSerializableException("Constructor parameter of $clazz has no name.") - for (param in kotlinConstructor.parameters) { - val name = param.name ?: throw NotSerializableException("Constructor parameter of $clazz has no name.") + val propertyReader = if (name in properties) { + // it's a publicly accessible property + val matchingProperty = properties[name]!! - if (name in properties) { - val matchingProperty = properties[name]!! + // Check that the method has a getter in java. + val getter = matchingProperty.readMethod ?: throw NotSerializableException( + "Property has no getter method for $name of $clazz. If using Java and the parameter name" + + "looks anonymous, check that you have the -parameters option specified in the Java compiler." + + "Alternately, provide a proxy serializer (SerializationCustomSerializer) if " + + "recompiling isn't an option.") - // Check that the method has a getter in java. - val getter = matchingProperty.readMethod ?: throw NotSerializableException("Property has no getter method for $name of $clazz. " + - "If using Java and the parameter name looks anonymous, check that you have the -parameters option specified in the Java compiler." + - "Alternately, provide a proxy serializer (SerializationCustomSerializer) if recompiling isn't an option") - val returnType = resolveTypeVariables(getter.genericReturnType, type) - if (constructorParamTakesReturnTypeOfGetter(returnType, getter.genericReturnType, param)) { - rc += PropertySerializer.make(name, PublicPropertyReader(getter), returnType, factory) + val returnType = resolveTypeVariables(getter.genericReturnType, type) + if (!constructorParamTakesReturnTypeOfGetter(returnType, getter.genericReturnType, param.value)) { + throw NotSerializableException( + "Property type $returnType for $name of $clazz differs from constructor parameter " + + "type ${param.value.type.javaType}") + } + + Pair(PublicPropertyReader(getter), returnType) } else { - throw NotSerializableException("Property type $returnType for $name of $clazz differs from constructor parameter type ${param.type.javaType}") + try { + val field = clazz.getDeclaredField(param.value.name) + Pair(PrivatePropertyReader(field, type), field.genericType) + } catch (e: NoSuchFieldException) { + 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") + } } - } else { - try { - val field = (clazz.getDeclaredField(param.name)) - rc += PropertySerializer.make(name, PrivatePropertyReader(field, type), field.genericType, factory) - } catch (e: NoSuchFieldException) { - 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") - } + this += PropertyAccessorConstructor( + param.index, + PropertySerializer.make(name, propertyReader.first, propertyReader.second, factory)) } - } - - return ConstructorDestructorMethods(rc, emptyList()) } /** @@ -138,26 +143,26 @@ private fun propertiesForSerializationFromConstructor( private fun propertiesForSerializationFromSetters( properties: Map, type: Type, - factory: SerializerFactory): ConstructorDestructorMethods { - val getters: MutableList = ArrayList(properties.size) - val setters: MutableList = ArrayList(properties.size) + factory: SerializerFactory): List { + return mutableListOf().apply { + var idx = 0 + properties.forEach { property -> + val getter: Method? = property.value.readMethod + val setter: Method? = property.value.writeMethod - properties.forEach { property -> - val getter: Method? = property.value.readMethod - val setter: Method? = property.value.writeMethod + if (getter == null || setter == null) return@forEach - if (getter == null || setter == null) return@forEach + // NOTE: There is no need to check return and parameter types vs the underlying type for + // the getter / setter vs property as if there is a difference then that property isn't reported + // by the BEAN inspector and thus we don't consider that case here - // NOTE: There is no need to check return and parameter types vs the underlying type for - // the getter / setter vs property as if there is a difference then that property isn't reported - // by the BEAN inspector and thus we don't consider that case here - - getters += PropertySerializer.make(property.key, PublicPropertyReader(getter), - resolveTypeVariables(getter.genericReturnType, type), factory) - setters += setter + this += PropertyAccessorGetterSetter ( + idx++, + PropertySerializer.make(property.key, PublicPropertyReader(getter), + resolveTypeVariables(getter.genericReturnType, type), factory), + setter) + } } - - return ConstructorDestructorMethods(getters, setters) } private fun constructorParamTakesReturnTypeOfGetter(getterReturnType: Type, rawGetterReturnType: Type, param: KParameter): Boolean { @@ -168,21 +173,24 @@ private fun constructorParamTakesReturnTypeOfGetter(getterReturnType: Type, rawG private fun propertiesForSerializationFromAbstract( clazz: Class<*>, type: Type, - factory: SerializerFactory): ConstructorDestructorMethods { + factory: SerializerFactory): List { // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans. val properties = Introspector.getBeanInfo(clazz).propertyDescriptors .filter { it.name != "class" } .sortedBy { it.name } .filterNot { it is IndexedPropertyDescriptor } - val rc: MutableList = ArrayList(properties.size) - for (property in properties) { - // Check that the method has a getter in java. - val getter = property.readMethod ?: throw NotSerializableException( - "Property has no getter method for ${property.name} of $clazz.") - val returnType = resolveTypeVariables(getter.genericReturnType, type) - rc += PropertySerializer.make(property.name, PublicPropertyReader(getter), returnType, factory) - } - return ConstructorDestructorMethods(rc, emptyList()) + + return mutableListOf().apply { + properties.withIndex().forEach { property -> + // Check that the method has a getter in java. + val getter = property.value.readMethod ?: throw NotSerializableException( + "Property has no getter method for ${property.value.name} of $clazz.") + val returnType = resolveTypeVariables(getter.genericReturnType, type) + this += PropertyAccessorConstructor( + property.index, + PropertySerializer.make(property.value.name, PublicPropertyReader(getter), returnType, factory)) + } + } } internal fun interfacesForSerialization(type: Type, serializerFactory: SerializerFactory): List { diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt index 93d8b0fbed..c4214394f3 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt @@ -23,9 +23,8 @@ class ThrowableSerializer(factory: SerializerFactory) : CustomSerializer.Proxy + extraProperties[property.getter.name] = property.getter.propertyReader.read(obj) } } catch (e: NotSerializableException) { logger.warn("Unexpected exception", e) @@ -90,4 +89,4 @@ class StackTraceElementSerializer(factory: SerializerFactory) : CustomSerializer override fun fromProxy(proxy: StackTraceElementProxy): StackTraceElement = StackTraceElement(proxy.declaringClass, proxy.methodName, proxy.fileName, proxy.lineNumber) data class StackTraceElementProxy(val declaringClass: String, val methodName: String, val fileName: String?, val lineNumber: Int) -} \ No newline at end of file +} diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java index 29f1b949d5..59a1465710 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java @@ -47,9 +47,9 @@ public class JavaPrivatePropertyTests { assertEquals(1, serializersByDescriptor.size()); ObjectSerializer cSerializer = ((ObjectSerializer)serializersByDescriptor.values().toArray()[0]); - assertEquals(1, cSerializer.getPropertySerializers().component1().size()); - Object[] propertyReaders = cSerializer.getPropertySerializers().component1().toArray(); - assertTrue (((PropertySerializer)propertyReaders[0]).getPropertyReader() instanceof PrivatePropertyReader); + assertEquals(1, cSerializer.getPropertySerializers().getSerializationOrder().size()); + Object[] propertyReaders = cSerializer.getPropertySerializers().getSerializationOrder().toArray(); + assertTrue (((PropertyAccessor)propertyReaders[0]).getGetter().getPropertyReader() instanceof PrivatePropertyReader); } @Test @@ -76,8 +76,8 @@ public class JavaPrivatePropertyTests { assertEquals(1, serializersByDescriptor.size()); ObjectSerializer cSerializer = ((ObjectSerializer)serializersByDescriptor.values().toArray()[0]); - assertEquals(1, cSerializer.getPropertySerializers().component1().size()); - Object[] propertyReaders = cSerializer.getPropertySerializers().component1().toArray(); - assertTrue (((PropertySerializer)propertyReaders[0]).getPropertyReader() instanceof PublicPropertyReader); + assertEquals(1, cSerializer.getPropertySerializers().getSerializationOrder().size()); + Object[] propertyReaders = cSerializer.getPropertySerializers().getSerializationOrder().toArray(); + assertTrue (((PropertyAccessor)propertyReaders[0]).getGetter().getPropertyReader() instanceof PublicPropertyReader); } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt index 51405e8c26..0ee20ab311 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt @@ -2,12 +2,23 @@ package net.corda.nodeapi.internal.serialization.amqp import junit.framework.TestCase.assertTrue import junit.framework.TestCase.assertEquals +import org.slf4j.Logger +import org.slf4j.LoggerFactory import org.junit.Test import org.apache.qpid.proton.amqp.Symbol import java.util.concurrent.ConcurrentHashMap class PrivatePropertyTests { - private val factory = testDefaultFactory() + private val factory = testDefaultFactoryNoEvolution() + + companion object { + val fields : Map = mapOf ( + "serializersByDesc" to SerializerFactory::class.java.getDeclaredField("serializersByDescriptor")).apply { + this.values.forEach { + it.isAccessible = true + } + } + } @Test fun testWithOnePrivateProperty() { @@ -53,16 +64,14 @@ class PrivatePropertyTests { val schemaAndBlob = SerializationOutput(factory).serializeAndReturnSchema(c1) assertEquals(1, schemaAndBlob.schema.types.size) - val field = SerializerFactory::class.java.getDeclaredField("serializersByDescriptor") - field.isAccessible = true @Suppress("UNCHECKED_CAST") - val serializersByDescriptor = field.get(factory) as ConcurrentHashMap> + val serializersByDescriptor = fields["serializersByDesc"]?.get(factory) as ConcurrentHashMap> val schemaDescriptor = schemaAndBlob.schema.types.first().descriptor.name serializersByDescriptor.filterKeys { (it as Symbol) == schemaDescriptor }.values.apply { assertEquals(1, this.size) assertTrue(this.first() is ObjectSerializer) - val propertySerializers = (this.first() as ObjectSerializer).propertySerializers.getters.toList() + val propertySerializers = (this.first() as ObjectSerializer).propertySerializers.serializationOrder.map { it.getter } assertEquals(2, propertySerializers.size) // a was public so should have a synthesised getter assertTrue(propertySerializers[0].propertyReader is PublicPropertyReader) @@ -84,16 +93,14 @@ class PrivatePropertyTests { val schemaAndBlob = SerializationOutput(factory).serializeAndReturnSchema(c1) assertEquals(1, schemaAndBlob.schema.types.size) - val field = SerializerFactory::class.java.getDeclaredField("serializersByDescriptor") - field.isAccessible = true @Suppress("UNCHECKED_CAST") - val serializersByDescriptor = field.get(factory) as ConcurrentHashMap> + val serializersByDescriptor = fields["serializersByDesc"]?.get(factory) as ConcurrentHashMap> val schemaDescriptor = schemaAndBlob.schema.types.first().descriptor.name serializersByDescriptor.filterKeys { (it as Symbol) == schemaDescriptor }.values.apply { assertEquals(1, this.size) assertTrue(this.first() is ObjectSerializer) - val propertySerializers = (this.first() as ObjectSerializer).propertySerializers.getters.toList() + val propertySerializers = (this.first() as ObjectSerializer).propertySerializers.serializationOrder.map { it.getter } assertEquals(2, propertySerializers.size) // as before, a is public so we'll use the getter method @@ -105,13 +112,24 @@ class PrivatePropertyTests { } } + @Suppress("UNCHECKED_CAST") @Test fun testNested() { data class Inner(private val a: Int) data class Outer(private val i: Inner) val c1 = Outer(Inner(1010101)) - val c2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(c1)) + val output = SerializationOutput(factory).serializeAndReturnSchema(c1) + println (output.schema) + + val serializersByDescriptor = fields["serializersByDesc"]!!.get(factory) as ConcurrentHashMap> + + // Inner and Outer + assertEquals(2, serializersByDescriptor.size) + + val schemaDescriptor = output.schema.types.first().descriptor.name + val c2 = DeserializationInput(factory).deserialize(output.obj) + assertEquals(c1, c2) } } \ No newline at end of file diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationPropertyOrdering.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationPropertyOrdering.kt new file mode 100644 index 0000000000..c23572db41 --- /dev/null +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationPropertyOrdering.kt @@ -0,0 +1,125 @@ +package net.corda.nodeapi.internal.serialization.amqp + +import org.junit.Test +import java.util.concurrent.ConcurrentHashMap +import kotlin.test.assertEquals +import org.apache.qpid.proton.amqp.Symbol +import java.lang.reflect.Method +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +class SerializationPropertyOrdering { + companion object { + val VERBOSE get() = false + + val sf = testDefaultFactoryNoEvolution() + } + + // Force object references to be ued to ensure we go through that code path + // this test shows (not now it's fixed) a bug whereby deserializing objects + // would break where refferenced objects were accessed before they'd been + // processed thanks to the way the blob was deserialized + @Test + fun refferenceOrdering() { + data class Reffed(val c: String, val b: String, val a: String) + data class User(val b: List, val a: List) + + val r1 = Reffed("do not", "or", "do") + val r2 = Reffed("do not", "or", "do") + val l = listOf(r1, r2, r1, r2, r1, r2) + + val u = User(l,l) + val output = TestSerializationOutput(VERBOSE, sf).serializeAndReturnSchema(u) + val input = DeserializationInput(sf).deserialize(output.obj) + } + + @Test + fun randomOrder() { + data class C(val c: Int, val d: Int, val b: Int, val e: Int, val a: Int) + + val c = C(3,4,2,5,1) + val output = TestSerializationOutput(VERBOSE, sf).serializeAndReturnSchema(c) + + // the schema should reflect the serialized order of properties, not the + // construction order + assertEquals(1, output.schema.types.size) + output.schema.types.firstOrNull()?.apply { + assertEquals(5, (this as CompositeType).fields.size) + assertEquals("a", this.fields[0].name) + assertEquals("b", this.fields[1].name) + assertEquals("c", this.fields[2].name) + assertEquals("d", this.fields[3].name) + assertEquals("e", this.fields[4].name) + } + + // and deserializing it should construct the object as it was and not in the order prescribed + // by the serialized form + val input = DeserializationInput(sf).deserialize(output.obj) + assertEquals(1, input.a) + assertEquals(2, input.b) + assertEquals(3, input.c) + assertEquals(4, input.d) + assertEquals(5, input.e) + } + + @Suppress("UNCHECKED_CAST") + @Test + fun randomOrderSetter() { + 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) + } + + val c = C() + + c.a = 100 + c.b = 200 + c.c = 300 + c.d = 400 + c.e = 500 + + val output = TestSerializationOutput(VERBOSE, sf).serializeAndReturnSchema(c) + + // the schema should reflect the serialized order of properties, not the + // construction order + assertEquals(1, output.schema.types.size) + output.schema.types.firstOrNull()?.apply { + assertEquals(5, (this as CompositeType).fields.size) + assertEquals("a", this.fields[0].name) + assertEquals("b", this.fields[1].name) + assertEquals("c", this.fields[2].name) + assertEquals("d", this.fields[3].name) + assertEquals("e", this.fields[4].name) + } + + // Test needs to look at a bunch of private variables, change the access semantics for them + val fields : Map = mapOf ( + "serializersByDesc" to SerializerFactory::class.java.getDeclaredField("serializersByDescriptor"), + "setter" to PropertyAccessorGetterSetter::class.java.getDeclaredField("setter")).apply { + this.values.forEach { + it.isAccessible = true + } + } + + val serializersByDescriptor = fields["serializersByDesc"]!!.get(sf) as ConcurrentHashMap> + val schemaDescriptor = output.schema.types.first().descriptor.name + + // make sure that each property accessor has a setter to ensure we're using getter / setter instantiation + serializersByDescriptor.filterKeys { (it as Symbol) == schemaDescriptor }.values.apply { + assertEquals(1, this.size) + assertTrue(this.first() is ObjectSerializer) + val propertyAccessors = (this.first() as ObjectSerializer).propertySerializers.serializationOrder as List + propertyAccessors.forEach { property -> assertNotNull(fields["setter"]!!.get(property) as Method?) } + } + + val input = DeserializationInput(sf).deserialize(output.obj) + assertEquals(100, input.a) + assertEquals(200, input.b) + assertEquals(300, input.c) + assertEquals(400, input.d) + assertEquals(500, input.e) + } +} \ No newline at end of file