Support for Java 14 records (#2709)

[youribonnaffe]

First attempt at supporting Java 14 records (JEP 359).

Records are simple DTO/POJO objects with final fields (components) and accessors.
Record's components are automatically serialized and the canonical constructor is
used for deserialization.

Implementation is still compatible with Java 8 and uses a bit of reflection to
access record's components. However the unit tests now require a JDK 14 to run.

The basic idea is to make record's components discovered as properties (similar to
beans having getters) and to make the canonical constructor accessible via implicit
parameter names.

---

(added by @cowtowncoder from discussions)

Relevant spec wrt naming, access:

https://cr.openjdk.java.net/~chegar/records/spec/records-serialization.07.html

[youribonnaffe]

I have a limited knowledge of the existing code base so I'm not sure the places I added specific behavior to handle recors are appropriate. It did not feel like a custom serializer/deserializer would be appropriate as annotations can be used. Also did not feel like it should be done in the annotation introspector (as I did in https://gist.github.com/youribonnaffe/03176be516c0ed06828ccc7d6c1724ce) as this feature could work with MapperFeature.USE_ANNOTATIONS=false). So I tried to find a place in between.

The changes should also be carefully reviewed for performance impacts. There are perhaps optimizations or better ways to use reflection or other things.

I'll also try to add more test cases, ideas and corner cases are welcome!

[youribonnaffe]

Also is there an easy way to test all Jackson's modules to check for regressions?

[GedMarc]

@youribonnaffe Check out - https://github.com/FasterXML/jackson-jdk11-compat-test
which tests modular structures and builds JLinks -

I can add one in there for a jdk 14 test for you, This is a pretty NB one for me, especially as September creeps up.

[cowtowncoder]

@youribonnaffe Excellent start, and good questions. Initially the main challenge from my perspective is that of how to test with JDK 14, as I do not have plans to change my dev platform yet to require JDK 14 for building.

But there are options to make that part pluggable (with profile, maybe?), or from different repo -- that is, implementation is in databind, but actual JDK 14 based tests in another repo. Initially I could run it manually but there should be a way to automate it with Travis to either trigger on build of databind itself or if not practical, with cron-style "run once per day" thing.

Such an approach would be very useful for other components too as right now there is no automated triggering of downstream dependencies, so sometimes breaks between components (most of the time just test fails [tests being bit fragile], but quite frequently also necessary changes to other components).

[youribonnaffe]

I thought too about moving the tests out in a different repository to avoid the JDK14 constraints. Or it could be a different test source set maybe, kind of like integration tests are sometimes in a different directory.

@GedMarc if I understand correctly the JDK11 compat test has its own tests but it is not running all existing tests against JDK 11 right?

[GedMarc]

@youribonnaffe correct - because the tests were intended to check JLink compatibility with builds, and everything jdk 9 and up, there are dependencies that aren't updated yet to support it,

I think it's a good location for it, i was gonna do the travis builds for the jlink ones there in anycase xD

[youribonnaffe]

Reworked the Maven file to introduce a JDK14 profile (activated if JDK14 is available) with a separated source folder that contains tests using records. This way it is still possible to build with JDK < 14. A dedicated module would probably be cleaner but here we have all the changes in a single PR so it makes it easier to review.

[ChrisHegarty]

@youribonnaffe You may want to store the j.l.rMethods, so as to avoid looking them up for each record, similar to https://gist.github.com/ChrisHegarty/0879f83c1379db7c7e46f83a9337b1aa

[ChrisHegarty]

@youribonnaffe maybe you could update the pom to enable records for Java 14+ ( since Java is soon on the way! )

index 82fb9ff33..9c845d8e0 100644
--- a/pom.xml
+++ b/pom.xml
@@ -215,7 +215,7 @@
       <!-- Build Record tests using Java 14 if JDK is available -->
       <id>java14</id>
       <activation>
