CORDA-908 - Support private properties in AMQP serialization (#2336)

CORDA-908 - Support private properties in AMQP serialization

* Review comments

* Fix tests

* Review Comments

* review comments

* review comments

docs/source/release-notes.rst

@@ -16,19 +16,31 @@ Unreleased
   That is the ability to alter an enum constant and, as long as certain rules are followed and the correct
   annotations applied, have older and newer instances of that enumeration be understood.
 
-* **AMQP Enabled**
+* **AMQP Enabled**:
+  AMQP Serialization is now enabled for both peer to peer communication and the writing of states to the vault. This change
+  brings a stable format Corda can support internally throughout it's lifetime that meets the needs of Corda and our
+  users.
 
-AMQP Serialization is now enabled for both peer to peer communication and writing states to the vault. This change
+  Details on the AMQP serialization framework can be found in the :doc:`serialization` document :ref:`here <amqp_ref>`.
-brings a stable format Corda can support internally throughout it's lifetime that meets the needs of Corda and our
+  This provides an introduction and overview of the framework whilst more specific details on object evolution as it relates to
-users.
+  serialization is similarly found in pages :doc:`serialization-default-evolution` and :doc:`serialization-enum-evolution`
+  respectively. Recommendations on how best to code CorDapps using your own :ref:`custom types <amqp_custom_types_ref>`.
+
+  .. note:: This release delivers the bulk of our transition from Kryo serialisation to AMQP serialisation. This means that many of the restrictions
+    that were documented in previous versions of Corda are now enforced. (https://docs.corda.net/releases/release-V1.0/serialization.html).
+
+    In particular, you are advised to review the section titled "Custom Types". To aid with the transition, we have included support
+    in this release for default construction of objects and their instantiation through getters as well as objects with inaccessible
+    private fields but it is not guaranteed that this support will continue into future versions; the restrictions documented at the
+    link above are the canonical source.
 
 * **Custom Serializers**
 
-To allow interop with third party libraries that cannot be recompiled we add functionality that allows custom serializers
+  To allow interop with third party libraries that cannot be recompiled we add functionality that allows custom serializers
-to be written for those classes. If needed, a proxy object can be created as an interim step that allows Corda's internal
+  to be written for those classes. If needed, a proxy object can be created as an interim step that allows Corda's internal
-serializers to operate on those types.
+  serializers to operate on those types.
 
-A good example of this is the SIMM valuation demo which has a number of such serializers defined in the plugin/customserializers package
+  A good example of this is the SIMM valuation demo which has a number of such serializers defined in the plugin/customserializers package
 
 Release 2.0
 ----------

docs/source/serialization.rst

@@ -1,6 +1,8 @@
 Object serialization
 ====================
 
+.. contents::
+
 What is serialization (and deserialization)?
 --------------------------------------------
 
@@ -52,7 +54,7 @@ was a compelling use case for the definition and development of a custom format
 
     #.  A desire to have a schema describing what has been serialized along-side the actual data:
 
-        #.  To assist with versioning, both in terms of being able to interpret long ago archivEd data (e.g. trades from
+        #.  To assist with versioning, both in terms of being able to interpret long ago archived data (e.g. trades from
             a decade ago, long after the code has changed) and between differing code versions.
         #.  To make it easier to write user interfaces that can navigate the serialized form of data.
         #.  To support cross platform (non-JVM) interaction, where the format of a class file is not so easily interpreted.
@@ -76,7 +78,7 @@ Finally, for the checkpointing of flows Corda will continue to use the existing
 
 This separation of serialization schemes into different contexts allows us to use the most suitable framework for that context rather than
 attempting to force a one size fits all approach. Where ``Kryo`` is more suited to the serialization of a programs stack frames, being more flexible
-than our AMQP framework in what it can construct and serialize, that flexibility makes it exceptionally difficult to make secure. Conversly
+than our AMQP framework in what it can construct and serialize, that flexibility makes it exceptionally difficult to make secure. Conversely
 our AMQP framework allows us to concentrate on a robust a secure framework that can be reasoned about thus made safer with far fewer unforeseen
 security holes.
 
@@ -282,22 +284,6 @@ serialised form
 
         val e2 = e.serialize().deserialize() // e2.c will be 20, not 100!!!
 
-.. warning:: Private properties in Kotlin classes render the class unserializable *unless* a public
-    getter is manually defined. For example:
-
-    .. container:: codeset
-
-        .. sourcecode:: kotlin
-
-            data class C(val a: Int, private val b: Int) {
-                // Without this C cannot be serialized
-                public fun getB() = b
-            }
-
-    .. note:: This is particularly relevant as IDE's can often point out where they believe a
-        property can be made private without knowing this can break Corda serialization. Should
-        this happen then a run time warning will be generated when the class fails to serialize
-
 Setter Instantiation
 ''''''''''''''''''''
 
@@ -330,6 +316,75 @@ For example:
             public void setC(int c) { this.c = c; }
         }
 
