CORDA-1498: serialization multiple transform bug (#3216)

* Fix issue when evolving enums with transformation chains

* Regenerate test data for deserializeWithRename test and unignore

* Further tweaks / remove debugging

* Formatting tweaks

* Address review comments

* Remove debug

* Add classname to serialization tranform exceptions

* Use direct node links instead of indexes to improve readability

* More readability tweaks

* More readability improvements

* rename require to requireThat to resolve conflict with kotlin libraries

* Add logging of error message

* Change requireThat helper to inline function

* remove unneeded toString

* Further tweaks

* Change NotSerializableException to more generic IOException

* Make exception context clearer

serialization/src/main/kotlin/net/corda/serialization/internal/NotSerializableExceptions.kt

@@ -0,0 +1,14 @@
+package net.corda.serialization.internal
+
+import java.io.IOException
+import java.io.NotSerializableException
+
+class NotSerializableDetailedException(classname: String?, val reason: String) : NotSerializableException(classname) {
+    override fun toString(): String {
+        return "Unable to serialize/deserialize $message: $reason"
+    }
+}
+
+// This exception is thrown when serialization isn't possible but at the point the exception
+// is thrown the classname isn't known. It's caught and rethrown as a [NotSerializableDetailedException]
+class NotSerializableWithReasonException(message: String?): IOException(message)

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

@@ -5,9 +5,9 @@ import net.corda.core.internal.uncheckedCast
 import net.corda.core.serialization.CordaSerializationTransformEnumDefault
 import net.corda.core.serialization.CordaSerializationTransformEnumDefaults
 import net.corda.core.serialization.CordaSerializationTransformRename
+import net.corda.serialization.internal.NotSerializableWithReasonException
 import org.apache.qpid.proton.amqp.DescribedType
 import org.apache.qpid.proton.codec.DescribedTypeConstructor
-import java.io.NotSerializableException
 
 /**
  * Enumerated type that represents each transform that can be applied to a class. Used as the key type in
@@ -47,25 +47,12 @@ enum class TransformTypes(val build: (Annotation) -> Transform) : DescribedType
          */
         override fun validate(list: List<Transform>, constants: Map<String, Int>) {
             uncheckedCast<List<Transform>, List<EnumDefaultSchemaTransform>>(list).forEach {
-                if (!constants.contains(it.new)) {
-                    throw NotSerializableException("Unknown enum constant ${it.new}")
-                }
-
-                if (!constants.contains(it.old)) {
-                    throw NotSerializableException(
-                            "Enum extension defaults must be to a valid constant: ${it.new} -> ${it.old}. ${it.old} " +
-                                    "doesn't exist in constant set $constants")
-                }
-
-                if (it.old == it.new) {
-                    throw NotSerializableException("Enum extension ${it.new} cannot default to itself")
-                }
-
-                if (constants[it.old]!! >= constants[it.new]!!) {
-                    throw NotSerializableException(
-                            "Enum extensions must default to older constants. ${it.new}[${constants[it.new]}] " +
-                                    "defaults to ${it.old}[${constants[it.old]}] which is greater")
-                }
+                requireThat(constants.contains(it.new)) {"Unknown enum constant ${it.new}"}
+                requireThat(constants.contains(it.old)) { "Enum extension defaults must be to a valid constant: ${it.new} -> ${it.old}. ${it.old} " +
+                        "doesn't exist in constant set $constants" }
+                requireThat(it.old != it.new) { "Enum extension ${it.new} cannot default to itself" }
+                requireThat(constants[it.old]!! < constants[it.new]!!) { "Enum extensions must default to older constants. ${it.new}[${constants[it.new]}] " +
+                        "defaults to ${it.old}[${constants[it.old]}] which is greater" }
             }
         }
     },
