diff --git a/.idea/compiler.xml b/.idea/compiler.xml index a33ce1e0f0..d8d9f1498a 100644 --- a/.idea/compiler.xml +++ b/.idea/compiler.xml @@ -159,4 +159,4 @@ - \ No newline at end of file + 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 103e0d082e..b2dfa16f53 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 @@ -21,12 +21,11 @@ import kotlin.reflect.jvm.javaType * @property constructorArgs used to hold the properties as sent to the object's constructor. Passed in as a * pre populated array as properties not present on the old constructor must be initialised in the factory */ -class EvolutionSerializer( +abstract class EvolutionSerializer( clazz: Type, factory: SerializerFactory, - private val oldReaders: Map, - override val kotlinConstructor: KFunction?, - private val constructorArgs: Array) : ObjectSerializer(clazz, factory) { + protected val oldReaders: Map, + override val kotlinConstructor: KFunction?) : ObjectSerializer(clazz, factory) { // explicitly set as empty to indicate it's unused by this type of serializer override val propertySerializers = PropertySerializersEvolution() @@ -35,12 +34,11 @@ class EvolutionSerializer( * Represents a parameter as would be passed to the constructor of the class as it was * when it was serialised and NOT how that class appears now * - * @param type The jvm type of the parameter * @param resultsIndex index into the constructor argument list where the read property * should be placed * @param property object to read the actual property value */ - data class OldParam(val type: Type, var resultsIndex: Int, val property: PropertySerializer) { + data class OldParam(var resultsIndex: Int, val property: PropertySerializer) { fun readProperty(obj: Any?, schemas: SerializationSchemas, input: DeserializationInput, new: Array) = property.readProperty(obj, schemas, input).apply { if(resultsIndex >= 0) { @@ -62,9 +60,10 @@ class EvolutionSerializer( */ private fun getEvolverConstructor(type: Type, oldArgs: Map): KFunction? { val clazz: Class<*> = type.asClass()!! + if (!isConcrete(clazz)) return null - val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.type) } + val oldArgumentSet = oldArgs.map { Pair(it.key as String?, it.value.property.resolvedType) } var maxConstructorVersion = Integer.MIN_VALUE var constructor: KFunction? = null @@ -82,6 +81,36 @@ class EvolutionSerializer( return constructor ?: constructorForDeserialization(type) } + private fun makeWithConstructor( + new: ObjectSerializer, + factory: SerializerFactory, + constructor: KFunction, + readersAsSerialized: Map): AMQPSerializer { + val constructorArgs = arrayOfNulls(constructor.parameters.size) + + constructor.parameters.withIndex().forEach { + readersAsSerialized.get(it.value.name!!)?.apply { + this.resultsIndex = it.index + } ?: if (!it.value.type.isMarkedNullable) { + throw NotSerializableException( + "New parameter ${it.value.name} is mandatory, should be nullable for evolution to worK") + } + } + return EvolutionSerializerViaConstructor (new.type, factory, readersAsSerialized, constructor, constructorArgs) + } + + private fun makeWithSetters( + new: ObjectSerializer, + factory: SerializerFactory, + constructor: KFunction, + readersAsSerialized: Map, + classProperties: Map): AMQPSerializer { + val setters = propertiesForSerializationFromSetters(classProperties, + new.type, + factory).associateBy({ it.getter.name }, { it }) + return EvolutionSerializerViaSetters (new.type, factory, readersAsSerialized, constructor, setters) + } + /** * Build a serialization object for deserialization only of objects serialised * as different versions of a class. @@ -95,46 +124,43 @@ class EvolutionSerializer( */ fun make(old: CompositeType, new: ObjectSerializer, factory: SerializerFactory): AMQPSerializer { - - val readersAsSerialized = linkedMapOf( - *(old.fields.map { - val returnType = try { - it.getTypeAsClass(factory.classloader) - } catch (e: ClassNotFoundException) { - throw NotSerializableException(e.message) - } - - it.name to OldParam( - returnType, - -1, - PropertySerializer.make( - it.name, PublicPropertyReader(null), returnType, factory)) - }.toTypedArray()) - ) - - val constructor = getEvolverConstructor(new.type, readersAsSerialized) ?: - throw NotSerializableException( - "Attempt to deserialize an interface: ${new.type}. Serialized form is invalid.") - - val constructorArgs = arrayOfNulls(constructor.parameters.size) - - constructor.parameters.withIndex().forEach { - readersAsSerialized.get(it.value.name!!)?.apply { - this.resultsIndex = it.index - } ?: if (!it.value.type.isMarkedNullable) { - throw NotSerializableException( - "New parameter ${it.value.name} is mandatory, should be nullable for evolution to worK") + // The order in which the properties were serialised is important and must be preserved + val readersAsSerialized = LinkedHashMap() + old.fields.forEach { + readersAsSerialized[it.name] = try { + OldParam(-1, PropertySerializer.make(it.name, EvolutionPropertyReader(), + it.getTypeAsClass(factory.classloader), factory)) + } catch (e: ClassNotFoundException) { + throw NotSerializableException(e.message) } } - return EvolutionSerializer(new.type, factory, readersAsSerialized, constructor, constructorArgs) + // cope with the situation where a generic interface was serialised as a type, in such cases + // return the synthesised object which is, given the absence of a constructor, a no op + val constructor = getEvolverConstructor(new.type, readersAsSerialized) ?: return new + + val classProperties = new.type.asClass()?.propertyDescriptors() ?: emptyMap() + + return if (classProperties.isNotEmpty() && constructor.parameters.isEmpty()) { + makeWithSetters(new, factory, constructor, readersAsSerialized, classProperties) + } + else { + makeWithConstructor(new, factory, constructor, readersAsSerialized) + } } } - override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput, offset: Int) { + override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput, debugIndent: Int) { throw UnsupportedOperationException("It should be impossible to write an evolution serializer") } +} +class EvolutionSerializerViaConstructor( + clazz: Type, + factory: SerializerFactory, + oldReaders: Map, + kotlinConstructor: KFunction?, + private val constructorArgs: Array) : EvolutionSerializer (clazz, factory, oldReaders, kotlinConstructor) { /** * Unlike a normal [readObject] call where we simply apply the parameter deserialisers * to the object list of values we need to map that list, which is ordered per the @@ -151,7 +177,36 @@ class EvolutionSerializer( oldReaders.values.zip(obj).map { it.first.readProperty(it.second, schemas, input, constructorArgs) } return javaConstructor?.newInstance(*(constructorArgs)) ?: - throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.") + throw NotSerializableException( + "Attempt to deserialize an interface: $clazz. Serialized form is invalid.") + } +} + +/** + * Specific instance of an [EvolutionSerializer] where the properties of the object are set via calling + * named setter functions on the instantiated object. + */ +class EvolutionSerializerViaSetters( + clazz: Type, + factory: SerializerFactory, + oldReaders: Map, + kotlinConstructor: KFunction?, + private val setters: Map) : EvolutionSerializer (clazz, factory, oldReaders, kotlinConstructor) { + + override fun readObject(obj: Any, schemas: SerializationSchemas, input: DeserializationInput): Any { + if (obj !is List<*>) throw NotSerializableException("Body of described type is unexpected $obj") + + val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException ( + "Failed to instantiate instance of object $clazz") + + // *must* read all the parameters in the order they were serialized + oldReaders.values.zip(obj).forEach { + // if that property still exists on the new object then set it + it.first.property.readProperty(it.second, schemas, input).apply { + setters[it.first.property.name]?.set(instance, this) + } + } + return instance } } @@ -177,12 +232,24 @@ class EvolutionSerializerGetter : EvolutionSerializerGetterBase() { override fun getEvolutionSerializer(factory: SerializerFactory, typeNotation: TypeNotation, newSerializer: AMQPSerializer, - schemas: SerializationSchemas): AMQPSerializer = - factory.getSerializersByDescriptor().computeIfAbsent(typeNotation.descriptor.name!!) { - when (typeNotation) { - is CompositeType -> EvolutionSerializer.make(typeNotation, newSerializer as ObjectSerializer, factory) - is RestrictedType -> EnumEvolutionSerializer.make(typeNotation, newSerializer, factory, schemas) + schemas: SerializationSchemas): AMQPSerializer { + return factory.getSerializersByDescriptor().computeIfAbsent(typeNotation.descriptor.name!!) { + when (typeNotation) { + is CompositeType -> EvolutionSerializer.make(typeNotation, newSerializer as ObjectSerializer, factory) + is RestrictedType -> { + // The fingerprint of a generic collection can be changed through bug fixes to the + // fingerprinting function making it appear as if the class has altered whereas it hasn't. + // Given we don't support the evolution of these generic containers, if it appears + // one has been changed, simply return the original serializer and associate it with + // both the new and old fingerprint + if (newSerializer is CollectionSerializer || newSerializer is MapSerializer) { + newSerializer + } else { + EnumEvolutionSerializer.make(typeNotation, newSerializer, factory, schemas) + } } } + } + } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt index 537912a044..9029dd171b 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializers.kt @@ -14,6 +14,9 @@ abstract class PropertyReader { abstract fun isNullable(): Boolean } +/** + * Accessor for those properties of a class that have defined getter functions. + */ class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() { init { readMethod?.isAccessible = true @@ -43,6 +46,10 @@ class PublicPropertyReader(private val readMethod: Method?) : PropertyReader() { override fun isNullable(): Boolean = readMethod?.returnsNullable() ?: false } +/** + * Accessor for those properties of a class that do not have defined getter functions. In which case + * we used reflection to remove the unreadable status from that property whilst it's accessed. + */ class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader() { init { loggerFor().warn("Create property Serializer for private property '${field.name}' not " @@ -68,6 +75,20 @@ class PrivatePropertyReader(val field: Field, parentType: Type) : PropertyReader } } +/** + * Special instance of a [PropertyReader] for use only by [EvolutionSerializer]s to make + * it explicit that no properties are ever actually read from an object as the evolution + * serializer should only be accessing the already serialized form. + */ +class EvolutionPropertyReader : PropertyReader() { + override fun read(obj: Any?): Any? { + throw UnsupportedOperationException("It should be impossible for an evolution serializer to " + + "be reading from an object") + } + + override fun isNullable() = true +} + /** * Represents a generic interface to a serializable property of an object. * @@ -102,12 +123,18 @@ abstract class PropertyAccessor( class PropertyAccessorGetterSetter( initialPosition: Int, getter: PropertySerializer, - private val setter: Method?) : PropertyAccessor(initialPosition, getter) { + private val setter: Method) : PropertyAccessor(initialPosition, getter) { + init { + /** + * Play nicely with Java interop, public methods aren't marked as accessible + */ + setter.isAccessible = true + } /** * Invokes the setter on the underlying object passing in the serialized value. */ override fun set(instance: Any, obj: Any?) { - setter?.invoke(instance, *listOf(obj).toTypedArray()) + setter.invoke(instance, *listOf(obj).toTypedArray()) } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt index 372cfed9be..e309882d7b 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt @@ -460,6 +460,6 @@ private fun fingerprintForObject( .putUnencodedChars(prop.getter.name) .putUnencodedChars(if (prop.getter.mandatory) NOT_NULLABLE_HASH else NULLABLE_HASH) } - interfacesForSerialization(type, factory).map { fingerprintForType(it, type, alreadySeen, hasher, factory, debugIndent+4) } + interfacesForSerialization(type, factory).map { fingerprintForType(it, type, alreadySeen, hasher, factory, debugIndent+1) } return hasher } 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..4ce172cc78 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,184 @@ 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?, var iser: Method?) { + override fun toString() = StringBuilder("").apply { + appendln("Property - ${field?.name ?: "null field"}\n") + appendln(" getter - ${getter?.name ?: "no getter"}") + appendln(" setter - ${setter?.name ?: "no setter"}") + appendln(" iser - ${iser?.name ?: "no isXYZ defined"}") + }.toString() + + constructor() : this(null, null, null, null) + + fun preferredGetter() : Method? = getter ?: iser +} + +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 { + clazz!!.declaredFields.forEach { property -> + classProperties.computeIfAbsent(property.name) { + PropertyDescriptor() + }.apply { + this.field = property + } + } + + // 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 } + // 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 + // + // In addition, only getters that take zero parameters and setters that take a single + // parameter will be considered + 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 { + when (groups[1]!!.value) { + "set" -> { + if (func.parameterCount == 1) { + if (setter == null) setter = func + else if (TypeToken.of(setter!!.genericReturnType).isSupertypeOf(func.genericReturnType)) { + setter = func + } + } + } + "get" -> { + if (func.parameterCount == 0) { + if (getter == null) getter = func + else if (TypeToken.of(getter!!.genericReturnType).isSupertypeOf(func.genericReturnType)) { + getter = func + } + } + } + "is" -> { + if (func.parameterCount == 0) { + val rtnType = TypeToken.of(func.genericReturnType) + if ((rtnType == TypeToken.of(Boolean::class.java)) + || (rtnType == TypeToken.of(Boolean::class.javaObjectType))) { + if (iser == null) iser = func + } + } + } + } + } + } + } + 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 { + 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(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") } + } else { + throw NotSerializableException( + "Constructor parameter $name doesn't refer to a property of class '$clazz'") } this += PropertyAccessorConstructor( @@ -141,23 +261,40 @@ internal fun propertiesForSerializationFromConstructor( * If we determine a class has a constructor that takes no parameters then check for pairs of getters / setters * and use those */ -private fun propertiesForSerializationFromSetters( +fun propertiesForSerializationFromSetters( properties: Map, type: Type, factory: SerializerFactory): List { 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.preferredGetter() + 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[0]!! + + if ((property.value.field != null) && + (!(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!!}") + } + + // make sure the setter returns the same type (within inheritance bounds) the getter accepts + if (!(TypeToken.of (setterType).isSupertypeOf(getter.returnType))) { + throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + + "takes parameter of type $setterType yet the defined getter returns a value of type " + + "${getter.returnType}") + } + this += PropertyAccessorGetterSetter( idx++, PropertySerializer.make(property.key, PublicPropertyReader(getter), resolveTypeVariables(getter.genericReturnType, type), factory), @@ -186,23 +323,19 @@ 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 + if (it.value.second.field == null) 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 +403,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/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt index 1608a22e83..2c7dea9605 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt @@ -120,16 +120,17 @@ val typeStrToType: Map, Class> = mapOf( Pair("byte", false) to Byte::class.javaObjectType ) +fun String.stripGenerics() : String = if(this.endsWith('>')) { + this.substring(0, this.indexOf('<')) +} else this + fun AMQPField.getTypeAsClass(classloader: ClassLoader) = typeStrToType[Pair(type, mandatory)] ?: when (type) { "string" -> String::class.java "binary" -> ByteArray::class.java - "*" -> if (requires.isEmpty()) Any::class.java else classloader.loadClass(requires[0]) - else -> { - classloader.loadClass( - if (type.endsWith("?>")) { - type.substring(0, type.indexOf('<')) - } else type) + "*" -> if (requires.isEmpty()) Any::class.java else { + classloader.loadClass(requires[0].stripGenerics()) } + else -> classloader.loadClass(type.stripGenerics()) } fun AMQPField.validateType(classloader: ClassLoader) = when (type) { 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/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java index bb710a841e..defd86d948 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java @@ -286,12 +286,8 @@ public class SetterConstructorTests { tm.setA(10); assertEquals("10", tm.getA()); - TypeMismatch post = new DeserializationInput(factory1).deserialize(new SerializationOutput(factory1).serialize(tm), - TypeMismatch.class); - - // because there is a type mismatch in the class, it won't return that info as a BEAN property and thus - // we won't serialise it and thus on deserialization it won't be initialized - Assertions.assertThatThrownBy(() -> post.getA()).isInstanceOf(NullPointerException.class); + Assertions.assertThatThrownBy(() -> new SerializationOutput(factory1).serialize(tm)).isInstanceOf ( + NotSerializableException.class); } @Test @@ -306,11 +302,7 @@ public class SetterConstructorTests { tm.setA("10"); assertEquals((Integer)10, tm.getA()); - TypeMismatch2 post = new DeserializationInput(factory1).deserialize(new SerializationOutput(factory1).serialize(tm), - TypeMismatch2.class); - - // because there is a type mismatch in the class, it won't return that info as a BEAN property and thus - // we won't serialise it and thus on deserialization it won't be initialized - assertEquals(null, post.getA()); + Assertions.assertThatThrownBy(() -> new SerializationOutput(factory1).serialize(tm)).isInstanceOf( + NotSerializableException.class); } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.kt index 164feb2bab..0fce099955 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.kt @@ -1,12 +1,21 @@ package net.corda.nodeapi.internal.serialization.amqp +import net.corda.core.crypto.Crypto.generateKeyPair +import net.corda.core.crypto.SignedData +import net.corda.core.crypto.sign import net.corda.core.serialization.DeprecatedConstructorForDeserialization import net.corda.core.serialization.SerializedBytes +import net.corda.nodeapi.internal.network.NetworkParameters +import net.corda.nodeapi.internal.network.NotaryInfo import net.corda.testing.common.internal.ProjectStructure.projectRootDir +import net.corda.testing.core.DUMMY_NOTARY_NAME +import net.corda.testing.core.TestIdentity +import org.junit.Ignore import org.junit.Test import java.io.File import java.io.NotSerializableException import java.net.URI +import java.time.Instant import kotlin.test.assertEquals // To regenerate any of the binary test files do the following @@ -462,4 +471,112 @@ class EvolvabilityTests { assertEquals(f, db3.f) assertEquals(-1, db3.g) } -} \ No newline at end of file + + // + // This test uses a NetworkParameters signed set of bytes generated by R3 Corda and + // is here to ensure we can still read them. This test exists because of the break in + // being able to deserialize an object serialized prior to some fixes to the fingerprinter. + // + // The file itself was generated from R3 Corda at commit + // 6a6b6f256 Skip cache invalidation during init() - caches are still null. + // + // To regenerate the file un-ignore the test below this one (regenerate broken network parameters), + // to regenerate at a specific version add that test to a checkout at the desired sha then take + // the resulting file and add to the repo, changing the filename as appropriate + // + @Test + fun readBrokenNetworkParameters(){ + val sf = testDefaultFactory() + sf.register(net.corda.nodeapi.internal.serialization.amqp.custom.InstantSerializer(sf)) + sf.register(net.corda.nodeapi.internal.serialization.amqp.custom.PublicKeySerializer) + + // + // filename breakdown + // networkParams - because this is a serialised set of network parameters + // r3corda - generated by R3 Corda instead of Corda + // 6a6b6f256 - Commit sha of the build that generated the file we're testing against + // + val resource = "networkParams.r3corda.6a6b6f256" + + val path = EvolvabilityTests::class.java.getResource(resource) + val f = File(path.toURI()) + val sc2 = f.readBytes() + val deserializedC = DeserializationInput(sf).deserialize(SerializedBytes>(sc2)) + val networkParams = DeserializationInput(sf).deserialize(deserializedC.raw) + + assertEquals(1000, networkParams.maxMessageSize) + assertEquals(1000, networkParams.maxTransactionSize) + assertEquals(3, networkParams.minimumPlatformVersion) + assertEquals(1, networkParams.notaries.size) + assertEquals(TestIdentity(DUMMY_NOTARY_NAME, 20).party, networkParams.notaries.firstOrNull()?.identity) + } + + // + // This test created a serialized and signed set of Network Parameters to test whether we + // can still deserialize them + // + @Test + @Ignore("This test simply regenerates the test file used for readBrokenNetworkParameters") + fun `regenerate broken network parameters`() { + // note: 6a6b6f256 is the sha that generates the file + val resource = "networkParams.." + val DUMMY_NOTARY = TestIdentity(DUMMY_NOTARY_NAME, 20).party + val networkParameters = NetworkParameters( + 3, listOf(NotaryInfo(DUMMY_NOTARY, false)),1000, 1000, Instant.EPOCH, 1 ) + + val sf = testDefaultFactory() + sf.register(net.corda.nodeapi.internal.serialization.amqp.custom.InstantSerializer(sf)) + sf.register(net.corda.nodeapi.internal.serialization.amqp.custom.PublicKeySerializer) + + val testOutput = TestSerializationOutput(true, sf) + val serialized = testOutput.serialize(networkParameters) + val keyPair = generateKeyPair() + val sig = keyPair.private.sign(serialized.bytes, keyPair.public) + val signed = SignedData(serialized, sig) + val signedAndSerialized = testOutput.serialize(signed) + + File(URI("$localPath/$resource")).writeBytes( signedAndSerialized.bytes) + } + + @Suppress("UNCHECKED_CAST") + @Test + fun getterSetterEvolver1() { + val resource = "EvolvabilityTests.getterSetterEvolver1" + val sf = testDefaultFactory() + + // + // Class as it was serialised + // + // data class C(var c: Int, var d: Int, var b: Int, var e: Int, var a: Int) { + // // This will force the serialization engine to use getter / setter + // // instantiation for the object rather than construction + // @ConstructorForDeserialization + // @Suppress("UNUSED") + // constructor() : this(0, 0, 0, 0, 0) + // } + // + // File(URI("$localPath/$resource")).writeBytes(SerializationOutput(sf).serialize(C(3,4,2,5,1)).bytes) + + // + // Class as it exists now, c has been removed + // + data class C(var d: Int, var b: Int, var e: Int, var a: Int) { + // This will force the serialization engine to use getter / setter + // instantiation for the object rather than construction + @ConstructorForDeserialization + @Suppress("UNUSED") + constructor() : this(0, 0, 0, 0) + } + + val path = EvolvabilityTests::class.java.getResource(resource) + val f = File(path.toURI()) + + val sc2 = f.readBytes() + val deserializedC = DeserializationInput(sf).deserialize(SerializedBytes(sc2)) + + assertEquals(1, deserializedC.a) + assertEquals(2, deserializedC.b) + assertEquals(4, deserializedC.d) + assertEquals(5, deserializedC.e) + } +} 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..ae35d46286 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,60 @@ 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, var 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) + val c2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(c1)) + assertEquals(0, c2.getA()) + assertEquals(44, c2.b) + } + + @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 +192,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) diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt index d8c1db9cf9..89b2dcbad4 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt @@ -20,6 +20,8 @@ import net.corda.nodeapi.internal.serialization.AllWhitelist import net.corda.nodeapi.internal.serialization.EmptyWhitelist import net.corda.nodeapi.internal.serialization.GeneratedAttachment import net.corda.nodeapi.internal.serialization.amqp.SerializerFactory.Companion.isPrimitive +import net.corda.nodeapi.internal.serialization.amqp.custom.BigDecimalSerializer +import net.corda.nodeapi.internal.serialization.amqp.custom.CurrencySerializer import net.corda.testing.contracts.DummyContract import net.corda.testing.core.BOB_NAME import net.corda.testing.core.SerializationEnvironmentRule @@ -36,11 +38,13 @@ import org.junit.Test import java.io.ByteArrayInputStream import java.io.IOException import java.io.NotSerializableException +import java.lang.reflect.Type import java.math.BigDecimal import java.nio.ByteBuffer import java.time.* import java.time.temporal.ChronoUnit import java.util.* +import java.util.concurrent.ConcurrentHashMap import kotlin.reflect.full.superclasses import kotlin.test.assertEquals import kotlin.test.assertNotNull @@ -1080,4 +1084,38 @@ class SerializationOutputTests { serdes(obj, factory, factory2, expectedEqual = false, expectDeserializedEqual = false) }.isInstanceOf(MissingAttachmentsException::class.java) } + + // + // Example stacktrace that this test is tryint to reproduce + // + // java.lang.IllegalArgumentException: + // net.corda.core.contracts.TransactionState -> + // data(net.corda.core.contracts.ContractState) -> + // net.corda.finance.contracts.asset.Cash$State -> + // amount(net.corda.core.contracts.Amount>) -> + // net.corda.core.contracts.Amount> -> + // displayTokenSize(java.math.BigDecimal) -> + // wrong number of arguments + // + // So the actual problem was objects with multiple getters. The code wasn't looking for one with zero + // properties, just taking the first one it found with with the most applicable type, and the reflection + // ordering of the methods was random, thus occasionally we select the wrong one + // + @Test + fun reproduceWrongNumberOfArguments() { + val field = SerializerFactory::class.java.getDeclaredField("serializersByType").apply { + this.isAccessible = true + } + + data class C(val a: Amount) + + val factory = testDefaultFactoryNoEvolution() + factory.register(net.corda.nodeapi.internal.serialization.amqp.custom.BigDecimalSerializer) + factory.register(net.corda.nodeapi.internal.serialization.amqp.custom.CurrencySerializer) + + val c = C(Amount(100, BigDecimal("1.5"), Currency.getInstance("USD"))) + + // were the issue not fixed we'd blow up here + SerializationOutput(factory).serialize(c) + } } \ No newline at end of file diff --git a/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.getterSetterEvolver1 b/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.getterSetterEvolver1 new file mode 100644 index 0000000000..c544432e6d Binary files /dev/null and b/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/EvolvabilityTests.getterSetterEvolver1 differ diff --git a/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/networkParams.r3corda.6a6b6f256 b/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/networkParams.r3corda.6a6b6f256 new file mode 100644 index 0000000000..dcdbaa7b5f Binary files /dev/null and b/node-api/src/test/resources/net/corda/nodeapi/internal/serialization/amqp/networkParams.r3corda.6a6b6f256 differ