+Inaccessible Private Properties
+```````````````````````````````
+
+Whilst the Corda AMQP serialization framework supports private object properties without publicly
+accessible getter methods this development idiom is strongly discouraged.
+
+For example.
+
+    .. container:: codeset
+
+        Kotlin:
+
+        .. sourcecode:: kotlin
+
+            data class C(val a: Int, private val b: Int)
+
+        Java:
+
+        .. sourcecode:: Java
+
+            class C {
+                public Integer a;
+                private Integer b;
+
+                C(Integer a, Integer b) {
+                    this.a = a;
+                    this.b = b;
+                }
+            }
+
+When designing stateful objects is should be remembered that they are not, despite appearances, traditional
+programmatic constructs. They are signed over, transformed, serialised, and relationally mapped. As such,
+all elements should be publicly accessible by design
+
+.. warning:: IDEs will indiciate erroneously that properties can be given something other than public
+    visibility. Ignore this as whilst it will work, as discussed above there are many reasons why this isn't
+    a good idea and those are beyond the scope of the IDEs inference rules
+
+Providing a public getter, as per the following example, is acceptable
+
+    .. container:: codeset
+
+        Kotlin:
+
+        .. sourcecode:: kotlin
+
+            data class C(val a: Int, private val b: Int) {
+                public fun getB() = b
+            }
+
+        Java:
+
+        .. sourcecode:: Java
+
+            class C {
+                public Integer a;
+                private Integer b;
+
+                C(Integer a, Integer b) {
+                    this.a = a;
+                    this.b = b;
+                }
+
+                public Integer getB() {
+                    return b;
+                }
+            }
+
+
 Enums
 `````
 

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

@@ -96,7 +96,7 @@ class EvolutionSerializer(
             old.fields.forEach {
                 val returnType = it.getTypeAsClass(factory.classloader)
                 oldArgs[it.name] = OldParam(
-                        returnType, idx++, PropertySerializer.make(it.name, null, returnType, factory))
+                        returnType, idx++, PropertySerializer.make(it.name, PublicPropertyReader(null), returnType, factory))
             }
 
             val readers = constructor.parameters.map {

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

@@ -26,6 +26,8 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS
         propertiesForSerialization(kotlinConstructor, clazz, factory)
     }
 
+    fun getPropertySerializers() = propertySerializers
+
     private val typeName = nameForType(clazz)
 
     override val typeDescriptor = Symbol.valueOf("$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}")

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

@@ -1,17 +1,74 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
-import net.corda.core.utilities.contextLogger
+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.
  */
