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

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<Any>? {
 internal fun <T : Any> propertiesForSerialization(
         kotlinConstructor: KFunction<T>?,
         type: Type,
-        factory: SerializerFactory) = PropertySerializers.make (
+        factory: SerializerFactory) = PropertySerializers.make(
-            if (kotlinConstructor != null) {
+        if (kotlinConstructor != null) {
-                propertiesForSerializationFromConstructor(kotlinConstructor, type, factory)
+            propertiesForSerializationFromConstructor(kotlinConstructor, type, factory)
-            } else {
+        } else {
-                propertiesForSerializationFromAbstract(type.asClass()!!, type, factory)
+            propertiesForSerializationFromAbstract(type.asClass()!!, type, factory)
-            }.sortedWith(PropertyAccessor))
+        }.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("(?<type>get|set|is)(?<var>\\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<out Any?>.propertyDescriptors(): Map<String, PropertyDescriptor> {
+    val classProperties = mutableMapOf<String, PropertyDescriptor>()
+
+    var clazz: Class<out Any?>? = 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<out T> { val a: T }
+        //      class D(override val a: String) : I<String>
+        // 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 <T : Any> propertiesForSerializationFromConstructor(
         kotlinConstructor: KFunction<T>,
         type: Type,
         factory: SerializerFactory): List<PropertyAccessor> {
     val clazz = (kotlinConstructor.returnType.classifier as KClass<*>).javaObjectType
-    // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans.
-    val properties = Introspector.getBeanInfo(clazz).propertyDescriptors
-            .filter { it.name != "class" }
-            .groupBy { it.name }
-            .mapValues { it.value[0] }
 
-    if (properties.isNotEmpty() && kotlinConstructor.parameters.isEmpty()) {
+    val classProperties = clazz.propertyDescriptors()
-        return propertiesForSerializationFromSetters(properties, type, factory)
+
+    if (classProperties.isNotEmpty() && kotlinConstructor.parameters.isEmpty()) {
+        return propertiesForSerializationFromSetters(classProperties, type, factory)
     }
 
     return mutableListOf<PropertyAccessor>().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) {
+            val propertyReader = if (name in classProperties) {
-                // it's a publicly accessible property
+                if (classProperties[name]!!.getter != null) {
-                val matchingProperty = properties[name]!!
+                    // it's a publicly accessible property
+                    val matchingProperty = classProperties[name]!!
 
-                // Check that the method has a getter in java.
+                    // Check that the method has a getter in java.
-                val getter = matchingProperty.readMethod ?: throw NotSerializableException(
+                    val getter = matchingProperty.getter ?: throw NotSerializableException(
-                        "Property has no getter method for $name of $clazz. If using Java and the parameter name"
+                            "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."
+                                    + "looks anonymous, check that you have the -parameters option specified in the "
-                                + "Alternately, provide a proxy serializer (SerializationCustomSerializer) if "
+                                    + "Java compiler. Alternately, provide a proxy serializer "
-                                + "recompiling isn't an option.")
+                                    + "(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.value)) {
+                    if (!constructorParamTakesReturnTypeOfGetter(returnType, getter.genericReturnType, param.value)) {
-                    throw NotSerializableException(
+                        throw NotSerializableException(
-                            "Property type '$returnType' for '$name' of '$clazz' differs from constructor parameter "
+                                "Property '$name' has type '$returnType' on class '$clazz' but differs from constructor " +
-                                    + "type '${param.value.type.javaType}'")
+                                "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 {
+                throw NotSerializableException(
-                    val field = clazz.getDeclaredField(param.value.name)
+                        "Constructor parameter $name doesn't refer to a property of class '$clazz'")
-                    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")
-                }
             }
 
             this += PropertyAccessorConstructor(
@@ -148,18 +251,26 @@ private fun propertiesForSerializationFromSetters(
     return mutableListOf<PropertyAccessorGetterSetter>().apply {
         var idx = 0
         properties.forEach { property ->
-            val getter: Method? = property.value.readMethod
+            val getter: Method? = property.value.getter
-            val setter: Method? = property.value.writeMethod
+            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
+            if (setter.parameterCount != 1) {
-            // the getter / setter vs property as if there is a difference then that property isn't reported
+                throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " +
-            // by the BEAN inspector and thus we don't consider that case here
+                        "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<PropertyAccessor> {
-    // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans.
+    val properties = clazz.propertyDescriptors()
-    val properties = Introspector.getBeanInfo(clazz).propertyDescriptors
-            .filter { it.name != "class" }
-            .sortedBy { it.name }
-            .filterNot { it is IndexedPropertyDescriptor }
 
-   return mutableListOf<PropertyAccessorConstructor>().apply {
+    return mutableListOf<PropertyAccessorConstructor>().apply {
-       properties.withIndex().forEach { property ->
+        properties.toList().withIndex().forEach {
-           // Check that the method has a getter in java.
+            val getter = it.value.second.getter ?: return@forEach
-           val getter = property.value.readMethod ?: throw NotSerializableException(
+            val returnType = resolveTypeVariables(getter.genericReturnType, type)
-                   "Property has no getter method for ${property.value.name} of $clazz.")
+            this += PropertyAccessorConstructor(
-           val returnType = resolveTypeVariables(getter.genericReturnType, type)
+                    it.index,
-           this += PropertyAccessorConstructor(
+                    PropertySerializer.make(it.value.first, PublicPropertyReader(getter), returnType, factory))
-                   property.index,
+        }
-                   PropertySerializer.make(property.value.name, PublicPropertyReader(getter), returnType, factory))
+    }
-       }
-   }
 }
 
 internal fun interfacesForSerialization(type: Type, serializerFactory: SerializerFactory): List<Type> {
@@ -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
     }

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

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<out T>(open val a: T)
+        class D(override val a: String) : B<String>(a)
+
+        val factory = testDefaultFactoryNoEvolution()
+
+        val bytes = SerializationOutput(factory).serialize(D("Test"))
+
+        DeserializationInput(factory).deserialize(bytes).apply { assertEquals("Test", this.a) }
+    }
+
+    interface implementsGenericInterfaceI<out T> {
+        val a: T
+    }
+
+    @Test
+    fun implemntsGenericInterface() {
+        class D(override val a: String) : implementsGenericInterfaceI<String>
+
+        val factory = testDefaultFactoryNoEvolution()
+
+        val bytes = SerializationOutput(factory).serialize(D("Test"))
+
+        DeserializationInput(factory).deserialize(bytes).apply { assertEquals("Test", this.a) }
+    }
 }

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)