CORDA-1747 - External serializes break for generic types (#3541)

* CORDA-1747 - External serializes break for generic types

* Review comments

* review comments

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/AMQPSerializationScheme.kt

@@ -11,6 +11,7 @@ import net.corda.core.internal.objectOrNewInstance
 import net.corda.core.internal.uncheckedCast
 import net.corda.core.serialization.*
 import net.corda.core.utilities.ByteSequence
+import net.corda.core.utilities.contextLogger
 import net.corda.serialization.internal.*
 import java.lang.reflect.Modifier
 import java.util.*
@@ -48,6 +49,8 @@ abstract class AbstractAMQPSerializationScheme(
     companion object {
         const val SCAN_SPEC_PROP_NAME = "amqp.custom.serialization.scanSpec"
 
+        private val logger = contextLogger()
+
         private val serializationWhitelists: List<SerializationWhitelist> by lazy {
             ServiceLoader.load(SerializationWhitelist::class.java, this::class.java.classLoader).toList() + DefaultWhitelist
         }
@@ -56,8 +59,10 @@ abstract class AbstractAMQPSerializationScheme(
             val scanSpec: String? = System.getProperty(SCAN_SPEC_PROP_NAME)
 
             if (scanSpec == null) {
+                logger.info ("scanSpec not set, not scanning for Custom Serializers")
                 emptyList()
             } else {
+                logger.info ("scanSpec = \"$scanSpec\", scanning for Custom Serializers")
                 scanClasspathForSerializers(scanSpec)
             }
         }
@@ -121,6 +126,7 @@ abstract class AbstractAMQPSerializationScheme(
                 factory.registerExternal(CorDappCustomSerializer(customSerializer, factory))
             }
         } else {
+            logger.info("Custom Serializer list loaded - not scanning classpath")
             cordappCustomSerializers.forEach { customSerializer ->
                 factory.registerExternal(CorDappCustomSerializer(customSerializer, factory))
             }

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/CorDappCustomSerializer.kt

@@ -1,5 +1,6 @@
 package net.corda.serialization.internal.amqp
 
+import com.google.common.reflect.TypeToken
 import net.corda.core.internal.uncheckedCast
 import net.corda.core.serialization.SerializationContext
 import net.corda.core.serialization.SerializationCustomSerializer
@@ -45,8 +46,10 @@ const val PROXY_TYPE = 1
  */
 class CorDappCustomSerializer(
         private val serializer: SerializationCustomSerializer<*, *>,
-        factory: SerializerFactory) : AMQPSerializer<Any>, SerializerFor {
+        factory: SerializerFactory
+) : AMQPSerializer<Any>, SerializerFor {
     override val revealSubclassesInSchema: Boolean get() = false
+
     private val types = serializer::class.supertypes.filter { it.jvmErasure == SerializationCustomSerializer::class }
             .flatMap { it.arguments }
             .map { it.type!!.javaType }
@@ -85,6 +88,12 @@ class CorDappCustomSerializer(
     ) = uncheckedCast<SerializationCustomSerializer<*, *>, SerializationCustomSerializer<Any?, Any?>>(
             serializer).fromProxy(uncheckedCast(proxySerializer.readObject(obj, schemas, input, context)))!!
 
-    override fun isSerializerFor(clazz: Class<*>) = clazz == type
+    /**
+     * For 3rd party plugin serializers we are going to exist on exact type matching. i.e. we will
+     * not support base class serializers for derivedtypes
+     */
+    override fun isSerializerFor(clazz: Class<*>) : Boolean {
+        return type.asClass()?.let { TypeToken.of(it) == TypeToken.of(clazz) } ?: false
+    }
 }
 

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/SerializerFactory.kt

@@ -32,6 +32,8 @@ data class FactorySchemaAndDescriptor(val schemas: SerializationSchemas, val typ
  * @property evolutionSerializerGetter controls how evolution serializers are generated by the factory. The normal
  * use case is an [EvolutionSerializer] type is returned. However, in some scenarios, primarily testing, this
  * can be altered to fit the requirements of the test.
+ * @property onlyCustomSerializers used for testing, when set will cause the factory to throw a
+ * [NotSerializableException] if it cannot find a registered custom serializer for a given type
  */
 // TODO: support for intern-ing of deserialized objects for some core types (e.g. PublicKey) for memory efficiency
 // TODO: maybe support for caching of serialized form of some core types for performance
@@ -54,13 +56,15 @@ open class SerializerFactory(
         private val serializersByType: MutableMap<Type, AMQPSerializer<Any>>,
         val serializersByDescriptor: MutableMap<Any, AMQPSerializer<Any>>,
         private val customSerializers: MutableList<SerializerFor>,
-        val transformsCache: MutableMap<String, EnumMap<TransformTypes, MutableList<Transform>>>
+        val transformsCache: MutableMap<String, EnumMap<TransformTypes, MutableList<Transform>>>,
+        private val onlyCustomSerializers: Boolean = false
 ) {
     @DeleteForDJVM
     constructor(whitelist: ClassWhitelist,
                 classCarpenter: ClassCarpenter,
                 evolutionSerializerGetter: EvolutionSerializerGetterBase = EvolutionSerializerGetter(),
-                fingerPrinter: FingerPrinter = SerializerFingerPrinter()
+                fingerPrinter: FingerPrinter = SerializerFingerPrinter(),
+                onlyCustomSerializers: Boolean = false
     ) : this(
             whitelist,
             classCarpenter,
@@ -69,7 +73,8 @@ open class SerializerFactory(
             ConcurrentHashMap(),
             ConcurrentHashMap(),
             CopyOnWriteArrayList(),
-            ConcurrentHashMap()
+            ConcurrentHashMap(),
+            onlyCustomSerializers
     )
 
     @DeleteForDJVM
@@ -77,8 +82,14 @@ open class SerializerFactory(
                 carpenterClassLoader: ClassLoader,
                 lenientCarpenter: Boolean = false,
                 evolutionSerializerGetter: EvolutionSerializerGetterBase = EvolutionSerializerGetter(),
-                fingerPrinter: FingerPrinter = SerializerFingerPrinter()
+                fingerPrinter: FingerPrinter = SerializerFingerPrinter(),
-    ) : this(whitelist, ClassCarpenterImpl(whitelist, carpenterClassLoader, lenientCarpenter), evolutionSerializerGetter, fingerPrinter)
+                onlyCustomSerializers: Boolean = false
+    ) : this(
+            whitelist,
+            ClassCarpenterImpl(whitelist, carpenterClassLoader, lenientCarpenter),
+            evolutionSerializerGetter,
+            fingerPrinter,
+            onlyCustomSerializers)
 
     init {
         fingerPrinter.setOwner(this)
@@ -236,6 +247,7 @@ open class SerializerFactory(
      * that expects to find getters and a constructor with a parameter for each property.
      */
     open fun register(customSerializer: CustomSerializer<out Any>) {
+        logger.info ("action=\"Registering custom serializer\", class=\"${customSerializer.type}\"")
         if (!serializersByDescriptor.containsKey(customSerializer.typeDescriptor)) {
             customSerializers += customSerializer
             serializersByDescriptor[customSerializer.typeDescriptor] = customSerializer
@@ -246,6 +258,7 @@ open class SerializerFactory(
     }
 
     fun registerExternal(customSerializer: CorDappCustomSerializer) {
+        logger.info ("action=\"Registering external serializer\", class=\"${customSerializer.type}\"")
         if (!serializersByDescriptor.containsKey(customSerializer.typeDescriptor)) {
             customSerializers += customSerializer
             serializersByDescriptor[customSerializer.typeDescriptor] = customSerializer
@@ -328,8 +341,12 @@ open class SerializerFactory(
         return get(type.asClass() ?: throw NotSerializableException("Unable to build composite type for $type"), type)
     }
 
-    private fun makeClassSerializer(clazz: Class<*>, type: Type, declaredType: Type): AMQPSerializer<Any> = serializersByType.computeIfAbsent(type) {
+    private fun makeClassSerializer(
-        logger.debug("class=${clazz.simpleName}, type=$type is a composite type")
+            clazz: Class<*>,
+            type: Type,
+            declaredType: Type
+    ): AMQPSerializer<Any> = serializersByType.computeIfAbsent(type) {
+        logger.debug { "class=${clazz.simpleName}, type=$type is a composite type" }
         if (clazz.isSynthetic) {
             // Explicitly ban synthetic classes, we have no way of recreating them when deserializing. This also
             // captures Lambda expressions and other anonymous functions
@@ -338,6 +355,9 @@ open class SerializerFactory(
             AMQPPrimitiveSerializer(clazz)
         } else {
             findCustomSerializer(clazz, declaredType) ?: run {
+                if (onlyCustomSerializers) {
+                    throw NotSerializableException("Only allowing custom serializers")
+                }
                 if (type.isArray()) {
                     // Don't need to check the whitelist since each element will come back through the whitelisting process.
                     if (clazz.componentType.isPrimitive) PrimArraySerializer.make(type, this)
@@ -363,9 +383,15 @@ open class SerializerFactory(
         for (customSerializer in customSerializers) {
             if (customSerializer.isSerializerFor(clazz)) {
                 val declaredSuperClass = declaredType.asClass()?.superclass
+
+
                 return if (declaredSuperClass == null
                         || !customSerializer.isSerializerFor(declaredSuperClass)
-                        || !customSerializer.revealSubclassesInSchema) {
+                        || !customSerializer.revealSubclassesInSchema
+                ) {
+                    logger.info ("action=\"Using custom serializer\", class=${clazz.typeName}, " +
+                            "declaredType=${declaredType.typeName}")
+
                     @Suppress("UNCHECKED_CAST")
                     customSerializer as? AMQPSerializer<Any>
                 } else {

serialization/src/test/kotlin/net/corda/serialization/internal/amqp/CorDappSerializerTests.kt

@@ -3,6 +3,7 @@ package net.corda.serialization.internal.amqp
 import org.junit.Test
 import net.corda.core.serialization.ClassWhitelist
 import net.corda.core.serialization.SerializationCustomSerializer
+import net.corda.serialization.internal.AllWhitelist
 import net.corda.serialization.internal.amqp.testutils.deserializeAndReturnEnvelope
 import net.corda.serialization.internal.amqp.testutils.deserialize
 import net.corda.serialization.internal.amqp.testutils.serializeAndReturnSchema
@@ -12,9 +13,20 @@ import org.assertj.core.api.Assertions
 import java.io.NotSerializableException
 import kotlin.test.assertEquals
 
-
 class CorDappSerializerTests {
-    data class NeedsProxy (val a: String)
+    data class NeedsProxy(val a: String)
+
+    private fun proxyFactory(
+            serializers: List<SerializationCustomSerializer<*, *>>
+    ) = SerializerFactory(
+            AllWhitelist,
+            ClassLoader.getSystemClassLoader(),
+            onlyCustomSerializers = true
+    ).apply {
+        serializers.forEach {
+            registerExternal(CorDappCustomSerializer(it, this))
+        }
+    }
 
     class NeedsProxyProxySerializer : SerializationCustomSerializer<NeedsProxy, NeedsProxyProxySerializer.Proxy> {
         data class Proxy(val proxy_a_: String)
@@ -25,7 +37,7 @@ class CorDappSerializerTests {
 
     // Standard proxy serializer used internally, here for comparison purposes
     class InternalProxySerializer(factory: SerializerFactory) :
-            CustomSerializer.Proxy<NeedsProxy, InternalProxySerializer.Proxy> (
+            CustomSerializer.Proxy<NeedsProxy, InternalProxySerializer.Proxy>(
                     NeedsProxy::class.java,
                     InternalProxySerializer.Proxy::class.java,
                     factory) {
@@ -48,14 +60,14 @@ class CorDappSerializerTests {
 
         val msg = "help"
 
-        proxyFactory.registerExternal (CorDappCustomSerializer(NeedsProxyProxySerializer(), proxyFactory))
+        proxyFactory.registerExternal(CorDappCustomSerializer(NeedsProxyProxySerializer(), proxyFactory))
-        internalProxyFactory.register (InternalProxySerializer(internalProxyFactory))
+        internalProxyFactory.register(InternalProxySerializer(internalProxyFactory))
 
         val needsProxy = NeedsProxy(msg)
 
-        val bAndSProxy = SerializationOutput(proxyFactory).serializeAndReturnSchema (needsProxy)
+        val bAndSProxy = SerializationOutput(proxyFactory).serializeAndReturnSchema(needsProxy)
-        val bAndSInternal = SerializationOutput(internalProxyFactory).serializeAndReturnSchema (needsProxy)
+        val bAndSInternal = SerializationOutput(internalProxyFactory).serializeAndReturnSchema(needsProxy)
-        val bAndSDefault = SerializationOutput(defaultFactory).serializeAndReturnSchema (needsProxy)
+        val bAndSDefault = SerializationOutput(defaultFactory).serializeAndReturnSchema(needsProxy)
 
         val objFromDefault = DeserializationInput(defaultFactory).deserializeAndReturnEnvelope(bAndSDefault.obj)
         val objFromInternal = DeserializationInput(internalProxyFactory).deserializeAndReturnEnvelope(bAndSInternal.obj)
@@ -68,14 +80,14 @@ class CorDappSerializerTests {
 
     @Test
     fun proxiedTypeIsNested() {
-        data class A (val a: Int, val b: NeedsProxy)
+        data class A(val a: Int, val b: NeedsProxy)
 
         val factory = testDefaultFactory()
-        factory.registerExternal (CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
+        factory.registerExternal(CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
 
         val tv1 = 100
         val tv2 = "pants schmants"
-        val bAndS = SerializationOutput(factory).serializeAndReturnSchema (A(tv1, NeedsProxy(tv2)))
+        val bAndS = SerializationOutput(factory).serializeAndReturnSchema(A(tv1, NeedsProxy(tv2)))
 
         val objFromDefault = DeserializationInput(factory).deserializeAndReturnEnvelope(bAndS.obj)
 
@@ -85,7 +97,7 @@ class CorDappSerializerTests {
 
     @Test
     fun testWithWhitelistNotAllowed() {
-        data class A (val a: Int, val b: NeedsProxy)
+        data class A(val a: Int, val b: NeedsProxy)
 
         class WL : ClassWhitelist {
             private val allowedClasses = emptySet<String>()
@@ -94,7 +106,7 @@ class CorDappSerializerTests {
         }
 
         val factory = SerializerFactory(WL(), ClassLoader.getSystemClassLoader())
-        factory.registerExternal (CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
+        factory.registerExternal(CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
 
         val tv1 = 100
         val tv2 = "pants schmants"
@@ -105,7 +117,7 @@ class CorDappSerializerTests {
 
     @Test
     fun testWithWhitelistAllowed() {
-        data class A (val a: Int, val b: NeedsProxy)
+        data class A(val a: Int, val b: NeedsProxy)
 
         class WL : ClassWhitelist {
             private val allowedClasses = setOf<String>(
@@ -116,7 +128,7 @@ class CorDappSerializerTests {
         }
 
         val factory = SerializerFactory(WL(), ClassLoader.getSystemClassLoader())
-        factory.registerExternal (CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
+        factory.registerExternal(CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
 
         val tv1 = 100
         val tv2 = "pants schmants"
@@ -131,7 +143,7 @@ class CorDappSerializerTests {
     // custom serializer bypasses the whitelist
     @Test
     fun testWithWhitelistAllowedOuterOnly() {
-        data class A (val a: Int, val b: NeedsProxy)
+        data class A(val a: Int, val b: NeedsProxy)
 
         class WL : ClassWhitelist {
             // explicitly don't add NeedsProxy
@@ -141,7 +153,7 @@ class CorDappSerializerTests {
         }
 
         val factory = SerializerFactory(WL(), ClassLoader.getSystemClassLoader())
-        factory.registerExternal (CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
+        factory.registerExternal(CorDappCustomSerializer(NeedsProxyProxySerializer(), factory))
 
         val tv1 = 100
         val tv2 = "pants schmants"
@@ -151,4 +163,136 @@ class CorDappSerializerTests {
         assertEquals(tv1, obj.a)
         assertEquals(tv2, obj.b.a)
     }
+
+    data class NeedsProxyGen<T>(val a: T)
+
+    class NeedsProxyGenProxySerializer :
+            SerializationCustomSerializer<NeedsProxyGen<*>, NeedsProxyGenProxySerializer.Proxy> {
+        data class Proxy(val proxy_a_: Any?)
+
+        override fun fromProxy(proxy: Proxy) = NeedsProxyGen(proxy.proxy_a_)
+        override fun toProxy(obj: NeedsProxyGen<*>) = Proxy(obj.a)
+    }
+
+    // Tests CORDA-1747
+    @Test
+    fun proxiedGeneric() {
+        val proxyFactory = proxyFactory(listOf(NeedsProxyGenProxySerializer()))
+
+        val msg = "help"
+
+
+        val blob = SerializationOutput(proxyFactory).serialize(NeedsProxyGen(msg))
+        val objFromProxy = DeserializationInput(proxyFactory).deserialize(blob)
+
+        assertEquals(msg, objFromProxy.a)
+    }
+
+    // Need an interface to restrict the generic to in the following test
+    interface Bound {
+        fun wibbleIt(): String
+    }
+
+    // test class to be serialized whose generic arg is restricted to instances
+    // of the Bound interface declared above
+    data class NeedsProxyGenBounded<T : Bound>(val a: T)
+
+    // Proxy for our test class
+    class NeedsProxyGenBoundedProxySerializer :
+            SerializationCustomSerializer<NeedsProxyGenBounded<*>,
+                    NeedsProxyGenBoundedProxySerializer.Proxy> {
+        data class Proxy(val proxy_a_: Bound)
+
+        override fun fromProxy(proxy: Proxy) = NeedsProxyGenBounded(proxy.proxy_a_)
+        override fun toProxy(obj: NeedsProxyGenBounded<*>) = Proxy(obj.a)
+    }
+
+    // Since we need a value for our test class that implements the interface
+    // we're restricting its generic property to, this is that class.
+    data class HasWibble(val a: String) : Bound {
+        override fun wibbleIt() = "wibble it, just a little bit!."
+    }
+
+    // Because we're enforcing all classes have proxy serializers we
+    // have to implement this to avoid the factory erroneously failing
+    class HasWibbleProxy :
+            SerializationCustomSerializer<HasWibble, HasWibbleProxy.Proxy> {
+        data class Proxy(val proxy_a_: String)
+
+        override fun fromProxy(proxy: Proxy) = HasWibble(proxy.proxy_a_)
+        override fun toProxy(obj: HasWibble) = Proxy(obj.a)
+    }
+
+    // Tests CORDA-1747 - Finally the actual bound generics test, on failure it will throw
+    @Test
+    fun proxiedBoundedGeneric() {
+        val proxyFactory = proxyFactory(listOf(NeedsProxyGenBoundedProxySerializer(), HasWibbleProxy()))
+
+        val blob = SerializationOutput(proxyFactory).serialize(NeedsProxyGenBounded(HasWibble("A")))
+        val objFromProxy = DeserializationInput(proxyFactory).deserialize(blob)
+
+        assertEquals("A", objFromProxy.a.a)
+    }
+
+    data class NeedsProxyGenContainer<T>(val a: List<T>)
+
+    class NeedsProxyGenContainerProxySerializer :
+            SerializationCustomSerializer<NeedsProxyGenContainer<*>,
+                    NeedsProxyGenContainerProxySerializer.Proxy> {
+        data class Proxy(val proxy_a_: List<*>)
+
+        override fun fromProxy(proxy: Proxy) = NeedsProxyGenContainer(proxy.proxy_a_)
+        override fun toProxy(obj: NeedsProxyGenContainer<*>) = Proxy(obj.a)
+    }
+
+    // Tests CORDA-1747
+    @Test
+    fun proxiedGenericContainer() {
+        val proxyFactory = proxyFactory(listOf(NeedsProxyGenContainerProxySerializer()))
+
+        val blob1 = SerializationOutput(proxyFactory).serialize(NeedsProxyGenContainer(listOf(1, 2, 3)))
+        val obj1 = DeserializationInput(proxyFactory).deserialize(blob1)
+
+        assertEquals(listOf(1, 2, 3), obj1.a)
+
+        val blob2 = SerializationOutput(proxyFactory).serialize(NeedsProxyGenContainer(listOf("1", "2", "3")))
+        val obj2 = DeserializationInput(proxyFactory).deserialize(blob2)
+
+        assertEquals(listOf("1", "2", "3"), obj2.a)
+
+        val blob3 = SerializationOutput(proxyFactory).serialize(NeedsProxyGenContainer(listOf("1", 2, "3")))
+        val obj3 = DeserializationInput(proxyFactory).deserialize(blob3)
+
+        assertEquals(listOf("1", 2, "3"), obj3.a)
+    }
+
+    open class Base<T>(val a: T)
+    class Derived<T>(a: T, val b: String) : Base<T>(a)
+
+    class BaseProxy :
+            SerializationCustomSerializer<Base<*>, BaseProxy.Proxy> {
+        data class Proxy(val proxy_a_: Any?)
+
+        override fun fromProxy(proxy: Proxy) = Base(proxy.proxy_a_)
+        override fun toProxy(obj: Base<*>) = Proxy(obj.a)
+    }
+
+    class DerivedProxy :
+            SerializationCustomSerializer<Derived<*>, DerivedProxy.Proxy> {
+        data class Proxy(val proxy_a_: Any?, val proxy_b_: String)
+
+        override fun fromProxy(proxy: Proxy) = Derived(proxy.proxy_a_, proxy.proxy_b_)
+        override fun toProxy(obj: Derived<*>) = Proxy(obj.a, obj.b)
+    }
+
+    // Tests CORDA-1747
+    @Test
+    fun proxiedInheritableGenerics() {
+        val proxyFactory = proxyFactory(listOf(BaseProxy(), DerivedProxy()))
+
+        val blob1 = SerializationOutput(proxyFactory).serialize(Base(100L))
+        DeserializationInput(proxyFactory).deserialize(blob1)
+        val blob2 = SerializationOutput(proxyFactory).serialize(Derived(100L, "Hey pants"))
+        DeserializationInput(proxyFactory).deserialize(blob2)
+    }
 }

serialization/src/test/kotlin/net/corda/serialization/internal/amqp/testutils/AMQPTestUtils.kt

@@ -17,9 +17,14 @@ import java.io.NotSerializableException
 import java.nio.file.StandardCopyOption.REPLACE_EXISTING
 
 fun testDefaultFactory() = SerializerFactory(AllWhitelist, ClassLoader.getSystemClassLoader())
+
 fun testDefaultFactoryNoEvolution(): SerializerFactory {
-    return SerializerFactory(AllWhitelist, ClassLoader.getSystemClassLoader(), evolutionSerializerGetter = EvolutionSerializerGetterTesting())
+    return SerializerFactory(
+            AllWhitelist,
+            ClassLoader.getSystemClassLoader(),
+            evolutionSerializerGetter = EvolutionSerializerGetterTesting())
 }
+
 fun testDefaultFactoryWithWhitelist() = SerializerFactory(EmptyWhitelist, ClassLoader.getSystemClassLoader())
 
 class TestSerializationOutput(