From 9e9fab7e1a1a51a1d01d01a39ef19e4c71e93082 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Fri, 2 Mar 2018 13:49:30 +0000 Subject: [PATCH] CORDA-1134 - Don't use private serializers for all CAPS properties (#2708) Whilst it does currently work it only does so by going down an incorrect code path. A property THING that is public and has a getter should be fetched using that getter, not trip into the private property accessor. --- .../serialization/amqp/SerializationHelper.kt | 74 ++++++++++++------- .../amqp/PrivatePropertyTests.kt | 28 +++++++ 2 files changed, 74 insertions(+), 28 deletions(-) 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 83e3486d5a..435498648a 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 @@ -136,7 +136,16 @@ fun Class.propertyDescriptors(): Map { this.field = property } } + clazz = clazz.superclass + } while (clazz != null) + // + // Running as two loops rather than one as we need to ensure we have captured all of the properties + // before looking for interacting methods and need to cope with the class hierarchy introducing + // new properties / methods + // + clazz = this + do { // Note: It is possible for a class to have multiple instances of a function where the types // differ. For example: // interface I { val a: T } @@ -148,14 +157,23 @@ fun Class.propertyDescriptors(): Map { // // In addition, only getters that take zero parameters and setters that take a single // parameter will be considered - clazz.declaredMethods?.map { func -> + clazz!!.declaredMethods?.map { func -> if (!Modifier.isPublic(func.modifiers)) return@map if (func.name == "getClass") return@map PropertyDescriptorsRegex.re.find(func.name)?.apply { - // take into account those constructor properties that don't directly map to a named - // property which are, by default, already added to the map - classProperties.computeIfAbsent(groups[2]!!.value.decapitalize()) { PropertyDescriptor() }.apply { + // matching means we have an func getX where the property could be x or X + // so having pre-loaded all of the properties we try to match to either case. If that + // fails the getter doesn't refer to a property directly, but may to a cosntructor + // parameter that shadows a property + val properties = + classProperties[groups[2]!!.value] ?: + classProperties[groups[2]!!.value.decapitalize()] ?: + // take into account those constructor properties that don't directly map to a named + // property which are, by default, already added to the map + classProperties.computeIfAbsent(groups[2]!!.value) { PropertyDescriptor() } + + properties.apply { when (groups[1]!!.value) { "set" -> { if (func.parameterCount == 1) { @@ -221,38 +239,38 @@ internal fun propertiesForSerializationFromConstructor( // it so just ignore it as it'll be supplied at runtime anyway on invocation val name = param.value.name ?: return@forEach - val propertyReader = if (name in classProperties) { - if (classProperties[name]!!.getter != null) { - // it's a publicly accessible property - val matchingProperty = classProperties[name]!! + // We will already have disambiguated getA for property A or a but we still need to cope + // with the case we don't know the case of A when the parameter doesn't match a property + // but has a getter + val matchingProperty = classProperties[name] ?: classProperties[name.capitalize()] ?: + throw NotSerializableException( + "Constructor parameter - \"$name\" - doesn't refer to a property of \"$clazz\"") - // Check that the method has a getter in java. - val getter = matchingProperty.getter ?: 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.") + // If the property has a getter we'll use that to retrieve it's value from the instance, if it doesn't + // *for *know* we switch to a reflection based method + val propertyReader = if (matchingProperty.getter != null) { + val getter = matchingProperty.getter ?: 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.value)) { - throw NotSerializableException( - "Property - \"$name\" - has type \"$returnType\" on \"$clazz\" but differs from constructor " + - "parameter type \"${param.value.type.javaType}\"") - } + val returnType = resolveTypeVariables(getter.genericReturnType, type) + if (!constructorParamTakesReturnTypeOfGetter(returnType, getter.genericReturnType, param.value)) { + throw NotSerializableException( + "Property - \"$name\" - has type \"$returnType\" on \"$clazz\" but differs from constructor " + + "parameter type \"${param.value.type.javaType}\"") + } - Pair(PublicPropertyReader(getter), returnType) - } else { - val field = classProperties[name]!!.field ?: + Pair(PublicPropertyReader(getter), returnType) + } else { + val field = classProperties[name]!!.field ?: 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") - Pair(PrivatePropertyReader(field, type), field.genericType) - } - } else { - throw NotSerializableException( - "Constructor parameter - \"$name\" - doesn't refer to a property of \"$clazz\"") + Pair(PrivatePropertyReader(field, type), field.genericType) } this += PropertyAccessorConstructor( diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt index ae35d46286..3f8efa4407 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt @@ -196,4 +196,32 @@ class PrivatePropertyTests { assertEquals(c1, c2) } + + // + // Reproduces CORDA-1134 + // + @Suppress("UNCHECKED_CAST") + @Test + fun allCapsProprtyNotPrivate() { + data class C (val CCC: String) + + val output = SerializationOutput(factory).serializeAndReturnSchema(C("this is nice")) + + val serializersByDescriptor = fields["serializersByDesc"]!!.get(factory) as ConcurrentHashMap> + + val schemaDescriptor = output.schema.types.first().descriptor.name + serializersByDescriptor.filterKeys { (it as Symbol) == schemaDescriptor }.values.apply { + assertEquals(1, size) + + assertTrue(this.first() is ObjectSerializer) + val propertySerializers = (this.first() as ObjectSerializer).propertySerializers.serializationOrder.map { it.getter } + + // CCC is the only property to be serialised + assertEquals(1, propertySerializers.size) + + // and despite being all caps it should still be a public getter + assertTrue(propertySerializers[0].propertyReader is PublicPropertyReader) + } + } + } \ No newline at end of file