CORDA-2860 relax property type checking (#5028)

* CORDA-2860 relax property type checking

* Remove redundant check

* Unit test

* Comment linking ticket to test

* More descriptive comment

* Fix test that was broken by jacocoData field being silently added to class

* Revert to previous behaviour around opaques, suppress validation
This commit is contained in:
Dominic Fox 2019-04-17 17:14:24 +01:00 committed by Anthony Keenan
parent 33e9f125db
commit a416d5025f
4 changed files with 48 additions and 20 deletions

View File

@ -4,6 +4,7 @@ import com.google.common.reflect.TypeToken
import net.corda.core.KeepForDJVM
import net.corda.core.internal.isPublic
import net.corda.core.serialization.SerializableCalculatedProperty
import net.corda.core.utilities.contextLogger
import net.corda.serialization.internal.amqp.MethodClassifier.*
import java.lang.reflect.Field
import java.lang.reflect.Method
@ -32,21 +33,20 @@ data class PropertyDescriptor(val field: Field?, val setter: Method?, val getter
*/
fun validate() {
getter?.apply {
val getterType = genericReturnType
field?.apply {
if (!getterType.isSupertypeOf(genericReturnType))
if (!getter.returnType.boxesOrIsAssignableFrom(field.type))
throw AMQPNotSerializableException(
declaringClass,
"Defined getter for parameter $name returns type $getterType " +
"Defined getter for parameter $name returns type ${getter.returnType} " +
"yet underlying type is $genericType")
}
}
setter?.apply {
val setterType = genericParameterTypes[0]!!
val setterType = setter.parameterTypes[0]!!
field?.apply {
if (!genericType.isSupertypeOf(setterType))
if (!field.type.boxesOrIsAssignableFrom(setterType))
throw AMQPNotSerializableException(
declaringClass,
"Defined setter for parameter $name takes parameter of type $setterType " +
@ -54,7 +54,7 @@ data class PropertyDescriptor(val field: Field?, val setter: Method?, val getter
}
getter?.apply {
if (!genericReturnType.isSupertypeOf(setterType))
if (!getter.returnType.boxesOrIsAssignableFrom(setterType))
throw AMQPNotSerializableException(
declaringClass,
"Defined setter for parameter $name takes parameter of type $setterType, " +
@ -64,6 +64,9 @@ data class PropertyDescriptor(val field: Field?, val setter: Method?, val getter
}
}
private fun Class<*>.boxesOrIsAssignableFrom(other: Class<*>) =
isAssignableFrom(other) || kotlin.javaPrimitiveType == other
private fun Type.isSupertypeOf(that: Type) = TypeToken.of(this).isSupertypeOf(that)
// match an uppercase letter that also has a corresponding lower case equivalent
@ -85,7 +88,7 @@ private val propertyMethodRegex = Regex("(?<type>get|set|is)(?<var>\\p{Lu}.*)")
* take a single parameter of a type compatible with exampleProperty and isExampleProperty must
* return a boolean
*/
internal fun Class<out Any?>.propertyDescriptors(): Map<String, PropertyDescriptor> {
internal fun Class<out Any?>.propertyDescriptors(warnInvalid: Boolean = true): Map<String, PropertyDescriptor> {
val fieldProperties = superclassChain().declaredFields().byFieldName()
return superclassChain().declaredMethods()
@ -93,8 +96,9 @@ internal fun Class<out Any?>.propertyDescriptors(): Map<String, PropertyDescript
.thatArePropertyMethods()
.withValidSignature()
.byNameAndClassifier(fieldProperties.keys)
.toClassProperties(fieldProperties)
.validated()
.toClassProperties(fieldProperties).run {
if (warnInvalid) validated() else this
}
}
/**

View File

@ -283,7 +283,7 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
}
private fun buildReadOnlyProperties(rawType: Class<*>): Map<PropertyName, LocalPropertyInformation> =
rawType.propertyDescriptors().asSequence().mapNotNull { (name, descriptor) ->
rawType.propertyDescriptors(warnIfNonComposable).asSequence().mapNotNull { (name, descriptor) ->
if (descriptor.field == null || descriptor.getter == null) null
else {
val paramType = (descriptor.getter.genericReturnType).resolveAgainstContext()
@ -310,7 +310,7 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
parameter.name to index
}.toMap()
return rawType.propertyDescriptors().asSequence().mapNotNull { (name, descriptor) ->
return rawType.propertyDescriptors(warnIfNonComposable).asSequence().mapNotNull { (name, descriptor) ->
val normalisedName = when {
name in constructorParameterIndices -> name
name.decapitalize() in constructorParameterIndices -> name.decapitalize()
@ -353,7 +353,7 @@ internal data class LocalTypeInformationBuilder(val lookup: LocalTypeLookup,
}
private fun getterSetterProperties(rawType: Class<*>): Sequence<Pair<String, LocalPropertyInformation>> =
rawType.propertyDescriptors().asSequence().mapNotNull { (name, descriptor) ->
rawType.propertyDescriptors(warnIfNonComposable).asSequence().mapNotNull { (name, descriptor) ->
if (descriptor.getter == null || descriptor.setter == null || descriptor.field == null) null
else {
val paramType = descriptor.getter.genericReturnType

View File

@ -6,14 +6,15 @@ import net.corda.core.serialization.deserialize
import net.corda.core.serialization.serialize
import net.corda.node.serialization.kryo.kryoMagic
import net.corda.node.services.statemachine.DataSessionMessage
import net.corda.serialization.internal.amqp.propertyDescriptors
import net.corda.testing.core.SerializationEnvironmentRule
import net.corda.testing.internal.kryoSpecific
import org.junit.Assert.assertArrayEquals
import org.junit.Assert.assertEquals
import org.junit.Assert.*
import org.junit.Rule
import org.junit.Test
import java.io.ByteArrayOutputStream
import java.util.*
import org.assertj.core.api.Assertions.assertThat
class SetsSerializationTest {
private companion object {
@ -64,4 +65,27 @@ class SetsSerializationTest {
}
assertArrayEquals(output.toByteArray(), serializedForm.bytes)
}
open class P
class VarOfP(var p: Set<P>)
/*
See CORDA-2860.
When a class has a var parameter of type Set<out T>, Kotlin generates getters and setters with the following (Java) signatures:
public Set<T> getP();
public void setP(Set<? extends T> p);
The PropertyDescriptor.validate method used to check that the return type of the getter was a supertype of the parameter type of the
setter. Unfortunately, Set<T> is not a strict supertype of Set<? extends T>, so this check would fail, throwing an exception.
We now check only for compatibility of the erased classes, so the call to propertyDescriptors() below should now succeed, returning the
property descriptor for "p".
*/
@Test
fun `type variance on setter getter pair does not fail validation`() {
assertThat(VarOfP::class.java.propertyDescriptors()).containsKey("p")
}
}

View File

@ -40,7 +40,7 @@ class LocalTypeModelTests {
@Test
fun `Primitives and collections`() {
assertInformation<CollectionHolder<UUID, LocalDateTime>>("CollectionHolder<UUID, LocalDateTime>")
assertInformation<CollectionHolder<UUID, String>>("CollectionHolder<UUID, String>")
assertInformation<StringKeyedCollectionHolder<Int>>("""
StringKeyedCollectionHolder<Integer>(list: List<Integer>, map: Map<String, Integer>, array: List<Integer>[]): CollectionHolder<String, Integer>
@ -85,9 +85,9 @@ class LocalTypeModelTests {
fun `interfaces and superclasses`() {
assertInformation<SuperSuper<Int, Int>>("SuperSuper<Integer, Integer>")
assertInformation<Super<UUID>>("Super<UUID>: SuperSuper<UUID, Double>")
assertInformation<Abstract<LocalDateTime>>("""
Abstract<LocalDateTime>: Super<LocalDateTime[]>, SuperSuper<LocalDateTime[], Double>
a: LocalDateTime[]
assertInformation<Abstract<String>>("""
Abstract<String>: Super<String[]>, SuperSuper<String[], Double>
a: String[]
b: Double
""")
assertInformation<Concrete>("""