CORDA-2870 improve error messages for non composable types (#5120)

* CORDA-2870 Add `reason` and `remedy` to `LocalTypeInformation.NonComposable`

When creating `LocalTypeInformation.NonComposable` pass in the `reason`
a type was not composable and the `remedy` to fix it. This required
changes in `LocalTypeInformationBuilder` to pass in this extra
information so that it can be used later.

The message that the `ObjectSerializer` includes in its
`NotSerializableException` now includes the extra information about the
non composable type.

* CORDA-2870 Include custom serializers in serialization error message

In `ObjectSerializer`, when a serialization exception is thrown,
include the registered custom serializers + their classloaders as part
of the error message.

This required making properties on `CustomSerializerRegistry` and
`LocalSerializerFactory` public.

Tidy up `LocalTypeInformationBuilder` error message text for
transitive non-composable types.

* CORDA-2870 Tidy up error thrown for unserializable objects

Fix `DeserializeSimpleTypesTests` and tidy up the code in
`ObjectSerializer` a bit.

* CORDA-2870 Remove non-composable warning logs in `LocalTypeInformationBuilder`

The flag `warnIfNonComposable` and its corresponding log lines are not
needed now that the non-composable error messages contain a lot of
information in them.

The `warnIfNonComposable` flag is now incorrect and has been renamed to
`validateProperties` and the function `suppressWarningsAnd` has been
changed to `suppressValidation`.

`propertyDescriptors` has also had its input boolean changed to
`validateProperties` to better represent what it is doing.

* CORDA-2870 Remove need for casting by moving variable to interface

Expose `customSerializerNames` in `LocalSerializerFactory` and
`CustomSerializerFactory`.
This commit is contained in:
Dan Newton 2019-05-16 10:05:49 +01:00 committed by Shams Asari
parent 0b63157a4b
commit cd73161513
10 changed files with 219 additions and 53 deletions

View File

@ -97,5 +97,6 @@ class CorDappCustomSerializer(
override fun isSerializerFor(clazz: Class<*>) =
TypeToken.of(type.asClass()) == TypeToken.of(clazz)
override fun toString(): String = "${this::class.java}(${serializer::class.java})"
}

View File

@ -26,6 +26,12 @@ class DuplicateCustomSerializerException(serializers: List<AMQPSerializer<*>>, c
" registered to serialize type $clazz")
interface CustomSerializerRegistry {
/**
* Retrieves the names of the registered custom serializers.
*/
val customSerializerNames: List<String>
/**
* Register a custom serializer for any type that cannot be serialized or deserialized by the default serializer
* that expects to find getters and a constructor with a parameter for each property.
@ -58,6 +64,12 @@ class CachingCustomSerializerRegistry(
val logger = contextLogger()
}
override val customSerializerNames: List<String>
get() = customSerializers.map { serializer ->
if (serializer is CorDappCustomSerializer) serializer.toString()
else "${serializer::class.java} - Classloader: ${serializer::class.java.classLoader}"
}
private data class CustomSerializerIdentifier(val actualTypeIdentifier: TypeIdentifier, val declaredTypeIdentifier: TypeIdentifier)
private sealed class CustomSerializerLookupResult {

View File

@ -31,6 +31,11 @@ interface LocalSerializerFactory {
*/
val classloader: ClassLoader
/**
* Retrieves the names of the registered custom serializers.
*/
val customSerializerNames: List<String>
/**
* Obtain an [AMQPSerializer] for an object of actual type [actualClass], and declared type [declaredType].
*/
@ -90,6 +95,9 @@ class DefaultLocalSerializerFactory(
val logger = contextLogger()
}
override val customSerializerNames: List<String>
get() = customSerializerRegistry.customSerializerNames
private data class ActualAndDeclaredType(val actualType: Class<*>, val declaredType: Type)
private val serializersByActualAndDeclaredType: MutableMap<ActualAndDeclaredType, AMQPSerializer<Any>> = DefaultCacheProvider.createCache()

View File

@ -14,10 +14,9 @@ interface ObjectSerializer : AMQPSerializer<Any> {
companion object {
fun make(typeInformation: LocalTypeInformation, factory: LocalSerializerFactory): ObjectSerializer {
if (typeInformation is LocalTypeInformation.NonComposable)
throw NotSerializableException(
"Trying to build an object serializer for ${typeInformation.typeIdentifier.prettyPrint(false)}, " +
"but it is not constructable from its public properties, and so requires a custom serialiser.")
if (typeInformation is LocalTypeInformation.NonComposable) {
throw NotSerializableException(nonComposableExceptionMessage(typeInformation, factory))
}
val typeDescriptor = factory.createDescriptor(typeInformation)
val typeNotation = TypeNotationGenerator.getTypeNotation(typeInformation, typeDescriptor)
@ -32,6 +31,22 @@ interface ObjectSerializer : AMQPSerializer<Any> {
}
}
private fun nonComposableExceptionMessage(
typeInformation: LocalTypeInformation.NonComposable,
factory: LocalSerializerFactory
): String {
val serializerInformation = factory.customSerializerNames.map { "Serializer: $it" }.let { serializers ->
when {
serializers.isNotEmpty() -> "Registered custom serializers:\n ${serializers.joinToString("\n ")}"
else -> "No custom serializers registered."
}
}
return "Unable to create an object serializer for type ${typeInformation.observedType}:\n" +
"${typeInformation.reason}\n\n" +
"${typeInformation.remedy}\n\n" +
"$serializerInformation\n"
}
private fun makeForAbstract(typeNotation: CompositeType,
typeInformation: LocalTypeInformation,
typeDescriptor: Symbol,

View File

@ -4,7 +4,6 @@ import com.google.common.reflect.TypeToken
import net.corda.core.KeepForDJVM
import net.corda.core.internal.isPublic
import net.corda.core.serialization.SerializableCalculatedProperty
import net.corda.core.utilities.contextLogger
import net.corda.serialization.internal.amqp.MethodClassifier.*
import java.lang.reflect.Field
import java.lang.reflect.Method
@ -88,7 +87,7 @@ private val propertyMethodRegex = Regex("(?<type>get|set|is)(?<var>\\p{Lu}.*)")
* take a single parameter of a type compatible with exampleProperty and isExampleProperty must
* return a boolean
*/
internal fun Class<out Any?>.propertyDescriptors(warnInvalid: Boolean = true): Map<String, PropertyDescriptor> {
internal fun Class<out Any?>.propertyDescriptors(validateProperties: Boolean = true): Map<String, PropertyDescriptor> {
val fieldProperties = superclassChain().declaredFields().byFieldName()
return superclassChain().declaredMethods()
@ -97,7 +96,7 @@ internal fun Class<out Any?>.propertyDescriptors(warnInvalid: Boolean = true): M
.withValidSignature()
.byNameAndClassifier(fieldProperties.keys)
.toClassProperties(fieldProperties).run {
if (warnInvalid) validated() else this
if (validateProperties) validated() else this
}
}

View File

@ -27,4 +27,8 @@ class ComposedSerializerFactory(
) : SerializerFactory,
LocalSerializerFactory by localSerializerFactory,
RemoteSerializerFactory by remoteSerializerFactory,
CustomSerializerRegistry by customSerializerRegistry
CustomSerializerRegistry by customSerializerRegistry {
override val customSerializerNames: List<String>
get() = customSerializerRegistry.customSerializerNames
}

View File

@ -259,7 +259,9 @@ sealed class LocalTypeInformation {
val properties: Map<PropertyName, LocalPropertyInformation>,
val superclass: LocalTypeInformation,
val interfaces: List<LocalTypeInformation>,
val typeParameters: List<LocalTypeInformation>) : LocalTypeInformation()
val typeParameters: List<LocalTypeInformation>,
val reason: String,
val remedy: String) : LocalTypeInformation()
/**
* Represents a type whose underlying class is a collection class such as [List] with a single type parameter.

View File

@ -37,7 +37,7 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
var resolutionContext: Type? = null,
var visited: Set<TypeIdentifier> = emptySet(),
val cycles: MutableList<LocalTypeInformation.Cycle> = mutableListOf(),
var warnIfNonComposable: Boolean = true) {
var validateProperties: Boolean = true) {
companion object {
private val logger = contextLogger()
@ -47,13 +47,13 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
* If we are examining the type of a read-only property, or a type flagged as [Opaque], then we do not need to warn
* if the [LocalTypeInformation] for that type (or any of its related types) is [LocalTypeInformation.NonComposable].
*/
private inline fun <T> suppressWarningsAnd(block: LocalTypeInformationBuilder.() -> T): T {
val previous = warnIfNonComposable
private inline fun <T> suppressValidation(block: LocalTypeInformationBuilder.() -> T): T {
val previous = validateProperties
return try {
warnIfNonComposable = false
validateProperties = false
block()
} finally {
warnIfNonComposable = previous
validateProperties = previous
}
}
@ -121,8 +121,8 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
isOpaque -> LocalTypeInformation.Opaque(
type,
typeIdentifier,
suppressWarningsAnd { buildNonAtomic(type, type, typeIdentifier, emptyList()) })
Exception::class.java.isAssignableFrom(type.asClass()) -> suppressWarningsAnd {
suppressValidation { buildNonAtomic(type, type, typeIdentifier, emptyList()) })
Exception::class.java.isAssignableFrom(type.asClass()) -> suppressValidation {
buildNonAtomic(type, type, typeIdentifier, emptyList())
}
else -> buildNonAtomic(type, type, typeIdentifier, emptyList())
@ -155,7 +155,7 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
rawType.isAbstractClass -> buildAbstract(type, typeIdentifier, buildTypeParameterInformation(type))
isOpaque -> LocalTypeInformation.Opaque(rawType,
typeIdentifier,
suppressWarningsAnd { buildNonAtomic(rawType, type, typeIdentifier, buildTypeParameterInformation(type)) })
suppressValidation { buildNonAtomic(rawType, type, typeIdentifier, buildTypeParameterInformation(type)) })
else -> buildNonAtomic(rawType, type, typeIdentifier, buildTypeParameterInformation(type))
}
}
@ -205,28 +205,51 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
val observedConstructor = constructorForDeserialization(type)
if (observedConstructor == null) {
if (warnIfNonComposable) {
logger.info("No unique deserialisation constructor found for class $rawType, type is marked as non-composable")
}
return LocalTypeInformation.NonComposable(type, typeIdentifier, null, buildReadOnlyProperties(rawType),
superclassInformation, interfaceInformation, typeParameterInformation)
return LocalTypeInformation.NonComposable(
observedType = type,
typeIdentifier = typeIdentifier,
constructor = null,
properties = buildReadOnlyProperties(rawType),
superclass = superclassInformation,
interfaces = interfaceInformation,
typeParameters = typeParameterInformation,
reason = "No unique deserialization constructor can be identified",
remedy = "Either annotate a constructor for this type with @ConstructorForDeserialization, or provide a custom serializer for it"
)
}
val constructorInformation = buildConstructorInformation(type, observedConstructor)
val properties = buildObjectProperties(rawType, constructorInformation)
val hasNonComposableProperties = properties.values.any { it.type is LocalTypeInformation.NonComposable }
if (!propertiesSatisfyConstructor(constructorInformation, properties)) {
val missingParameters = missingMandatoryConstructorProperties(constructorInformation, properties).map { it.name }
return LocalTypeInformation.NonComposable(
observedType = type,
typeIdentifier = typeIdentifier,
constructor = constructorInformation,
properties = properties,
superclass = superclassInformation,
interfaces = interfaceInformation,
typeParameters = typeParameterInformation,
reason = "Mandatory constructor parameters $missingParameters are missing from the readable properties ${properties.keys}",
remedy = "Either provide getters or readable fields for $missingParameters, or provide a custom serializer for this type"
)
}
if (!propertiesSatisfyConstructor(constructorInformation, properties) || hasNonComposableProperties) {
if (warnIfNonComposable) {
if (hasNonComposableProperties) {
logger.info("Type ${type.typeName} has non-composable properties and has been marked as non-composable")
} else {
logger.info("Properties of type ${type.typeName} do not satisfy its constructor, type has been marked as non-composable")
}
}
return LocalTypeInformation.NonComposable(type, typeIdentifier, constructorInformation, properties, superclassInformation,
interfaceInformation, typeParameterInformation)
val nonComposableProperties = properties.filterValues { it.type is LocalTypeInformation.NonComposable }
if (nonComposableProperties.isNotEmpty()) {
return LocalTypeInformation.NonComposable(
observedType = type,
typeIdentifier = typeIdentifier,
constructor = constructorInformation,
properties = properties,
superclass = superclassInformation,
interfaces = interfaceInformation,
typeParameters = typeParameterInformation,
reason = nonComposablePropertiesErrorReason(nonComposableProperties),
remedy = "Either ensure that the properties ${nonComposableProperties.keys} are serializable, or provide a custom serializer for this type"
)
}
val evolutionConstructors = evolutionConstructors(type).map { ctor ->
@ -256,6 +279,37 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
}
}
private fun missingMandatoryConstructorProperties(
constructorInformation: LocalConstructorInformation,
properties: Map<PropertyName, LocalPropertyInformation>
): List<LocalConstructorParameterInformation> {
if (!constructorInformation.hasParameters) return emptyList()
val indicesAddressedByProperties = properties.values.asSequence().mapNotNull {
when (it) {
is LocalPropertyInformation.ConstructorPairedProperty -> it.constructorSlot.parameterIndex
is LocalPropertyInformation.PrivateConstructorPairedProperty -> it.constructorSlot.parameterIndex
else -> null
}
}.toSet()
return (0 until constructorInformation.parameters.size).mapNotNull { index ->
val parameter = constructorInformation.parameters[index]
when {
constructorInformation.parameters[index].isMandatory && index !in indicesAddressedByProperties -> parameter
else -> null
}
}
}
private fun nonComposablePropertiesErrorReason(nonComposableProperties: Map<PropertyName, LocalPropertyInformation>): String {
val reasons = nonComposableProperties.entries.joinToString("\n") { (key, value) ->
"$key [${value.type.observedType}]: ${(value.type as LocalTypeInformation.NonComposable).reason}"
.replace("\n", "\n ")
}
return "Has properties ${nonComposableProperties.keys} of types that are not serializable:\n" + reasons
}
private fun buildSuperclassInformation(type: Type): LocalTypeInformation =
resolveAndBuild(type.asClass().genericSuperclass)
@ -283,12 +337,12 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
}
private fun buildReadOnlyProperties(rawType: Class<*>): Map<PropertyName, LocalPropertyInformation> =
rawType.propertyDescriptors(warnIfNonComposable).asSequence().mapNotNull { (name, descriptor) ->
rawType.propertyDescriptors(validateProperties).asSequence().mapNotNull { (name, descriptor) ->
if (descriptor.field == null || descriptor.getter == null) null
else {
val paramType = (descriptor.getter.genericReturnType).resolveAgainstContext()
// Because this parameter is read-only, we don't need to warn if its type is non-composable.
val paramTypeInformation = suppressWarningsAnd {
val paramTypeInformation = suppressValidation {
build(paramType, TypeIdentifier.forGenericType(paramType, resolutionContext ?: rawType))
}
val isMandatory = paramType.asClass().isPrimitive || !descriptor.getter.returnsNullable()
@ -310,7 +364,7 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
parameter.name to index
}.toMap()
return rawType.propertyDescriptors(warnIfNonComposable).asSequence().mapNotNull { (name, descriptor) ->
return rawType.propertyDescriptors(validateProperties).asSequence().mapNotNull { (name, descriptor) ->
val normalisedName = when {
name in constructorParameterIndices -> name
name.decapitalize() in constructorParameterIndices -> name.decapitalize()
@ -353,7 +407,7 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
}
private fun getterSetterProperties(rawType: Class<*>): Sequence<Pair<String, LocalPropertyInformation>> =
rawType.propertyDescriptors(warnIfNonComposable).asSequence().mapNotNull { (name, descriptor) ->
rawType.propertyDescriptors(validateProperties).asSequence().mapNotNull { (name, descriptor) ->
if (descriptor.getter == null || descriptor.setter == null || descriptor.field == null) null
else {
val paramType = descriptor.getter.genericReturnType

View File

@ -549,16 +549,31 @@ class DeserializeSimpleTypesTests {
@Test
fun classHasNoPublicConstructor() {
assertFailsWithMessage("Trying to build an object serializer for ${Garbo::class.java.name}, " +
"but it is not constructable from its public properties, and so requires a custom serialiser.") {
assertFailsWithMessage(
"""Unable to create an object serializer for type class ${Garbo::class.java.name}:
Mandatory constructor parameters [value] are missing from the readable properties []
Either provide getters or readable fields for [value], or provide a custom serializer for this type
No custom serializers registered.
"""
) {
TestSerializationOutput(VERBOSE, sf1).serializeAndReturnSchema(Garbo.make(1))
}
}
@Test
fun propertyClassHasNoPublicConstructor() {
assertFailsWithMessage("Trying to build an object serializer for ${Greta::class.java.name}, " +
"but it is not constructable from its public properties, and so requires a custom serialiser.") {
assertFailsWithMessage(
"""Unable to create an object serializer for type class ${Greta::class.java.name}:
Has properties [garbo] of types that are not serializable:
garbo [class ${Garbo::class.java.name}]: Mandatory constructor parameters [value] are missing from the readable properties []
Either ensure that the properties [garbo] are serializable, or provide a custom serializer for this type
No custom serializers registered.
"""
) {
TestSerializationOutput(VERBOSE, sf1).serializeAndReturnSchema(Greta(Garbo.make(1)))
}
}

View File

@ -8,7 +8,6 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import java.lang.reflect.Type
import java.time.LocalDateTime
import java.util.*
class LocalTypeModelTests {
@ -16,6 +15,13 @@ class LocalTypeModelTests {
private val descriptorBasedSerializerRegistry = DefaultDescriptorBasedSerializerRegistry()
private val customSerializerRegistry: CustomSerializerRegistry = CachingCustomSerializerRegistry(descriptorBasedSerializerRegistry)
private val model = ConfigurableLocalTypeModel(WhitelistBasedTypeModelConfiguration(AllWhitelist, customSerializerRegistry))
private val emptyCustomSerializerRegistry = object: CustomSerializerRegistry {
override val customSerializerNames: List<String> = emptyList()
override fun register(customSerializer: CustomSerializer<out Any>) {}
override fun registerExternal(customSerializer: CorDappCustomSerializer) {}
override fun findCustomSerializer(clazz: Class<*>, declaredType: Type): AMQPSerializer<Any>? = null
}
private val modelWithoutOpacity = ConfigurableLocalTypeModel(WhitelistBasedTypeModelConfiguration(AllWhitelist, emptyCustomSerializerRegistry))
interface CollectionHolder<K, V> {
val list: List<V>
@ -134,20 +140,70 @@ class LocalTypeModelTests {
""")
}
class TransitivelyNonComposable(val a: String, val b: Exception)
class TransitivelyNonComposable(
val a: String,
val b: Exception,
val c: MissingConstructorParameter,
val d: AnotherTransitivelyNonComposable
)
class AnotherTransitivelyNonComposable(val e: String, val f: Exception, val g: OneMoreTransitivelyNonComposable)
class OneMoreTransitivelyNonComposable(val h: String, val i: Exception)
class MissingConstructorParameter(val a: String, b: Exception)
@Test
fun `non-composable types`() {
val serializerRegistry = object: CustomSerializerRegistry {
override fun register(customSerializer: CustomSerializer<out Any>) {}
override fun registerExternal(customSerializer: CorDappCustomSerializer) {}
override fun findCustomSerializer(clazz: Class<*>, declaredType: Type): AMQPSerializer<Any>? = null
fun `no unique deserialization constructor creates non-composable type`() {
modelWithoutOpacity.inspect(typeOf<Exception>()).let { typeInformation ->
assertTrue(typeInformation is LocalTypeInformation.NonComposable)
typeInformation as LocalTypeInformation.NonComposable
assertEquals(
"No unique deserialization constructor can be identified",
typeInformation.reason
)
assertEquals(
"Either annotate a constructor for this type with @ConstructorForDeserialization, or provide a custom serializer for it",
typeInformation.remedy
)
}
}
@Test
fun `missing constructor parameters creates non-composable type`() {
modelWithoutOpacity.inspect(typeOf<MissingConstructorParameter>()).let { typeInformation ->
assertTrue(typeInformation is LocalTypeInformation.NonComposable)
typeInformation as LocalTypeInformation.NonComposable
assertEquals(
"Mandatory constructor parameters [b] are missing from the readable properties [a]",
typeInformation.reason
)
assertEquals(
"Either provide getters or readable fields for [b], or provide a custom serializer for this type",
typeInformation.remedy
)
}
}
@Test
fun `transitive types are non-composable creates non-composable type`() {
modelWithoutOpacity.inspect(typeOf<TransitivelyNonComposable>()).let { typeInformation ->
assertTrue(typeInformation is LocalTypeInformation.NonComposable)
typeInformation as LocalTypeInformation.NonComposable
assertEquals(
"""
Has properties [b, c, d] of types that are not serializable:
b [${Exception::class.java}]: No unique deserialization constructor can be identified
c [${MissingConstructorParameter::class.java}]: Mandatory constructor parameters [b] are missing from the readable properties [a]
d [${AnotherTransitivelyNonComposable::class.java}]: Has properties [f, g] of types that are not serializable:
f [${Exception::class.java}]: No unique deserialization constructor can be identified
g [${OneMoreTransitivelyNonComposable::class.java}]: Has properties [i] of types that are not serializable:
i [${Exception::class.java}]: No unique deserialization constructor can be identified
""".trimIndent(), typeInformation.reason
)
assertEquals(
"Either ensure that the properties [b, c, d] are serializable, or provide a custom serializer for this type",
typeInformation.remedy
)
}
val modelWithoutOpacity = ConfigurableLocalTypeModel(WhitelistBasedTypeModelConfiguration(AllWhitelist, serializerRegistry) )
assertTrue(modelWithoutOpacity.inspect(typeOf<Exception>()) is LocalTypeInformation.NonComposable)
assertTrue(modelWithoutOpacity.inspect(typeOf<TransitivelyNonComposable>()) is LocalTypeInformation.NonComposable)
}
private inline fun <reified T> assertInformation(expected: String) {