Make WireTransaction @CordaSerializable (#1299)

And do everything necessary for AttachmentClassLoaderTests to be passing in AMQP mode

* Changes following code review by @fenryka and @rick-r3

core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt

@@ -8,6 +8,7 @@ import net.corda.core.crypto.keys
 import net.corda.core.identity.Party
 import net.corda.core.internal.Emoji
 import net.corda.core.node.ServicesForResolution
+import net.corda.core.serialization.CordaSerializable
 import java.security.PublicKey
 import java.security.SignatureException
 import java.util.function.Predicate
@@ -17,6 +18,7 @@ import java.util.function.Predicate
  * by a [SignedTransaction] that carries the signatures over this payload.
  * The identity of the transaction is the Merkle tree root of its components (see [MerkleTree]).
  */
+@CordaSerializable
 data class WireTransaction(
         /** Pointers to the input states on the ledger, identified by (tx identity hash, output index). */
         override val inputs: List<StateRef>,

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

@@ -8,6 +8,7 @@ import org.apache.qpid.proton.amqp.DescribedType
 import org.apache.qpid.proton.amqp.UnsignedByte
 import org.apache.qpid.proton.codec.Data
 import java.io.NotSerializableException
+import java.lang.reflect.ParameterizedType
 import java.lang.reflect.Type
 import java.nio.ByteBuffer
 import java.util.*
@@ -118,7 +119,7 @@ class DeserializationInput(internal val serializerFactory: SerializerFactory) {
         if (obj is DescribedType) {
             // Look up serializer in factory by descriptor
             val serializer = serializerFactory.get(obj.descriptor, schema)
-            if (serializer.type != type && !serializer.type.isSubClassOf(type))
+            if (serializer.type != type && with(serializer.type) { !isSubClassOf(type) && !materiallyEquivalentTo(type) })
                 throw NotSerializableException("Described type with descriptor ${obj.descriptor} was " +
                         "expected to be of type $type but was ${serializer.type}")
             return serializer.readObject(obj.described, schema, this)
@@ -128,4 +129,12 @@ class DeserializationInput(internal val serializerFactory: SerializerFactory) {
             return obj
         }
     }
-}
+
+    /**
+     * TODO: Currently performs rather basic checks aimed in particular at [java.util.List<Command<?>>]
+     * In the future tighter control might be needed
+     */
+    private fun Type.materiallyEquivalentTo(that: Type): Boolean =
+        asClass() == that.asClass() && this is ParameterizedType && that is ParameterizedType &&
+                actualTypeArguments.size == that.actualTypeArguments.size
+}

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

@@ -19,25 +19,17 @@ class DeserializedParameterizedType(private val rawType: Class<*>, private val p
         if (params.size != rawType.typeParameters.size) {
             throw NotSerializableException("Expected ${rawType.typeParameters.size} for ${rawType.name} but found ${params.size}")
         }
-        // We do not check bounds.  Both our use cases (Collection and Map) are not bounded.
-        if (rawType.typeParameters.any { boundedType(it) }) throw NotSerializableException("Bounded types in ParameterizedTypes not supported, but found a bound in $rawType")
     }
 
     private fun boundedType(type: TypeVariable<out Class<out Any>>): Boolean {
         return !(type.bounds.size == 1 && type.bounds[0] == Object::class.java)
     }
 
-    val isFullyWildcarded: Boolean = params.all { it == SerializerFactory.AnyType }
-
     private val _typeName: String = makeTypeName()
 
     private fun makeTypeName(): String {
-        return if (isFullyWildcarded) {
+        val paramsJoined = params.map { it.typeName }.joinToString(", ")
-            rawType.name
+        return "${rawType.name}<$paramsJoined>"
-        } else {
-            val paramsJoined = params.map { it.typeName }.joinToString(", ")
-            "${rawType.name}<$paramsJoined>"
-        }
     }
 
     companion object {
@@ -96,7 +88,7 @@ class DeserializedParameterizedType(private val rawType: Class<*>, private val p
                             typeStart = pos++
                         } else if (!needAType) {
                             throw NotSerializableException("Not expecting a type")
-                        } else if (params[pos] == '*') {
+                        } else if (params[pos] == '?') {
                             pos++
                         } else if (!params[pos].isJavaIdentifierStart()) {
                             throw NotSerializableException("Invalid character at start of type: ${params[pos]}")

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

@@ -4,15 +4,13 @@ import com.google.common.hash.Hasher
 import com.google.common.hash.Hashing
 import net.corda.core.crypto.toBase64
 import net.corda.core.utilities.OpaqueBytes
+import net.corda.core.utilities.loggerFor
 import org.apache.qpid.proton.amqp.DescribedType
 import org.apache.qpid.proton.amqp.UnsignedLong
 import org.apache.qpid.proton.codec.Data
 import org.apache.qpid.proton.codec.DescribedTypeConstructor
 import java.io.NotSerializableException
-import java.lang.reflect.GenericArrayType
+import java.lang.reflect.*
-import java.lang.reflect.ParameterizedType
-import java.lang.reflect.Type
-import java.lang.reflect.TypeVariable
 import java.util.*
 
 import net.corda.nodeapi.internal.serialization.carpenter.Field as CarpenterField
@@ -316,6 +314,9 @@ private val NULLABLE_HASH: String = "Nullable = true"
 private val NOT_NULLABLE_HASH: String = "Nullable = false"
 private val ANY_TYPE_HASH: String = "Any type = true"
 private val TYPE_VARIABLE_HASH: String = "Type variable = true"
+private val WILDCARD_TYPE_HASH: String = "Wild card = true"
+
+private val logger by lazy { loggerFor<Schema>() }
 
 /**
  * The method generates a fingerprint for a given JVM [Type] that should be unique to the schema representation.
@@ -386,11 +387,16 @@ private fun fingerprintForType(type: Type, contextType: Type?, alreadySeen: Muta
             } else if (type is TypeVariable<*>) {
                 // TODO: include bounds
                 hasher.putUnencodedChars(type.name).putUnencodedChars(TYPE_VARIABLE_HASH)
-            } else {
+            } else if (type is WildcardType) {
+                hasher.putUnencodedChars(type.typeName).putUnencodedChars(WILDCARD_TYPE_HASH)
+            }
+            else {
                 throw NotSerializableException("Don't know how to hash")
             }
         } catch(e: NotSerializableException) {
-            throw NotSerializableException("${e.message} -> $type")
+            val msg = "${e.message} -> $type"
+            logger.error(msg, e)
+            throw NotSerializableException(msg)
         }
     }
 }

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

@@ -2,6 +2,7 @@ package net.corda.nodeapi.internal.serialization.amqp
 
 import com.google.common.reflect.TypeToken
 import org.apache.qpid.proton.codec.Data
+import java.beans.IndexedPropertyDescriptor
 import java.beans.Introspector
 import java.io.NotSerializableException
 import java.lang.reflect.*
@@ -92,7 +93,7 @@ private fun constructorParamTakesReturnTypeOfGetter(getter: Method, param: KPara
 
 private fun propertiesForSerializationFromAbstract(clazz: Class<*>, type: Type, factory: SerializerFactory): Collection<PropertySerializer> {
     // 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 }
+    val properties = Introspector.getBeanInfo(clazz).propertyDescriptors.filter { it.name != "class" }.sortedBy { it.name }.filterNot { it is IndexedPropertyDescriptor }
     val rc: MutableList<PropertySerializer> = ArrayList(properties.size)
     for (property in properties) {
         // Check that the method has a getter in java.

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

@@ -323,6 +323,7 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) {
             }
             is ParameterizedType -> "${nameForType(type.rawType)}<${type.actualTypeArguments.joinToString { nameForType(it) }}>"
             is GenericArrayType -> "${nameForType(type.genericComponentType)}[]"
+            is WildcardType -> "Any"
             else -> throw NotSerializableException("Unable to render type $type to a string.")
         }
 

node-api/src/test/kotlin/net/corda/nodeapi/AttachmentClassLoaderTests.kt

@@ -15,6 +15,8 @@ import net.corda.core.transactions.LedgerTransaction
 import net.corda.core.transactions.TransactionBuilder
 import net.corda.core.utilities.ByteSequence
 import net.corda.core.utilities.OpaqueBytes
+import net.corda.core.utilities.loggerFor
+import net.corda.nodeapi.internal.serialization.AMQP_ENABLED
 import net.corda.nodeapi.internal.serialization.SerializeAsTokenContextImpl
 import net.corda.nodeapi.internal.serialization.WireTransactionSerializer
 import net.corda.nodeapi.internal.serialization.withTokenContext
@@ -330,7 +332,8 @@ class AttachmentClassLoaderTests : TestDependencyInjectionBase() {
     }
 
     @Test
-    fun `test deserialize of WireTransaction where contract cannot be found`() {
+    // Kryo verifies/loads attachments on deserialization, whereas AMQP currently does not
+    fun `test deserialize of WireTransaction where contract cannot be found`() = kryoSpecific {
         val child = ClassLoaderForTests()
         val contractClass = Class.forName("net.corda.contracts.isolated.AnotherDummyContract", true, child)
         val contract = contractClass.newInstance() as DummyContractBackdoor
@@ -350,8 +353,19 @@ class AttachmentClassLoaderTests : TestDependencyInjectionBase() {
         // use empty attachmentStorage
 
         val e = assertFailsWith(MissingAttachmentsException::class) {
-            bytes.deserialize(context = P2P_CONTEXT.withAttachmentStorage(MockAttachmentStorage()))
+            val mockAttStorage = MockAttachmentStorage()
+            bytes.deserialize(context = P2P_CONTEXT.withAttachmentStorage(mockAttStorage))
+
+            if(mockAttStorage.openAttachment(attachmentRef) == null) {
+                throw MissingAttachmentsException(listOf(attachmentRef))
+            }
         }
         assertEquals(attachmentRef, e.ids.single())
     }
-}
+
+    private fun kryoSpecific(function: () -> Unit) = if(!AMQP_ENABLED) {
+        function()
+    } else {
+        loggerFor<AttachmentClassLoaderTests>().info("Ignoring Kryo specific test")
+    }
+}

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

@@ -34,6 +34,11 @@ class DeserializedParameterizedTypeTests {
         verify("java.util.Map<java.lang.String, java.lang.Integer> ")
     }
 
+    @Test
+    fun `test list of commands`() {
+        verify("java.util.List<net.corda.core.contracts.Command<?>>")
+    }
+
     @Test(expected = NotSerializableException::class)
     fun `test trailing text`() {
         verify("java.util.Map<java.lang.String, java.lang.Integer>foo")