Merge pull request #2383 from corda/kat/feature/deterministicSerilaizer

CORDA-914 - Deterministic property ordering for AMQP serialization

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

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt

@@ -61,7 +61,14 @@ abstract class CustomSerializer<T : Any> : AMQPSerializer<T>, 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<T : Any> : AMQPSerializer<T>, 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)
                 }
             }
         }

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt

@@ -20,7 +20,7 @@ class EvolutionSerializer(
         override val kotlinConstructor: KFunction<Any>?) : 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

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<Any> {
     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<Field> {
-        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)
         }
     }
 

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

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<PropertySerializer>().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<PropertySerializer>().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<PropertySerializer>().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<PropertyAccessor> {
+        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<PropertyAccessor>) {
+    companion object {
+        fun make(serializationOrder: List<PropertyAccessor>) =
+                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<PropertyAccessor>) : PropertySerializers(serializationOrder) {
+    override val byConstructor get() = true
+}
+
+class PropertySerializersSetter(
+        serializationOrder: List<PropertyAccessor>) : PropertySerializers(serializationOrder) {
+    override val byConstructor get() = false
+}
+
+class PropertySerializersEvolution : PropertySerializers(emptyList()) {
+    override val byConstructor get() = false
+}
+
+

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

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<PropertySerializer>,
-        val setters: Collection<Method?>)
-
 /**
  * Code for finding the constructor we will use for deserialization.
  *
@@ -74,17 +68,22 @@ internal fun constructorForDeserialization(type: Type): KFunction<Any>? {
  * 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 <T : Any> propertiesForSerialization(kotlinConstructor: KFunction<T>?, type: Type, factory: SerializerFactory): ConstructorDestructorMethods {
-    val clazz = type.asClass()!!
-    return if (kotlinConstructor != null) propertiesForSerializationFromConstructor(kotlinConstructor, type, factory) else propertiesForSerializationFromAbstract(clazz, type, factory)
-}
+internal fun <T : Any> propertiesForSerialization(
+        kotlinConstructor: KFunction<T>?,
+        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 <T : Any> propertiesForSerializationFromConstructor(
+internal fun <T : Any> propertiesForSerializationFromConstructor(
         kotlinConstructor: KFunction<T>,
         type: Type,
-        factory: SerializerFactory): ConstructorDestructorMethods {
+        factory: SerializerFactory): List<PropertyAccessor> {
     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 <T : Any> propertiesForSerializationFromConstructor(
         return propertiesForSerializationFromSetters(properties, type, factory)
     }
 
-    val rc: MutableList<PropertySerializer> = ArrayList(kotlinConstructor.parameters.size)
+    return mutableListOf<PropertyAccessor>().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 <T : Any> propertiesForSerializationFromConstructor(
 private fun propertiesForSerializationFromSetters(
         properties: Map<String, PropertyDescriptor>,
         type: Type,
-        factory: SerializerFactory): ConstructorDestructorMethods {
-    val getters: MutableList<PropertySerializer> = ArrayList(properties.size)
-    val setters: MutableList<Method?> = ArrayList(properties.size)
+        factory: SerializerFactory): List<PropertyAccessor> {
+    return mutableListOf<PropertyAccessorGetterSetter>().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<PropertyAccessor> {
     // 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<PropertySerializer> = 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<PropertyAccessorConstructor>().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<Type> {

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt

@@ -23,9 +23,8 @@ class ThrowableSerializer(factory: SerializerFactory) : CustomSerializer.Proxy<T
             // Try and find a constructor
             try {
                 val constructor = constructorForDeserialization(obj.javaClass)
-                val props = propertiesForSerialization(constructor, obj.javaClass, factory)
-                for (prop in props.getters) {
-                    extraProperties[prop.name] = prop.propertyReader.read(obj)
+                propertiesForSerializationFromConstructor(constructor!!, obj.javaClass, factory).forEach { property ->
+                    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)
-}
+}

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

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<String, java.lang.reflect.Field> = 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<Any, AMQPSerializer<Any>>
+        val serializersByDescriptor = fields["serializersByDesc"]?.get(factory) as ConcurrentHashMap<Any, AMQPSerializer<Any>>
 
         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<Any, AMQPSerializer<Any>>
+        val serializersByDescriptor = fields["serializersByDesc"]?.get(factory) as ConcurrentHashMap<Any, AMQPSerializer<Any>>
 
         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<Any, AMQPSerializer<Any>>
+
+        // 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)
     }
 }

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<Reffed>, val a: List<Reffed>)
+
+        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<String, java.lang.reflect.Field> = 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<Any, AMQPSerializer<Any>>
+        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<PropertyAccessorGetterSetter>
+            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)
+    }
+}