CORDA-553 - Review comments

docs/source/release-notes.rst

@@ -6,10 +6,11 @@ Here are release notes for each snapshot release from M9 onwards.
 Unreleased
 ----------
 
-** Enum Class Evolution
+* **Enum Class Evolution**
-   With the addition of AMQP serialization Corda now supports enum constant evolution, that is the abilit to alter
+  With the addition of AMQP serialization Corda now supports enum constant evolution.
-   an enum constant and, as long as certain rules ar followed and the correct annotations applied, have older and
+
-   newer instances of that enumeration be understood.
+  That is the ability to alter an enum constant and, as long as certain rules are followed and the correct
+  annotations applied, have older and newer instances of that enumeration be understood.
 
 Release 2.0
 ----------

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolutionSerializer.kt

@@ -1,33 +1,33 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
+import net.corda.core.internal.uncheckedCast
 import org.apache.qpid.proton.amqp.Symbol
 import org.apache.qpid.proton.codec.Data
 import java.io.NotSerializableException
+import java.lang.UnsupportedOperationException
 import java.lang.reflect.Type
-import java.util.*
 
 /**
- * @property clazz The enum as it exists now, not as it did when it was seialised (either in the past
+ * @property clazz The enum as it exists now, not as it did when it was serialized (either in the past
  * or future).
  * @property factory the [SerializerFactory] that is building this serialization object.
- * @property conversions A mapping between all potential enum constants that could've been assigned as
+ * @property conversions A mapping between all potential enum constants that could've been assigned to
- * existed at serialisation and those that exist now
+ * an instance of the enum as it existed at serialisation and those that exist now
  * @property ordinals Convenience mapping of constant to ordinality
- *
  */
 class EnumEvolutionSerializer(
         clazz: Type,
         factory: SerializerFactory,
-        private val conversions : Map<String, String>,
+        private val conversions: Map<String, String>,
-        private val ordinals : Map<String, Int>) : AMQPSerializer<Any> {
+        private val ordinals: Map<String, Int>) : AMQPSerializer<Any> {
     override val type: Type = clazz
     override val typeDescriptor = Symbol.valueOf("$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}")!!
 
     companion object {
-        private fun MutableMap<String, String>.mapInPlace(f : (String)->String) {
+        private fun MutableMap<String, String>.mapInPlace(f: (String) -> String) {
-            val i = this.iterator()
+            val i = iterator()
-            while(i.hasNext()) {
+            while (i.hasNext()) {
-                val curr = (i.next())
+                val curr = i.next()
                 curr.setValue(f(curr.value))
             }
         }
@@ -35,7 +35,7 @@ class EnumEvolutionSerializer(
         /**
          * Builds an Enum Evolver serializer.
          *
-         * @param old The description of the enum as it existed at the time of serialisation take from the
+         * @param old The description of the enum as it existed at the time of serialisation taken from the
          * received AMQP header
          * @param new The Serializer object we built based on the current state of the enum class on our classpath
          * @param factory the [SerializerFactory] that is building this serialization object.
@@ -53,21 +53,19 @@ class EnumEvolutionSerializer(
 
             // if either of these isn't of the cast type then something has gone terribly wrong
             // elsewhere in the code
-            @Suppress("UNCHECKED_CAST")
+            val defaultRules: List<EnumDefaultSchemaTransform>? = uncheckedCast(transforms[TransformTypes.EnumDefault])
-            val defaultRules = transforms[TransformTypes.EnumDefault] as? List<EnumDefaultSchemaTransform>
+            val renameRules: List<RenameSchemaTransform>? = uncheckedCast(transforms[TransformTypes.Rename])
-            @Suppress("UNCHECKED_CAST")
-            val renameRules = transforms[TransformTypes.Rename] as? List<RenameSchemaTransform>
 
             // What values exist on the enum as it exists on the class path
             val localValues = new.type.asClass()!!.enumConstants.map { it.toString() }
 
-            val conversions : MutableMap<String, String> = new.type.asClass()!!.enumConstants.map { it.toString() }
+            val conversions: MutableMap<String, String> = localValues
                     .union(defaultRules?.map { it.new }?.toSet() ?: emptySet())
                     .union(renameRules?.map { it.to } ?: emptySet())
                     .associateBy({ it }, { it })
                     .toMutableMap()
 
-            val rules : MutableMap<String, String> = mutableMapOf()
+            val rules: MutableMap<String, String> = mutableMapOf()
             rules.putAll(defaultRules?.associateBy({ it.new }, { it.old }) ?: emptyMap())
             rules.putAll(renameRules?.associateBy({ it.to }, { it.from }) ?: emptyMap())
 
@@ -79,7 +77,7 @@ class EnumEvolutionSerializer(
             // rather tricky when you don't have access to the actual type, so this is a nice way to be able
             // to precompute and pass to the actual object
             var idx = 0
-            return EnumEvolutionSerializer(new.type, factory, conversions, localValues.associateBy( {it}, { idx++ }))
+            return EnumEvolutionSerializer(new.type, factory, conversions, localValues.associateBy({ it }, { idx++ }))
         }
     }
 
@@ -87,17 +85,17 @@ class EnumEvolutionSerializer(
         val enumName = (obj as List<*>)[0] as String
 
         if (enumName !in conversions) {
-            throw NotSerializableException ("No rule to evolve enum constant $type::$enumName")
+            throw NotSerializableException("No rule to evolve enum constant $type::$enumName")
         }
 
         return type.asClass()!!.enumConstants[ordinals[conversions[enumName]]!!]
     }
 
     override fun writeClassInfo(output: SerializationOutput) {
-        throw IllegalAccessException("It should be impossible to write an evolution serializer")
+        throw UnsupportedOperationException("It should be impossible to write an evolution serializer")
     }
 
     override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput) {
-        throw IllegalAccessException("It should be impossible to write an evolution serializer")
+        throw UnsupportedOperationException("It should be impossible to write an evolution serializer")
     }
 }

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/EvolutionSerializer.kt

@@ -109,7 +109,7 @@ class EvolutionSerializer(
     }
 
     override fun writeObject(obj: Any, data: Data, type: Type, output: SerializationOutput) {
-        throw IllegalAccessException("It should be impossible to write an evolution serializer")
+        throw UnsupportedOperationException("It should be impossible to write an evolution serializer")
     }
 
     /**

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TansformTypes.kt

@@ -1,5 +1,6 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
+import net.corda.core.internal.uncheckedCast
 import net.corda.core.serialization.CordaSerializationTransformEnumDefault
 import net.corda.core.serialization.CordaSerializationTransformEnumDefaults
 import net.corda.core.serialization.CordaSerializationTransformRename
@@ -43,7 +44,7 @@ enum class TransformTypes(val build: (Annotation) -> Transform) : DescribedType
          * @param constants The list of enum constants on the type the transforms are being applied to
          */
         override fun validate(l : List<Transform>, constants: Set<String>) {
-            @Suppress("UNCHECKED_CAST") (l as List<EnumDefaultSchemaTransform>).forEach {
+            uncheckedCast<List<Transform>, List<EnumDefaultSchemaTransform>>(l).forEach {
                 if (!constants.contains(it.old)) {
                     throw NotSerializableException(
                             "Enum extension defaults must be to a valid constant: ${it.new} -> ${it.old}. ${it.old} " +

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/TransformsSchema.kt

@@ -146,10 +146,8 @@ class EnumDefaultSchemaTransform(val old: String, val new: String) : Transform()
 /**
  * Transform applied to either a class or enum where a property is renamed
  *
- * @property from the name at time of change of the property
+ * @property from the name of the property or constant prior to being changed, i.e. what it was
- * @property to the new name of the property
+ * @property to the new name of the property or constant after the change has been made, i.e. what it is now
- *
- *
  */
 class RenameSchemaTransform(val from: String, val to: String) : Transform() {
     companion object : DescribedTypeConstructor<RenameSchemaTransform> {

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolveTests.kt

@@ -27,7 +27,7 @@ class EnumEvolveTests {
 
     @Test
     fun deserialiseNewerSetToUnknown() {
-        val resource = "${this.javaClass.simpleName}.${testName()}"
+        val resource = "${javaClass.simpleName}.${testName()}"
         val sf = testDefaultFactory()
 
         data class C (val e : DeserializeNewerSetToUnknown)
@@ -55,7 +55,7 @@ class EnumEvolveTests {
 
     @Test
     fun deserialiseNewerSetToUnknown2() {
-        val resource = "${this.javaClass.simpleName}.${testName()}"
+        val resource = "${javaClass.simpleName}.${testName()}"
         val sf = testDefaultFactory()
 
         data class C(val e: DeserializeNewerSetToUnknown2)
@@ -95,7 +95,7 @@ class EnumEvolveTests {
     // Lets test to see if they forgot to provide an upgrade rule
     @Test
     fun deserialiseNewerWithNoRule() {
-        val resource = "${this.javaClass.simpleName}.${testName()}"
+        val resource = "${javaClass.simpleName}.${testName()}"
         val sf = testDefaultFactory()
 
         data class C(val e: DeserializeNewerWithNoRule)
@@ -143,7 +143,7 @@ class EnumEvolveTests {
 
     @Test
     fun deserializeWithRename() {
-        val resource = "${this.javaClass.simpleName}.${testName()}"
+        val resource = "${javaClass.simpleName}.${testName()}"
         val sf = testDefaultFactory()
 
         data class C(val e: DeserializeWithRename)
@@ -265,7 +265,7 @@ class EnumEvolveTests {
 
     @Test
     fun multiOperations() {
-        val resource = "${this.javaClass.simpleName}.${testName()}"
+        val resource = "${javaClass.simpleName}.${testName()}"
         val sf = testDefaultFactory()
 
         data class C(val e: MultiOperations)