Some review comments on records support #1
Conversation
ChrisHegarty
commented
Jun 30, 2020
ChrisHegarty
commented
- Update pom support to be Java 14+
- Store Method references to avoid subsequent lookup
- All test records should be public
1) Update pom support to be Java 14+ 2) Store Method references to avoid subsequent lookup 3) All test records should be public
| <source>14</source> | ||
| <target>14</target> | ||
| <!-- Enable Java 14+ for all sources so that Intellij picks the right language level --> | ||
| <release>${java.vm.specification.version}</release> |
There was a problem hiding this comment.
Using this property, in my setup Intellij does not pickup the correct version for language level support
Would putting 14 be acceptable?
There was a problem hiding this comment.
It works fine for me with maven, but i have not tried with Intellij.
The reason I proposed this change is so that the release version will work with Java versions greater than 14, say e.g. 15 and beyond. java.vm.specification.version is a standard Java SE system property, and Maven exposes all properties from java.lang.System, so it is surprising that it is not working in Intellij. It's ok to drop this, for now. We can figure out if/how we want the version to roll later, if needed.
| @@ -52,7 +52,7 @@ public void testDeserializeSimpleRecord_DisableAnnotationIntrospector() throws I | |||
| assertEquals(new SimpleRecord(123, "Bob"), value); | |||
| } | |||
|
|
|||
| record RecordOfRecord(SimpleRecord record) { | |||
| public record RecordOfRecord(SimpleRecord record) { | |||
There was a problem hiding this comment.
All test records should be public
Why? (just for me to understand :))
There was a problem hiding this comment.
In Java 14 a record's canonical constructor is always public ( it has the public modifier ).
In Java 15 (and beyond) a record's canonical constructor inherits the access control modifier from that of the record declaration.
So without the change to make the record declarations public, then in Java 15 the canonical constructor is package-private, and by default Jackson cannot locale it. Now, this could be an issue or change with the default deserialisation policy in the version of the latest branch that I tested against.
There was a problem hiding this comment.
We might want to try access of non-public Records too, for parity with POJOs. Although I don't know if it is possible to configure access for tests in a way that this is actually allowed (I assume module-info declarations are needed?).
There was a problem hiding this comment.
Adding tests to check for non-public Records would be good ( we have such in the JDK serialization unit tests ).
When running the existing RecordTest in my local environment with a JDK 15 early-access build I did see failures - since the record's canonical constructor was not public (it was package-private inherited from the record declaration). These failures may be because I was using the tip of jackson-databind where the default visibility of creator differs? Or maybe because of not explicitly setting the visibility checker in the test? I'm not sure. Either way, it should be possible to write tests where the record (and it's canonical constructor) are not accessible to Jackson ( package-private ), and ensure that whatever deserialization/visibility configuration work as expected.
| <activation> | ||
| <jdk>14</jdk> | ||
| <jdk>[14,</jdk> |
| private static final String RECORD_COMPONENT_CLASS_NAME = "java.lang.reflect.RecordComponent"; | ||
| private static final String RECORD_COMPONENT_GET_NAME = "getName"; | ||
| private static final String RECORD_COMPONENT_GET_TYPE = "getType"; | ||
| static { |
There was a problem hiding this comment.
👍 and it has been suggested by @cowtowncoder as well
|
I have changes in the works to replace the user of core reflection, in RecordUtils, with Method Handles. I'm still verifying the changes, but in general the use of method handles will be more performant than that of core reflection. Again, this should not be a blocker, and can be considered separately, on its own merit. |
|
Use of MethodHandle may make sense and can be done (added in JDK7, right?), assuming they are usable on Android too. But for actual property access |