-        <jdk>14</jdk>
+        <jdk>[14,</jdk>
       </activation>
       <build>
         <plugins>
@@ -244,8 +244,7 @@
             <configuration>
               <optimize>true</optimize>
               <!-- Enable Java 14 for all sources so that Intellij picks the right language level -->
-              <source>14</source>
-              <target>14</target>
+              <release>${java.vm.specification.version}</release>
               <compilerArgs>
                 <arg>-parameters</arg>
                 <arg>--enable-preview</arg>```
 

[youribonnaffe]

Thanks @ChrisHegarty, your suggestions make sense. Would you like to open a PR against mine so we keep track of your contributions?

[ChrisHegarty]

Thanks @ChrisHegarty, your suggestions make sense. Would you like to open a PR against mine so we keep track of your contributions?

Sure, I'm on the tip of jackson-databind (or master branch) . Your java-14-records branch seems a little behind. Can you update it?

[ChrisHegarty]

We should also make the records in the tests explicitly public, rather than package-private (default). Will add the to the PR too.

[ChrisHegarty]

@youribonnaffe I created the following PR for your java-14-records branch:
youribonnaffe#1

If this is not the correct process, or it would be more straightforward to do something else just let me know.

[cowtowncoder]

Definitely needs javadocs to explain why is this needed.

[cowtowncoder]

Should be cached so that this lookup is not executed for every access, but I guess that can be done as a post-cleanup.

[cowtowncoder]

I think this makes sense as an approach. May want to rebase against 2.12 however, as 3.0 (master) is still long ways out.

Aside from tweaks to efficiency wrt RecordUtil, I think I would go for bit bigger change for creator discovery: if Class is Record, I would just create completely different discovery process (branch to different logic for Record type), in which finding the canonical constructor-backed creator can be more easily located and much of complexity avoided. I assume users can customize aspects so that some/most annotation detection is needed (for @JacksonInject, renaming?), but at least selection criteria is simple and need not be reconciled with general POJO approach.
This would avoid adding more complexity in main path (which is rather messy at this point, due for rewrite... but not right now)

[cowtowncoder]

Ok one immediate suggestion: would it be possible to create a new PR with subset of changes:

1. PR targets 2.12 branch
2. Adds only unit tests(s) and pom.xml changes needed to run tests, but no other changes

I could merge that in quickly and it would also allow addition of other JDK14+ tests; and give more time figuring out actual implementation.

[lpandzic]

Parameter names module already works with records, what is the reason why we need explicit support for records?

[ChrisHegarty]

@lpandzic A record has a private final field for each component of that in its component declaration, as well as a public accessor of the same name. The Jackson implementation can avoid the need to suppress java language access control checks if it retrieves the component values through the accessor, rather than scraping the private field.

As of time-of-writing, the unreleased Java 15, records are only instantiable through their canonical constructor. A record's fields are immutable - not mutable even with suitably privileged reflective code. Jackson should create record instances through their canonical constructor - which is being proposed here. This PR is certainly going in the right direction, with respect to the longterm view of record serialization.

[lpandzic]

Yes I know and understand that, @ChrisHegarty . https://github.com/FasterXML/jackson-module-parameter-names exists since ~2014 and solves that problem for immutable regular classes that only have constructor and accessors without resorting to field, method or constructor access overrides. So isn't this basically just reinventing the wheel?

[ChrisHegarty]

@lpandzic Ok, I have to admit that I'm not overly familiar with this code, so maybe what you are suggesting makes perfect sense. I am not yet sure.

Just to clarify my previous point; With records, there is a clear and specified way (in Java Language Specification and the Java Platform) to interact with their state, accessors and canonical constructor. The addition of a canonical constructor is new and there is a systematic way to locate it - based on the record component types, names, and order. All of which is reified in the class file. So if the point is that changes in Jackson should be done elsewhere, then that may be fine. But if the point is that no changes in Jackson are required at all to support records, then I would have to disagree with that.

[youribonnaffe]

Thanks for all the comments, nice to see interest on this topic 😄

@cowtowncoder I created a new PR as suggested against 2.12 with the tests only: #2781 (and I included @ChrisHegarty suggestion to support Java 14+ SDK).
The tests in the change of course don't pass, next step would probably to try out the parameter names module as suggested by @lpandzic. I was not familiar with it when I worked on this PR.

[cowtowncoder]

I am not against using more direct access pattern in this special case: if and when component type declarations are already needed to get types and finding canonical constructor (one with all components/fields in expected order), getting implicit name from there makes sense to me.
I think the only (?) downside is that it would not be possible to override AnnotationIntrospector method to find alternate implicit name, but since call to find explicit name is still made that does not seem like big omission.

[youribonnaffe]

@lpandzic I tested using the ParameterNamesModule, some tests are failing:

[ERROR]   RecordTest.testSerializeJsonRenameRecord:99 expected:<{"[id":123,"]rename":"Bob"}> but was:<{"[]rename":"Bob"}>
[ERROR] Errors:
[ERROR]   RecordTest.testDeserializeSimpleRecord_DisableAnnotationIntrospector:52 » InvalidDefinition
[ERROR]   RecordTest.testSerializeJsonIgnoreRecord:74 » InvalidDefinition No serializer ...
[ERROR]   RecordTest.testSerializeRecordOfRecord:63 » InvalidDefinition No serializer fo...
[ERROR]   RecordTest.testSerializeSimpleRecord:26 » InvalidDefinition No serializer foun...
[ERROR]   RecordTest.testSerializeSimpleRecord_DisableAnnotationIntrospector:43 » InvalidDefinition
[INFO]
[ERROR] Tests run: 9, Failures: 1, Errors: 5, Skipped: 0

A simple test as testDeserializeSimpleRecord works but testSerializeSimpleRecord does not probably because the fields are not picked up (no get methods for records).

[lpandzic]

@youribonnaffe serialization is not part that parameter names module resolves, it's only focused on deserialization. This part should of course be added for records.
@cowtowncoder This is a long story I'd say and I foretold that people would want to have this incorporated into core project at one point, as well as that they would prefer property based creators over delegating ones. I believe now is the right time to address both of these concerns so we can commit to fixing those things and keeping the design and architecture consistent across records and non records.

[lpandzic]

And just to add one more bit: as java embraces immutability more and more this will only become a bigger issue for everyone involved, records are just the start, inline (former value) types are coming too...

[GedMarc]

No the entire problems with records will be relating to the chained/fluent property syntax change in records, to identify the switch between properties and fields.

This is also an issue in commons-beanutils and el/jsf as well with records. Because getters and setters have been used and abused it's a very simple decision to change the format to mark the difference in purpose.
This is a change that needs to go pretty much everywhere, and JSF is going to cry which is strange considering records go directly towards it.

In beanutils, I am adding a property that will search accordingly for properties on the new syntax, using setters as fieldName(Type) and getters as fieldName()

Currently i am using Field access with open clauses in module-info's to circumvent this

[cowtowncoder]

@lpandzic I actually disagree with the idea that solving this problem should necessitate tackling the general difficult issue of finding constructor for POJOs. I am not sure why that would follow -- I much rather solve the relatively self-contained case of Records, properly, and then tackle POJO part separately. We seem to fundamentally disagree here so I am not going to spend much more time on rehashing earlier discussions.

I still plan on doing separate incremental improvements for 2.12 (#1498), and then sometime after bigger rewrite. But I do not want that to be coupled with support needed for Records where there is clear spec to follow wrt discovery of properties intended.

And I do agree that improved handling of constructors matters; yet again I do not see the need for use of an annotation or two as a huge problem.

[lpandzic]

Mainly because of the question on how to handle single parameter constructors for records and classes. If records use property based and classes delegating based constructors by default this will be inconsistent and surprising to users. If they both use delegating it will also be surprising and spawn more instances of #1498.

[cowtowncoder]

I don't see Records ever using delegating creators, but just properties based, for what that is worth.

Hoping to get back to this PR, it is one of 2 must-haves for 2.12.

[cowtowncoder]

@youribonnaffe I think I will try merging this bit by bit as there is currently merge fail (due to addition of RecordTest from different PR). I think this works in general; I will see if I can slightly change just 2 parts:

1. Locating of primary Constructor for Records (redirect handling to be specific to Records ignoring annotations etc)
2. Detection of accessors (which needs special handling due to lack of get-prefix)

Thank you again for contributing this -- it should help a lot going forward.

[youribonnaffe]

Thanks for looking at the PR, it could really benefit from having the eyes of an expert here ;)
Feel free to take pieces from it of course, to me the most valuable thing for now are the tests, the rest can be rewritten again and again

[cowtowncoder]

@youribonnaffe I am making some progress, even if slowly, after realizing that implementing #2800 should probably help solve the question on non-standard naming of Record field accessors. So I will get that implemented with an eye to supporting Record types as well -- it will open up other configurability possibilities too, I think, for Scala and Kotlin modules.
But the idea is that the default naming strategy can be changed for Record types quite nicely and safely.

[cowtowncoder]

@youribonnaffe I struggled mightily in figuring out any clean way, but finally I think I managed get a non-horrible solution for Creator discovery. Thank you very much for providing this patch -- I doubt I would been able to make it happen for 2.12 without your help. Tests were of particular help, but implementation suggestion was valuable as well.

I am not 100% confident that there aren't edge cases, so it'd be great to add more test cases for various configuration settings; but I think this is a good starting point.

[youribonnaffe]

@cowtowncoder thanks you so much for your work on it. I'm very honored to contribute to a project that I use daily at work!
If there are issues related to records, feel free to ping me, I would be happy to continue t on this topic.

Nice catch on the empty record test case.

[youribonnaffe]

I see that in master the Maven profile to run jdk14 specific tests has been dropped, is it because master will only support recent JDKs?

[cowtowncoder]

@youribonnaffe Oh. No, it is not intentional but most likely due to merge settings that drop changes for pom.xml on conflict -- this because they often cause merge conflict.

I can merge it manually, or if you get a chance first, could you do a PR?

And once again I REALLY like this addition, thank you for helping get it in. In fact, it might even help a bit on completing the last major thing (#1498) I hope to still get in 2.12..

[youribonnaffe]

I opened a PR for this #2829

[cowtowncoder]

Merged.