Prevent non-null values from being discarded

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/EvolutionSerializerFactory.kt

@@ -103,15 +103,8 @@ class DefaultEvolutionSerializerFactory(
     private fun RemoteTypeInformation.Composable.validateEvolvability(localProperties: Map<PropertyName, LocalPropertyInformation>) {
         val remotePropertyNames = properties.keys
         val localPropertyNames = localProperties.keys
-        val deletedProperties = remotePropertyNames - localPropertyNames
         val newProperties = localPropertyNames - remotePropertyNames
 
-        // Here is where we can exercise a veto on evolutions that remove properties.
-        if (deletedProperties.isNotEmpty() && mustPreserveDataWhenEvolving)
-            throw EvolutionSerializationException(this,
-                    "Property ${deletedProperties.first()} of remote ContractState type is not present in local type, " +
-                            "and context is configured to prevent forwards-compatible deserialization.")
-
         // Check mandatory-ness of constructor-set properties.
         newProperties.forEach { propertyName ->
             if (localProperties[propertyName]!!.mustBeProvided) throw EvolutionSerializationException(
@@ -170,5 +163,6 @@ class DefaultEvolutionSerializerFactory(
                     this,
                     constructor,
                     properties,
-                    classLoader)
+                    classLoader,
+                    mustPreserveDataWhenEvolving)
 }

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectBuilder.kt

@@ -180,23 +180,38 @@ private class ConstructorBasedObjectBuilder(
  * An [ObjectBuilder] that wraps an underlying [ObjectBuilder], routing the property values assigned to its slots to the
  * matching slots in the underlying builder, and discarding values for which the underlying builder has no slot.
  */
-class EvolutionObjectBuilder(private val localBuilder: ObjectBuilder, private val slotAssignments: IntArray): ObjectBuilder {
+class EvolutionObjectBuilder(private val localBuilder: ObjectBuilder,
+                             private val slotAssignments: IntArray,
+                             private val remoteProperties: List<String>,
+                             private val mustPreserveData: Boolean): ObjectBuilder {
 
     companion object {
+
+        const val DISCARDED : Int = -1
+
         /**
          * Construct an [EvolutionObjectBuilder] for the specified type, constructor and properties, mapping the list of
          * properties defined in the remote type into the matching slots on the local type's [ObjectBuilder], and discarding
          * any for which there is no matching slot.
          */
-        fun makeProvider(typeIdentifier: TypeIdentifier, constructor: LocalConstructorInformation, localProperties: Map<String, LocalPropertyInformation>, remoteProperties: List<String>): () -> ObjectBuilder {
+        fun makeProvider(typeIdentifier: TypeIdentifier,
+                         constructor: LocalConstructorInformation,
+                         localProperties: Map<String, LocalPropertyInformation>,
+                         remoteTypeInformation: RemoteTypeInformation.Composable,
+                         mustPreserveData: Boolean): () -> ObjectBuilder {
             val localBuilderProvider = ObjectBuilder.makeProvider(typeIdentifier, constructor, localProperties)
 
-            val reroutedIndices = remoteProperties.map { propertyName ->
-                localBuilderProvider.propertySlots[propertyName] ?: -1
+            val remotePropertyNames = remoteTypeInformation.properties.keys.sorted()
+            val reroutedIndices = remotePropertyNames.map { propertyName ->
+                localBuilderProvider.propertySlots[propertyName] ?: DISCARDED
             }.toIntArray()
 
             return {
-                EvolutionObjectBuilder(localBuilderProvider(), reroutedIndices)
+                EvolutionObjectBuilder(
+                        localBuilderProvider(),
+                        reroutedIndices,
+                        remotePropertyNames,
+                        mustPreserveData)
             }
         }
     }
@@ -207,7 +222,16 @@ class EvolutionObjectBuilder(private val localBuilder: ObjectBuilder, private va
 
     override fun populate(slot: Int, value: Any?) {
         val slotAssignment = slotAssignments[slot]
-        if (slotAssignment != -1) localBuilder.populate(slotAssignment, value)
+        if (slotAssignment == DISCARDED) {
+            if (mustPreserveData && value != null) {
+                throw NotSerializableException(
+                        "Non-null value $value provided for property ${remoteProperties[slot]}, " +
+                        "which is not supported in this version"
+                )
+            }
+        } else {
+            localBuilder.populate(slotAssignment, value)
+        }
     }
 
     override fun build(): Any = localBuilder.build()

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/ObjectSerializer.kt

@@ -167,13 +167,21 @@ class EvolutionObjectSerializer(
         private val reader: ComposableObjectReader): ObjectSerializer {
 
     companion object {
-        fun make(localTypeInformation: LocalTypeInformation.Composable, remoteTypeInformation: RemoteTypeInformation.Composable, constructor: LocalConstructorInformation,
-                 properties: Map<String, LocalPropertyInformation>, classLoader: ClassLoader): EvolutionObjectSerializer {
+        fun make(localTypeInformation: LocalTypeInformation.Composable,
+                 remoteTypeInformation: RemoteTypeInformation.Composable, constructor: LocalConstructorInformation,
+                 properties: Map<String, LocalPropertyInformation>,
+                 classLoader: ClassLoader,
+                 mustPreserveData: Boolean): EvolutionObjectSerializer {
             val propertySerializers = makePropertySerializers(properties, remoteTypeInformation.properties, classLoader)
             val reader = ComposableObjectReader(
                     localTypeInformation.typeIdentifier,
                     propertySerializers,
-                    EvolutionObjectBuilder.makeProvider(localTypeInformation.typeIdentifier, constructor, properties, remoteTypeInformation.properties.keys.sorted()))
+                    EvolutionObjectBuilder.makeProvider(
+                            localTypeInformation.typeIdentifier,
+                            constructor,
+                            properties,
+                            remoteTypeInformation,
+                            mustPreserveData))
 
             return EvolutionObjectSerializer(
                     localTypeInformation.observedType,

serialization/src/test/kotlin/net/corda/serialization/internal/amqp/EvolutionSerializerFactoryTests.kt

@@ -1,54 +1,64 @@
 package net.corda.serialization.internal.amqp
 
-import net.corda.serialization.internal.amqp.testutils.deserializeAndReturnEnvelope
-import net.corda.serialization.internal.amqp.testutils.serialize
-import net.corda.serialization.internal.amqp.testutils.testDefaultFactory
+import com.natpryce.hamkrest.should.shouldMatch
+import net.corda.core.serialization.SerializedBytes
+import net.corda.core.serialization.serialize
+import net.corda.serialization.internal.AllWhitelist
+import net.corda.serialization.internal.amqp.GenericsTests.Companion.localPath
+import net.corda.serialization.internal.amqp.testutils.*
+import net.corda.serialization.internal.carpenter.ClassCarpenterImpl
 import net.corda.serialization.internal.model.RemoteTypeInformation
 import net.corda.serialization.internal.model.TypeIdentifier
 import org.junit.Test
-import kotlin.test.assertFailsWith
-import kotlin.test.assertNotNull
-import kotlin.test.assertNull
+import java.io.File
+import java.io.NotSerializableException
+import java.net.URI
+import kotlin.test.*
 
 class EvolutionSerializerFactoryTests {
 
-    private val factory = testDefaultFactory()
+    private val factory = SerializerFactoryBuilder.build(
+            AllWhitelist,
+            ClassCarpenterImpl(AllWhitelist, ClassLoader.getSystemClassLoader()),
+            descriptorBasedSerializerRegistry = DefaultDescriptorBasedSerializerRegistry(),
+            mustPreserveDataWhenEvolving = true)
+
+    // Version of the class as it was serialised
+    //
+    // data class C(val a: Int, val b: Int?)
+    //
+    // Version of the class as it's used in the test
+    data class C(val a: Int)
 
     @Test
     fun preservesDataWhenFlagSet() {
-        val nonStrictEvolutionSerializerFactory = DefaultEvolutionSerializerFactory(
-                factory,
-                ClassLoader.getSystemClassLoader(),
-                false)
+        val resource = "${javaClass.simpleName}.${testName()}"
 
-        val strictEvolutionSerializerFactory = DefaultEvolutionSerializerFactory(
-                factory,
-                ClassLoader.getSystemClassLoader(),
-                true)
+        val withNullResource = "${resource}_with_null"
+        val withoutNullResource = "${resource}_without_null"
 
-        @Suppress("unused")
-        class C(val importantFieldA: Int)
-        val (_, env) = DeserializationInput(factory).deserializeAndReturnEnvelope(
-                SerializationOutput(factory).serialize(C(1)))
+        // Uncomment to re-generate test files
+        // val withNullOriginal = C(1, null)
+        // val withoutNullOriginal = C(1, 1)
+        // File(URI("$localPath/$withNullResource")).writeBytes(
+        //         SerializationOutput(factory).serialize(withNullOriginal).bytes)
+        // File(URI("$localPath/$withoutNullResource")).writeBytes(
+        //         SerializationOutput(factory).serialize(withoutNullOriginal).bytes)
 
-        val remoteTypeInformation = AMQPRemoteTypeModel().interpret(SerializationSchemas(env.schema, env.transformsSchema))
-                .values.find { it.typeIdentifier == TypeIdentifier.forClass(C::class.java) }
-                as RemoteTypeInformation.Composable
+        val withoutNullUrl = javaClass.getResource(withoutNullResource)
+        val withNullUrl = javaClass.getResource(withNullResource)
 
-        val withAddedField = remoteTypeInformation.copy(properties = remoteTypeInformation.properties.plus(
-            "importantFieldB" to remoteTypeInformation.properties["importantFieldA"]!!))
+        // We can deserialize the evolved instance where the original value of 'b' is null.
+        val withNullTarget = DeserializationInput(factory).deserialize(SerializedBytes<C>(withNullUrl.readBytes()))
+        assertEquals(1, withNullTarget.a)
 
-        val localTypeInformation = factory.getTypeInformation(C::class.java)
-
-        // No evolution required with original fields.
-        assertNull(strictEvolutionSerializerFactory.getEvolutionSerializer(remoteTypeInformation, localTypeInformation))
-
-        // Returns an evolution serializer if the fields have changed.
-        assertNotNull(nonStrictEvolutionSerializerFactory.getEvolutionSerializer(withAddedField, localTypeInformation))
-
-        // Fails in strict mode if the remote type information includes a field not included in the local type.
-        assertFailsWith<EvolutionSerializationException> {
-            strictEvolutionSerializerFactory.getEvolutionSerializer(withAddedField, localTypeInformation)
+        // We cannot deserialize the evolved instance where the original value of 'b' is non-null.
+        try {
+            DeserializationInput(factory).deserialize(SerializedBytes<C>(withoutNullUrl.readBytes()))
+            fail("Expected deserialisation of object with non-null value for 'b' to fail")
+        } catch (e: NotSerializableException) {
+            assertTrue(e.message!!.contains(
+                    "Non-null value 1 provided for property b, which is not supported in this version"))
         }
     }