From 57ba9cdf06f1ef13afbf42ac7a0e3fa6a306f681 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Fri, 2 Feb 2018 16:58:43 +0000 Subject: [PATCH] CORDA-915 - Replace BEANS introspector with standard reflection (#2400) * CORDA-915 - Replace BEANS introspector with standard reflection Removes lib dependency and puts something in place we can better control * CORDA-915 - Review comment corrections * Review Comments --- .../serialization/amqp/SerializationHelper.kt | 239 +++++++++++++----- .../amqp/JavaPrivatePropertyTests.java | 109 ++++++++ .../serialization/amqp/GenericsTests.kt | 25 ++ .../amqp/PrivatePropertyTests.kt | 67 ++++- 4 files changed, 374 insertions(+), 66 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 9dd0fb30cf..b65ec08f58 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 @@ -6,11 +6,9 @@ import net.corda.core.serialization.ClassWhitelist import net.corda.core.serialization.CordaSerializable import net.corda.core.serialization.SerializationContext import org.apache.qpid.proton.codec.Data -import java.beans.IndexedPropertyDescriptor -import java.beans.Introspector -import java.beans.PropertyDescriptor import java.io.NotSerializableException import java.lang.reflect.* +import java.lang.reflect.Field import java.util.* import kotlin.reflect.KClass import kotlin.reflect.KFunction @@ -72,62 +70,167 @@ internal fun constructorForDeserialization(type: Type): KFunction? { internal fun propertiesForSerialization( kotlinConstructor: KFunction?, type: Type, - factory: SerializerFactory) = PropertySerializers.make ( - if (kotlinConstructor != null) { - propertiesForSerializationFromConstructor(kotlinConstructor, type, factory) - } else { - propertiesForSerializationFromAbstract(type.asClass()!!, type, factory) - }.sortedWith(PropertyAccessor)) + 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)) +/** + * Encapsulates the property of a class and its potential getter and setter methods. + * + * @property field a property of a class. + * @property setter the method of a class that sets the field. Determined by locating + * a function called setXyz on the class for the property named in field as xyz. + * @property getter the method of a class that returns a fields value. Determined by + * locating a function named getXyz for the property named in field as xyz. + */ +data class PropertyDescriptor(var field: Field?, var setter: Method?, var getter: Method?) { + override fun toString() = StringBuilder("").apply { + appendln("Property - ${field?.name ?: "null field"}\n") + appendln(" getter - ${getter?.name ?: "no getter"}") + append(" setter - ${setter?.name ?: "no setter"}") + }.toString() +} + +object PropertyDescriptorsRegex { + // match an uppercase letter that also has a corresponding lower case equivalent + val re = Regex("(?get|set|is)(?\\p{Lu}.*)") +} + +/** + * Collate the properties of a class and match them with their getter and setter + * methods as per a JavaBean. + * + * for a property + * exampleProperty + * + * We look for methods + * setExampleProperty + * getExampleProperty + * isExampleProperty + * + * Where setExampleProperty must return a type compatible with exampleProperty, getExampleProperty must + * take a single parameter of a type compatible with exampleProperty and isExampleProperty must + * return a boolean + */ +fun Class.propertyDescriptors(): Map { + val classProperties = mutableMapOf() + + var clazz: Class? = this + do { + // get the properties declared on this instance of class + clazz!!.declaredFields.forEach { classProperties.put(it.name, PropertyDescriptor(it, null, null)) } + + // then pair them up with the declared getter and setter + // Note: It is possible for a class to have multipleinstancess of a function where the types + // differ. For example: + // interface I { val a: T } + // class D(override val a: String) : I + // instances of D will have both + // getA - returning a String (java.lang.String) and + // getA - returning an Object (java.lang.Object) + // In this instance we take the most derived object + clazz.declaredMethods?.map { + PropertyDescriptorsRegex.re.find(it.name)?.apply { + try { + classProperties.getValue(groups[2]!!.value.decapitalize()).apply { + when (groups[1]!!.value) { + "set" -> { + if (setter == null) setter = it + else if (TypeToken.of(setter!!.genericReturnType).isSupertypeOf(it.genericReturnType)) { + setter = it + } + } + "get" -> { + if (getter == null) getter = it + else if (TypeToken.of(getter!!.genericReturnType).isSupertypeOf(it.genericReturnType)) { + getter = it + } + } + "is" -> { + val rtnType = TypeToken.of(it.genericReturnType) + if ((rtnType == TypeToken.of(Boolean::class.java)) + || (rtnType == TypeToken.of(Boolean::class.javaObjectType))) { + if (getter == null) getter = it + } + } + } + } + } catch (e: NoSuchElementException) { + // handles the getClass case from java.lang.Object + return@apply + } + } + } + clazz = clazz?.superclass + } while (clazz != null) + + return classProperties +} + +/** + * From a constructor, determine which properties of a class are to be serialized. + * + * @param kotlinConstructor The constructor to be used to instantiate instances of the class + * @param type The class's [Type] + * @param factory The factory generating the serializer wrapping this function. + */ internal fun propertiesForSerializationFromConstructor( kotlinConstructor: KFunction, type: Type, factory: SerializerFactory): List { val clazz = (kotlinConstructor.returnType.classifier as KClass<*>).javaObjectType - // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans. - val properties = Introspector.getBeanInfo(clazz).propertyDescriptors - .filter { it.name != "class" } - .groupBy { it.name } - .mapValues { it.value[0] } - if (properties.isNotEmpty() && kotlinConstructor.parameters.isEmpty()) { - return propertiesForSerializationFromSetters(properties, type, factory) + val classProperties = clazz.propertyDescriptors() + + if (classProperties.isNotEmpty() && kotlinConstructor.parameters.isEmpty()) { + return propertiesForSerializationFromSetters(classProperties, type, factory) } return mutableListOf().apply { kotlinConstructor.parameters.withIndex().forEach { param -> - val name = param.value.name ?: throw NotSerializableException("Constructor parameter of $clazz has no name.") + val name = param.value.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]!! + val propertyReader = if (name in classProperties) { + if (classProperties[name]!!.getter != null) { + // it's a publicly accessible property + val matchingProperty = classProperties[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.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 type '$returnType' for '$name' of '$clazz' 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 class '$clazz' but differs from constructor " + + "parameter type '${param.value.type.javaType}'") + } + + Pair(PublicPropertyReader(getter), returnType) + } else { + 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") + } } - - Pair(PublicPropertyReader(getter), returnType) } else { - 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") - } + throw NotSerializableException( + "Constructor parameter $name doesn't refer to a property of class '$clazz'") } this += PropertyAccessorConstructor( @@ -148,18 +251,26 @@ private fun propertiesForSerializationFromSetters( return mutableListOf().apply { var idx = 0 properties.forEach { property -> - val getter: Method? = property.value.readMethod - val setter: Method? = property.value.writeMethod + val getter: Method? = property.value.getter + val setter: Method? = property.value.setter 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 + if (setter.parameterCount != 1) { + throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + + "takes too many arguments") + } - this += PropertyAccessorGetterSetter ( + val setterType = setter.parameterTypes.getOrNull(0)!! + if (!(TypeToken.of(property.value.field?.genericType!!).isSupertypeOf(setterType))) { + throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + + "takes parameter of type $setterType yet underlying type is " + + "${property.value.field?.genericType!!}") + } + + this += PropertyAccessorGetterSetter( idx++, - PropertySerializer.make(property.key, PublicPropertyReader(getter), + PropertySerializer.make(property.value.field!!.name, PublicPropertyReader(getter), resolveTypeVariables(getter.genericReturnType, type), factory), setter) } @@ -186,23 +297,17 @@ private fun propertiesForSerializationFromAbstract( clazz: Class<*>, type: Type, factory: SerializerFactory): List { - // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans. - val properties = Introspector.getBeanInfo(clazz).propertyDescriptors - .filter { it.name != "class" } - .sortedBy { it.name } - .filterNot { it is IndexedPropertyDescriptor } + val properties = clazz.propertyDescriptors() - return mutableListOf().apply { - properties.withIndex().forEach { property -> - // Check that the method has a getter in java. - val getter = property.value.readMethod ?: throw NotSerializableException( - "Property has no getter method for ${property.value.name} of $clazz.") - val returnType = resolveTypeVariables(getter.genericReturnType, type) - this += PropertyAccessorConstructor( - property.index, - PropertySerializer.make(property.value.name, PublicPropertyReader(getter), returnType, factory)) - } - } + return mutableListOf().apply { + properties.toList().withIndex().forEach { + val getter = it.value.second.getter ?: return@forEach + val returnType = resolveTypeVariables(getter.genericReturnType, type) + this += PropertyAccessorConstructor( + it.index, + PropertySerializer.make(it.value.first, PublicPropertyReader(getter), returnType, factory)) + } + } } internal fun interfacesForSerialization(type: Type, serializerFactory: SerializerFactory): List { @@ -270,7 +375,11 @@ private fun resolveTypeVariables(actualType: Type, contextType: Type?): Type { // TODO: surely we check it is concrete at this point with no TypeVariables return if (resolvedType is TypeVariable<*>) { val bounds = resolvedType.bounds - return if (bounds.isEmpty()) SerializerFactory.AnyType else if (bounds.size == 1) resolveTypeVariables(bounds[0], contextType) else throw NotSerializableException("Got bounded type $actualType but only support single bound.") + return if (bounds.isEmpty()) { + SerializerFactory.AnyType + } else if (bounds.size == 1) { + resolveTypeVariables(bounds[0], contextType) + } else throw NotSerializableException("Got bounded type $actualType but only support single bound.") } else { resolvedType } diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java index 59a1465710..b13cc2cc48 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaPrivatePropertyTests.java @@ -23,6 +23,115 @@ public class JavaPrivatePropertyTests { public String getA() { return a; } } + static class B { + private Boolean b; + + B(Boolean b) { this.b = b; } + + public Boolean isB() { + return this.b; + } + } + + static class B2 { + private Boolean b; + + public Boolean isB() { + return this.b; + } + + public void setB(Boolean b) { + this.b = b; + } + } + + static class B3 { + private Boolean b; + + // break the BEAN format explicitly (i.e. it's not isB) + public Boolean isb() { + return this.b; + } + + public void setB(Boolean b) { + this.b = b; + } + } + + static class C3 { + private Integer a; + + public Integer getA() { + return this.a; + } + + public Boolean isA() { + return this.a > 0; + } + + public void setA(Integer a) { + this.a = a; + } + } + + @Test + public void singlePrivateBooleanWithConstructor() throws NotSerializableException, NoSuchFieldException, IllegalAccessException { + EvolutionSerializerGetterBase evolutionSerializerGetter = new EvolutionSerializerGetter(); + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + evolutionSerializerGetter); + SerializationOutput ser = new SerializationOutput(factory); + DeserializationInput des = new DeserializationInput(factory); + + B b = new B(true); + B b2 = des.deserialize(ser.serialize(b), B.class); + assertEquals (b.b, b2.b); + } + + @Test + public void singlePrivateBooleanWithNoConstructor() throws NotSerializableException, NoSuchFieldException, IllegalAccessException { + EvolutionSerializerGetterBase evolutionSerializerGetter = new EvolutionSerializerGetter(); + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + evolutionSerializerGetter); + SerializationOutput ser = new SerializationOutput(factory); + DeserializationInput des = new DeserializationInput(factory); + + B2 b = new B2(); + b.setB(false); + B2 b2 = des.deserialize(ser.serialize(b), B2.class); + assertEquals (b.b, b2.b); + } + + @Test + public void testCapitilsationOfIs() throws NotSerializableException, NoSuchFieldException, IllegalAccessException { + EvolutionSerializerGetterBase evolutionSerializerGetter = new EvolutionSerializerGetter(); + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + evolutionSerializerGetter); + SerializationOutput ser = new SerializationOutput(factory); + DeserializationInput des = new DeserializationInput(factory); + + B3 b = new B3(); + b.setB(false); + B3 b2 = des.deserialize(ser.serialize(b), B3.class); + + // since we can't find a getter for b (isb != isB) then we won't serialize that parameter + assertEquals (null, b2.b); + } + + @Test + public void singlePrivateIntWithBoolean() throws NotSerializableException, NoSuchFieldException, IllegalAccessException { + EvolutionSerializerGetterBase evolutionSerializerGetter = new EvolutionSerializerGetter(); + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + evolutionSerializerGetter); + SerializationOutput ser = new SerializationOutput(factory); + DeserializationInput des = new DeserializationInput(factory); + + C3 c = new C3(); + c.setA(12345); + C3 c2 = des.deserialize(ser.serialize(c), C3.class); + + assertEquals (c.a, c2.a); + } + @Test public void singlePrivateWithConstructor() throws NotSerializableException, NoSuchFieldException, IllegalAccessException { EvolutionSerializerGetterBase evolutionSerializerGetter = new EvolutionSerializerGetter(); diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/GenericsTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/GenericsTests.kt index 60830cda88..6490f48522 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/GenericsTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/GenericsTests.kt @@ -501,4 +501,29 @@ class GenericsTests { assertEquals(state.context.a, des1.obj.state.context.a) } + fun implemntsGeneric() { + open class B(open val a: T) + class D(override val a: String) : B(a) + + val factory = testDefaultFactoryNoEvolution() + + val bytes = SerializationOutput(factory).serialize(D("Test")) + + DeserializationInput(factory).deserialize(bytes).apply { assertEquals("Test", this.a) } + } + + interface implementsGenericInterfaceI { + val a: T + } + + @Test + fun implemntsGenericInterface() { + class D(override val a: String) : implementsGenericInterfaceI + + val factory = testDefaultFactoryNoEvolution() + + val bytes = SerializationOutput(factory).serialize(D("Test")) + + DeserializationInput(factory).deserialize(bytes).apply { assertEquals("Test", this.a) } + } } 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 0ee20ab311..a79416ff6f 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 @@ -6,6 +6,8 @@ import org.slf4j.Logger import org.slf4j.LoggerFactory import org.junit.Test import org.apache.qpid.proton.amqp.Symbol +import org.assertj.core.api.Assertions +import java.io.NotSerializableException import java.util.concurrent.ConcurrentHashMap class PrivatePropertyTests { @@ -29,6 +31,15 @@ class PrivatePropertyTests { assertEquals(c1, c2) } + @Test + fun testWithOnePrivatePropertyBoolean() { + data class C(private val b: Boolean) + + C(false).apply { + assertEquals(this, DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(this))) + } + } + @Test fun testWithOnePrivatePropertyNullableNotNull() { data class C(private val b: String?) @@ -56,6 +67,61 @@ class PrivatePropertyTests { assertEquals(c1, c2) } + @Test + fun testWithInheritance() { + open class B(val a: String, protected val b: String) + class D (a: String, b: String) : B (a, b) { + override fun equals(other: Any?): Boolean = when (other) { + is D -> other.a == a && other.b == b + else -> false + } + } + + val d1 = D("clump", "lump") + val d2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(d1)) + + assertEquals(d1, d2) + } + + @Test + fun testMultiArgSetter() { + @Suppress("UNUSED") + data class C(private var a: Int, val b: Int) { + // This will force the serialization engine to use getter / setter + // instantiation for the object rather than construction + @ConstructorForDeserialization + constructor() : this(0, 0) + + fun setA(a: Int, b: Int) { this.a = a } + fun getA() = a + } + + val c1 = C(33, 44) + Assertions.assertThatThrownBy { + SerializationOutput(factory).serialize(c1) + }.isInstanceOf(NotSerializableException::class.java).hasMessageContaining( + "Defined setter for parameter a takes too many arguments") + } + + @Test + fun testBadTypeArgSetter() { + @Suppress("UNUSED") + data class C(private var a: Int, val b: Int) { + @ConstructorForDeserialization + constructor() : this(0, 0) + + fun setA(a: String) { this.a = a.toInt() } + fun getA() = a + } + + val c1 = C(33, 44) + Assertions.assertThatThrownBy { + SerializationOutput(factory).serialize(c1) + }.isInstanceOf(NotSerializableException::class.java).hasMessageContaining( + "Defined setter for parameter a takes parameter of type class java.lang.String " + + "yet underlying type is int ") + } + @Test fun testWithOnePublicOnePrivateProperty2() { data class C(val a: Int, private val b: Int) @@ -127,7 +193,6 @@ class PrivatePropertyTests { // 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)