Bring enum serializer into line with AMQP

AMQP doesn't define an enum type itself yet the old implementation
listed our snum type as that in the schema despite what we are actually
doing which is serialising the enum down as a list of a string and an
int accompanied by a list of AMQP choices that represent all of the enum
values

Review Comments

Use fingerprinting of the enum types to catch whenever they're changed,
include the enum constants in the fingerprint to avoid any collisions

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt

@@ -118,7 +118,7 @@ class DeserializationInput(internal val serializerFactory: SerializerFactory) {
                             "is outside of the bounds for the list of size: ${objectHistory.size}")
 
                 val objectRetrieved = objectHistory[objectIndex]
-                if (!objectRetrieved::class.java.isSubClassOf(type))
+                if (!objectRetrieved::class.java.isSubClassOf(type.asClass()!!))
                     throw NotSerializableException("Existing reference type mismatch. Expected: '$type', found: '${objectRetrieved::class.java}'")
                 objectRetrieved
             } else {

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumSerializer.kt

@@ -4,6 +4,10 @@ import org.apache.qpid.proton.codec.Data
 import java.lang.reflect.Type
 import java.io.NotSerializableException
 
+/**
+ * Our definition of an enum with the AMQP spec is a list (of two items, a string and an int) that is
+ * a restricted type with a number of choices associated with it
+ */
 class EnumSerializer(declaredType: Type, declaredClass: Class<*>, factory: SerializerFactory) : AMQPSerializer<Any> {
     override val type: Type = declaredType
     override val typeDescriptor = "$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}"
@@ -12,7 +16,7 @@ class EnumSerializer(declaredType: Type, declaredClass: Class<*>, factory: Seria
     init {
         typeNotation = RestrictedType(
                 SerializerFactory.nameForType(declaredType),
-                null, emptyList(), "enum", Descriptor(typeDescriptor, null),
+                null, emptyList(), "list", Descriptor(typeDescriptor, null),
                 declaredClass.enumConstants.zip(IntRange(0, declaredClass.enumConstants.size)).map {
                     Choice(it.first.toString(), it.second.toString())
                 })

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt

@@ -191,12 +191,10 @@ sealed class TypeNotation : DescribedType {
     companion object {
         fun get(obj: Any): TypeNotation {
             val describedType = obj as DescribedType
-            if (describedType.descriptor == CompositeType.DESCRIPTOR) {
-                return CompositeType.get(describedType)
-            } else if (describedType.descriptor == RestrictedType.DESCRIPTOR) {
-                return RestrictedType.get(describedType)
-            } else {
-                throw NotSerializableException("Unexpected descriptor ${describedType.descriptor}.")
+            return when (describedType.descriptor) {
+                CompositeType.DESCRIPTOR -> CompositeType.get(describedType)
+                RestrictedType.DESCRIPTOR -> RestrictedType.get(describedType)
+                else -> throw NotSerializableException("Unexpected descriptor ${describedType.descriptor}.")
             }
         }
     }
@@ -360,6 +358,7 @@ data class ReferencedObject(private val refCounter: Int) : DescribedType {
 }
 
 private val ARRAY_HASH: String = "Array = true"
+private val ENUM_HASH: String = "Enum = true"
 private val ALREADY_SEEN_HASH: String = "Already seen = true"
 private val NULLABLE_HASH: String = "Nullable = true"
 private val NOT_NULLABLE_HASH: String = "Nullable = false"
@@ -390,7 +389,7 @@ internal fun fingerprintForDescriptors(vararg typeDescriptors: String): String {
     return hasher.hash().asBytes().toBase64()
 }
 
-private fun Hasher.fingerprintWithCustomSerializerOrElse(factory: SerializerFactory, clazz: Class<*>, declaredType: Type, block: () -> Hasher) : Hasher {
+private fun Hasher.fingerprintWithCustomSerializerOrElse(factory: SerializerFactory, clazz: Class<*>, declaredType: Type, block: () -> Hasher): Hasher {
     // Need to check if a custom serializer is applicable
     val customSerializer = factory.findCustomSerializer(clazz, declaredType)
     return if (customSerializer != null) {
@@ -408,50 +407,58 @@ private fun fingerprintForType(type: Type, contextType: Type?, alreadySeen: Muta
     } else {
         alreadySeen += type
         try {
-            if (type is SerializerFactory.AnyType) {
-                hasher.putUnencodedChars(ANY_TYPE_HASH)
-            } else if (type is Class<*>) {
-                when {
-                    type.isArray -> fingerprintForType(type.componentType, contextType, alreadySeen, hasher, factory).putUnencodedChars(ARRAY_HASH)
-                    SerializerFactory.isPrimitive(type) ||
-                    isCollectionOrMap(type) ||
-                    type.isEnum -> hasher.putUnencodedChars(type.name)
-                    else ->
+            when (type) {
+                is SerializerFactory.AnyType -> hasher.putUnencodedChars(ANY_TYPE_HASH)
+                is Class<*> -> {
+                    if (type.isArray) {
+                        fingerprintForType(type.componentType, contextType, alreadySeen, hasher, factory).putUnencodedChars(ARRAY_HASH)
+                    } else if (SerializerFactory.isPrimitive(type)) {
+                        hasher.putUnencodedChars(type.name)
+                    } else if (isCollectionOrMap(type)) {
+                        hasher.putUnencodedChars(type.name)
+                    } else if (type.isEnum) {
+                        // ensures any change to the enum (adding constants) will trigger the need for evolution
+                        hasher.apply {
+                            type.enumConstants.forEach {
+                                putUnencodedChars(it.toString())
+                            }
+                        }.putUnencodedChars(type.name).putUnencodedChars(ENUM_HASH)
+                    } else {
                         hasher.fingerprintWithCustomSerializerOrElse(factory, type, type) {
-                        if (type.kotlin.objectInstance != null) {
-                            // TODO: name collision is too likely for kotlin objects, we need to introduce some reference
-                            // to the CorDapp but maybe reference to the JAR in the short term.
-                            hasher.putUnencodedChars(type.name)
-                        } else {
-                            fingerprintForObject(type, type, alreadySeen, hasher, factory)
+                            if (type.kotlin.objectInstance != null) {
+                                // TODO: name collision is too likely for kotlin objects, we need to introduce some reference
+                                // to the CorDapp but maybe reference to the JAR in the short term.
+                                hasher.putUnencodedChars(type.name)
+                            } else {
+                                fingerprintForObject(type, type, alreadySeen, hasher, factory)
+                            }
                         }
                     }
                 }
-            } else if (type is ParameterizedType) {
-                // Hash the rawType + params
-                val clazz = type.rawType as Class<*>
-                val startingHash = if (isCollectionOrMap(clazz)) {
-                    hasher.putUnencodedChars(clazz.name)
-                } else {
-                    hasher.fingerprintWithCustomSerializerOrElse(factory, clazz, type) {
-                        fingerprintForObject(type, type, alreadySeen, hasher, factory)
+                is ParameterizedType -> {
+                    // Hash the rawType + params
+                    val clazz = type.rawType as Class<*>
+                    val startingHash = if (isCollectionOrMap(clazz)) {
+                        hasher.putUnencodedChars(clazz.name)
+                    } else {
+                        hasher.fingerprintWithCustomSerializerOrElse(factory, clazz, type) {
+                            fingerprintForObject(type, type, alreadySeen, hasher, factory)
+                        }
+                    }
+                    // ... and concatentate the type data for each parameter type.
+                    type.actualTypeArguments.fold(startingHash) { orig, paramType ->
+                        fingerprintForType(paramType, type, alreadySeen, orig, factory)
                     }
                 }
-                // ... and concatentate the type data for each parameter type.
-                type.actualTypeArguments.fold(startingHash) { orig, paramType -> fingerprintForType(paramType, type, alreadySeen, orig, factory) }
-            } else if (type is GenericArrayType) {
-                // Hash the element type + some array hash
-                fingerprintForType(type.genericComponentType, contextType, alreadySeen, hasher, factory).putUnencodedChars(ARRAY_HASH)
-            } else if (type is TypeVariable<*>) {
-                // TODO: include bounds
-                hasher.putUnencodedChars(type.name).putUnencodedChars(TYPE_VARIABLE_HASH)
-            } else if (type is WildcardType) {
-                hasher.putUnencodedChars(type.typeName).putUnencodedChars(WILDCARD_TYPE_HASH)
+            // Hash the element type + some array hash
+                is GenericArrayType -> fingerprintForType(type.genericComponentType, contextType, alreadySeen,
+                        hasher, factory).putUnencodedChars(ARRAY_HASH)
+            // TODO: include bounds
+                is TypeVariable<*> -> hasher.putUnencodedChars(type.name).putUnencodedChars(TYPE_VARIABLE_HASH)
+                is WildcardType -> hasher.putUnencodedChars(type.typeName).putUnencodedChars(WILDCARD_TYPE_HASH)
+                else -> throw NotSerializableException("Don't know how to hash")
             }
-            else {
-                throw NotSerializableException("Don't know how to hash")
-            }
-        } catch(e: NotSerializableException) {
+        } catch (e: NotSerializableException) {
             val msg = "${e.message} -> $type"
             logger.error(msg, e)
             throw NotSerializableException(msg)

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt

@@ -16,7 +16,7 @@ import java.util.concurrent.ConcurrentHashMap
 import java.util.concurrent.CopyOnWriteArrayList
 import javax.annotation.concurrent.ThreadSafe
 
-data class schemaAndDescriptor (val schema: Schema, val typeDescriptor: Any)
+data class schemaAndDescriptor(val schema: Schema, val typeDescriptor: Any)
 
 /**
  * Factory of serializers designed to be shared across threads and invocations.
@@ -38,17 +38,20 @@ data class schemaAndDescriptor (val schema: Schema, val typeDescriptor: Any)
 // TODO: need to rethink matching of constructor to properties in relation to implementing interfaces and needing those properties etc.
 // TODO: need to support super classes as well as interfaces with our current code base... what's involved?  If we continue to ban, what is the impact?
 @ThreadSafe
-class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
+class SerializerFactory(val whitelist: ClassWhitelist, cl: ClassLoader) {
     private val serializersByType = ConcurrentHashMap<Type, AMQPSerializer<Any>>()
     private val serializersByDescriptor = ConcurrentHashMap<Any, AMQPSerializer<Any>>()
     private val customSerializers = CopyOnWriteArrayList<CustomSerializer<out Any>>()
     private val classCarpenter = ClassCarpenter(cl)
-    val classloader : ClassLoader
+    val classloader: ClassLoader
         get() = classCarpenter.classloader
 
-    fun getEvolutionSerializer(typeNotation: TypeNotation, newSerializer: ObjectSerializer) : AMQPSerializer<Any> {
+    private fun getEvolutionSerializer(typeNotation: TypeNotation, newSerializer: AMQPSerializer<Any>): AMQPSerializer<Any> {
         return serializersByDescriptor.computeIfAbsent(typeNotation.descriptor.name!!) {
-            EvolutionSerializer.make(typeNotation as CompositeType, newSerializer, this)
+            when (typeNotation) {
+                is CompositeType -> EvolutionSerializer.make(typeNotation, newSerializer as ObjectSerializer, this)
+                is RestrictedType -> throw NotSerializableException("Enum evolution is not currently supported")
+            }
         }
     }
 
@@ -66,7 +69,8 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
         val actualType: Type = inferTypeVariables(actualClass, declaredClass, declaredType) ?: declaredType
 
         val serializer = when {
-            (Collection::class.java.isAssignableFrom(declaredClass)) -> { serializersByType.computeIfAbsent(declaredType) {
+            (Collection::class.java.isAssignableFrom(declaredClass)) -> {
+                serializersByType.computeIfAbsent(declaredType) {
                     CollectionSerializer(declaredType as? ParameterizedType ?: DeserializedParameterizedType(
                             declaredClass, arrayOf(AnyType), null), this)
                 }
@@ -91,17 +95,17 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
      * type.
      */
     // TODO: test GenericArrayType
-    private fun inferTypeVariables(actualClass: Class<*>?, declaredClass: Class<*>, declaredType: Type): Type? {
-        if (declaredType is ParameterizedType) {
-            return inferTypeVariables(actualClass, declaredClass, declaredType)
-        } else if (declaredType is Class<*>) {
-            // Nothing to infer, otherwise we'd have ParameterizedType
-            return actualClass
-        } else if (declaredType is GenericArrayType) {
-            val declaredComponent = declaredType.genericComponentType
-            return inferTypeVariables(actualClass?.componentType, declaredComponent.asClass()!!, declaredComponent)?.asArray()
-        } else return null
-    }
+    private fun inferTypeVariables(actualClass: Class<*>?, declaredClass: Class<*>, declaredType: Type): Type? =
+            when (declaredType) {
+                is ParameterizedType -> inferTypeVariables(actualClass, declaredClass, declaredType)
+                // Nothing to infer, otherwise we'd have ParameterizedType
+                is Class<*> -> actualClass
+                is GenericArrayType -> {
+                    val declaredComponent = declaredType.genericComponentType
+                    inferTypeVariables(actualClass?.componentType, declaredComponent.asClass()!!, declaredComponent)?.asArray()
+                }
+                else -> null
+            }
 
     /**
      * Try and infer concrete types for any generics type variables for the actual class encountered, based on the declared
@@ -118,8 +122,7 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
                 if (implementationChain != null) {
                     val start = implementationChain.last()
                     val rest = implementationChain.dropLast(1).drop(1)
-                    val resolver = rest.reversed().fold(TypeResolver().where(start, declaredType)) {
-                        resolved, chainEntry ->
+                    val resolver = rest.reversed().fold(TypeResolver().where(start, declaredType)) { resolved, chainEntry ->
                         val newResolved = resolved.resolveType(chainEntry)
                         TypeResolver().where(chainEntry, newResolved)
                     }
@@ -195,7 +198,7 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
                 // doesn't match that of the serialised object then we are dealing with  different
                 // instance of the class, as such we need to build an EvolutionSerialiser
                 if (serialiser.typeDescriptor != typeNotation.descriptor.name) {
-                    getEvolutionSerializer(typeNotation, serialiser as ObjectSerializer)
+                    getEvolutionSerializer(typeNotation, serialiser)
                 }
             } catch (e: ClassNotFoundException) {
                 if (sentinel || (typeNotation !is CompositeType)) throw e
@@ -211,16 +214,14 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
     }
 
     private fun processSchemaEntry(typeNotation: TypeNotation) = when (typeNotation) {
-            is CompositeType -> processCompositeType(typeNotation) // java.lang.Class (whether a class or interface)
-            is RestrictedType -> processRestrictedType(typeNotation) // Collection / Map, possibly with generics
-        }
-
-    private fun processRestrictedType(typeNotation: RestrictedType): AMQPSerializer<Any> {
-        // TODO: class loader logic, and compare the schema.
-        val type = typeForName(typeNotation.name, classloader)
-        return get(null, type)
+        is CompositeType -> processCompositeType(typeNotation) // java.lang.Class (whether a class or interface)
+        is RestrictedType -> processRestrictedType(typeNotation) // Collection / Map, possibly with generics
     }
 
+    // TODO: class loader logic, and compare the schema.
+    private fun processRestrictedType(typeNotation: RestrictedType) = get(null,
+            typeForName(typeNotation.name, classloader))
+
     private fun processCompositeType(typeNotation: CompositeType): AMQPSerializer<Any> {
         // TODO: class loader logic, and compare the schema.
         val type = typeForName(typeNotation.name, classloader)
@@ -234,7 +235,7 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
             findCustomSerializer(clazz, declaredType) ?: run {
                 if (type.isArray()) {
                     // Allow Object[] since this can be quite common (i.e. an untyped array)
-                    if(type.componentType() != Object::class.java) whitelisted(type.componentType())
+                    if (type.componentType() != Object::class.java) whitelisted(type.componentType())
                     if (clazz.componentType.isPrimitive) PrimArraySerializer.make(type, this)
                     else ArraySerializer.make(type, this)
                 } else if (clazz.kotlin.objectInstance != null) {

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumTests.kt

@@ -164,7 +164,7 @@ class EnumTests {
         DeserializationInput(sf1).deserialize(SerializedBytes<C>(sc2))
     }
 
-    @Test
+    @Test(expected = NotSerializableException::class)
     fun changedEnum2() {
         val path = EnumTests::class.java.getResource("EnumTests.changedEnum2")
         val f = File(path.toURI())
@@ -173,10 +173,10 @@ class EnumTests {
 
         // DO NOT CHANGE THIS, it's important we serialise with a value that doesn't
         // change position in the upated enum class
-        val a = OldBras2.UNDERWIRE
 
         // Original version of the class for the serialised version of this class
         //
+        // val a = OldBras2.UNDERWIRE
         // val sc = SerializationOutput(sf1).serialize(C(a))
         // f.writeBytes(sc.bytes)
         // println(path)
@@ -184,8 +184,6 @@ class EnumTests {
         val sc2 = f.readBytes()
 
         // we expect this to throw
-        val obj = DeserializationInput(sf1).deserialize(SerializedBytes<C>(sc2))
-
-        assertEquals(a, obj.a)
+        DeserializationInput(sf1).deserialize(SerializedBytes<C>(sc2))
     }
 }