From 61b8bb4c6baecaf441c25bd7acececc339da7b4a Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Tue, 26 Jun 2018 09:31:35 +0100 Subject: [PATCH] CORDA-1662 - Corda Serialization Evolution breaksdown with Java classes (#3427) Nullability logic was relying on annotations that Kotlin applies by default but is left to the developer in Javaland. Change this around so it works for both. In Kotlin, the property must be nullable, in Java, it can't be a primitive. --- .../serialization/amqp/EvolutionSerializer.kt | 34 ++++-- .../serialization/amqp/SerializationHelper.kt | 15 +++ .../serialization/amqp/SerializerFactory.kt | 11 +- .../serialization/carpenter/ClassCarpenter.kt | 6 +- ...ticInitialisationOfSerializedObjectTest.kt | 8 +- .../internal/amqp/JavaEvolutionTests.java | 100 ++++++++++++++++++ .../JavaEvolutionTests.testN1AddsNullableInt | Bin 0 -> 259 bytes .../JavaEvolutionTests.testN2AddsPrimitive | Bin 0 -> 272 bytes 8 files changed, 159 insertions(+), 15 deletions(-) create mode 100644 serialization/src/test/java/net/corda/serialization/internal/amqp/JavaEvolutionTests.java create mode 100644 serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN1AddsNullableInt create mode 100644 serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN2AddsPrimitive diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt index 0aaca3850c..c852468c1c 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt @@ -10,6 +10,7 @@ import java.io.NotSerializableException import kotlin.reflect.KFunction import kotlin.reflect.full.findAnnotation import kotlin.reflect.jvm.javaType +import kotlin.reflect.jvm.jvmErasure /** @@ -109,12 +110,29 @@ abstract class EvolutionSerializer( readersAsSerialized: Map): AMQPSerializer { val constructorArgs = arrayOfNulls(constructor.parameters.size) + // Java doesn't care about nullability unless it's a primitive in which + // case it can't be referenced. Unfortunately whilst Kotlin does apply + // Nullability annotations we cannot use them here as they aren't + // retained at runtime so we cannot rely on the absence of + // any particular NonNullable annotation type to indicate cross + // compiler nullability + val isKotlin = (new.type.javaClass.declaredAnnotations.any { + it.annotationClass.qualifiedName == "kotlin.Metadata" + }) + constructor.parameters.withIndex().forEach { - readersAsSerialized.get(it.value.name!!)?.apply { - this.resultsIndex = it.index - } ?: if (!it.value.type.isMarkedNullable) { - throw NotSerializableException( - "New parameter ${it.value.name} is mandatory, should be nullable for evolution to work") + if ((readersAsSerialized[it.value.name!!] ?.apply { this.resultsIndex = it.index }) == null) { + // If there is no value in the byte stream to map to the parameter of the constructor + // this is ok IFF it's a Kotlin class and the parameter is non nullable OR + // its a Java class and the parameter is anything but an unboxed primitive. + // Otherwise we throw the error and leave + if ((isKotlin && !it.value.type.isMarkedNullable) + || (!isKotlin && isJavaPrimitive(it.value.type.jvmErasure.java)) + ) { + throw NotSerializableException( + "New parameter \"${it.value.name}\" is mandatory, should be nullable for evolution " + + "to work, isKotlinClass=$isKotlin type=${it.value.type}") + } } } return EvolutionSerializerViaConstructor (new.type, factory, readersAsSerialized, constructor, constructorArgs) @@ -143,8 +161,10 @@ abstract class EvolutionSerializer( * @param factory the [SerializerFactory] associated with the serialization * context this serializer is being built for */ - fun make(old: CompositeType, new: ObjectSerializer, - factory: SerializerFactory): AMQPSerializer { + fun make(old: CompositeType, + new: ObjectSerializer, + factory: SerializerFactory + ): AMQPSerializer { // The order in which the properties were serialised is important and must be preserved val readersAsSerialized = LinkedHashMap() old.fields.forEach { 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 14061faa0b..546673a6bb 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 @@ -570,3 +570,18 @@ fun Class<*>.objectInstance() = } } } + +fun isJavaPrimitive(type: Class<*>) = type in JavaPrimitiveTypes.primativeTypes + +private object JavaPrimitiveTypes { + val primativeTypes = hashSetOf>( + Boolean::class.java, + Char::class.java, + Byte::class.java, + Short::class.java, + Int::class.java, + Long::class.java, + Float::class.java, + Double::class.java, + Void::class.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 699af1fb1f..a32e2dd677 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 @@ -40,14 +40,19 @@ data class FactorySchemaAndDescriptor(val schemas: SerializationSchemas, val typ // TODO: need to support super classes as well as interfaces with our current code base... what's involved? If we continue to ban, what is the impact? @ThreadSafe open class SerializerFactory( - val whitelist: ClassWhitelist, - cl: ClassLoader, - private val evolutionSerializerGetter: EvolutionSerializerGetterBase = EvolutionSerializerGetter()) { + val whitelist: ClassWhitelist, + cl: ClassLoader, + private val evolutionSerializerGetter: EvolutionSerializerGetterBase = EvolutionSerializerGetter() +) { private val serializersByType = ConcurrentHashMap>() private val serializersByDescriptor = ConcurrentHashMap>() private val customSerializers = CopyOnWriteArrayList() private val transformsCache = ConcurrentHashMap>>() + init { + fingerPrinter.setOwner(this) + } + open val classCarpenter = ClassCarpenter(cl, whitelist) val classloader: ClassLoader diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt index 2c98ebbc07..89277e956d 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt @@ -79,8 +79,10 @@ private val jlClass get() = Type.getInternalName(Class::class.java) * * Equals/hashCode methods are not yet supported. */ -class ClassCarpenter(cl: ClassLoader = Thread.currentThread().contextClassLoader, - val whitelist: ClassWhitelist) { +class ClassCarpenter( + cl: ClassLoader = Thread.currentThread().contextClassLoader, + val whitelist: ClassWhitelist +) { // TODO: Generics. // TODO: Sandbox the generated code when a security manager is in use. // TODO: Generate equals/hashCode. diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/StaticInitialisationOfSerializedObjectTest.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/StaticInitialisationOfSerializedObjectTest.kt index 1a85fc784b..71925aa1da 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/StaticInitialisationOfSerializedObjectTest.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/StaticInitialisationOfSerializedObjectTest.kt @@ -99,8 +99,10 @@ class StaticInitialisationOfSerializedObjectTest { // Version of a serializer factory that will allow the class carpenter living on the // factory to have a different whitelist applied to it than the factory - class TestSerializerFactory(wl1: ClassWhitelist, wl2: ClassWhitelist) : - SerializerFactory(wl1, ClassLoader.getSystemClassLoader()) { + class TestSerializerFactory( + wl1: ClassWhitelist, + wl2: ClassWhitelist + ) : SerializerFactory(wl1, ClassLoader.getSystemClassLoader()) { override val classCarpenter = ClassCarpenter(ClassLoader.getSystemClassLoader(), wl2) } @@ -141,4 +143,4 @@ class StaticInitialisationOfSerializedObjectTest { DeserializationInput(sf2).deserialize(SerializedBytes(bytes)) }.isInstanceOf(NotSerializableException::class.java) } -} \ No newline at end of file +} diff --git a/serialization/src/test/java/net/corda/serialization/internal/amqp/JavaEvolutionTests.java b/serialization/src/test/java/net/corda/serialization/internal/amqp/JavaEvolutionTests.java new file mode 100644 index 0000000000..fa8449840e --- /dev/null +++ b/serialization/src/test/java/net/corda/serialization/internal/amqp/JavaEvolutionTests.java @@ -0,0 +1,100 @@ +package net.corda.serialization.internal.amqp; + +import kotlin.Suppress; +import net.corda.core.serialization.SerializedBytes; +import net.corda.serialization.internal.amqp.testutils.AMQPTestUtilsKt; +import net.corda.serialization.internal.amqp.testutils.TestSerializationContext; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import java.io.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +public class JavaEvolutionTests { + @Rule + public final ExpectedException exception = ExpectedException.none(); + + // Class as it was when it was serialized and written to disk. Uncomment + // if the test referencing the object needs regenerating. + /* + static class N1 { + private String word; + public N1(String word) { this.word = word; } + public String getWord() { return word; } + } + */ + // Class as it exists now with the newly added element + static class N1 { + private String word; + private Integer wibble; + + public N1(String word, Integer wibble) { + this.word = word; + this.wibble = wibble; + } + public String getWord() { return word; } + public Integer getWibble() { return wibble; } + } + + // Class as it was when it was serialized and written to disk. Uncomment + // if the test referencing the object needs regenerating. + /* + static class N2 { + private String word; + public N2(String word) { this.word = word; } + public String getWord() { return word; } + } + */ + + // Class as it exists now with the newly added element + @SuppressWarnings("unused") + static class N2 { + private String word; + private float wibble; + + public N2(String word, float wibble) { + this.word = word; + this.wibble = wibble; + } + public String getWord() { return word; } + public float getWibble() { return wibble; } + } + + SerializerFactory factory = AMQPTestUtilsKt.testDefaultFactory(); + + @Test + public void testN1AddsNullableInt() throws IOException { + // Uncomment to regenerate the base state of the test + /* + N1 n = new N1("potato"); + AMQPTestUtilsKt.writeTestResource(this, new SerializationOutput(factory).serialize( + n, TestSerializationContext.testSerializationContext)); + */ + + N1 n2 = new DeserializationInput(factory).deserialize( + new SerializedBytes<>(AMQPTestUtilsKt.readTestResource(this)), + N1.class, + TestSerializationContext.testSerializationContext); + assertEquals(n2.getWord(), "potato"); + assertNull(n2.getWibble()); + } + + @Test + public void testN2AddsPrimitive() throws IOException { + // Uncomment to regenerate the base state of the test + /* + N2 n = new N2("This is only a test"); + + AMQPTestUtilsKt.writeTestResource(this, new SerializationOutput(factory).serialize( + n, TestSerializationContext.testSerializationContext)); + */ + + exception.expect(NotSerializableException.class); + new DeserializationInput(factory).deserialize( + new SerializedBytes<>(AMQPTestUtilsKt.readTestResource(this)), + N2.class, + TestSerializationContext.testSerializationContext); + } +} diff --git a/serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN1AddsNullableInt b/serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN1AddsNullableInt new file mode 100644 index 0000000000000000000000000000000000000000..334de45fa2125fd3cd88fd994135a3dc7056df85 GIT binary patch literal 259 zcmYe!FG@*dWME)uIGO|`85kH3yk}-utdy5pqL&Pkv&u6_);4zY2=MpT&h~dHF^lvK zEb>UVwLQSexR9+Nza+6FAFi3{z*5Eoix}a2)&o;n7h1#3(ko6a%1q43tV%4&%+J%y z%qvMP%1g}AOUx}S(DOq4aZm{W literal 0 HcmV?d00001 diff --git a/serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN2AddsPrimitive b/serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN2AddsPrimitive new file mode 100644 index 0000000000000000000000000000000000000000..ffa5f54f8bbc6d748ec22cf7d83ed43d5214f632 GIT binary patch literal 272 zcmYe!FG@*dWME)uIGO|`85kH3{9$HTtdy5pqL&Pkv(h(pDKd%*4#|lscJWN{ji~TS zGck9xwLKulxKKDGBePfmi1PDtDisnHN>YnU;3hF0Sju={5hI+>dSEK+LTk9edc~Rf#2;`FVPoc_pbud5JlCiMfRZdR~cTiLPb&Ii(=E5TL2WDt<-|u5h!M52!H_ o<4A`CiUV!Z literal 0 HcmV?d00001