mirror of
https://github.com/corda/corda.git
synced 2025-01-13 16:30:25 +00:00
CORDA-1498 - serialization multiple transform bug (#3391)
* 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 # Conflicts: # node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolvabilityTests.kt # serialization/src/test/resources/net/corda/serialization/internal/amqp/EnumEvolveTests.deserializeWithRename.1.C # serialization/src/test/resources/net/corda/serialization/internal/amqp/EnumEvolveTests.deserializeWithRename.2.C # serialization/src/test/resources/net/corda/serialization/internal/amqp/EnumEvolveTests.deserializeWithRename.3.C * Fix merge conflicts * Fix broken test * Revert changes to serialized classes
This commit is contained in:
Anthony Keenan
Katelyn Baker
parent
371031dd3b
commit
5be8c9a102
@ -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)
|
||||
@ -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<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" }
|
||||
}
|
||||
}
|
||||
},
|
||||
@ -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<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
|
||||
//
|
||||
@ -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())
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -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<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,
|
||||
@ -240,10 +245,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 ?: "")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -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)
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user