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 0000000000..334de45fa2 Binary files /dev/null and b/serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN1AddsNullableInt differ 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 0000000000..ffa5f54f8b Binary files /dev/null and b/serialization/src/test/resources/net/corda/serialization/internal/amqp/JavaEvolutionTests.testN2AddsPrimitive differ