Skip to content

Commit

Permalink
CORDA-978 - Getter selection fix
Browse files Browse the repository at this point in the history 
Backport from master - only select getters that take no parameters
  • Loading branch information
Katelyn Baker committed Feb 5, 2018
1 parent e042964 commit 7ca83eb
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 18 deletions.
35 changes: 22 additions & 13 deletions ...-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,36 +127,45 @@ fun Class<out Any?>.propertyDescriptors(): Map<String, PropertyDescriptor> {
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<out T> { val a: T }
// class D(override val a: String) : I<String>
// instances of D will have both
// 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
}
}
}
}
Expand Down
9 changes: 4 additions & 5 deletions ...api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/PrivatePropertyTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
38 changes: 38 additions & 0 deletions ...src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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.Issued<java.util.Currency>>) ->
// net.corda.core.contracts.Amount<net.corda.core.contracts.Issued<java.util.Currency>> ->
// 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<Currency>)

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<Currency>(100, BigDecimal("1.5"), Currency.getInstance("USD")))

// were the issue not fixed we'd blow up here
SerializationOutput(factory).serialize(c)
}
}

0 comments on commit 7ca83eb

Please sign in to comment.