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 b65ec08f58..b4c15a0110 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 @@ -127,7 +127,7 @@ fun Class.propertyDescriptors(): Map { clazz!!.declaredFields.forEach { classProperties.put(it.name, PropertyDescriptor(it, null, null)) } // then pair them up with the declared getter and setter - // Note: It is possible for a class to have multipleinstancess of a function where the types + // Note: It is possible for a class to have multiple instancess of a function where the types // differ. For example: // interface I { val a: T } // class D(override val a: String) : I @@ -135,28 +135,37 @@ fun Class.propertyDescriptors(): Map { // getA - returning a String (java.lang.String) and // getA - returning an Object (java.lang.Object) // In this instance we take the most derived object - clazz.declaredMethods?.map { - PropertyDescriptorsRegex.re.find(it.name)?.apply { + // + // In addition, only getters that take zero parameters and setters that take a single + // parameter will be considered + clazz.declaredMethods?.map { func -> + PropertyDescriptorsRegex.re.find(func.name)?.apply { try { classProperties.getValue(groups[2]!!.value.decapitalize()).apply { when (groups[1]!!.value) { "set" -> { - if (setter == null) setter = it - else if (TypeToken.of(setter!!.genericReturnType).isSupertypeOf(it.genericReturnType)) { - setter = it + if (func.parameterCount == 1) { + if (setter == null) setter = func + else if (TypeToken.of(setter!!.genericReturnType).isSupertypeOf(func.genericReturnType)) { + setter = func + } } } "get" -> { - if (getter == null) getter = it - else if (TypeToken.of(getter!!.genericReturnType).isSupertypeOf(it.genericReturnType)) { - getter = it + if (func.parameterCount == 0) { + if (getter == null) getter = func + else if (TypeToken.of(getter!!.genericReturnType).isSupertypeOf(func.genericReturnType)) { + getter = func + } } } "is" -> { - val rtnType = TypeToken.of(it.genericReturnType) - if ((rtnType == TypeToken.of(Boolean::class.java)) - || (rtnType == TypeToken.of(Boolean::class.javaObjectType))) { - if (getter == null) getter = it + if (func.parameterCount == 0) { + val rtnType = TypeToken.of(func.genericReturnType) + if ((rtnType == TypeToken.of(Boolean::class.java)) + || (rtnType == TypeToken.of(Boolean::class.javaObjectType))) { + if (getter == null) getter = func + } } } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt index a79416ff6f..ae35d46286 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt @@ -86,7 +86,7 @@ class PrivatePropertyTests { @Test fun testMultiArgSetter() { @Suppress("UNUSED") - data class C(private var a: Int, val b: Int) { + data class C(private var a: Int, var b: Int) { // This will force the serialization engine to use getter / setter // instantiation for the object rather than construction @ConstructorForDeserialization @@ -97,10 +97,9 @@ class PrivatePropertyTests { } val c1 = C(33, 44) - Assertions.assertThatThrownBy { - SerializationOutput(factory).serialize(c1) - }.isInstanceOf(NotSerializableException::class.java).hasMessageContaining( - "Defined setter for parameter a takes too many arguments") + val c2 = DeserializationInput(factory).deserialize(SerializationOutput(factory).serialize(c1)) + assertEquals(0, c2.getA()) + assertEquals(44, c2.b) } @Test 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 d8c1db9cf9..89b2dcbad4 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 @@ -20,6 +20,8 @@ import net.corda.nodeapi.internal.serialization.AllWhitelist import net.corda.nodeapi.internal.serialization.EmptyWhitelist import net.corda.nodeapi.internal.serialization.GeneratedAttachment import net.corda.nodeapi.internal.serialization.amqp.SerializerFactory.Companion.isPrimitive +import net.corda.nodeapi.internal.serialization.amqp.custom.BigDecimalSerializer +import net.corda.nodeapi.internal.serialization.amqp.custom.CurrencySerializer import net.corda.testing.contracts.DummyContract import net.corda.testing.core.BOB_NAME import net.corda.testing.core.SerializationEnvironmentRule @@ -36,11 +38,13 @@ import org.junit.Test import java.io.ByteArrayInputStream import java.io.IOException import java.io.NotSerializableException +import java.lang.reflect.Type import java.math.BigDecimal import java.nio.ByteBuffer import java.time.* import java.time.temporal.ChronoUnit import java.util.* +import java.util.concurrent.ConcurrentHashMap import kotlin.reflect.full.superclasses import kotlin.test.assertEquals import kotlin.test.assertNotNull @@ -1080,4 +1084,38 @@ class SerializationOutputTests { serdes(obj, factory, factory2, expectedEqual = false, expectDeserializedEqual = false) }.isInstanceOf(MissingAttachmentsException::class.java) } + + // + // Example stacktrace that this test is tryint to reproduce + // + // java.lang.IllegalArgumentException: + // net.corda.core.contracts.TransactionState -> + // data(net.corda.core.contracts.ContractState) -> + // net.corda.finance.contracts.asset.Cash$State -> + // amount(net.corda.core.contracts.Amount>) -> + // net.corda.core.contracts.Amount> -> + // displayTokenSize(java.math.BigDecimal) -> + // wrong number of arguments + // + // So the actual problem was objects with multiple getters. The code wasn't looking for one with zero + // properties, just taking the first one it found with with the most applicable type, and the reflection + // ordering of the methods was random, thus occasionally we select the wrong one + // + @Test + fun reproduceWrongNumberOfArguments() { + val field = SerializerFactory::class.java.getDeclaredField("serializersByType").apply { + this.isAccessible = true + } + + data class C(val a: Amount) + + val factory = testDefaultFactoryNoEvolution() + factory.register(net.corda.nodeapi.internal.serialization.amqp.custom.BigDecimalSerializer) + factory.register(net.corda.nodeapi.internal.serialization.amqp.custom.CurrencySerializer) + + val c = C(Amount(100, BigDecimal("1.5"), Currency.getInstance("USD"))) + + // were the issue not fixed we'd blow up here + SerializationOutput(factory).serialize(c) + } } \ No newline at end of file