From f19bcea82f53d056963f56554910a07a27a318a5 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Wed, 11 Apr 2018 18:32:23 +0100 Subject: [PATCH 1/5] CORDA-1229 - Setter serialization fails with lists Looks like the super / sub type inference of setter param vs getter param is the wrong way around. Also, Setter Type should be the generic type, not just the type the property must be a supertype of the setter parameter the getter must be a supertype of the setter parameter --- docs/source/changelog.rst | 3 ++ .../serialization/amqp/SerializationHelper.kt | 7 ++-- .../amqp/SetterConstructorTests.java | 34 +++++++++++++++++++ .../amqp/SerializationOutputTests.kt | 1 - 4 files changed, 41 insertions(+), 4 deletions(-) diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 9e239532b1..6887eab922 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -7,6 +7,9 @@ release, see :doc:`upgrade-notes`. Unreleased ========== +* Fix CORDA-1229. Setter based serialization was broken with generic types when the property was stored + as the interface type, List for example. + * java.security.cert.CRLReason added to the default Whitelist. * java.security.cert.X509CRL serialization support added. 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 f9d9efad63..3b8b6dceff 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 @@ -305,7 +305,8 @@ fun propertiesForSerializationFromSetters( "takes too many arguments") } - val setterType = setter.parameterTypes[0]!! + //val setterType = setter.parameterTypes[0]!! + val setterType = setter.genericParameterTypes[0]!! if ((property.value.field != null) && (!(TypeToken.of(property.value.field?.genericType!!).isSupertypeOf(setterType)))) { @@ -315,10 +316,10 @@ fun propertiesForSerializationFromSetters( } // make sure the setter returns the same type (within inheritance bounds) the getter accepts - if (!(TypeToken.of (setterType).isSupertypeOf(getter.returnType))) { + if (!(TypeToken.of (getter.genericReturnType).isSupertypeOf(setterType))) { throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + "takes parameter of type $setterType yet the defined getter returns a value of type " + - "${getter.returnType}") + "${getter.returnType} [${getter.genericReturnType}]") } this += PropertyAccessorGetterSetter( idx++, diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java index a0ab7276dd..2c2b938ab9 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java @@ -7,6 +7,8 @@ import org.junit.Test; import static org.junit.Assert.*; import java.io.NotSerializableException; +import java.util.ArrayList; +import java.util.List; public class SetterConstructorTests { @@ -64,6 +66,13 @@ public class SetterConstructorTests { public void setC(int c) { this.c = c; } } + static class CIntList { + private List l; + + public List getL() { return l; } + public void setL(List l) { this.l = l; } + } + static class Inner1 { private String a; @@ -315,4 +324,29 @@ public class SetterConstructorTests { Assertions.assertThatThrownBy(() -> new SerializationOutput(factory1).serialize(tm)).isInstanceOf( NotSerializableException.class); } + + // This not blowing up means it's working + @Test + public void intList() throws NotSerializableException { + CIntList cil = new CIntList(); + + List l = new ArrayList<>(); + l.add(1); + l.add(2); + l.add(3); + + cil.setL(l); + + EvolutionSerializerGetterBase evolutionSerialiserGetter = new EvolutionSerializerGetter(); + FingerPrinter fingerPrinter = new SerializerFingerPrinter(); + SerializerFactory factory1 = new SerializerFactory( + AllWhitelist.INSTANCE, + ClassLoader.getSystemClassLoader(), + evolutionSerialiserGetter, + fingerPrinter); + + // if we've got super / sub types on the setter vs the unerlaying type the wrong way around this will + // explode + new SerializationOutput(factory1).serialize(cil); + } } 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 a41f5b33c0..5b10cde6a9 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 @@ -1310,6 +1310,5 @@ class SerializationOutputTests(private val compression: CordaSerializationEncodi C(12).serializeE() }.withMessageContaining("has synthetic fields and is likely a nested inner class") } - } From 84914aa5c8fbf3175ffafc8d0a68b3b91eaae6b7 Mon Sep 17 00:00:00 2001 From: Kat Baker Date: Thu, 12 Apr 2018 11:18:39 +0100 Subject: [PATCH 2/5] Remove commented out code --- .../nodeapi/internal/serialization/amqp/SerializationHelper.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 3b8b6dceff..d5220dde49 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 @@ -304,8 +304,7 @@ fun propertiesForSerializationFromSetters( throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + "takes too many arguments") } - - //val setterType = setter.parameterTypes[0]!! + val setterType = setter.genericParameterTypes[0]!! if ((property.value.field != null) && From c783c431b1d99c5f8a35e4e1957786518b55c917 Mon Sep 17 00:00:00 2001 From: Kat Baker Date: Thu, 12 Apr 2018 11:23:13 +0100 Subject: [PATCH 3/5] Fix broken comment --- .../internal/serialization/amqp/SerializationHelper.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 d5220dde49..5f92cb9ed2 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 @@ -304,7 +304,7 @@ fun propertiesForSerializationFromSetters( throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + "takes too many arguments") } - + val setterType = setter.genericParameterTypes[0]!! if ((property.value.field != null) && @@ -314,7 +314,7 @@ fun propertiesForSerializationFromSetters( "${property.value.field?.genericType!!}") } - // make sure the setter returns the same type (within inheritance bounds) the getter accepts + // make sure the getter returns the same type (within inheritance bounds) the setter accepts if (!(TypeToken.of (getter.genericReturnType).isSupertypeOf(setterType))) { throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + "takes parameter of type $setterType yet the defined getter returns a value of type " + From 03850dabc2956cfa7a704eb360e4c044923040c9 Mon Sep 17 00:00:00 2001 From: Kat Baker Date: Thu, 12 Apr 2018 11:58:56 +0100 Subject: [PATCH 4/5] Review comments --- docs/source/changelog.rst | 4 ++-- .../internal/serialization/amqp/SerializationHelper.kt | 2 +- .../internal/serialization/amqp/SetterConstructorTests.java | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 6887eab922..8d0ee6cc3e 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -7,8 +7,8 @@ release, see :doc:`upgrade-notes`. Unreleased ========== -* Fix CORDA-1229. Setter based serialization was broken with generic types when the property was stored - as the interface type, List for example. +* Fix CORDA-1229. Setter-based serialization was broken with generic types when the property was stored + as the raw type, List for example. * java.security.cert.CRLReason added to the default Whitelist. 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 5f92cb9ed2..119b1e0211 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 @@ -314,7 +314,7 @@ fun propertiesForSerializationFromSetters( "${property.value.field?.genericType!!}") } - // make sure the getter returns the same type (within inheritance bounds) the setter accepts + // Make sure the getter returns the same type (within inheritance bounds) the setter accepts. if (!(TypeToken.of (getter.genericReturnType).isSupertypeOf(setterType))) { throw NotSerializableException("Defined setter for parameter ${property.value.field?.name} " + "takes parameter of type $setterType yet the defined getter returns a value of type " + diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java index 2c2b938ab9..2e819dfbe7 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java @@ -345,8 +345,8 @@ public class SetterConstructorTests { evolutionSerialiserGetter, fingerPrinter); - // if we've got super / sub types on the setter vs the unerlaying type the wrong way around this will - // explode + // if we've got super / sub types on the setter vs the underlying type the wrong way around this will + // explode. See CORDA-1229 (https://r3-cev.atlassian.net/browse/CORDA-1229) new SerializationOutput(factory1).serialize(cil); } } From e6d352e44632317d0bc9b773167ffa3790338711 Mon Sep 17 00:00:00 2001 From: Kat Baker Date: Thu, 12 Apr 2018 12:09:17 +0100 Subject: [PATCH 5/5] Review comments --- .../internal/serialization/amqp/SetterConstructorTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java index 2e819dfbe7..e9ac48b063 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java @@ -347,6 +347,6 @@ public class SetterConstructorTests { // if we've got super / sub types on the setter vs the underlying type the wrong way around this will // explode. See CORDA-1229 (https://r3-cev.atlassian.net/browse/CORDA-1229) - new SerializationOutput(factory1).serialize(cil); + new DeserializationInput(factory1).deserialize(new SerializationOutput(factory1).serialize(cil), CIntList.class); } }