@@ -83,21 +70,56 @@ enum class TransformTypes(val build: (Annotation) -> Transform) : DescribedType
          * @param constants The list of enum constants on the type the transforms are being applied to
          */
         override fun validate(list: List<Transform>, constants: Map<String, Int>) {
-            object : Any() {
-                val from: MutableSet<String> = mutableSetOf()
-                val to: MutableSet<String> = mutableSetOf()
-            }.apply {
-                @Suppress("UNCHECKED_CAST") (list as List<RenameSchemaTransform>).forEach { rename ->
-                    if (rename.to in this.to || rename.from in this.from) {
-                        throw NotSerializableException("Cyclic renames are not allowed (${rename.to})")
-                    }
+            data class Node(val transform: RenameSchemaTransform, var next: Node?, var prev: Node?, var visitedBy: Node? = null) {
+                fun visit(visitedBy: Node) {
+                    this.visitedBy = visitedBy
+                }
+                val visited get() = visitedBy != null
+            }
 
-                    this.to.add(rename.from)
-                    this.from.add(rename.to)
+            val graph = mutableListOf<Node>()
+            // Keep two maps of forward links and back links in order to build the graph in one pass
+            val forwardLinks = hashMapOf<String, Node>()
+            val reverseLinks = hashMapOf<String, Node>()
+
+            // build a dependency graph
+            val transforms: List<RenameSchemaTransform> = uncheckedCast(list)
+            transforms.forEach { rename ->
+                requireThat(!forwardLinks.contains(rename.from)) { "There are multiple transformations from ${rename.from}, which is not allowed" }
+                requireThat(!reverseLinks.contains(rename.to)) { "There are multiple transformations to ${rename.to}, which is not allowed" }
+                val node = Node(rename, forwardLinks[rename.to], reverseLinks[rename.from])
+                graph.add(node)
+                node.next?.prev = node
+                node.prev?.next = node
+                forwardLinks[rename.from] = node
+                reverseLinks[rename.to] = node
+            }
+
+            // Check that every property in the current type is at the end of a renaming chain, if it is in one
+            constants.keys.forEach {
+                requireThat(reverseLinks[it]?.next == null) { "$it is specified as a previously evolved type, but it also exists in the current type" }
+            }
+
+            // Check for cyclic dependencies
+            graph.forEach {
+                if (it.visited) return@forEach
+                // Find an unvisited node
+                var currentNode = it
+                currentNode.visit(it)
+                while (currentNode.next != null) {
+                    currentNode = currentNode.next!!
+                    if (currentNode.visited) {
+                        requireThat(currentNode.visitedBy != it) { "Cyclic renames are not allowed (${currentNode.transform.from})" }
+                        // we have found the start of another non-cyclic chain of dependencies
+                        // if they were cyclic we would have gone round in a loop and already thrown
+                        break
+                    }
+                    currentNode.visit(it)
                 }
             }
         }
     }
+
     // Transform used to test the unknown handler, leave this at as the final constant, uncomment
     // when regenerating test cases - if Java had a pre-processor this would be much neater
     //
@@ -121,9 +143,7 @@ enum class TransformTypes(val build: (Annotation) -> Transform) : DescribedType
         override fun newInstance(obj: Any?): TransformTypes {
             val describedType = obj as DescribedType
 
-            if (describedType.descriptor != DESCRIPTOR) {
-                throw NotSerializableException("Unexpected descriptor ${describedType.descriptor}.")
-            }
+            requireThat(describedType.descriptor == DESCRIPTOR) { "Unexpected descriptor ${describedType.descriptor}." }
 
             return try {
                 values()[describedType.described as Int]
@@ -133,5 +153,11 @@ enum class TransformTypes(val build: (Annotation) -> Transform) : DescribedType
         }
 
         override fun getTypeClass(): Class<*> = TransformTypes::class.java
+
+        protected inline fun requireThat(expr: Boolean, errorMessage: () -> String) {
+            if (!expr) {
+                throw NotSerializableWithReasonException(errorMessage())
+            }
+        }
     }
 }

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

@@ -3,6 +3,10 @@ package net.corda.serialization.internal.amqp
 import net.corda.core.KeepForDJVM
 import net.corda.core.serialization.CordaSerializationTransformEnumDefault
 import net.corda.core.serialization.CordaSerializationTransformRename
+import net.corda.core.utilities.contextLogger
+import net.corda.core.utilities.trace
+import net.corda.serialization.internal.NotSerializableDetailedException
+import net.corda.serialization.internal.NotSerializableWithReasonException
 import org.apache.qpid.proton.amqp.DescribedType
 import org.apache.qpid.proton.codec.DescribedTypeConstructor
 import java.io.NotSerializableException
