diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/NotSerializableExceptions.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/NotSerializableExceptions.kt new file mode 100644 index 0000000000..d05ae2b5c7 --- /dev/null +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/NotSerializableExceptions.kt @@ -0,0 +1,14 @@ +package net.corda.nodeapi.internal.serialization + +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) \ No newline at end of file diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformTypes.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformTypes.kt index 09c9be7746..43ec1d156a 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformTypes.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformTypes.kt @@ -4,9 +4,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.nodeapi.internal.serialization.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 @@ -45,25 +45,12 @@ enum class TransformTypes(val build: (Annotation) -> Transform) : DescribedType */ override fun validate(list: List, constants: Map) { uncheckedCast, List>(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" } } } }, @@ -81,21 +68,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, constants: Map) { - object : Any() { - val from: MutableSet = mutableSetOf() - val to: MutableSet = mutableSetOf() - }.apply { - @Suppress("UNCHECKED_CAST") (list as List).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() + // Keep two maps of forward links and back links in order to build the graph in one pass + val forwardLinks = hashMapOf() + val reverseLinks = hashMapOf() + + // build a dependency graph + val transforms: List = 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 // @@ -118,9 +140,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] @@ -130,5 +150,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()) + } + } } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformsSchema.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformsSchema.kt index 50c38be9f5..9be6c31e93 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformsSchema.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformsSchema.kt @@ -2,6 +2,10 @@ package net.corda.nodeapi.internal.serialization.amqp 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.nodeapi.internal.serialization.NotSerializableDetailedException +import net.corda.nodeapi.internal.serialization.NotSerializableWithReasonException import org.apache.qpid.proton.amqp.DescribedType import org.apache.qpid.proton.codec.DescribedTypeConstructor import java.io.NotSerializableException @@ -191,6 +195,7 @@ class RenameSchemaTransform(val from: String, val to: String) : Transform() { data class TransformsSchema(val types: Map>>) : DescribedType { companion object : DescribedTypeConstructor { 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, @@ -240,10 +245,17 @@ data class TransformsSchema(val types: Map>>) { - 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 ?: "") } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolvabilityTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolvabilityTests.kt index 2769c31798..0eac230736 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolvabilityTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolvabilityTests.kt @@ -1,8 +1,11 @@ package net.corda.nodeapi.internal.serialization.amqp import net.corda.core.serialization.* +import net.corda.nodeapi.internal.serialization.NotSerializableDetailedException import net.corda.testing.common.internal.ProjectStructure.projectRootDir import org.assertj.core.api.Assertions +import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.Test import java.io.File import java.io.NotSerializableException @@ -441,6 +444,69 @@ 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)) + }.isInstanceOfSatisfying(NotSerializableDetailedException::class.java) { ex -> + assertThat(ex.reason).isEqualToIgnoringCase("There are multiple transformations from D, which is not allowed") + assertThat(ex.message).endsWith(RejectMultipleRenameFrom::class.simpleName) + } + } + // // In this example we will have attempted to rename D back to C // @@ -533,5 +599,4 @@ class EnumEvolvabilityTests { SerializationOutput(sf).serialize(C(RejectBadDefaultToSelf.D)) }.isInstanceOf(NotSerializableException::class.java) } - }