From ab76ef4debccd2395a371b3e38a615546423c16a Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Fri, 5 Jan 2018 15:05:10 +0000 Subject: [PATCH 01/10] REMOVING FLAKEY TESTS TILL THEYRE STABILISED --- .../kotlin/net/corda/node/services/AttachmentLoadingTests.kt | 3 +++ .../kotlin/net/corda/node/services/DistributedServiceTests.kt | 3 +++ 2 files changed, 6 insertions(+) diff --git a/node/src/integration-test/kotlin/net/corda/node/services/AttachmentLoadingTests.kt b/node/src/integration-test/kotlin/net/corda/node/services/AttachmentLoadingTests.kt index 7fdc7a6d06..3da63fedde 100644 --- a/node/src/integration-test/kotlin/net/corda/node/services/AttachmentLoadingTests.kt +++ b/node/src/integration-test/kotlin/net/corda/node/services/AttachmentLoadingTests.kt @@ -28,6 +28,7 @@ import net.corda.testing.internal.withoutTestSerialization import net.corda.testing.services.MockAttachmentStorage import net.corda.testing.internal.rigorousMock import org.junit.Assert.assertEquals +import org.junit.Ignore import org.junit.Rule import org.junit.Test import java.net.URLClassLoader @@ -99,6 +100,7 @@ class AttachmentLoadingTests { assertEquals(expected, actual) } + @Ignore("Test has undeterministic capacity to hang, ignore till fixed") @Test fun `test that attachments retrieved over the network are not used for code`() = withoutTestSerialization { driver { @@ -111,6 +113,7 @@ class AttachmentLoadingTests { Unit } + @Ignore("Test has undeterministic capacity to hang, ignore till fixed") @Test fun `tests that if the attachment is loaded on both sides already that a flow can run`() = withoutTestSerialization { driver { diff --git a/node/src/integration-test/kotlin/net/corda/node/services/DistributedServiceTests.kt b/node/src/integration-test/kotlin/net/corda/node/services/DistributedServiceTests.kt index 32a09d06a9..92723d52d8 100644 --- a/node/src/integration-test/kotlin/net/corda/node/services/DistributedServiceTests.kt +++ b/node/src/integration-test/kotlin/net/corda/node/services/DistributedServiceTests.kt @@ -20,6 +20,7 @@ import net.corda.testing.driver.driver import net.corda.testing.node.ClusterSpec import net.corda.testing.node.NotarySpec import org.assertj.core.api.Assertions.assertThat +import org.junit.Ignore import org.junit.Test import rx.Observable import java.util.* @@ -73,6 +74,7 @@ class DistributedServiceTests { // TODO Use a dummy distributed service rather than a Raft Notary Service as this test is only about Artemis' ability // to handle distributed services + @Ignore("Test has undeterministic capacity to hang, ignore till fixed") @Test fun `requests are distributed evenly amongst the nodes`() = setup { // Issue 100 pounds, then pay ourselves 50x2 pounds @@ -101,6 +103,7 @@ class DistributedServiceTests { } // TODO This should be in RaftNotaryServiceTests + @Ignore("Test has undeterministic capacity to hang, ignore till fixed") @Test fun `cluster survives if a notary is killed`() = setup { // Issue 100 pounds, then pay ourselves 10x5 pounds From f4ad8d3e70547c6ff6804355ec911bb09cbb309a Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Thu, 4 Jan 2018 15:36:42 +0000 Subject: [PATCH 02/10] CORDA-902 - AMQP Setter Construction when empty / no constructor --- .../amqp/CorDappCustomSerializer.kt | 2 +- .../serialization/amqp/CustomSerializer.kt | 2 +- .../serialization/amqp/EvolutionSerializer.kt | 2 +- .../serialization/amqp/ObjectSerializer.kt | 52 ++- .../internal/serialization/amqp/Schema.kt | 9 +- .../serialization/amqp/SerializationHelper.kt | 60 +++- .../serialization/amqp/SerializationOutput.kt | 16 + .../amqp/custom/ThrowableSerializer.kt | 2 +- .../amqp/SetterConstructorTests.java | 295 ++++++++++++++++++ .../serialization/amqp/AMQPTestUtils.kt | 17 - 10 files changed, 419 insertions(+), 38 deletions(-) create mode 100644 node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/SetterConstructorTests.java diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt index 72373488b3..8e5e249457 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CorDappCustomSerializer.kt @@ -70,7 +70,7 @@ class CorDappCustomSerializer( data.withDescribed(descriptor) { data.withList { - for (property in proxySerializer.propertySerializers) { + for (property in proxySerializer.propertySerializers.getters) { property.writeProperty(proxy, this, output) } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt index 06cef5b6cd..647da2fd05 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/CustomSerializer.kt @@ -132,7 +132,7 @@ abstract class CustomSerializer : AMQPSerializer, SerializerFor { override fun writeDescribedObject(obj: T, data: Data, type: Type, output: SerializationOutput) { val proxy = toProxy(obj) data.withList { - for (property in proxySerializer.propertySerializers) { + for (property in proxySerializer.propertySerializers.getters) { property.writeProperty(proxy, this, output) } } 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 797943cf93..663a153cbd 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 @@ -20,7 +20,7 @@ class EvolutionSerializer( override val kotlinConstructor: KFunction?) : ObjectSerializer(clazz, factory) { // explicitly set as empty to indicate it's unused by this type of serializer - override val propertySerializers: Collection = emptyList() + override val propertySerializers = ConstructorDestructorMethods (emptyList(), emptyList()) /** * Represents a parameter as would be passed to the constructor of the class as it was diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt index b405b99a98..bd171d9f00 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt @@ -21,7 +21,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS private val logger = contextLogger() } - open internal val propertySerializers: Collection by lazy { + open internal val propertySerializers: ConstructorDestructorMethods by lazy { propertiesForSerialization(kotlinConstructor, clazz, factory) } @@ -37,7 +37,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS for (iface in interfaces) { output.requireSerializer(iface) } - for (property in propertySerializers) { + for (property in propertySerializers.getters) { property.writeClassInfo(output) } } @@ -48,7 +48,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS data.withDescribed(typeNotation.descriptor) { // Write list withList { - for (property in propertySerializers) { + for (property in propertySerializers.getters) { property.writeProperty(obj, this, output) } } @@ -60,22 +60,58 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS schemas: SerializationSchemas, input: DeserializationInput): Any = ifThrowsAppend({ clazz.typeName }) { if (obj is List<*>) { - if (obj.size > propertySerializers.size) { + if (obj.size > propertySerializers.getters.size) { throw NotSerializableException("Too many properties in described type $typeName") } - val params = obj.zip(propertySerializers).map { it.second.readProperty(it.first, schemas, input) } - construct(params) + + return if (propertySerializers.setters.isEmpty()) { + readObjectBuildViaConstructor(obj, schemas, input) + } else { + readObjectBuildViaSetters(obj, schemas, input) + } } else throw NotSerializableException("Body of described type is unexpected $obj") } + private fun readObjectBuildViaConstructor( + obj: List<*>, + schemas: SerializationSchemas, + input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ + logger.debug { "Calling construction based construction" } + + return construct(obj.zip(propertySerializers.getters).map { it.second.readProperty(it.first, schemas, input) }) + } + + private fun readObjectBuildViaSetters( + obj: List<*>, + schemas: SerializationSchemas, + input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ + logger.debug { "Calling setter based construction" } + + val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException ( + "Failed to instantiate instance of object $clazz") + + // read the properties out of the serialised form + val propertiesFromBlob = obj + .zip(propertySerializers.getters) + .map { it.second.readProperty(it.first, schemas, input) } + + // one by one take a property and invoke the setter on the class + propertySerializers.setters.zip(propertiesFromBlob).forEach { + it.first?.invoke(instance, *listOf(it.second).toTypedArray()) + } + + return instance + } + private fun generateFields(): List { - return propertySerializers.map { Field(it.name, it.type, it.requires, it.default, null, it.mandatory, false) } + return propertySerializers.getters.map { + Field(it.name, it.type, it.requires, it.default, null, it.mandatory, false) + } } private fun generateProvides(): List = interfaces.map { nameForType(it) } fun construct(properties: List): Any { - logger.debug { "Calling constructor: '$javaConstructor' with properties '$properties'" } return javaConstructor?.newInstance(*properties.toTypedArray()) ?: diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt index bb38933ed0..79b2a57b93 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt @@ -419,9 +419,12 @@ private fun isCollectionOrMap(type: Class<*>) = (Collection::class.java.isAssign private fun fingerprintForObject(type: Type, contextType: Type?, alreadySeen: MutableSet, hasher: Hasher, factory: SerializerFactory): Hasher { // Hash the class + properties + interfaces val name = type.asClass()?.name ?: throw NotSerializableException("Expected only Class or ParameterizedType but found $type") - propertiesForSerialization(constructorForDeserialization(type), contextType ?: type, factory).fold(hasher.putUnencodedChars(name)) { orig, prop -> - fingerprintForType(prop.resolvedType, type, alreadySeen, orig, factory).putUnencodedChars(prop.name).putUnencodedChars(if (prop.mandatory) NOT_NULLABLE_HASH else NULLABLE_HASH) - } + propertiesForSerialization(constructorForDeserialization(type), contextType ?: type, factory).getters + .fold(hasher.putUnencodedChars(name)) { orig, prop -> + fingerprintForType(prop.resolvedType, type, alreadySeen, orig, factory) + .putUnencodedChars(prop.name) + .putUnencodedChars(if (prop.mandatory) NOT_NULLABLE_HASH else NULLABLE_HASH) + } interfacesForSerialization(type, factory).map { fingerprintForType(it, type, alreadySeen, hasher, factory) } return hasher } 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 b3f6d5ce53..a90bf7a706 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 @@ -2,12 +2,14 @@ package net.corda.nodeapi.internal.serialization.amqp import com.google.common.primitives.Primitives import com.google.common.reflect.TypeToken +import io.netty.util.internal.EmptyArrays import net.corda.core.serialization.ClassWhitelist import net.corda.core.serialization.CordaSerializable import net.corda.core.serialization.SerializationContext import org.apache.qpid.proton.codec.Data import java.beans.IndexedPropertyDescriptor import java.beans.Introspector +import java.beans.PropertyDescriptor import java.io.NotSerializableException import java.lang.reflect.* import java.util.* @@ -26,6 +28,10 @@ import kotlin.reflect.jvm.javaType @Retention(AnnotationRetention.RUNTIME) annotation class ConstructorForDeserialization +data class ConstructorDestructorMethods( + val getters : Collection, + val setters : Collection) + /** * Code for finding the constructor we will use for deserialization. * @@ -67,18 +73,30 @@ internal fun constructorForDeserialization(type: Type): KFunction? { * Note, you will need any Java classes to be compiled with the `-parameters` option to ensure constructor parameters have * names accessible via reflection. */ -internal fun propertiesForSerialization(kotlinConstructor: KFunction?, type: Type, factory: SerializerFactory): Collection { +internal fun propertiesForSerialization(kotlinConstructor: KFunction?, type: Type, factory: SerializerFactory): ConstructorDestructorMethods { val clazz = type.asClass()!! return if (kotlinConstructor != null) propertiesForSerializationFromConstructor(kotlinConstructor, type, factory) else propertiesForSerializationFromAbstract(clazz, type, factory) } fun isConcrete(clazz: Class<*>): Boolean = !(clazz.isInterface || Modifier.isAbstract(clazz.modifiers)) -private fun propertiesForSerializationFromConstructor(kotlinConstructor: KFunction, type: Type, factory: SerializerFactory): Collection { +private fun propertiesForSerializationFromConstructor( + kotlinConstructor: KFunction, + type: Type, + factory: SerializerFactory): ConstructorDestructorMethods { val clazz = (kotlinConstructor.returnType.classifier as KClass<*>).javaObjectType // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans. - val properties = Introspector.getBeanInfo(clazz).propertyDescriptors.filter { it.name != "class" }.groupBy { it.name }.mapValues { it.value[0] } + val properties = Introspector.getBeanInfo(clazz).propertyDescriptors + .filter { it.name != "class" } + .groupBy { it.name } + .mapValues { it.value[0] } + + if (properties.isNotEmpty() && kotlinConstructor.parameters.isEmpty()) { + return propertiesForSerializationFromGetters(properties, type, factory) + } + val rc: MutableList = ArrayList(kotlinConstructor.parameters.size) + for (param in kotlinConstructor.parameters) { val name = param.name ?: throw NotSerializableException("Constructor parameter of $clazz has no name.") @@ -103,7 +121,37 @@ private fun propertiesForSerializationFromConstructor(kotlinConstructo throw NotSerializableException("Property type $returnType for $name of $clazz differs from constructor parameter type ${param.type.javaType}") } } - return rc + + return ConstructorDestructorMethods(rc, emptyList()) +} + +/** + * If we determine a class has a constructor that takes no parameters then check for pairs of getters / setters + * and use those + */ +private fun propertiesForSerializationFromGetters( + properties : Map, + type: Type, + factory: SerializerFactory): ConstructorDestructorMethods { + val getters : MutableList = ArrayList(properties.size) + val setters : MutableList = ArrayList(properties.size) + + properties.forEach { property -> + val getter: Method? = property.value.readMethod + val setter: Method? = property.value.writeMethod + + if (getter == null || setter == null) return@forEach + + // NOTE: For now we're ignoring type mismatches. I.e. where the setter or getter type differs + // from the underlying property as in this case the BEAN inspector doesn't return them as a property + // and thus they arne't visible here + + getters += PropertySerializer.make(property.key, getter, resolveTypeVariables(getter.genericReturnType, type), + factory) + setters += setter + } + + return ConstructorDestructorMethods(getters, setters) } private fun constructorParamTakesReturnTypeOfGetter(getterReturnType: Type, rawGetterReturnType: Type, param: KParameter): Boolean { @@ -111,7 +159,7 @@ private fun constructorParamTakesReturnTypeOfGetter(getterReturnType: Type, rawG return typeToken.isSupertypeOf(getterReturnType) || typeToken.isSupertypeOf(rawGetterReturnType) } -private fun propertiesForSerializationFromAbstract(clazz: Class<*>, type: Type, factory: SerializerFactory): Collection { +private fun propertiesForSerializationFromAbstract(clazz: Class<*>, type: Type, factory: SerializerFactory): ConstructorDestructorMethods { // Kotlin reflection doesn't work with Java getters the way you might expect, so we drop back to good ol' beans. val properties = Introspector.getBeanInfo(clazz).propertyDescriptors.filter { it.name != "class" }.sortedBy { it.name }.filterNot { it is IndexedPropertyDescriptor } val rc: MutableList = ArrayList(properties.size) @@ -121,7 +169,7 @@ private fun propertiesForSerializationFromAbstract(clazz: Class<*>, type: Type, val returnType = resolveTypeVariables(getter.genericReturnType, type) rc += PropertySerializer.make(property.name, getter, returnType, factory) } - return rc + return ConstructorDestructorMethods(rc, emptyList()) } internal fun interfacesForSerialization(type: Type, serializerFactory: SerializerFactory): List { diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutput.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutput.kt index 2c0c4ece9f..901df19ac2 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutput.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutput.kt @@ -8,6 +8,10 @@ import java.nio.ByteBuffer import java.util.* import kotlin.collections.LinkedHashSet +data class BytesAndSchemas( + val obj: SerializedBytes, + val schema: Schema, + val transformsSchema: TransformsSchema) /** * Main entry point for serializing an object to AMQP. @@ -35,6 +39,18 @@ open class SerializationOutput(internal val serializerFactory: SerializerFactory } } + + @Throws(NotSerializableException::class) + fun serializeAndReturnSchema(obj: T): BytesAndSchemas { + try { + val blob = _serialize(obj) + val schema = Schema(schemaHistory.toList()) + return BytesAndSchemas(blob, schema, TransformsSchema.build(schema, serializerFactory)) + } finally { + andFinally() + } + } + internal fun andFinally() { objectHistory.clear() serializerHistory.clear() diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt index e78cf2b3d8..5700c08be7 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/ThrowableSerializer.kt @@ -24,7 +24,7 @@ class ThrowableSerializer(factory: SerializerFactory) : CustomSerializer.Proxy post.getA()).isInstanceOf(NullPointerException.class); + } + + @Test + public void typeMistmatch2() throws NotSerializableException { + SerializerFactory factory1 = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader()); + + TypeMismatch2 tm = new TypeMismatch2(); + tm.setA("10"); + assertEquals((Integer)10, tm.getA()); + + TypeMismatch2 post = new DeserializationInput(factory1).deserialize(new SerializationOutput(factory1).serialize(tm), + TypeMismatch2.class); + + // because there is a type mismatch in the class, it won't return that info as a BEAN property and thus + // we won't serialise it and thus on deserialization it won't be initialized + assertEquals(null, post.getA()); + } +} diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt index 43fe82a6ee..08d7af6ebb 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt @@ -30,20 +30,3 @@ class TestSerializationOutput( fun testName(): String = Thread.currentThread().stackTrace[2].methodName -data class BytesAndSchemas( - val obj: SerializedBytes, - val schema: Schema, - val transformsSchema: TransformsSchema) - -// Extension for the serialize routine that returns the scheme encoded into the -// bytes as well as the bytes for simple testing -@Throws(NotSerializableException::class) -fun SerializationOutput.serializeAndReturnSchema(obj: T): BytesAndSchemas { - try { - val blob = _serialize(obj) - val schema = Schema(schemaHistory.toList()) - return BytesAndSchemas(blob, schema, TransformsSchema.build(schema, serializerFactory)) - } finally { - andFinally() - } -} From f230e2670b574f5a15f3a8077aea4e48016d6332 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Thu, 4 Jan 2018 16:16:48 +0000 Subject: [PATCH 03/10] REVIEW COMMENTS --- .../internal/serialization/amqp/ObjectSerializer.kt | 4 ++-- .../internal/serialization/amqp/SerializationHelper.kt | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt index bd171d9f00..fa86263665 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt @@ -76,7 +76,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS obj: List<*>, schemas: SerializationSchemas, input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ - logger.debug { "Calling construction based construction" } + logger.trace ("Calling construction based construction for ${clazz.typeName}") return construct(obj.zip(propertySerializers.getters).map { it.second.readProperty(it.first, schemas, input) }) } @@ -85,7 +85,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS obj: List<*>, schemas: SerializationSchemas, input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ - logger.debug { "Calling setter based construction" } + logger.trace ("Calling setter based construction for ${clazz.typeName}") val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException ( "Failed to instantiate instance of object $clazz") 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 a90bf7a706..5e3e28711a 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 @@ -92,7 +92,7 @@ private fun propertiesForSerializationFromConstructor( .mapValues { it.value[0] } if (properties.isNotEmpty() && kotlinConstructor.parameters.isEmpty()) { - return propertiesForSerializationFromGetters(properties, type, factory) + return propertiesForSerializationFromSetters(properties, type, factory) } val rc: MutableList = ArrayList(kotlinConstructor.parameters.size) @@ -129,7 +129,7 @@ private fun propertiesForSerializationFromConstructor( * If we determine a class has a constructor that takes no parameters then check for pairs of getters / setters * and use those */ -private fun propertiesForSerializationFromGetters( +private fun propertiesForSerializationFromSetters( properties : Map, type: Type, factory: SerializerFactory): ConstructorDestructorMethods { @@ -142,9 +142,9 @@ private fun propertiesForSerializationFromGetters( if (getter == null || setter == null) return@forEach - // NOTE: For now we're ignoring type mismatches. I.e. where the setter or getter type differs - // from the underlying property as in this case the BEAN inspector doesn't return them as a property - // and thus they arne't visible here + // NOTE: There is no need to check return and parameter types vs the underlying type for + // the getter / setter vs property as if there is a difference then that property isn't reported + // by the BEAN inspector and thus we don't consider that case here getters += PropertySerializer.make(property.key, getter, resolveTypeVariables(getter.genericReturnType, type), factory) From 3bf84ebbd4dde676553acfa22206683aa99b1b90 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Thu, 4 Jan 2018 16:24:23 +0000 Subject: [PATCH 04/10] Review Comments --- .../internal/serialization/amqp/ObjectSerializer.kt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt index fa86263665..6cba3bbea5 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt @@ -2,6 +2,7 @@ package net.corda.nodeapi.internal.serialization.amqp import net.corda.core.utilities.contextLogger import net.corda.core.utilities.debug +import net.corda.core.utilities.trace import net.corda.nodeapi.internal.serialization.amqp.SerializerFactory.Companion.nameForType import org.apache.qpid.proton.amqp.Symbol import org.apache.qpid.proton.codec.Data @@ -76,7 +77,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS obj: List<*>, schemas: SerializationSchemas, input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ - logger.trace ("Calling construction based construction for ${clazz.typeName}") + logger.trace { "Calling construction based construction for ${clazz.typeName}" } return construct(obj.zip(propertySerializers.getters).map { it.second.readProperty(it.first, schemas, input) }) } @@ -85,7 +86,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS obj: List<*>, schemas: SerializationSchemas, input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ - logger.trace ("Calling setter based construction for ${clazz.typeName}") + logger.trace { "Calling setter based construction for ${clazz.typeName}" } val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException ( "Failed to instantiate instance of object $clazz") @@ -112,7 +113,7 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS private fun generateProvides(): List = interfaces.map { nameForType(it) } fun construct(properties: List): Any { - logger.debug { "Calling constructor: '$javaConstructor' with properties '$properties'" } + logger.trace { "Calling constructor: '$javaConstructor' with properties '$properties'" } return javaConstructor?.newInstance(*properties.toTypedArray()) ?: throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.") From 9d66214f4a97710109df3baecb1244013c7d41ea Mon Sep 17 00:00:00 2001 From: Andrzej Cichocki Date: Fri, 5 Jan 2018 16:34:03 +0000 Subject: [PATCH 05/10] CORDA-891 Convert NodeSchedulerServiceTest into a unit test (#2273) --- .../services/events/NodeSchedulerService.kt | 38 +- .../corda/node/utilities/AffinityExecutor.kt | 29 -- .../events/NodeSchedulerServiceTest.kt | 365 ++++++------------ .../testing/internal/InternalTestUtils.kt | 4 + 4 files changed, 134 insertions(+), 302 deletions(-) diff --git a/node/src/main/kotlin/net/corda/node/services/events/NodeSchedulerService.kt b/node/src/main/kotlin/net/corda/node/services/events/NodeSchedulerService.kt index 8c18043db7..372ef392b8 100644 --- a/node/src/main/kotlin/net/corda/node/services/events/NodeSchedulerService.kt +++ b/node/src/main/kotlin/net/corda/node/services/events/NodeSchedulerService.kt @@ -14,6 +14,7 @@ import net.corda.core.flows.FlowLogicRefFactory import net.corda.core.internal.ThreadBox import net.corda.core.internal.VisibleForTesting import net.corda.core.internal.concurrent.flatMap +import net.corda.core.internal.join import net.corda.core.internal.until import net.corda.core.node.StateLoader import net.corda.core.schemas.PersistentStateRef @@ -24,12 +25,11 @@ import net.corda.node.internal.CordaClock import net.corda.node.internal.MutableClock import net.corda.node.services.api.FlowStarter import net.corda.node.services.api.SchedulerService -import net.corda.node.utilities.AffinityExecutor import net.corda.node.utilities.PersistentMap import net.corda.nodeapi.internal.persistence.CordaPersistence import net.corda.nodeapi.internal.persistence.NODE_DATABASE_PREFIX import org.apache.activemq.artemis.utils.ReusableLatch -import java.time.Clock +import org.slf4j.Logger import java.time.Instant import java.util.* import java.util.concurrent.* @@ -51,23 +51,21 @@ import com.google.common.util.concurrent.SettableFuture as GuavaSettableFuture * is the outcome of the activity in order to schedule another activity. Once we have implemented more persistence * in the nodes, maybe we can consider multiple activities and whether the activities have been completed or not, * but that starts to sound a lot like off-ledger state. - * - * @param schedulerTimerExecutor The executor the scheduler blocks on waiting for the clock to advance to the next - * activity. Only replace this for unit testing purposes. This is not the executor the [FlowLogic] is launched on. */ @ThreadSafe class NodeSchedulerService(private val clock: CordaClock, private val database: CordaPersistence, private val flowStarter: FlowStarter, private val stateLoader: StateLoader, - private val schedulerTimerExecutor: Executor = Executors.newSingleThreadExecutor(), private val unfinishedSchedules: ReusableLatch = ReusableLatch(), - private val serverThread: AffinityExecutor, - private val flowLogicRefFactory: FlowLogicRefFactory) + private val serverThread: Executor, + private val flowLogicRefFactory: FlowLogicRefFactory, + private val log: Logger = staticLog, + scheduledStates: MutableMap = createMap()) : SchedulerService, SingletonSerializeAsToken() { companion object { - private val log = contextLogger() + private val staticLog get() = contextLogger() /** * Wait until the given [Future] is complete or the deadline is reached, with support for [MutableClock] implementations * used in demos or testing. This will substitute a Fiber compatible Future so the current @@ -131,7 +129,7 @@ class NodeSchedulerService(private val clock: CordaClock, * or [Throwable] is available in the original. * * We need this so that we do not block the actual thread when calling get(), but instead allow a Quasar context - * switch. There's no need to checkpoint our [Fiber]s as there's no external effect of waiting. + * switch. There's no need to checkpoint our [co.paralleluniverse.fibers.Fiber]s as there's no external effect of waiting. */ private fun makeStrandFriendlySettableFuture(future: Future) = QuasarSettableFuture().also { g -> when (future) { @@ -140,6 +138,9 @@ class NodeSchedulerService(private val clock: CordaClock, else -> throw IllegalArgumentException("Cannot make future $future Strand friendly.") } } + + @VisibleForTesting + internal val schedulingAsNextFormat = "Scheduling as next {}" } @Entity @@ -152,20 +153,17 @@ class NodeSchedulerService(private val clock: CordaClock, var scheduledAt: Instant = Instant.now() ) - private class InnerState { - var scheduledStates = createMap() - + private class InnerState(var scheduledStates: MutableMap) { var scheduledStatesQueue: PriorityQueue = PriorityQueue({ a, b -> a.scheduledAt.compareTo(b.scheduledAt) }) var rescheduled: GuavaSettableFuture? = null } - private val mutex = ThreadBox(InnerState()) - + private val mutex = ThreadBox(InnerState(scheduledStates)) // We need the [StateMachineManager] to be constructed before this is called in case it schedules a flow. fun start() { mutex.locked { - scheduledStatesQueue.addAll(scheduledStates.all().map { it.second }.toMutableList()) + scheduledStatesQueue.addAll(scheduledStates.values) rescheduleWakeUp() } } @@ -206,6 +204,7 @@ class NodeSchedulerService(private val clock: CordaClock, } } + private val schedulerTimerExecutor = Executors.newSingleThreadExecutor() /** * This method first cancels the [java.util.concurrent.Future] for any pending action so that the * [awaitWithDeadline] used below drops through without running the action. We then create a new @@ -223,7 +222,7 @@ class NodeSchedulerService(private val clock: CordaClock, } if (scheduledState != null) { schedulerTimerExecutor.execute { - log.trace { "Scheduling as next $scheduledState" } + log.trace(schedulingAsNextFormat, scheduledState) // This will block the scheduler single thread until the scheduled time (returns false) OR // the Future is cancelled due to rescheduling (returns true). if (!awaitWithDeadline(clock, scheduledState.scheduledAt, ourRescheduledFuture)) { @@ -236,6 +235,11 @@ class NodeSchedulerService(private val clock: CordaClock, } } + @VisibleForTesting + internal fun join() { + schedulerTimerExecutor.join() + } + private fun onTimeReached(scheduledState: ScheduledStateRef) { serverThread.execute { var flowName: String? = "(unknown)" diff --git a/node/src/main/kotlin/net/corda/node/utilities/AffinityExecutor.kt b/node/src/main/kotlin/net/corda/node/utilities/AffinityExecutor.kt index 89c7e68716..096a958a22 100644 --- a/node/src/main/kotlin/net/corda/node/utilities/AffinityExecutor.kt +++ b/node/src/main/kotlin/net/corda/node/utilities/AffinityExecutor.kt @@ -1,11 +1,9 @@ package net.corda.node.utilities import com.google.common.util.concurrent.SettableFuture -import com.google.common.util.concurrent.Uninterruptibles import java.util.* import java.util.concurrent.CompletableFuture import java.util.concurrent.Executor -import java.util.concurrent.LinkedBlockingQueue import java.util.concurrent.ScheduledThreadPoolExecutor import java.util.function.Supplier @@ -83,31 +81,4 @@ interface AffinityExecutor : Executor { } while (!f.get()) } } - - /** - * An executor useful for unit tests: allows the current thread to block until a command arrives from another - * thread, which is then executed. Inbound closures/commands stack up until they are cleared by looping. - * - * @param alwaysQueue If true, executeASAP will never short-circuit and will always queue up. - */ - class Gate(private val alwaysQueue: Boolean = false) : AffinityExecutor { - private val thisThread = Thread.currentThread() - private val commandQ = LinkedBlockingQueue() - - override val isOnThread: Boolean - get() = !alwaysQueue && Thread.currentThread() === thisThread - - override fun execute(command: Runnable) { - Uninterruptibles.putUninterruptibly(commandQ, command) - } - - fun waitAndRun() { - val runnable = Uninterruptibles.takeUninterruptibly(commandQ) - runnable.run() - } - - override fun flush() { - throw UnsupportedOperationException() - } - } } diff --git a/node/src/test/kotlin/net/corda/node/services/events/NodeSchedulerServiceTest.kt b/node/src/test/kotlin/net/corda/node/services/events/NodeSchedulerServiceTest.kt index f771b5a197..de82ec5bbf 100644 --- a/node/src/test/kotlin/net/corda/node/services/events/NodeSchedulerServiceTest.kt +++ b/node/src/test/kotlin/net/corda/node/services/events/NodeSchedulerServiceTest.kt @@ -1,323 +1,176 @@ package net.corda.node.services.events -import co.paralleluniverse.fibers.Suspendable -import com.codahale.metrics.MetricRegistry +import com.google.common.util.concurrent.MoreExecutors import com.nhaarman.mockito_kotlin.* import net.corda.core.contracts.* -import net.corda.core.crypto.generateKeyPair import net.corda.core.flows.FlowLogic import net.corda.core.flows.FlowLogicRef import net.corda.core.flows.FlowLogicRefFactory -import net.corda.core.identity.AbstractParty -import net.corda.core.identity.CordaX500Name -import net.corda.core.identity.Party -import net.corda.core.internal.concurrent.doneFuture -import net.corda.core.node.NodeInfo -import net.corda.core.node.ServiceHub -import net.corda.core.serialization.SingletonSerializeAsToken -import net.corda.core.transactions.TransactionBuilder -import net.corda.core.utilities.NetworkHostAndPort import net.corda.core.utilities.days -import net.corda.node.internal.FlowStarterImpl -import net.corda.node.internal.cordapp.CordappLoader -import net.corda.node.internal.cordapp.CordappProviderImpl -import net.corda.node.services.api.MonitoringService -import net.corda.node.services.api.ServiceHubInternal -import net.corda.node.services.persistence.DBCheckpointStorage -import net.corda.node.services.statemachine.FlowLogicRefFactoryImpl -import net.corda.node.services.statemachine.StateMachineManager -import net.corda.node.services.statemachine.StateMachineManagerImpl -import net.corda.node.services.vault.NodeVaultService -import net.corda.node.utilities.AffinityExecutor -import net.corda.node.internal.configureDatabase -import net.corda.node.services.api.NetworkMapCacheInternal -import net.corda.node.services.config.NodeConfiguration -import net.corda.nodeapi.internal.persistence.CordaPersistence -import net.corda.nodeapi.internal.persistence.DatabaseConfig -import net.corda.testing.* -import net.corda.testing.contracts.DummyContract import net.corda.testing.internal.rigorousMock -import net.corda.testing.node.* -import net.corda.testing.node.MockServices.Companion.makeTestDataSourceProperties -import net.corda.testing.services.MockAttachmentStorage -import org.assertj.core.api.Assertions.assertThat -import org.junit.After -import org.junit.Before +import net.corda.core.internal.FlowStateMachine +import net.corda.core.internal.concurrent.openFuture +import net.corda.core.internal.uncheckedCast +import net.corda.core.node.StateLoader +import net.corda.node.services.api.FlowStarter +import net.corda.nodeapi.internal.persistence.CordaPersistence +import net.corda.nodeapi.internal.persistence.DatabaseTransaction +import net.corda.testing.internal.doLookup +import net.corda.testing.node.TestClock import org.junit.Rule import org.junit.Test +import org.junit.rules.TestWatcher +import org.junit.runner.Description +import org.slf4j.Logger import java.time.Clock import java.time.Instant -import java.util.concurrent.CountDownLatch -import java.util.concurrent.Executors -import java.util.concurrent.TimeUnit -import kotlin.test.assertTrue -class NodeSchedulerServiceTest : SingletonSerializeAsToken() { - private companion object { - val ALICE_KEY = TestIdentity(ALICE_NAME, 70).keyPair - val DUMMY_IDENTITY_1 = getTestPartyAndCertificate(Party(CordaX500Name("Dummy", "Madrid", "ES"), generateKeyPair().public)) - val DUMMY_NOTARY = TestIdentity(DUMMY_NOTARY_NAME, 20).party - val myInfo = NodeInfo(listOf(NetworkHostAndPort("mockHost", 30000)), listOf(DUMMY_IDENTITY_1), 1, serial = 1L) +class NodeSchedulerServiceTest { + private val mark = Instant.now() + private val testClock = TestClock(rigorousMock().also { + doReturn(mark).whenever(it).instant() + }) + private val database = rigorousMock().also { + doAnswer { + val block: DatabaseTransaction.() -> Any? = uncheckedCast(it.arguments[0]) + rigorousMock().block() + }.whenever(it).transaction(any()) } - + private val flowStarter = rigorousMock().also { + doReturn(openFuture>()).whenever(it).startFlow(any>(), any()) + } + private val transactionStates = mutableMapOf>() + private val stateLoader = rigorousMock().also { + doLookup(transactionStates).whenever(it).loadState(any()) + } + private val flows = mutableMapOf>() + private val flowLogicRefFactory = rigorousMock().also { + doLookup(flows).whenever(it).toFlowLogic(any()) + } + private val log = rigorousMock().also { + doReturn(false).whenever(it).isTraceEnabled + doNothing().whenever(it).trace(any(), any()) + } + private val scheduler = NodeSchedulerService( + testClock, + database, + flowStarter, + stateLoader, + serverThread = MoreExecutors.directExecutor(), + flowLogicRefFactory = flowLogicRefFactory, + log = log, + scheduledStates = mutableMapOf()).apply { start() } @Rule @JvmField - val testSerialization = SerializationEnvironmentRule(true) - private val flowLogicRefFactory = FlowLogicRefFactoryImpl(FlowLogicRefFactoryImpl::class.java.classLoader) - private val realClock: Clock = Clock.systemUTC() - private val stoppedClock: Clock = Clock.fixed(realClock.instant(), realClock.zone) - private val testClock = TestClock(stoppedClock) - - private val schedulerGatedExecutor = AffinityExecutor.Gate(true) - - abstract class Services : ServiceHubInternal, TestReference - - private lateinit var services: Services - private lateinit var scheduler: NodeSchedulerService - private lateinit var smmExecutor: AffinityExecutor.ServiceAffinityExecutor - private lateinit var database: CordaPersistence - private lateinit var countDown: CountDownLatch - private lateinit var smmHasRemovedAllFlows: CountDownLatch - private lateinit var kms: MockKeyManagementService - private lateinit var mockSMM: StateMachineManager - var calls: Int = 0 - - /** - * Have a reference to this test added to [ServiceHub] so that when the [FlowLogic] runs it can access the test instance. - * The [TestState] is serialized and deserialized so attempting to use a transient field won't work, as it just - * results in NPE. - */ - interface TestReference { - val testReference: NodeSchedulerServiceTest - } - - @Before - fun setup() { - countDown = CountDownLatch(1) - smmHasRemovedAllFlows = CountDownLatch(1) - calls = 0 - val dataSourceProps = makeTestDataSourceProperties() - database = configureDatabase(dataSourceProps, DatabaseConfig(), rigorousMock()) - val identityService = makeTestIdentityService() - kms = MockKeyManagementService(identityService, ALICE_KEY) - val configuration = rigorousMock().also { - doReturn(true).whenever(it).devMode - doReturn(null).whenever(it).devModeOptions - } - val validatedTransactions = MockTransactionStorage() - database.transaction { - services = rigorousMock().also { - doReturn(configuration).whenever(it).configuration - doReturn(MonitoringService(MetricRegistry())).whenever(it).monitoringService - doReturn(validatedTransactions).whenever(it).validatedTransactions - doReturn(rigorousMock().also { - doReturn(doneFuture(null)).whenever(it).nodeReady - }).whenever(it).networkMapCache - doReturn(myInfo).whenever(it).myInfo - doReturn(kms).whenever(it).keyManagementService - doReturn(CordappProviderImpl(CordappLoader.createWithTestPackages(listOf("net.corda.testing.contracts")), MockAttachmentStorage())).whenever(it).cordappProvider - doReturn(NodeVaultService(testClock, kms, validatedTransactions, database.hibernateConfig)).whenever(it).vaultService - doReturn(this@NodeSchedulerServiceTest).whenever(it).testReference - - } - smmExecutor = AffinityExecutor.ServiceAffinityExecutor("test", 1) - mockSMM = StateMachineManagerImpl(services, DBCheckpointStorage(), smmExecutor, database) - scheduler = NodeSchedulerService(testClock, database, FlowStarterImpl(smmExecutor, mockSMM, flowLogicRefFactory), validatedTransactions, schedulerGatedExecutor, serverThread = smmExecutor, flowLogicRefFactory = flowLogicRefFactory) - mockSMM.changes.subscribe { change -> - if (change is StateMachineManager.Change.Removed && mockSMM.allStateMachines.isEmpty()) { - smmHasRemovedAllFlows.countDown() - } - } - mockSMM.start(emptyList()) - scheduler.start() + val tearDown = object : TestWatcher() { + override fun succeeded(description: Description) { + scheduler.join() + verifyNoMoreInteractions(flowStarter) } } - private var allowedUnsuspendedFiberCount = 0 - @After - fun tearDown() { - // We need to make sure the StateMachineManager is done before shutting down executors. - if (mockSMM.allStateMachines.isNotEmpty()) { - smmHasRemovedAllFlows.await() - } - smmExecutor.shutdown() - smmExecutor.awaitTermination(60, TimeUnit.SECONDS) - database.close() - mockSMM.stop(allowedUnsuspendedFiberCount) + private class Event(time: Instant) { + val stateRef = rigorousMock() + val flowLogic = rigorousMock>() + val ssr = ScheduledStateRef(stateRef, time) } - // Ignore IntelliJ when it says these properties can be private, if they are we cannot serialise them - // in AMQP. - @Suppress("MemberVisibilityCanPrivate") - class TestState(val flowLogicRef: FlowLogicRef, val instant: Instant, val myIdentity: Party) : LinearState, SchedulableState { - override val participants: List - get() = listOf(myIdentity) - - override val linearId = UniqueIdentifier() - - override fun nextScheduledActivity(thisStateRef: StateRef, flowLogicRefFactory: FlowLogicRefFactory): ScheduledActivity? { - return ScheduledActivity(flowLogicRef, instant) + private fun schedule(time: Instant) = Event(time).apply { + val logicRef = rigorousMock() + transactionStates[stateRef] = rigorousMock>().also { + doReturn(rigorousMock().also { + doReturn(ScheduledActivity(logicRef, time)).whenever(it).nextScheduledActivity(same(stateRef)!!, any()) + }).whenever(it).data } + flows[logicRef] = flowLogic + scheduler.scheduleStateActivity(ssr) } - class TestFlowLogic(private val increment: Int = 1) : FlowLogic() { - @Suspendable - override fun call() { - (serviceHub as TestReference).testReference.calls += increment - (serviceHub as TestReference).testReference.countDown.countDown() - } + private fun assertWaitingFor(event: Event, total: Int = 1) { + // The timeout is to make verify wait, which is necessary as we're racing the NSS thread i.e. we often get here just before the trace: + verify(log, timeout(5000).times(total)).trace(NodeSchedulerService.schedulingAsNextFormat, event.ssr) } - class Command : TypeOnlyCommandData() + private fun assertStarted(event: Event) { + // Like in assertWaitingFor, use timeout to make verify wait as we often race the call to startFlow: + verify(flowStarter, timeout(5000)).startFlow(same(event.flowLogic)!!, any()) + } @Test fun `test activity due now`() { - val time = stoppedClock.instant() - scheduleTX(time) - - assertThat(calls).isEqualTo(0) - schedulerGatedExecutor.waitAndRun() - countDown.await() - assertThat(calls).isEqualTo(1) + assertStarted(schedule(mark)) } @Test fun `test activity due in the past`() { - val time = stoppedClock.instant() - 1.days - scheduleTX(time) - - assertThat(calls).isEqualTo(0) - schedulerGatedExecutor.waitAndRun() - countDown.await() - assertThat(calls).isEqualTo(1) + assertStarted(schedule(mark - 1.days)) } @Test fun `test activity due in the future`() { - val time = stoppedClock.instant() + 1.days - scheduleTX(time) - - val backgroundExecutor = Executors.newSingleThreadExecutor() - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } - assertThat(calls).isEqualTo(0) + val event = schedule(mark + 1.days) + assertWaitingFor(event) testClock.advanceBy(1.days) - backgroundExecutor.shutdown() - assertTrue(backgroundExecutor.awaitTermination(60, TimeUnit.SECONDS)) - countDown.await() - assertThat(calls).isEqualTo(1) + assertStarted(event) } @Test fun `test activity due in the future and schedule another earlier`() { - val time = stoppedClock.instant() + 1.days - scheduleTX(time + 1.days) - - val backgroundExecutor = Executors.newSingleThreadExecutor() - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } - assertThat(calls).isEqualTo(0) - scheduleTX(time, 3) - - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } + val event2 = schedule(mark + 2.days) + val event1 = schedule(mark + 1.days) + assertWaitingFor(event1) testClock.advanceBy(1.days) - countDown.await() - assertThat(calls).isEqualTo(3) - backgroundExecutor.shutdown() - assertTrue(backgroundExecutor.awaitTermination(60, TimeUnit.SECONDS)) + assertStarted(event1) + assertWaitingFor(event2, 2) + testClock.advanceBy(1.days) + assertStarted(event2) } @Test fun `test activity due in the future and schedule another later`() { - allowedUnsuspendedFiberCount = 1 - val time = stoppedClock.instant() + 1.days - scheduleTX(time) - - val backgroundExecutor = Executors.newSingleThreadExecutor() - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } - assertThat(calls).isEqualTo(0) - scheduleTX(time + 1.days, 3) - + val event1 = schedule(mark + 1.days) + val event2 = schedule(mark + 2.days) + assertWaitingFor(event1) testClock.advanceBy(1.days) - countDown.await() - assertThat(calls).isEqualTo(1) - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } + assertStarted(event1) + assertWaitingFor(event2) testClock.advanceBy(1.days) - backgroundExecutor.shutdown() - assertTrue(backgroundExecutor.awaitTermination(60, TimeUnit.SECONDS)) + assertStarted(event2) } @Test fun `test activity due in the future and schedule another for same time`() { - val time = stoppedClock.instant() + 1.days - scheduleTX(time) - - val backgroundExecutor = Executors.newSingleThreadExecutor() - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } - assertThat(calls).isEqualTo(0) - scheduleTX(time, 3) - + val eventA = schedule(mark + 1.days) + val eventB = schedule(mark + 1.days) + assertWaitingFor(eventA) testClock.advanceBy(1.days) - countDown.await() - assertThat(calls).isEqualTo(1) - backgroundExecutor.shutdown() - assertTrue(backgroundExecutor.awaitTermination(60, TimeUnit.SECONDS)) + assertStarted(eventA) + assertStarted(eventB) + } + + @Test + fun `test activity due in the future and schedule another for same time then unschedule second`() { + val eventA = schedule(mark + 1.days) + val eventB = schedule(mark + 1.days) + scheduler.unscheduleStateActivity(eventB.stateRef) + assertWaitingFor(eventA) + testClock.advanceBy(1.days) + assertStarted(eventA) } @Test fun `test activity due in the future and schedule another for same time then unschedule original`() { - val time = stoppedClock.instant() + 1.days - val scheduledRef1 = scheduleTX(time) - - val backgroundExecutor = Executors.newSingleThreadExecutor() - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } - assertThat(calls).isEqualTo(0) - scheduleTX(time, 3) - - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } - database.transaction { - scheduler.unscheduleStateActivity(scheduledRef1!!.ref) - } + val eventA = schedule(mark + 1.days) + val eventB = schedule(mark + 1.days) + scheduler.unscheduleStateActivity(eventA.stateRef) + assertWaitingFor(eventA) // XXX: Shouldn't it be waiting for eventB now? testClock.advanceBy(1.days) - countDown.await() - assertThat(calls).isEqualTo(3) - backgroundExecutor.shutdown() - assertTrue(backgroundExecutor.awaitTermination(60, TimeUnit.SECONDS)) + assertStarted(eventB) } @Test fun `test activity due in the future then unschedule`() { - val scheduledRef1 = scheduleTX(stoppedClock.instant() + 1.days) - - val backgroundExecutor = Executors.newSingleThreadExecutor() - backgroundExecutor.execute { schedulerGatedExecutor.waitAndRun() } - assertThat(calls).isEqualTo(0) - - database.transaction { - scheduler.unscheduleStateActivity(scheduledRef1!!.ref) - } + scheduler.unscheduleStateActivity(schedule(mark + 1.days).stateRef) testClock.advanceBy(1.days) - assertThat(calls).isEqualTo(0) - backgroundExecutor.shutdown() - assertTrue(backgroundExecutor.awaitTermination(60, TimeUnit.SECONDS)) - } - - private fun scheduleTX(instant: Instant, increment: Int = 1): ScheduledStateRef? { - var scheduledRef: ScheduledStateRef? = null - database.transaction { - apply { - val freshKey = kms.freshKey() - val state = TestState(flowLogicRefFactory.createForRPC(TestFlowLogic::class.java, increment), instant, DUMMY_IDENTITY_1.party) - val builder = TransactionBuilder(null).apply { - addOutputState(state, DummyContract.PROGRAM_ID, DUMMY_NOTARY) - addCommand(Command(), freshKey) - } - val usefulTX = services.signInitialTransaction(builder, freshKey) - val txHash = usefulTX.id - - services.recordTransactions(usefulTX) - scheduledRef = ScheduledStateRef(StateRef(txHash, 0), state.instant) - scheduler.scheduleStateActivity(scheduledRef!!) - } - } - return scheduledRef } } diff --git a/testing/test-utils/src/main/kotlin/net/corda/testing/internal/InternalTestUtils.kt b/testing/test-utils/src/main/kotlin/net/corda/testing/internal/InternalTestUtils.kt index ccf34284e7..60a7c40b00 100644 --- a/testing/test-utils/src/main/kotlin/net/corda/testing/internal/InternalTestUtils.kt +++ b/testing/test-utils/src/main/kotlin/net/corda/testing/internal/InternalTestUtils.kt @@ -1,5 +1,6 @@ package net.corda.testing.internal +import com.nhaarman.mockito_kotlin.doAnswer import net.corda.core.crypto.Crypto import net.corda.core.identity.CordaX500Name import net.corda.core.utilities.loggerFor @@ -108,3 +109,6 @@ fun createDevNodeCaCertPath( val nodeCa = createDevNodeCa(intermediateCa, legalName) return Triple(rootCa, intermediateCa, nodeCa) } + +/** Application of [doAnswer] that gets a value from the given [map] using the arg at [argIndex] as key. */ +fun doLookup(map: Map<*, *>, argIndex: Int = 0) = doAnswer { map[it.arguments[argIndex]] } From e2286a75e5d3e51f58f8f6904763a91c9ef6ae52 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Fri, 5 Jan 2018 17:34:32 +0000 Subject: [PATCH 06/10] CORDA-902 - Serialization documentation (#2323) CORDA-902 - Serialization documentation --- docs/source/serialization.rst | 109 ++++++++++++++++++++++++++++++---- 1 file changed, 98 insertions(+), 11 deletions(-) diff --git a/docs/source/serialization.rst b/docs/source/serialization.rst index b4f87b912a..347152c175 100644 --- a/docs/source/serialization.rst +++ b/docs/source/serialization.rst @@ -210,22 +210,109 @@ Here are the rules to adhere to for support of your own types: Classes ``````` +General Rules +''''''''''''' + + #. The class must be compiled with parameter names included in the ``.class`` file. This is the default in Kotlin + but must be turned on in Java (``-parameters`` command line option to ``javac``). + #. The class is annotated with ``@CordaSerializable``. + #. The declared types of constructor arguments, getters, and setters must be supported, and where generics are used the + generic parameter must be a supported type, an open wildcard (``*``), or a bounded wildcard which is currently + widened to an open wildcard. + #. Any superclass must adhere to the same rules, but can be abstract. + #. Object graph cycles are not supported, so an object cannot refer to itself, directly or indirectly. + +Constructor Instantiation +''''''''''''''''''''''''' + +The primary way the AMQP serialization framework for Corda instantiates objects is via a defined constructor. This is +used to first determine which properties of an object are to be serialised then, on deserialization, it is used to +instantiate the object with the serialized values. + +This is the recommended design idiom for serializable objects in Corda as it allows for immutable state objects to +be created + + #. A Java Bean getter for each of the properties in the constructor, with the names matching up. For example, for a constructor + parameter ``foo``, there must be a getter called ``getFoo()``. If the type of ``foo`` is boolean, the getter may + optionally be called ``isFoo()``. This is why the class must be compiled with parameter names turned on. #. A constructor which takes all of the properties that you wish to record in the serialized form. This is required in order for the serialization framework to reconstruct an instance of your class. #. If more than one constructor is provided, the serialization framework needs to know which one to use. The ``@ConstructorForDeserialization`` annotation can be used to indicate which one. For a Kotlin class, without the ``@ConstructorForDeserialization`` annotation, the *primary constructor* will be selected. - #. The class must be compiled with parameter names included in the ``.class`` file. This is the default in Kotlin - but must be turned on in Java (``-parameters`` command line option to ``javac``). - #. A Java Bean getter for each of the properties in the constructor, with the names matching up. For example, for a constructor - parameter ``foo``, there must be a getter called ``getFoo()``. If the type of ``foo`` is boolean, the getter may - optionally be called ``isFoo()``. This is why the class must be compiled with parameter names turned on. - #. The class is annotated with ``@CordaSerializable``. - #. The declared types of constructor arguments / getters must be supported, and where generics are used the - generic parameter must be a supported type, an open wildcard (``*``), or a bounded wildcard which is currently - widened to an open wildcard. - #. Any superclass must adhere to the same rules, but can be abstract. - #. Object graph cycles are not supported, so an object cannot refer to itself, directly or indirectly. + +In Kotlin, this maps cleanly to a data class where there getters are synthesized automatically. For example, + +.. container:: codeset + + .. sourcecode:: kotlin + + data class Example (val a: Int, val b: String) + +Both properties a and b will be included in the serialised form. However, as stated above, properties not mentioned in +the constructor will not be serialised. For example, in the following code property c will not be considered part of the +serialised form + +.. container:: codeset + + .. sourcecode:: kotlin + + data class Example (val a: Int, val b: String) { + var c: Int = 20 + } + + var e = Example (10, "hello") + e.c = 100; + + val e2 = e.serialize().deserialize() // e2.c will be 20, not 100!!! + +.. warning:: Private properties in Kotlin classes render the class unserializable *unless* a public + getter is manually defined. For example: + + .. container:: codeset + + .. sourcecode:: kotlin + + data class C(val a: Int, private val b: Int) { + // Without this C cannot be serialized + public fun getB() = b + } + + .. note:: This is particularly relevant as IDE's can often point out where they believe a + property can be made private without knowing this can break Corda serialization. Should + this happen then a run time warning will be generated when the class fails to serialize + +Setter Instantiation +'''''''''''''''''''' + +As an alternative to constructor based initialisation Corda can also determine the important elements of an +object by inspecting the getter and setter methods present on a class. If a class has **only** a default +constructor **and** properties then the serializable properties will be determined by the presence of +both a getter and setter for that property that are both publicly visible. I.e. the class adheres to +the classic *idiom* of mutable JavaBeans. + +On deserialization, a default instance will first be created and then, in turn, the setters invoked +on that object to populate the correct values. + +For example: + +.. container:: codeset + + .. sourcecode:: Java + + class Example { + private int a; + private int b; + private int c; + + public int getA() { return a; } + public int getB() { return b; } + public int getC() { return c; } + + public void setA(int a) { this.a = a; } + public void setB(int b) { this.b = b; } + public void setC(int c) { this.c = c; } + } Enums ````` From 6a07576c96575afa4c67f88c6b3a3cfcc105d2c9 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Fri, 5 Jan 2018 17:44:53 +0000 Subject: [PATCH 07/10] CORDA-834 - Class default evolution serialisation documentation (#2209) CORDA-834 - Class default evolution serialisation documentation --- .../serialization-default-evolution.rst | 237 ++++++++++++++++++ docs/source/serialization.rst | 7 +- 2 files changed, 242 insertions(+), 2 deletions(-) create mode 100644 docs/source/serialization-default-evolution.rst diff --git a/docs/source/serialization-default-evolution.rst b/docs/source/serialization-default-evolution.rst new file mode 100644 index 0000000000..282ca452b8 --- /dev/null +++ b/docs/source/serialization-default-evolution.rst @@ -0,0 +1,237 @@ +Default Class Evolution +======================= + +.. contents:: + +Whilst more complex evolutionary modifications to classes require annotating, Corda's serialization +framework supports several minor modifications to classes without any external modification save +the actual code changes. These are: + + #. Adding nullable properties + #. Adding non nullable properties if, and only if, an annotated constructor is provided + #. Removing properties + #. Reordering constructor parameters + +Adding Nullable Properties +-------------------------- + +The serialization framework allows nullable properties to be freely added. For example: + +.. container:: codeset + + .. sourcecode:: kotlin + + // Initial instance of the class + data class Example1 (val a: Int, b: String) // (Version A) + + // Class post addition of property c + data class Example1 (val a: Int, b: String, c: Int?) // (Version B) + +A node with version A of class ``Example1`` will be able to deserialize a blob serialized by a node with it +at version B as the framework would treat it as a removed property. + +A node with the class at version B will be able to deserialize a serialized version A of ``Example1`` without +any modification as the property is nullable and will thus provide null to the constructor. + +Adding Non Nullable Properties +------------------------------ + +If a non null property is added, unlike nullable properties, some additional code is required for +this to work. Consider a similar example to our nullable example above + +.. container:: codeset + + .. sourcecode:: kotlin + + // Initial instance of the class + data class Example2 (val a: Int, b: String) // (Version A) + + // Class post addition of property c + data class Example1 (val a: Int, b: String, c: Int) { // (Version B) + @DeprecatedConstructorForDeserialization(1) + constructor (a: Int, b: String) : this(a, b, 0) // 0 has been determined as a sensible default + } + +For this to work we have had to add a new constructor that allows nodes with the class at version B to create an +instance from serialised form of that class from an older version, in this case version A as per our example +above. A sensible default for the missing value is provided for instantiation of the non null property. + +.. note:: The ``@DeprecatedConstructorForDeserialization`` annotation is important, this signifies to the + serialization framework that this constructor should be considered for building instances of the + object when evolution is required. + + Furthermore, the integer parameter passed to the constructor if the annotation indicates a precedence + order, see the discussion below. + +As before, instances of the class at version A will be able to deserialize serialized forms of example B as it +will simply treat them as if the property has been removed (as from its perspective, they will have been.) + + +Constructor Versioning +~~~~~~~~~~~~~~~~~~~~~~ + +If, over time, multiple non nullable properties are added, then a class will potentially have to be able +to deserialize a number of different forms of the class. Being able to select the correct constructor is +important to ensure the maximum information is extracted. + +Consider this example: + + +.. container:: codeset + + .. sourcecode:: kotlin + + // The original version of the class + data class Example3 (val a: Int, val b: Int) + +.. container:: codeset + + .. sourcecode:: kotlin + + // The first alteration, property c added + data class Example3 (val a: Int, val b: Int, val c: Int) + +.. container:: codeset + + .. sourcecode:: kotlin + + // The second alteration, property d added + data class Example3 (val a: Int, val b: Int, val c: Int, val d: Int) + +.. container:: codeset + + .. sourcecode:: kotlin + + // The third alteration, and how it currently exists, property e added + data class Example3 (val a: Int, val b: Int, val c: Int, val d: Int, val: Int e) { + // NOTE: version number purposefully omitted from annotation for demonstration purposes + @DeprecatedConstructorForDeserialization + constructor (a: Int, b: Int) : this(a, b, -1, -1, -1) // alt constructor 1 + @DeprecatedConstructorForDeserialization + constructor (a: Int, b: Int, c: Int) : this(a, b, c, -1, -1) // alt constructor 2 + @DeprecatedConstructorForDeserialization + constructor (a: Int, b: Int, c: Int, d) : this(a, b, c, d, -1) // alt constructor 3 + } + +In this case, the deserializer has to be able to deserialize instances of class ``Example3`` that were serialized as, for example: + +.. container:: codeset + + .. sourcecode:: kotlin + + Example3 (1, 2) // example I + Example3 (1, 2, 3) // example II + Example3 (1, 2, 3, 4) // example III + Example3 (1, 2, 3, 4, 5) // example IV + +Examples I, II, and III would require evolution and thus selection of constructor. Now, with no versioning applied there +is ambiguity as to which constructor should be used. For example, example II could use 'alt constructor 2' which matches +it's arguments most tightly or 'alt constructor 1' and not instantiate parameter c. + +``constructor (a: Int, b: Int, c: Int) : this(a, b, c, -1, -1)`` + +or + +``constructor (a: Int, b: Int) : this(a, b, -1, -1, -1)`` + +Whilst it may seem trivial which should be picked, it is still ambiguous, thus we use a versioning number in the constructor +annotation which gives a strict precedence order to constructor selection. Therefore, the proper form of the example would +be: + +.. container:: codeset + + .. sourcecode:: kotlin + + // The third alteration, and how it currently exists, property e added + data class Example3 (val a: Int, val b: Int, val c: Int, val d: Int, val: Int e) { + // NOTE: version number purposefully omitted from annotation for demonstration purposes + @DeprecatedConstructorForDeserialization(1) + constructor (a: Int, b: Int) : this(a, b, -1, -1, -1) // alt constructor 1 + @DeprecatedConstructorForDeserialization(2) + constructor (a: Int, b: Int, c: Int) : this(a, b, c, -1, -1) // alt constructor 2 + @DeprecatedConstructorForDeserialization(3) + constructor (a: Int, b: Int, c: Int, d) : this(a, b, c, d, -1) // alt constructor 3 + } + +Constructors are selected in strict descending order taking the one that enables construction. So, deserializing examples I to IV would +give us: + +.. container:: codeset + + .. sourcecode:: kotlin + + Example3 (1, 2, -1, -1, -1) // example I + Example3 (1, 2, 3, -1, -1) // example II + Example3 (1, 2, 3, 4, -1) // example III + Example3 (1, 2, 3, 4, 5) // example IV + +Removing Properties +------------------- + +Property removal is effectively a mirror of adding properties (both nullable and non nullable) given that this functionality +is required to facilitate the addition of properties. When this state is detected by the serialization framework, properties +that don't have matching parameters in the main constructor are simply omitted from object construction. + +.. container:: codeset + + .. sourcecode:: kotlin + + // Initial instance of the class + data class Example4 (val a: Int?, val b: String?, val c: Int?) // (Version A) + + + // Class post removal of property 'a' + data class Example4 (val b: String?, c: Int?) // (Version B) + + +In practice, what this means is removing nullable properties is possible. However, removing non nullable properties isn't because +a node receiving a message containing a serialized form of an object with fewer properties than it requires for construction has +no capacity to guess at what values should or could be used as sensible defaults. When those properties are nullable it simply sets +them to null. + +Reordering Constructor Parameter Order +-------------------------------------- + +Properties (in Kotlin this corresponds to constructor parameters) may be reordered freely. The evolution serializer will create a +mapping between how a class was serialized and its current constructor parameter order. This is important to our AMQP framework as it +constructs objects using their primary (or annotated) constructor. The ordering of whose parameters will have determined the way +an object's properties were serialised into the byte stream. + +For an illustrative example consider a simple class: + +.. Container:: codeset + + .. sourcecode:: kotlin + + data class Example5 (val a: Int, val b: String) + + val e = Example5(999, "hello") + +When we serialize ``e`` its properties will be encoded in order of its primary constructors parameters, so: + +``999,hello`` + +Were those parameters to be reordered post serialisation then deserializing, without evolution, would fail with a basic +type error as we'd attempt to create the new value of ``Example5`` with the values provided in the wrong order: + +.. Container:: codeset + + .. sourcecode:: kotlin + + // changed post serialisation + data class Example5 (val b: String, val a: Int) + + .. sourcecode:: shell + + | 999 | hello | <--- Extract properties to pass to constructor from byte stream + | | + | +--------------------------+ + +--------------------------+ | + | | + deserializedValue = Example5(999, "hello") <--- Resulting attempt at construction + | | + | \ + | \ <--- Will clearly fail as 999 is not a + | \ string and hello is not an integer + data class Example5 (val b: String, val a: Int) + diff --git a/docs/source/serialization.rst b/docs/source/serialization.rst index 347152c175..1b51f95110 100644 --- a/docs/source/serialization.rst +++ b/docs/source/serialization.rst @@ -373,6 +373,9 @@ Future Enhancements Type Evolution -------------- -When we move to AMQP as the serialization format, we will be adding explicit support for interoperability of different versions of the same code. -We will describe here the rules and features for evolvability as part of a future update to the documentation. +Type evolution is the mechanisms by which classes can be altered over time yet still remain serializable and deserializable across +all versions of the class. This ensures an object serialized with an older idea of what the class "looked like" can be deserialized +and a version of the current state of the class instantiated. + +More detail can be found in :doc:`serialization-default-evolution` From 83a0a2fa3cc8a517d8ce1524b04a22b3f69a3db0 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Fri, 5 Jan 2018 19:30:17 +0000 Subject: [PATCH 08/10] Enum evolution documentation (#2189) * CORDA-553 - Documentation * CORDA-553 - Documentation * Review comments * review comments * DOCUMENTATION: Serilization docs review updates --- docs/source/serialization-enum-evolution.rst | 335 +++++++++++++++++++ docs/source/serialization.rst | 47 ++- 2 files changed, 364 insertions(+), 18 deletions(-) create mode 100644 docs/source/serialization-enum-evolution.rst diff --git a/docs/source/serialization-enum-evolution.rst b/docs/source/serialization-enum-evolution.rst new file mode 100644 index 0000000000..1b48594fe9 --- /dev/null +++ b/docs/source/serialization-enum-evolution.rst @@ -0,0 +1,335 @@ +Enum Evolution +============== + +.. contents:: + +In the continued development of a CorDapp an enumerated type that was fit for purpose at one time may +require changing. Normally, this would be problematic as anything serialised (and kept in a vault) would +run the risk of being unable to be deserialized in the future or older versions of the app still alive +within a compatibility zone may fail to deserialize a message. + +To facilitate backward and forward support for alterations to enumerated types Corda's serialization +framework supports the evolution of such types through a well defined framework that allows different +versions to interoperate with serialised versions of an enumeration of differing versions. + +This is achieved through the use of certain annotations. Whenever a change is made, an annotation +capturing the change must be added (whilst it can be omitted any interoperability will be lost). Corda +supports two modifications to enumerated types, adding new constants, and renaming existing constants + +.. warning:: Once added evolution annotations MUST NEVER be removed from a class, doing so will break + both forward and backward compatibility for this version of the class and any version moving + forward + +The Purpose of Annotating Changes +--------------------------------- + +The biggest hurdle to allowing enum constants to be changed is that there will exist instances of those +classes, either serialized in a vault or on nodes with the old, unmodified, version of the class that we +must be able to interoperate with. Thus if a received data structure references an enum assigned a constant +value that doesn't exist on the running JVM, a solution is needed. + +For this, we use the annotations to allow developers to express their backward compatible intentions. + +In the case of renaming constants this is somewhat obvious, the deserializing node will simply treat any +constants it doesn't understand as their "old" values, i.e. those values that it currently knows about. + +In the case of adding new constants the developer must chose which constant (that existed *before* adding +the new one) a deserializing system should treat any instances of the new one as. + +.. note:: Ultimately, this may mean some design compromises are required. If an enumeration is + planned as being often extended and no sensible defaults will exist then including a constant + in the original version of the class that all new additions can default to may make sense + +Evolution Transmission +---------------------- + +An object serializer, on creation, will inspect the class it represents for any evolution annotations. +If a class is thus decorated those rules will be encoded as part of any serialized representation of a +data structure containing that class. This ensures that on deserialization the deserializing object will +have access to any transformative rules it needs to build a local instance of the serialized object. + +Evolution Precedence +-------------------- + +On deserialization (technically on construction of a serialization object that facilitates serialization +and deserialization) a class's fingerprint is compared to the fingerprint received as part of the AMQP +header of the corresponding class. If they match then we are sure that the two class versions are functionally +the same and no further steps are required save the deserialization of the serialized information into an instance +of the class. + +If, however, the fingerprints differ then we know that the class we are attempting to deserialize is different +than the version we will be deserializing it into. What we cannot know is which version is newer, at least +not by examining the fingerprint + +.. note:: Corda's AMQP fingerprinting for enumerated types include the type name and the enum constants + +Newer vs older is important as the deserializer needs to use the more recent set of transforms to ensure it +can transform the serialised object into the form as it exists in the deserializer. Newness is determined simply +by length of the list of all transforms. This is sufficient as transform annotations should only ever be added + +.. warning:: technically there is nothing to prevent annotations being removed in newer versions. However, + this will break backward compatibility and should thus be avoided unless a rigorous upgrade procedure + is in place to cope with all deployed instances of the class and all serialised versions existing + within vaults. + +Thus, on deserialization, there will be two options to chose from in terms of transformation rules + + #. Determined from the local class and the annotations applied to it (the local copy) + #. Parsed from the AMQP header (the remote copy) + +Which set is used will simply be the largest. + +Renaming Constants +------------------ + +Renamed constants are marked as such with the ``@CordaSerializationTransformRenames`` meta annotation that +wraps a list of ``@CordaSerializationTransformRename`` annotations. Each rename requiring an instance in the +list. + +Each instance must provide the new name of the constant as well as the old. For example, consider the following enumeration: + +.. container:: codeset + + .. sourcecode:: kotlin + + enum class Example { + A, B, C + } + +If we were to rename constant C to D this would be done as follows: + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformRenames ( + CordaSerializationTransformRename("D", "C") + ) + enum class Example { + A, B, D + } + +.. note:: The parameters to the ``CordaSerializationTransformRename`` annotation are defined as 'to' and 'from, + so in the above example it can be read as constant D (given that is how the class now exists) was renamed + from C + +In the case where a single rename has been applied the meta annotation may be omitted. Thus, the following is +functionally identical to the above: + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformRename("D", "C") + enum class Example { + A, B, D + } + +However, as soon as a second rename is made the meta annotation must be used. For example, if at some time later +B is renamed to E: + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformRenames ( + CordaSerializationTransformRename(from = "B", to = "E"), + CordaSerializationTransformRename(from = "C", to = "D") + ) + enum class Example { + A, E, D + } + +Rules +~~~~~ + + #. A constant cannot be renamed to match an existing constant, this is enforced through language constraints + #. A constant cannot be renamed to a value that matches any previous name of any other constant + +If either of these covenants are inadvertently broken, a ``NotSerializableException`` will be thrown on detection +by the serialization engine as soon as they are detected. Normally this will be the first time an object doing +so is serialized. However, in some circumstances, it could be at the point of deserialization. + +Adding Constants +---------------- + +Enumeration constants can be added with the ``@CordaSerializationTransformEnumDefaults`` meta annotation that +wraps a list of ``CordaSerializationTransformEnumDefault`` annotations. For each constant added an annotation +must be included that signifies, on deserialization, which constant value should be used in place of the +serialised property if that value doesn't exist on the version of the class as it exists on the deserializing +node. + +.. container:: codeset + + .. sourcecode:: kotlin + + enum class Example { + A, B, C + } + +If we were to add the constant D + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformEnumDefaults ( + CordaSerializationTransformEnumDefault("D", "C") + ) + enum class Example { + A, B, C, D + } + +.. note:: The parameters to the ``CordaSerializationTransformEnumDefault`` annotation are defined as 'new' and 'old', + so in the above example it can be read as constant D should be treated as constant C if you, the deserializing + node, don't know anything about constant D + +.. note:: Just as with the ``CordaSerializationTransformRename`` transformation if a single transform is being applied + then the meta transform may be omitted. + + .. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformEnumDefault("D", "C") + enum class Example { + A, B, C, D + } + +New constants may default to any other constant older than them, including constants that have also been added +since inception. In this example, having added D (above) we add the constant E and chose to default it to D + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformEnumDefaults ( + CordaSerializationTransformEnumDefault("E", "D"), + CordaSerializationTransformEnumDefault("D", "C") + ) + enum class Example { + A, B, C, D, E + } + +.. note:: Alternatively, we could have decided both new constants should have been defaulted to the first + element + + .. sourcecode:: kotlin + + @CordaSerializationTransformEnumDefaults ( + CordaSerializationTransformEnumDefault("E", "A"), + CordaSerializationTransformEnumDefault("D", "A") + ) + enum class Example { + A, B, C, D, E + } + +When deserializing the most applicable transform will be applied. Continuing the above example, deserializing +nodes could have three distinct views on what the enum Example looks like (annotations omitted for brevity) + +.. container:: codeset + + .. sourcecode:: kotlin + + // The original version of the class. Will deserialize: - + // A -> A + // B -> B + // C -> C + // D -> C + // E -> C + enum class Example { + A, B, C + } + + .. sourcecode:: kotlin + + // The class as it existed after the first addition. Will deserialize: + // A -> A + // B -> B + // C -> C + // D -> D + // E -> D + enum class Example { + A, B, C, D + } + + .. sourcecode:: kotlin + + // The current state of the class. All values will deserialize as themselves + enum class Example { + A, B, C, D, E + } + +Thus, when deserializing a value that has been encoded as E could be set to one of three constants (E, D, and C) +depending on how the deserializing node understands the class. + +Rules +~~~~~ + + #. New constants must be added to the end of the existing list of constants + #. Defaults can only be set to "older" constants, i.e. those to the left of the new constant in the list + #. Constants must never be removed once added + #. New constants can be renamed at a later date using the appropriate annotation + #. When renamed, if a defaulting annotation refers to the old name, it should be left as is + +Combining Evolutions +--------------------- + +Renaming constants and adding constants can be combined over time as a class changes freely. Added constants can +in turn be renamed and everything will continue to be deserializeable. For example, consider the following enum: + +.. container:: codeset + + .. sourcecode:: kotlin + + enum class OngoingExample { A, B, C } + +For the first evolution, two constants are added, D and E, both of which are set to default to C when not present + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformEnumDefaults ( + CordaSerializationTransformEnumDefault("E", "C"), + CordaSerializationTransformEnumDefault("D", "C") + ) + enum class OngoingExample { A, B, C, D, E } + +Then lets assume constant C is renamed to CAT + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformEnumDefaults ( + CordaSerializationTransformEnumDefault("E", "C"), + CordaSerializationTransformEnumDefault("D", "C") + ) + @CordaSerializationTransformRename("C", "CAT") + enum class OngoingExample { A, B, CAT, D, E } + +Note how the first set of modifications still reference C, not CAT. This is as it should be and will +continue to work as expected. + +Subsequently is is fine to add an additional new constant that references the renamed value. + +.. container:: codeset + + .. sourcecode:: kotlin + + @CordaSerializationTransformEnumDefaults ( + CordaSerializationTransformEnumDefault("F", "CAT"), + CordaSerializationTransformEnumDefault("E", "C"), + CordaSerializationTransformEnumDefault("D", "C") + ) + @CordaSerializationTransformRename("C", "CAT") + enum class OngoingExample { A, B, CAT, D, E, F } + +Unsupported Evolutions +---------------------- + +The following evolutions are not currently supports + + #. Removing constants + #. Reordering constants diff --git a/docs/source/serialization.rst b/docs/source/serialization.rst index 1b51f95110..ed5a278feb 100644 --- a/docs/source/serialization.rst +++ b/docs/source/serialization.rst @@ -47,13 +47,12 @@ It's reproduced here as an example of both ways you can do this for a couple of AMQP ==== -.. note:: AMQP serialization is not currently live and will be turned on in a future release. - -The long term goal is to migrate the current serialization format for everything except checkpoints away from the current -``Kryo``-based format to a more sustainable, self-describing and controllable format based on AMQP 1.0. The primary drivers for that move are: +Originally Corda used a ``Kryo``-based serialization scheme throughout for all serialization contexts. However, it was realised there +was a compelling use case for the definition and development of a custom format based upon AMQP 1.0. The primary drivers for this were #. A desire to have a schema describing what has been serialized along-side the actual data: - #. To assist with versioning, both in terms of being able to interpret long ago archived data (e.g. trades from + + #. To assist with versioning, both in terms of being able to interpret long ago archivEd data (e.g. trades from a decade ago, long after the code has changed) and between differing code versions. #. To make it easier to write user interfaces that can navigate the serialized form of data. #. To support cross platform (non-JVM) interaction, where the format of a class file is not so easily interpreted. @@ -65,7 +64,24 @@ The long term goal is to migrate the current serialization format for everything data poked directly into their fields without an opportunity to validate consistency or intercept attempts to manipulate supposed invariants. -Documentation on that format, and how JVM classes are translated to AMQP, will be linked here when it is available. +Delivering this is an ongoing effort by the Corda development team. At present, the ``Kryo``-based format is still used by the RPC framework on +both the client and server side. However, it is planned that this will move to the AMQP framework when ready. + +The AMQP framework is currently used for: + + #. The peer to peer context, representing inter-node communication. + #. The persistence layer, representing contract states persisted into the vault. + +Finally, for the checkpointing of flows Corda will continue to use the existing ``Kryo`` scheme. + +This separation of serialization schemes into different contexts allows us to use the most suitable framework for that context rather than +attempting to force a one size fits all approach. Where ``Kryo`` is more suited to the serialization of a programs stack frames, being more flexible +than our AMQP framework in what it can construct and serialize, that flexibility makes it exceptionally difficult to make secure. Conversly +our AMQP framework allows us to concentrate on a robust a secure framework that can be reasoned about thus made safer with far fewer unforeseen +security holes. + +.. note:: Selection of serialization context should, for the most part, be opaque to CorDapp developers, the Corda framework selecting + the correct context as confugred. .. For information on our choice of AMQP 1.0, see :doc:`amqp-choice`. For detail on how we utilise AMQP 1.0 and represent objects in AMQP types, see :doc:`amqp-format`. @@ -319,14 +335,6 @@ Enums #. All enums are supported, provided they are annotated with ``@CordaSerializable``. -.. warning:: Use of enums in CorDapps requires potentially deeper consideration than in other application environments - due to the challenges of simultaneously upgrading the code on all nodes. It is therefore important to consider the code - evolution perspective, since an older version of the enum code cannot - accommodate a newly added element of the enum in a new version of the enum code. See `Type Evolution`_. Hence, enums are - a good fit for genuinely static data that will *never* change. e.g. Days of the week is not going to be extended any time - soon and is indeed an enum in the Java library. A Buy or Sell indicator is another. However, something like - Trade Type or Currency Code is likely not, since who's to say a new trade type or currency will not come along soon. For - those it is better to choose another representation: perhaps just a string. Exceptions `````````` @@ -363,10 +371,6 @@ Future Enhancements static method responsible for returning the singleton instance. #. Instance internalizing support. We will add support for identifying classes that should be resolved against an instances map to avoid creating many duplicate instances that are equal. Similar to ``String.intern()``. - #. Enum evolution support. We *may* introduce an annotation that can be applied to an enum element to indicate that - if an unrecognised enum entry is deserialized from a newer version of the code, it should be converted to that - element in the older version of the code. This is dependent on identifying a suitable use case, since it does - mutate the data when transported to another node, which could be considered hazardous. .. Type Evolution: @@ -379,3 +383,10 @@ and a version of the current state of the class instantiated. More detail can be found in :doc:`serialization-default-evolution` +Enum Evolution +`````````````` +Corda supports interoperability of enumerated type versions. This allows such types to be changed over time without breaking +backward (or forward) compatibility. The rules and mechanisms for doing this are discussed in :doc:`serialization-enum-evolution`` + + + From 59244276592ba6575829a5e6a47797e9ba8d759c Mon Sep 17 00:00:00 2001 From: Anthony Keenan <34482776+anthonykr3@users.noreply.github.com> Date: Mon, 8 Jan 2018 11:54:41 +0000 Subject: [PATCH 09/10] Make additionalcorddapps property on cordform node internal (#2329) --- .../cordformation/src/main/kotlin/net/corda/plugins/Node.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle-plugins/cordformation/src/main/kotlin/net/corda/plugins/Node.kt b/gradle-plugins/cordformation/src/main/kotlin/net/corda/plugins/Node.kt index bb16a654f8..e5397ad5aa 100644 --- a/gradle-plugins/cordformation/src/main/kotlin/net/corda/plugins/Node.kt +++ b/gradle-plugins/cordformation/src/main/kotlin/net/corda/plugins/Node.kt @@ -28,7 +28,7 @@ class Node(private val project: Project) : CordformNode() { * @note Type is any due to gradle's use of "GStrings" - each value will have "toString" called on it */ var cordapps = mutableListOf() - var additionalCordapps = mutableListOf() + internal var additionalCordapps = mutableListOf() internal lateinit var nodeDir: File private set internal lateinit var rootDir: File From c5149bab9ff91a7217ca3d84a2e1afa5cea020ca Mon Sep 17 00:00:00 2001 From: Rick Parker Date: Mon, 8 Jan 2018 12:11:55 +0000 Subject: [PATCH 10/10] Backport of ENT-1303 applied to 3.0-RC3 (#2332) --- .../net/corda/core/utilities/ByteArrays.kt | 49 ++++++++++++++----- .../OpaqueBytesSubSequenceSerializer.kt | 2 +- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt b/core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt index cf8b1f915d..ecbad03375 100644 --- a/core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt +++ b/core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt @@ -13,8 +13,22 @@ import javax.xml.bind.DatatypeConverter */ @CordaSerializable sealed class ByteSequence : Comparable { + constructor() { + this._bytes = COPY_BYTES + } + /** - * The underlying bytes. + * This constructor allows to bypass calls to [bytes] for functions in this class if the implementation + * of [bytes] makes a copy of the underlying [ByteArray] (as [OpaqueBytes] does for safety). This improves + * performance. It is recommended to use this constructor rather than the default constructor. + */ + constructor(uncopiedBytes: ByteArray) { + this._bytes = uncopiedBytes + } + + /** + * The underlying bytes. Some implementations may choose to make a copy of the underlying [ByteArray] for + * security reasons. For example, [OpaqueBytes]. */ abstract val bytes: ByteArray /** @@ -26,8 +40,11 @@ sealed class ByteSequence : Comparable { */ abstract val offset: Int + private val _bytes: ByteArray + get() = if (field === COPY_BYTES) bytes else field + /** Returns a [ByteArrayInputStream] of the bytes */ - fun open() = ByteArrayInputStream(bytes, offset, size) + fun open() = ByteArrayInputStream(_bytes, offset, size) /** * Create a sub-sequence backed by the same array. @@ -38,19 +55,22 @@ sealed class ByteSequence : Comparable { fun subSequence(offset: Int, size: Int): ByteSequence { require(offset >= 0) require(offset + size <= this.size) + // Intentionally use bytes rather than _bytes, to mirror the copy-or-not behaviour of that property. return if (offset == 0 && size == this.size) this else of(bytes, this.offset + offset, size) } companion object { /** * Construct a [ByteSequence] given a [ByteArray] and optional offset and size, that represents that potentially - * sub-sequence of bytes. The returned implementation is optimised when the whole [ByteArray] is the sequence. + * sub-sequence of bytes. */ @JvmStatic @JvmOverloads fun of(bytes: ByteArray, offset: Int = 0, size: Int = bytes.size): ByteSequence { - return if (offset == 0 && size == bytes.size && size != 0) OpaqueBytes(bytes) else OpaqueBytesSubSequence(bytes, offset, size) + return OpaqueBytesSubSequence(bytes, offset, size) } + + private val COPY_BYTES: ByteArray = ByteArray(0) } /** @@ -65,7 +85,7 @@ sealed class ByteSequence : Comparable { * Copy this sequence, complete with new backing array. This can be helpful to break references to potentially * large backing arrays from small sub-sequences. */ - fun copy(): ByteSequence = of(bytes.copyOfRange(offset, offset + size)) + fun copy(): ByteSequence = of(_bytes.copyOfRange(offset, offset + size)) /** * Compare byte arrays byte by byte. Arrays that are shorter are deemed less than longer arrays if all the bytes @@ -73,10 +93,12 @@ sealed class ByteSequence : Comparable { */ override fun compareTo(other: ByteSequence): Int { val min = minOf(this.size, other.size) + val thisBytes = this._bytes + val otherBytes = other._bytes // Compare min bytes for (index in 0 until min) { - val unsignedThis = java.lang.Byte.toUnsignedInt(this.bytes[this.offset + index]) - val unsignedOther = java.lang.Byte.toUnsignedInt(other.bytes[other.offset + index]) + val unsignedThis = java.lang.Byte.toUnsignedInt(thisBytes[this.offset + index]) + val unsignedOther = java.lang.Byte.toUnsignedInt(otherBytes[other.offset + index]) if (unsignedThis != unsignedOther) { return Integer.signum(unsignedThis - unsignedOther) } @@ -89,7 +111,7 @@ sealed class ByteSequence : Comparable { if (this === other) return true if (other !is ByteSequence) return false if (this.size != other.size) return false - return subArraysEqual(this.bytes, this.offset, this.size, other.bytes, other.offset) + return subArraysEqual(this._bytes, this.offset, this.size, other._bytes, other.offset) } private fun subArraysEqual(a: ByteArray, aOffset: Int, length: Int, b: ByteArray, bOffset: Int): Boolean { @@ -103,14 +125,15 @@ sealed class ByteSequence : Comparable { } override fun hashCode(): Int { + val thisBytes = _bytes var result = 1 for (index in offset until (offset + size)) { - result = 31 * result + bytes[index] + result = 31 * result + thisBytes[index] } return result } - override fun toString(): String = "[${bytes.copyOfRange(offset, offset + size).toHexString()}]" + override fun toString(): String = "[${_bytes.copyOfRange(offset, offset + size).toHexString()}]" } /** @@ -118,7 +141,7 @@ sealed class ByteSequence : Comparable { * In an ideal JVM this would be a value type and be completely overhead free. Project Valhalla is adding such * functionality to Java, but it won't arrive for a few years yet! */ -open class OpaqueBytes(bytes: ByteArray) : ByteSequence() { +open class OpaqueBytes(bytes: ByteArray) : ByteSequence(bytes) { companion object { /** * Create [OpaqueBytes] from a sequence of [Byte] values. @@ -147,7 +170,7 @@ open class OpaqueBytes(bytes: ByteArray) : ByteSequence() { } /** - * Copy [size] bytes from this [ByteArray] starting from [offset] into a new [ByteArray]. + * Wrap [size] bytes from this [ByteArray] starting from [offset] into a new [ByteArray]. */ fun ByteArray.sequence(offset: Int = 0, size: Int = this.size) = ByteSequence.of(this, offset, size) @@ -165,7 +188,7 @@ fun String.parseAsHex(): ByteArray = DatatypeConverter.parseHexBinary(this) /** * Class is public for serialization purposes */ -class OpaqueBytesSubSequence(override val bytes: ByteArray, override val offset: Int, override val size: Int) : ByteSequence() { +class OpaqueBytesSubSequence(override val bytes: ByteArray, override val offset: Int, override val size: Int) : ByteSequence(bytes) { init { require(offset >= 0 && offset < bytes.size) require(size >= 0 && size <= bytes.size) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/OpaqueBytesSubSequenceSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/OpaqueBytesSubSequenceSerializer.kt index 71b683da6f..6744828bcc 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/OpaqueBytesSubSequenceSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/custom/OpaqueBytesSubSequenceSerializer.kt @@ -14,7 +14,7 @@ class OpaqueBytesSubSequenceSerializer(factory: SerializerFactory) : CustomSerializer.Proxy(OpaqueBytesSubSequence::class.java, OpaqueBytes::class.java, factory) { override val additionalSerializers: Iterable> = emptyList() - override fun toProxy(obj: OpaqueBytesSubSequence): OpaqueBytes = obj.copy() as OpaqueBytes + override fun toProxy(obj: OpaqueBytesSubSequence): OpaqueBytes = OpaqueBytes(obj.copy().bytes) override fun fromProxy(proxy: OpaqueBytes): OpaqueBytesSubSequence = OpaqueBytesSubSequence(proxy.bytes, proxy.offset, proxy.size) } \ No newline at end of file