From 66e6f9014037cc3de442fb509dc9f05e0d6187bf Mon Sep 17 00:00:00 2001 From: Viktor Kolomeyko <31008341+vkolomeyko@users.noreply.github.com> Date: Tue, 22 Aug 2017 16:53:55 +0100 Subject: [PATCH] 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/transactions/WireTransaction.kt | 2 ++ .../amqp/DeserializationInput.kt | 13 ++++++++++-- .../amqp/DeserializedParameterizedType.kt | 14 +++---------- .../internal/serialization/amqp/Schema.kt | 18 +++++++++++------ .../serialization/amqp/SerializationHelper.kt | 3 ++- .../serialization/amqp/SerializerFactory.kt | 1 + .../nodeapi/AttachmentClassLoaderTests.kt | 20 ++++++++++++++++--- .../DeserializedParameterizedTypeTests.kt | 5 +++++ 8 files changed, 53 insertions(+), 23 deletions(-) diff --git a/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt b/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt index 970ca4d728..c3492655f1 100644 --- a/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt +++ b/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, diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt index 621b01d509..7e2fe9f749 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt +++ b/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>] + * 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 +} \ No newline at end of file diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializedParameterizedType.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializedParameterizedType.kt index 5da78fabeb..3209866821 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializedParameterizedType.kt +++ b/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>): 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) { - rawType.name - } else { - val paramsJoined = params.map { it.typeName }.joinToString(", ") - "${rawType.name}<$paramsJoined>" - } + val paramsJoined = params.map { it.typeName }.joinToString(", ") + return "${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]}") 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 c7783b2376..27f6ca1bef 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 @@ -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.ParameterizedType -import java.lang.reflect.Type -import java.lang.reflect.TypeVariable +import java.lang.reflect.* 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() } /** * 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) } } } 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 f7e11e7dbd..c2cf4a37ee 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 @@ -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 { // 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 = ArrayList(properties.size) for (property in properties) { // Check that the method has a getter in java. diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt index 6ee9e5be37..5792bb6011 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt +++ b/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.") } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/AttachmentClassLoaderTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/AttachmentClassLoaderTests.kt index 827394f7b1..3773a0dee6 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/AttachmentClassLoaderTests.kt +++ b/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().info("Ignoring Kryo specific test") + } +} \ No newline at end of file diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializedParameterizedTypeTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializedParameterizedTypeTests.kt index 469127061d..5be8f67d57 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializedParameterizedTypeTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializedParameterizedTypeTests.kt @@ -34,6 +34,11 @@ class DeserializedParameterizedTypeTests { verify("java.util.Map ") } + @Test + fun `test list of commands`() { + verify("java.util.List>") + } + @Test(expected = NotSerializableException::class) fun `test trailing text`() { verify("java.util.Mapfoo")