Property type checking is overly strict

Description

When scanning the properties of a class during type analysis for serialisation, the types of getter and setter methods, and of backing fields, are checked to make sure that they line up. A getter should return a type that is assignable from the type of the corresponding backing field (if there is one), and a setter should receive a type that is assignable to the type returned by the corresponding getter.

A bug in the implementation of this checking meant that in the case of the getter/backing field pair, the return type of the getter was checked against itself, rather than against the type of the backing field. This would ordinarily have no effect (besides making the check redundant), since we would expect a type always to be assignable from itself. However, some versions of Guava prior to 25.1-jre erroneously return false when asked to compare TypeToken.of(type).isSuperclassOf(TypeToken.of(type)) for some types. This has caused the check to fail on TraversableType.commands, which has the type List<Command<*>>, when an earlier version of Guava is in scope - an error which looks like:

This has been reported on StackOverflow: https://stackoverflow.com/questions/55400626/corda-4-0-transaction-serialization-issue/55444039#55444039

A related issue occurred for B3i when attempting to serialize a class with a mutable field of type Set<T>, e.g.

Kotlin generates a getter for property which returns Set<T>, but a setter which accepts Set<? extends T>, which is correct behaviour for an immutable set. However, Guava does not recognise Set<T> as a superclass of Set<? extends T>, so the validation check on the getter/setter pair fails.

This can be worked around with an annotation which instructs Kotlin not to generate the wildcard on the setter:

However, it would be better not to enforce an overly-strict compatibility rule in the first place.

The validation checks on field/getter/setter types were introduced in Corda v4, and have caused issues for clients which they did not experience with Corda v3. We should do one of the following:

  • Remove them altogether, restoring the previous status quo.

  • Keep them, but ensure that they are correct for all cases (but note we have already encountered cases, such as the one above, which we did not anticipate and for which we had not tests in place).

  • Keep them but make them more lenient, e.g. checking only the assignability of erased types, and perhaps writing a warning to the logs rather than blowing up with an exception.

Status

Assignee

Dominic.Fox@r3.com

Reporter

Dominic.Fox@r3.com

Labels

None

Priority

High

Fix versions

Ported to...

None

Feature Team

Select team

CVSS Vector

None

Severity

High

Affects versions

Configure