From cacdba872eabdbb14fa5b81192f5977c8c72b879 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Wed, 10 Jan 2018 11:41:49 +0000 Subject: [PATCH] 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 | 28 +++-- docs/source/serialization.rst | 91 +++++++++++--- .../serialization/amqp/EvolutionSerializer.kt | 2 +- .../serialization/amqp/ObjectSerializer.kt | 2 + .../serialization/amqp/PropertySerializer.kt | 100 +++++++++++---- .../serialization/amqp/SerializationHelper.kt | 69 ++++++----- .../serialization/amqp/SerializerFactory.kt | 2 + .../amqp/custom/ThrowableSerializer.kt | 2 +- .../serialization/amqp/ErrorMessageTests.java | 2 + .../amqp/JavaPrivatePropertyTests.java | 79 ++++++++++++ .../serialization/amqp/AMQPTestUtils.kt | 2 - .../serialization/amqp/ErrorMessagesTests.kt | 9 ++ .../amqp/PrivatePropertyTests.kt | 117 ++++++++++++++++++ .../carpenter/ClassCarpenterTestUtils.kt | 9 +- 14 files changed, 430 insertions(+), 84 deletions(-) create mode 100644 node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java create mode 100644 node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt diff --git a/docs/source/release-notes.rst b/docs/source/release-notes.rst index 64d9ad8881..7b3e96fe4c 100644 --- a/docs/source/release-notes.rst +++ b/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 -brings a stable format Corda can support internally throughout it's lifetime that meets the needs of Corda and our -users. + Details on the AMQP serialization framework can be found in the :doc:`serialization` document :ref:`here `. + This provides an introduction and overview of the framework whilst more specific details on object evolution as it relates to + 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 `. + + .. 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 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. + 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 + 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 ---------- diff --git a/docs/source/serialization.rst b/docs/source/serialization.rst index ed5a278feb..f217d749b9 100644 --- a/docs/source/serialization.rst +++ b/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 ````` 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 663a153cbd..9293729306 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 @@ -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 { 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 6cba3bbea5..a9d099b2e3 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 @@ -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)}") 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 d296a8fd91..af7c089552 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,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().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. */ -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 - } - - 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 - } + return isJVMPrimitive || !(propertyReader.isNullable()) } companion object { - private val logger = contextLogger() - fun make(name: String, readMethod: Method?, resolvedType: Type, factory: SerializerFactory): PropertySerializer { - readMethod?.isAccessible = true + fun make(name: String, readMethod: PropertyReader, resolvedType: Type, factory: SerializerFactory): PropertySerializer { 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() } } 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 5e3e28711a..e7b21f5e38 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 @@ -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, - val setters : Collection) + val getters: Collection, + val setters: Collection) /** * Code for finding the constructor we will use for deserialization. @@ -100,26 +101,31 @@ private fun propertiesForSerializationFromConstructor( for (param in kotlinConstructor.parameters) { val name = param.name ?: throw NotSerializableException("Constructor parameter of $clazz has no name.") - val matchingProperty = properties[name] ?: - try { - 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") - } + 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") - val returnType = resolveTypeVariables(getter.genericReturnType, type) - if (constructorParamTakesReturnTypeOfGetter(returnType, getter.genericReturnType, param)) { - rc += PropertySerializer.make(name, getter, returnType, factory) + // 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) + } 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 propertiesForSerializationFromConstructor( * and use those */ private fun propertiesForSerializationFromSetters( - properties : Map, + properties: Map, type: Type, factory: SerializerFactory): ConstructorDestructorMethods { - val getters : MutableList = ArrayList(properties.size) - val setters : MutableList = ArrayList(properties.size) + val getters: MutableList = ArrayList(properties.size) + val setters: MutableList = 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), - factory) + getters += PropertySerializer.make(property.key, PublicPropertyReader(getter), + 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 = 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()) } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt index 0dbc3f8be9..a6c587bc6e 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt @@ -40,6 +40,7 @@ open class SerializerFactory(val whitelist: ClassWhitelist, cl: ClassLoader) { val transformsCache = ConcurrentHashMap>>() 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 = "?" } } + 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 5700c08be7..93d8b0fbed 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 @@ -25,7 +25,7 @@ class ThrowableSerializer(factory: SerializerFactory) : CustomSerializer.Proxy> serializersByDescriptor = + (ConcurrentHashMap>) 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> serializersByDescriptor = + (ConcurrentHashMap>) 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); + } +} diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt index 08d7af6ebb..8bbe597b26 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt +++ b/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()) diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/ErrorMessagesTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/ErrorMessagesTests.kt index 15f543da0d..6aab88660a 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/ErrorMessagesTests.kt +++ b/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) 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 new file mode 100644 index 0000000000..51405e8c26 --- /dev/null +++ b/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> + + 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> + + 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) + } +} \ No newline at end of file diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenterTestUtils.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenterTestUtils.kt index 8773140f6d..a27b6440d3 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenterTestUtils.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenterTestUtils.kt @@ -35,9 +35,16 @@ fun Schema.mangleNames(names: List): 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