-sealed class PropertySerializer(val name: String, val readMethod: Method?, val resolvedType: Type) {
+sealed class PropertySerializer(val name: String, val propertyReader: PropertyReader, val resolvedType: Type) {
     abstract fun writeClassInfo(output: SerializationOutput)
     abstract fun writeProperty(obj: Any?, data: Data, output: SerializationOutput)
     abstract fun readProperty(obj: Any?, schemas: SerializationSchemas, input: DeserializationInput): Any?
@@ -44,25 +101,11 @@ sealed class PropertySerializer(val name: String, val readMethod: Method?, val r
             }
 
     private fun generateMandatory(): Boolean {
-        return isJVMPrimitive || readMethod?.returnsNullable() == false
+        return isJVMPrimitive || !(propertyReader.isNullable())
-    }
-
-    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.
-            logger.error("Unexpected internal Kotlin error", e)
-            return true
-        }
     }
 
     companion object {
-        private val logger = contextLogger()
+        fun make(name: String, readMethod: PropertyReader, resolvedType: Type, factory: SerializerFactory): PropertySerializer {
-        fun make(name: String, readMethod: Method?, resolvedType: Type, factory: SerializerFactory): PropertySerializer {
-            readMethod?.isAccessible = true
             if (SerializerFactory.isPrimitive(resolvedType)) {
                 return when (resolvedType) {
                     Char::class.java, Character::class.java -> AMQPCharPropertySerializer(name, readMethod)
@@ -78,7 +121,8 @@ sealed class PropertySerializer(val name: String, val readMethod: Method?, val r
      * A property serializer for a complex type (another object).
      */
     class DescribedTypePropertySerializer(
-            name: String, readMethod: Method?,
+            name: String,
+            readMethod: PropertyReader,
             resolvedType: Type,
             private val lazyTypeSerializer: () -> AMQPSerializer<*>) : PropertySerializer(name, readMethod, resolvedType) {
         // This is lazy so we don't get an infinite loop when a method returns an instance of the class.
@@ -90,12 +134,15 @@ sealed class PropertySerializer(val name: String, val readMethod: Method?, val r
             }
         }
 
-        override fun readProperty(obj: Any?, schemas: SerializationSchemas, input: DeserializationInput): Any? = ifThrowsAppend({ nameForDebug }) {
+        override fun readProperty(
+                obj: Any?,
+                schemas: SerializationSchemas,
+                input: DeserializationInput): Any? = ifThrowsAppend({ nameForDebug }) {
             input.readObjectOrNull(obj, schemas, resolvedType)
         }
 
         override fun writeProperty(obj: Any?, data: Data, output: SerializationOutput) = ifThrowsAppend({ nameForDebug }) {
-            output.writeObjectOrNull(readMethod!!.invoke(obj), data, resolvedType)
+            output.writeObjectOrNull(propertyReader.read(obj), data, resolvedType)
         }
 
         private val nameForDebug = "$name(${resolvedType.typeName})"
@@ -104,7 +151,10 @@ sealed class PropertySerializer(val name: String, val readMethod: Method?, val r
     /**
      * A property serializer for most AMQP primitive type (Int, String, etc).
      */
-    class AMQPPrimitivePropertySerializer(name: String, readMethod: Method?, resolvedType: Type) : PropertySerializer(name, readMethod, resolvedType) {
+    class AMQPPrimitivePropertySerializer(
+            name: String,
+            readMethod: PropertyReader,
+            resolvedType: Type) : PropertySerializer(name, readMethod, resolvedType) {
         override fun writeClassInfo(output: SerializationOutput) {}
 
         override fun readProperty(obj: Any?, schemas: SerializationSchemas, input: DeserializationInput): Any? {
@@ -112,7 +162,7 @@ sealed class PropertySerializer(val name: String, val readMethod: Method?, val r
         }
 
         override fun writeProperty(obj: Any?, data: Data, output: SerializationOutput) {
-            val value = readMethod!!.invoke(obj)
+            val value = propertyReader.read(obj)
             if (value is ByteArray) {
                 data.putObject(Binary(value))
             } else {
@@ -126,7 +176,7 @@ sealed class PropertySerializer(val name: String, val readMethod: Method?, val r
      * value of the character is stored in numeric UTF-16 form and on deserialisation requires explicit
      * casting back to a char otherwise it's treated as an Integer and a TypeMismatch occurs
      */
-    class AMQPCharPropertySerializer(name: String, readMethod: Method?) :
+    class AMQPCharPropertySerializer(name: String, readMethod: PropertyReader) :
             PropertySerializer(name, readMethod, Character::class.java) {
         override fun writeClassInfo(output: SerializationOutput) {}
 
@@ -135,7 +185,7 @@ sealed class PropertySerializer(val name: String, val readMethod: Method?, val r
         }
 
         override fun writeProperty(obj: Any?, data: Data, output: SerializationOutput) {
-            val input = readMethod!!.invoke(obj)
+            val input = propertyReader.read(obj)
             if (input != null) data.putShort((input as Char).toShort()) else data.putNull()
         }
     }

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

@@ -20,6 +20,7 @@ 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,8 +30,8 @@ import kotlin.reflect.jvm.javaType
 annotation class ConstructorForDeserialization
 
 data class ConstructorDestructorMethods(
-        val getters : Collection<PropertySerializer>,
+        val getters: Collection<PropertySerializer>,
-        val setters : Collection<Method?>)
+        val setters: Collection<Method?>)
 
 /**
  * Code for finding the constructor we will use for deserialization.
@@ -100,26 +101,31 @@ private fun <T : Any> propertiesForSerializationFromConstructor(
     for (param in kotlinConstructor.parameters) {
         val name = param.name ?: throw NotSerializableException("Constructor parameter of $clazz has no name.")
 
-        val matchingProperty = properties[name] ?:
+        if (name in properties) {
-                try {
+            val matchingProperty = properties[name]!!
-                    clazz.getDeclaredField(param.name)
-                    throw NotSerializableException("Property '$name' or its getter is non public, this renders class '$clazz' unserializable")
-                } 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")
-                }
 
-        // Check that the method has a getter in java.
+            // Check that the method has a getter in java.
-        val getter = matchingProperty.readMethod ?: throw NotSerializableException("Property has no getter method for $name of $clazz. " +
+            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." +
+                    "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")
+                    "Alternately, provide a proxy serializer (SerializationCustomSerializer) if recompiling isn't an option")
-        val returnType = resolveTypeVariables(getter.genericReturnType, type)
+            val returnType = resolveTypeVariables(getter.genericReturnType, type)
-        if (constructorParamTakesReturnTypeOfGetter(returnType, getter.genericReturnType, param)) {
+            if (constructorParamTakesReturnTypeOfGetter(returnType, getter.genericReturnType, param)) {
-            rc += PropertySerializer.make(name, getter, returnType, factory)
+                rc += PropertySerializer.make(name, PublicPropertyReader(getter), returnType, factory)
+            } else {
+                throw NotSerializableException("Property type $returnType for $name of $clazz differs from constructor parameter type ${param.type.javaType}")
+            }
         } else {
-            throw NotSerializableException("Property type $returnType for $name of $clazz differs from constructor parameter type ${param.type.javaType}")
+            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")
+            }
         }
+
     }
 
     return ConstructorDestructorMethods(rc, emptyList())
@@ -130,11 +136,11 @@ private fun <T : Any> propertiesForSerializationFromConstructor(
  * and use those
  */
 private fun propertiesForSerializationFromSetters(
-        properties : Map<String, PropertyDescriptor>,
+        properties: Map<String, PropertyDescriptor>,
         type: Type,
         factory: SerializerFactory): ConstructorDestructorMethods {
-    val getters : MutableList<PropertySerializer> = ArrayList(properties.size)
+    val getters: MutableList<PropertySerializer> = ArrayList(properties.size)
-    val setters : MutableList<Method?> = ArrayList(properties.size)
+    val setters: MutableList<Method?> = ArrayList(properties.size)
 
     properties.forEach { property ->
         val getter: Method? = property.value.readMethod
@@ -146,8 +152,8 @@ private fun propertiesForSerializationFromSetters(
         // 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, getter, resolveTypeVariables(getter.genericReturnType, type),
+        getters += PropertySerializer.make(property.key, PublicPropertyReader(getter),
-                factory)
+                resolveTypeVariables(getter.genericReturnType, type), factory)
         setters += setter
     }
 
@@ -159,15 +165,22 @@ private fun constructorParamTakesReturnTypeOfGetter(getterReturnType: Type, rawG
     return typeToken.isSupertypeOf(getterReturnType) || typeToken.isSupertypeOf(rawGetterReturnType)
 }
 
-private fun propertiesForSerializationFromAbstract(clazz: Class<*>, type: Type, factory: SerializerFactory): ConstructorDestructorMethods {
+private fun propertiesForSerializationFromAbstract(
+        clazz: Class<*>,
+        type: Type,
+        factory: SerializerFactory): ConstructorDestructorMethods {
     // 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 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 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, getter, returnType, factory)
+        rc += PropertySerializer.make(property.name, PublicPropertyReader(getter), returnType, factory)
     }
     return ConstructorDestructorMethods(rc, emptyList())
 }

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

@@ -40,6 +40,7 @@ open class SerializerFactory(val whitelist: ClassWhitelist, cl: ClassLoader) {
     val transformsCache = ConcurrentHashMap<String, EnumMap<TransformTypes, MutableList<Transform>>>()
 
     open val classCarpenter = ClassCarpenter(cl, whitelist)
+
     val classloader: ClassLoader
         get() = classCarpenter.classloader
 
@@ -381,3 +382,4 @@ open class SerializerFactory(val whitelist: ClassWhitelist, cl: ClassLoader) {
         override fun toString(): String = "?"
     }
 }
+

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

@@ -25,7 +25,7 @@ class ThrowableSerializer(factory: SerializerFactory) : CustomSerializer.Proxy<T
                 val constructor = constructorForDeserialization(obj.javaClass)
                 val props = propertiesForSerialization(constructor, obj.javaClass, factory)
                 for (prop in props.getters) {
-                    extraProperties[prop.name] = prop.readMethod!!.invoke(obj)
+                    extraProperties[prop.name] = prop.propertyReader.read(obj)
                 }
             } catch (e: NotSerializableException) {
                 logger.warn("Unexpected exception", e)

node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/ErrorMessageTests.java

@@ -2,10 +2,12 @@ package net.corda.nodeapi.internal.serialization.amqp;
 
 import net.corda.nodeapi.internal.serialization.AllWhitelist;
 import org.assertj.core.api.Assertions;
+import org.junit.Ignore;
 import org.junit.Test;
 
 import java.io.NotSerializableException;
 
+@Ignore("Current behaviour allows for the serialization of objects with private members, this will be disallowed at some point in the future")
 public class ErrorMessageTests {
     private String errMsg(String property, String testname) {
         return "Property '"

node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java

@@ -0,0 +1,79 @@
+package net.corda.nodeapi.internal.serialization.amqp;
+
+import net.corda.nodeapi.internal.serialization.AllWhitelist;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+import java.io.NotSerializableException;
+import java.lang.reflect.Field;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class JavaPrivatePropertyTests {
+    static class C {
+        private String a;
+
+        C(String a) { this.a = a; }
+    }
+
+    static class C2 {
+        private String a;
+
+        C2(String a) { this.a = a; }
+
+        public String getA() { return a; }
+    }
+
+    @Test
+    public void singlePrivateWithConstructor() throws NotSerializableException, NoSuchFieldException, IllegalAccessException {
+        SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader());
+        SerializationOutput ser = new SerializationOutput(factory);
+        DeserializationInput des = new DeserializationInput(factory);
+
+        C c = new C("dripping taps");
+        C c2 = des.deserialize(ser.serialize(c), C.class);
+
+        assertEquals (c.a, c2.a);
+
+        //
+        // Now ensure we actually got a private property serializer
+        //
+        Field f = SerializerFactory.class.getDeclaredField("serializersByDescriptor");
+        f.setAccessible(true);
+
+        ConcurrentHashMap<Object, AMQPSerializer<Object>> serializersByDescriptor =
+                (ConcurrentHashMap<Object, AMQPSerializer<Object>>) f.get(factory);
+
+        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);
+    }
+
+    @Test
+    public void singlePrivateWithConstructorAndGetter()
+            throws NotSerializableException, NoSuchFieldException, IllegalAccessException {
+        SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader());
+        SerializationOutput ser = new SerializationOutput(factory);
+        DeserializationInput des = new DeserializationInput(factory);
+
+        C2 c = new C2("dripping taps");
+        C2 c2 = des.deserialize(ser.serialize(c), C2.class);
+
+        assertEquals (c.a, c2.a);
+
+        //
+        // Now ensure we actually got a private property serializer
+        //
+        Field f = SerializerFactory.class.getDeclaredField("serializersByDescriptor");
+        f.setAccessible(true);
+        ConcurrentHashMap<Object, AMQPSerializer<Object>> serializersByDescriptor =
+                (ConcurrentHashMap<Object, AMQPSerializer<Object>>) f.get(factory);
+
+        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);
+    }
+}

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt

@@ -1,10 +1,8 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
-import net.corda.core.serialization.SerializedBytes
 import org.apache.qpid.proton.codec.Data
 import net.corda.nodeapi.internal.serialization.AllWhitelist
 import net.corda.nodeapi.internal.serialization.EmptyWhitelist
-import java.io.NotSerializableException
 
 fun testDefaultFactory() = SerializerFactory(AllWhitelist, ClassLoader.getSystemClassLoader())
 fun testDefaultFactoryWithWhitelist() = SerializerFactory(EmptyWhitelist, ClassLoader.getSystemClassLoader())

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/ErrorMessagesTests.kt

@@ -1,6 +1,7 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
 import org.assertj.core.api.Assertions
+import org.junit.Ignore
 import org.junit.Test
 import java.io.NotSerializableException
 
@@ -12,6 +13,8 @@ class ErrorMessagesTests {
     private fun errMsg(property:String, testname: String) =
             "Property '$property' or its getter is non public, this renders class 'class $testname\$C' unserializable -> class $testname\$C"
 
+    // Java allows this to be set at the class level yet Kotlin doesn't for some reason
+    @Ignore("Current behaviour allows for the serialization of objects with private members, this will be disallowed at some point in the future")
     @Test
     fun privateProperty() {
         data class C(private val a: Int)
@@ -25,6 +28,8 @@ class ErrorMessagesTests {
         }.isInstanceOf(NotSerializableException::class.java).hasMessage(errMsg("a", testname))
     }
 
+    // Java allows this to be set at the class level yet Kotlin doesn't for some reason
+    @Ignore("Current behaviour allows for the serialization of objects with private members, this will be disallowed at some point in the future")
     @Test
     fun privateProperty2() {
         data class C(val a: Int, private val b: Int)
@@ -38,6 +43,8 @@ class ErrorMessagesTests {
         }.isInstanceOf(NotSerializableException::class.java).hasMessage(errMsg("b", testname))
     }
 
+    // Java allows this to be set at the class level yet Kotlin doesn't for some reason
+    @Ignore("Current behaviour allows for the serialization of objects with private members, this will be disallowed at some point in the future")
     @Test
     fun privateProperty3() {
         // despite b being private, the getter we've added is public and thus allows for the serialisation
@@ -54,6 +61,8 @@ class ErrorMessagesTests {
         val c = DeserializationInput(sf).deserialize(bytes)
     }
 
+    // Java allows this to be set at the class level yet Kotlin doesn't for some reason
+    @Ignore("Current behaviour allows for the serialization of objects with private members, this will be disallowed at some point in the future")
     @Test
     fun protectedProperty() {
         data class C(protected val a: Int)

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt

@@ -0,0 +1,117 @@
+package net.corda.nodeapi.internal.serialization.amqp
+
+import junit.framework.TestCase.assertTrue
+import junit.framework.TestCase.assertEquals
+import org.junit.Test
+import org.apache.qpid.proton.amqp.Symbol
+import java.util.concurrent.ConcurrentHashMap
+
+class PrivatePropertyTests {
+    private val factory = testDefaultFactory()
+
+    @Test
+    fun testWithOnePrivateProperty() {
+        data class C(private val b: String)
+
+        val c1 = C("Pants are comfortable sometimes")
+        val c2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(c1))
+        assertEquals(c1, c2)
+    }
+
+    @Test
+    fun testWithOnePrivatePropertyNullableNotNull() {
+        data class C(private val b: String?)
+
+        val c1 = C("Pants are comfortable sometimes")
+        val c2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(c1))
+        assertEquals(c1, c2)
+    }
+
+    @Test
+    fun testWithOnePrivatePropertyNullableNull() {
+        data class C(private val b: String?)
+
+        val c1 = C(null)
+        val c2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(c1))
+        assertEquals(c1, c2)
+    }
+
+    @Test
+    fun testWithOnePublicOnePrivateProperty() {
+        data class C(val a: Int, private val b: Int)
+
+        val c1 = C(1, 2)
+        val c2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(c1))
+        assertEquals(c1, c2)
+    }
+
+    @Test
+    fun testWithOnePublicOnePrivateProperty2() {
+        data class C(val a: Int, private val b: Int)
+
+        val c1 = C(1, 2)
+        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 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()
+            assertEquals(2, propertySerializers.size)
+            // a was public so should have a synthesised getter
+            assertTrue(propertySerializers[0].propertyReader is PublicPropertyReader)
+
+            // b is private and thus won't have teh getter so we'll have reverted
+            // to using reflection to remove the inaccessible property
+            assertTrue(propertySerializers[1].propertyReader is PrivatePropertyReader)
+        }
+    }
+
+    @Test
+    fun testGetterMakesAPublicReader() {
+        data class C(val a: Int, private val b: Int) {
+            @Suppress("UNUSED")
+            fun getB() = b
+        }
+
+        val c1 = C(1, 2)
+        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 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()
+            assertEquals(2, propertySerializers.size)
+
+            // as before, a is public so we'll use the getter method
+            assertTrue(propertySerializers[0].propertyReader is PublicPropertyReader)
+
+            // the getB() getter explicitly added means we should use the "normal" public
+            // method reader rather than the private oen
+            assertTrue(propertySerializers[1].propertyReader is PublicPropertyReader)
+        }
+    }
+
+    @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))
+        assertEquals(c1, c2)
+    }
+}

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenterTestUtils.kt

@@ -35,9 +35,16 @@ fun Schema.mangleNames(names: List<String>): Schema {
     return Schema(types = newTypes)
 }
 
+/**
+ * Custom implementation of a [SerializerFactory] where we need to give it a class carpenter
+ * rather than have it create its own
+ */
+class SerializerFactoryExternalCarpenter(override val classCarpenter: ClassCarpenter)
+    : SerializerFactory (classCarpenter.whitelist, classCarpenter.classloader)
+
 open class AmqpCarpenterBase(whitelist: ClassWhitelist) {
     var cc = ClassCarpenter(whitelist = whitelist)
-    var factory = SerializerFactory(AllWhitelist, cc.classloader)
+    var factory = SerializerFactoryExternalCarpenter(cc)
 
     fun serialise(clazz: Any) = SerializationOutput(factory).serialize(clazz)
     fun testName(): String = Thread.currentThread().stackTrace[2].methodName