@@ -192,6 +196,7 @@ class RenameSchemaTransform(val from: String, val to: String) : Transform() {
 data class TransformsSchema(val types: Map<String, EnumMap<TransformTypes, MutableList<Transform>>>) : DescribedType {
     companion object : DescribedTypeConstructor<TransformsSchema> {
         val DESCRIPTOR = AMQPDescriptorRegistry.TRANSFORM_SCHEMA.amqpDescriptor
+        private val logger = contextLogger()
 
         /**
          * Takes a class name and either returns a cached instance of the TransformSet for it or, on a cache miss,
@@ -239,10 +244,17 @@ data class TransformsSchema(val types: Map<String, EnumMap<TransformTypes, Mutab
                 type: String,
                 sf: SerializerFactory,
                 map: MutableMap<String, EnumMap<TransformTypes, MutableList<Transform>>>) {
-            get(type, sf).apply {
-                if (isNotEmpty()) {
-                    map[type] = this
+            try {
+                get(type, sf).apply {
+                    if (isNotEmpty()) {
+                        map[type] = this
+                    }
                 }
+            } catch (e: NotSerializableWithReasonException) {
+                val message = "Error running transforms for $type: ${e.message}"
+                logger.error(message)
+                logger.trace { e.toString() }
+                throw NotSerializableDetailedException(type, e.message ?: "")
             }
         }
 

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

@@ -1,9 +1,12 @@
 package net.corda.serialization.internal.amqp
 
 import net.corda.core.serialization.*
+import net.corda.serialization.internal.NotSerializableDetailedException
 import net.corda.serialization.internal.amqp.testutils.*
 import net.corda.testing.common.internal.ProjectStructure.projectRootDir
+import org.assertj.core.api.Assertions.assertThat
 import org.assertj.core.api.Assertions.assertThatThrownBy
+import org.assertj.core.api.Condition
 import org.junit.Test
 import java.io.NotSerializableException
 import java.net.URI
@@ -442,6 +445,68 @@ class EnumEvolvabilityTests {
         assertTrue(envelope.transformsSchema.types[WithUnknownTest::class.java.name]!!.containsKey(TransformTypes.Unknown))
     }
 
+    //
+    // In this test we check that multiple transforms of a property are accepted
+    //
+    @CordaSerializationTransformRenames(
+            CordaSerializationTransformRename(from = "A", to = "B"),
+            CordaSerializationTransformRename(from = "B", to = "C")
+    )
+    enum class AcceptMultipleRename { C }
+
+    @Test
+    fun acceptMultipleRename() {
+        data class C(val e: AcceptMultipleRename)
+
+        val sf = testDefaultFactory()
+        SerializationOutput(sf).serialize(C(AcceptMultipleRename.C))
+    }
+
+    //
+    // In this example we will try to rename two different things to the same thing,
+    // which is not allowed
+    //
+    @CordaSerializationTransformRenames(
+            CordaSerializationTransformRename(from = "D", to = "C"),
+            CordaSerializationTransformRename(from = "E", to = "C")
+    )
+    enum class RejectMultipleRenameTo { A, B, C }
+
+    @Test
+    fun rejectMultipleRenameTo() {
+        data class C(val e: RejectMultipleRenameTo)
+
+        val sf = testDefaultFactory()
+        assertThatThrownBy {
+            SerializationOutput(sf).serialize(C(RejectMultipleRenameTo.A))
+        }.isInstanceOfSatisfying(NotSerializableDetailedException::class.java) { ex ->
+            assertThat(ex.reason).isEqualToIgnoringCase("There are multiple transformations to C, which is not allowed")
+            assertThat(ex.message).endsWith(RejectMultipleRenameTo::class.simpleName)
+        }
+    }
+
+    //
+    // In this example we will try to rename two different things from the same thing,
+    // which is not allowed
+    //
+    @CordaSerializationTransformRenames(
+            CordaSerializationTransformRename(from = "D", to = "C"),
+            CordaSerializationTransformRename(from = "D", to = "B")
+    )
+    enum class RejectMultipleRenameFrom { A, B, C }
+
+    @Test
+    fun rejectMultipleRenameFrom() {
+        data class C(val e: RejectMultipleRenameFrom)
+
+        val sf = testDefaultFactory()
+        assertThatThrownBy {
+            SerializationOutput(sf).serialize(C(RejectMultipleRenameFrom.A))
+        }.isInstanceOf(NotSerializableException::class.java)
+        .hasToString("Unable to serialize/deserialize net.corda.serialization.internal.amqp.EnumEvolvabilityTests\$RejectMultipleRenameFrom: " +
+                "There are multiple transformations from D, which is not allowed")
+    }
+
     //
     // In this example we will have attempted to rename D back to C
     //
@@ -534,5 +599,4 @@ class EnumEvolvabilityTests {
             SerializationOutput(sf).serialize(C(RejectBadDefaultToSelf.D))
         }.isInstanceOf(NotSerializableException::class.java)
     }
-
 }

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

@@ -150,7 +150,6 @@ class EnumEvolveTests {
     // Finally, the version we're using to test with
     enum class DeserializeWithRename { A, B, C }
 
-    @Ignore("https://r3-cev.atlassian.net/browse/CORDA-1498")
     @Test
     fun deserializeWithRename() {
         val resource = "${javaClass.simpleName}.${testName()}"