Merge pull request #2312 from corda/kat/feature/betterPrivateMemberErrMsg

CORDA-882 - Better err messages when serializer encounters private property

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

@@ -1,9 +1,9 @@
 package net.corda.nodeapi.internal.serialization.amqp
 
-import net.corda.core.serialization.ClassWhitelist
-import net.corda.core.serialization.CordaSerializable
 import com.google.common.primitives.Primitives
 import com.google.common.reflect.TypeToken
+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
@@ -81,10 +81,17 @@ private fun <T : Any> propertiesForSerializationFromConstructor(kotlinConstructo
     val rc: MutableList<PropertySerializer> = ArrayList(kotlinConstructor.parameters.size)
     for (param in kotlinConstructor.parameters) {
         val name = param.name ?: throw NotSerializableException("Constructor parameter of $clazz has no name.")
+
         val matchingProperty = properties[name] ?:
-                throw NotSerializableException("No property matching constructor parameter named '$name' of '$clazz'. " +
-                        "If using Java, check that you have the -parameters option specified in the Java compiler. " +
-                        "Alternately, provide a proxy serializer (SerializationCustomSerializer) if recompiling isn't an option")
+                try {
+                    clazz.getDeclaredField(param.name)
+                    throw NotSerializableException("Property '$name' or it's getter is non public, this renders class '$clazz' unserializable")
+                } catch (e: NoSuchFieldException) {
+                    throw NotSerializableException("No property matching constructor parameter named '$name' of '$clazz'. " +
+                            "If using Java, check that you have the -parameters option specified in the Java compiler. " +
+                            "Alternately, provide a proxy serializer (SerializationCustomSerializer) if recompiling isn't an option")
+                }
+
         // Check that the method has a getter in java.
         val getter = matchingProperty.readMethod ?: throw NotSerializableException("Property has no getter method for $name of $clazz. " +
                 "If using Java and the parameter name looks anonymous, check that you have the -parameters option specified in the Java compiler." +

node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/ErrorMessageTests.java

@@ -0,0 +1,40 @@
+package net.corda.nodeapi.internal.serialization.amqp;
+
+import net.corda.nodeapi.internal.serialization.AllWhitelist;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+import java.io.NotSerializableException;
+
+public class ErrorMessageTests {
+    private String errMsg(String property, String testname) {
+        return "Property '"
+                + property
+                + "' or it's getter is non public, this renders class 'class "
+                + testname
+                + "$C' unserializable -> class "
+                + testname
+                + "$C";
+    }
+
+    static class C {
+        public Integer a;
+
+        public C(Integer a) {
+            this.a = a;
+        }
+
+        private Integer getA() { return this.a; }
+    }
+
+    @Test
+    public void testJavaConstructorAnnotations() {
+        SerializerFactory factory1 = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader());
+        SerializationOutput ser = new SerializationOutput(factory1);
+
+        Assertions.assertThatThrownBy(() -> ser.serialize(new C(1)))
+                .isInstanceOf(NotSerializableException.class)
+                .hasMessage(errMsg("a", getClass().getName()));
+    }
+
+}

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

@@ -0,0 +1,69 @@
+package net.corda.nodeapi.internal.serialization.amqp
+
+import org.assertj.core.api.Assertions
+import org.junit.Test
+import java.io.NotSerializableException
+
+class ErrorMessagesTests {
+    companion object {
+        val VERBOSE get() = false
+    }
+
+    private fun errMsg(property:String, testname: String) =
+            "Property '$property' or it's getter is non public, this renders class 'class $testname\$C' unserializable -> class $testname\$C"
+
+    @Test
+    fun privateProperty() {
+        data class C(private val a: Int)
+
+        val sf = testDefaultFactory()
+
+        val testname = "${javaClass.name}\$${testName()}"
+
+        Assertions.assertThatThrownBy {
+            TestSerializationOutput(VERBOSE, sf).serialize(C(1))
+        }.isInstanceOf(NotSerializableException::class.java).hasMessage(errMsg("a", testname))
+    }
+
+    @Test
+    fun privateProperty2() {
+        data class C(val a: Int, private val b: Int)
+
+        val sf = testDefaultFactory()
+
+        val testname = "${javaClass.name}\$${testName()}"
+
+        Assertions.assertThatThrownBy {
+            TestSerializationOutput(VERBOSE, sf).serialize(C(1, 2))
+        }.isInstanceOf(NotSerializableException::class.java).hasMessage(errMsg("b", testname))
+    }
+
+    @Test
+    fun privateProperty3() {
+        // despite b being private, the getter we've added is public and thus allows for the serialisation
+        // of the object
+        data class C(val a: Int, private val b: Int) {
+            public fun getB() = b
+        }
+
+        val sf = testDefaultFactory()
+
+        val testname = "${javaClass.name}\$${testName()}"
+
+        val bytes = TestSerializationOutput(VERBOSE, sf).serialize(C(1, 2))
+        val c = DeserializationInput(sf).deserialize(bytes)
+    }
+
+    @Test
+    fun protectedProperty() {
+        data class C(protected val a: Int)
+
+        val sf = testDefaultFactory()
+
+        val testname = "${javaClass.name}\$${testName()}"
+
+        Assertions.assertThatThrownBy {
+            TestSerializationOutput(VERBOSE, sf).serialize(C(1))
+        }.isInstanceOf(NotSerializableException::class.java).hasMessage(errMsg("a", testname))
+    }
+}