From 06a6eace67fa44205cc0b2d8aaf3bf7afcc63a3f Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Fri, 2 Mar 2018 13:13:00 +0000 Subject: [PATCH] CORDA-1115 - Cannot serialize private nested objects (#2665) * CORDA-1115 - Cannot serialize private nested objects Shown up by the simm-valuation-demo the problem was where a private object field of an object was being serialised within the outer objects context (see tests added for example) Fix is to switch from Kotlin reflection back to Java. Additional fix to the test where it was comparing two lists of state references in a flow and they weren't equal because they weren't in the same order... This I assume is just an oversight (in that them being in a different order but otherwise the same is actually fine) so converting to set comparison * Fix forward port issue where fingerprinting has moved * Review Comments * Review Comments * Review Comments * Gran -> Grab --- .../serialization/amqp/FingerPrinter.kt | 2 +- .../serialization/amqp/SerializationHelper.kt | 45 +++++++++++++++++++ .../serialization/amqp/SerializerFactory.kt | 13 +++--- .../amqp/SerializationOutputTests.kt | 32 ++++++++++++- .../kotlin/net/corda/vega/flows/SimmFlow.kt | 3 +- 5 files changed, 87 insertions(+), 8 deletions(-) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/FingerPrinter.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/FingerPrinter.kt index e5cc478acb..79ee00f5e5 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/FingerPrinter.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/FingerPrinter.kt @@ -153,7 +153,7 @@ class SerializerFingerPrinter : FingerPrinter { }.putUnencodedChars(type.name).putUnencodedChars(ENUM_HASH) } else { hasher.fingerprintWithCustomSerializerOrElse(factory!!, type, type) { - if (type.kotlin.objectInstance != null) { + if (type.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) 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 83e3486d5a..4c70dc0cd2 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 @@ -513,3 +513,48 @@ fun ClassWhitelist.hasAnnotationInHierarchy(type: Class<*>): Boolean { || type.interfaces.any { hasAnnotationInHierarchy(it) } || (type.superclass != null && hasAnnotationInHierarchy(type.superclass)) } + +/** + * By default use Kotlin reflection and grab the objectInstance. However, that doesn't play nicely with nested + * private objects. Even setting the accessibility override (setAccessible) still causes an + * [IllegalAccessException] when attempting to retrieve the value of the INSTANCE field. + * + * Whichever reference to the class Kotlin reflection uses, override (set from setAccessible) on that field + * isn't set even when it was explicitly set as accessible before calling into the kotlin reflection routines. + * + * For example + * + * clazz.getDeclaredField("INSTANCE")?.apply { + * isAccessible = true + * kotlin.objectInstance // This throws as the INSTANCE field isn't accessible + * } + * + * Therefore default back to good old java reflection and simply look for the INSTANCE field as we are never going + * to serialize a companion object. + * + * As such, if objectInstance fails access, revert to Java reflection and try that + */ +fun Class<*>.objectInstance() = + try { + this.kotlin.objectInstance + } catch (e: IllegalAccessException) { + // Check it really is an object (i.e. it has no constructor) + if (constructors.isNotEmpty()) null + else { + try { + this.getDeclaredField("INSTANCE")?.let { field -> + // and must be marked as both static and final (>0 means they're set) + if (modifiers and Modifier.STATIC == 0 || modifiers and Modifier.FINAL == 0) null + else { + val accessibility = field.isAccessible + field.isAccessible = true + val obj = field.get(null) + field.isAccessible = accessibility + obj + } + } + } catch (e: NoSuchFieldException) { + null + } + } + } 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 011b04dbf7..94f319f697 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 @@ -267,12 +267,15 @@ open class SerializerFactory( // 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) else ArraySerializer.make(type, this) - } else if (clazz.kotlin.objectInstance != null) { - whitelist.requireWhitelisted(clazz) - SingletonSerializer(clazz, clazz.kotlin.objectInstance!!, this) } else { - whitelist.requireWhitelisted(type) - ObjectSerializer(type, this) + val singleton = clazz.objectInstance() + if (singleton != null) { + whitelist.requireWhitelisted(clazz) + SingletonSerializer(clazz, singleton, this) + } else { + whitelist.requireWhitelisted(type) + ObjectSerializer(type, this) + } } } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt index f8aa4fc768..e69a949dbd 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt @@ -47,6 +47,22 @@ import kotlin.test.assertEquals import kotlin.test.assertNotNull import kotlin.test.assertTrue +object AckWrapper { + object Ack + fun serialize() { + val factory = testDefaultFactoryNoEvolution() + SerializationOutput(factory).serialize(Ack) + } +} + +object PrivateAckWrapper { + private object Ack + fun serialize() { + val factory = testDefaultFactoryNoEvolution() + SerializationOutput(factory).serialize(Ack) + } +} + @RunWith(Parameterized::class) class SerializationOutputTests(private val compression: CordaSerializationEncoding?) { private companion object { @@ -1148,4 +1164,18 @@ class SerializationOutputTests(private val compression: CordaSerializationEncodi assertEquals(encodingNotPermittedFormat.format(compression), message) } } -} \ No newline at end of file + + @Test + fun nestedObjects() { + // The "test" is that this doesn't throw, anything else is a success + AckWrapper.serialize() + } + + @Test + fun privateNestedObjects() { + // The "test" is that this doesn't throw, anything else is a success + PrivateAckWrapper.serialize() + } + +} + diff --git a/samples/simm-valuation-demo/src/main/kotlin/net/corda/vega/flows/SimmFlow.kt b/samples/simm-valuation-demo/src/main/kotlin/net/corda/vega/flows/SimmFlow.kt index cfccafbc4d..2424101214 100644 --- a/samples/simm-valuation-demo/src/main/kotlin/net/corda/vega/flows/SimmFlow.kt +++ b/samples/simm-valuation-demo/src/main/kotlin/net/corda/vega/flows/SimmFlow.kt @@ -305,7 +305,8 @@ object SimmFlow { @Suspendable private fun agreePortfolio(portfolio: Portfolio): SignedTransaction { logger.info("Handshake finished, awaiting Simm offer") - require(offer.dealBeingOffered.portfolio == portfolio.refs) + + require(offer.dealBeingOffered.portfolio.toSet() == portfolio.refs.toSet()) val seller = TwoPartyDealFlow.Instigator( replyToSession,