Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Java Record deserialization #148

Merged
merged 7 commits into from
Jun 13, 2024

Conversation

TomaszGaweda
Copy link
Contributor

@TomaszGaweda TomaszGaweda commented Jun 11, 2024

Added support for record deserialization.

It is based on new Java's reflection methods, added with records. If record classes are not supported, code should follow normal path.

Difference from standard deserialization: properties are not set one-by-one, but caches in values array and then passed to all-arg constructor (always present in records)

@TomaszGaweda TomaszGaweda changed the base branch from 2.18 to master June 11, 2024 16:43
@TomaszGaweda TomaszGaweda changed the base branch from master to 2.18 June 11, 2024 16:54
private static Method getTypeMethod;

static {
Method isRecordMethod;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid this one... see note below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pom.xml Outdated Show resolved Hide resolved
@cowtowncoder
Copy link
Member

@TomaszGaweda GREAT contribution, thank you! I added some notes on changes needed; nothing major, just some streamlining.

One thing I'd need, aside from doing review, is CLA, from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(unless you have done one before; apologies if I missed it)

This is only needed once before merging the first contribution. Usually it's easiest to print, fill & sign, scan/photo, email to cla at fasterxml dot com.
Once I get it, I can merge the PR (pending code review etc)

Once again, big thank you for contribution this & let's get it merged in for 2.18(.0)!

@TomaszGaweda
Copy link
Contributor Author

BTW. Should I somehow notice that RecordHelpers is based on Hazelcast's JavaRecordSerializer? I am Hazelcast's contractor, but this work is done privately. Hz code is under Apache 2 as well

@TomaszGaweda
Copy link
Contributor Author

CLA signed and sent, CR comments addressed :)

@cowtowncoder
Copy link
Member

I think RecordHelpers is general enough -- code in jackson-databind has detection code that is quite similar -- and used by many projects, so unless it's a clear-cut copy I probably would not add a reference there.

@TomaszGaweda
Copy link
Contributor Author

Getting required method references is indeed generic enough - core logic had to be implemented here. I've rerequested review @cowtowncoder, thanks for your support!

@cowtowncoder
Copy link
Member

Thank you @TomaszGaweda -- made some final tweaks (wrt since tags, add release notes), ready to go!

In future there may need to try to support annotation override (via jackson annotations extension), but for basic usage this should work nicely..

@cowtowncoder cowtowncoder merged commit a62264f into FasterXML:2.18 Jun 13, 2024
4 checks passed
@cowtowncoder
Copy link
Member

Hmmh. Unfortunately, didn't realize this but I don't think our Record test is running at all...

Will need to figure why, how to fix -- it appears to try to use JUnit 5, although all (other) tests are JUnit4

Assert.assertEquals(expectedString, json);

Cow object = jsonParser.beanFrom(Cow.class, json);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is some feedback on other variants of input data.

a) different order of fields in JSON than in Java record
input: {"object":{"Foo":"Bar"}, "message":"MOO"}
result: Failed to create an instance of jr.Java17RecordTest$Cow due to (java.lang.IllegalArgumentException): argument type mismatch

This should get fixed.

b) missing field in JSON
input: {"message":"MOO"}
result: Failed to create an instance of jr.Java17RecordTest$Cow due to (java.lang.IllegalArgumentException): wrong number of arguments

I am wondering how deserialization to records should handle missing data as the all-args record constructor requires all fields and if there should also be support for Optional like provided by jackson-modules-java8.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darxriggs Could you file a separate issue for the problem with ordering? We definitely need more testing here. It can include part about missing values as well (no need for 2 issues).
As to missing values: should pass null, although this will be problematic for many cases like primitives.
These can be addressed incrementally; Optional isn't yet supported for anything else yet so it can be deferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @darxriggs, I've included your feedback in a follow-up: #163

@cowtowncoder
Copy link
Member

Ok, no, this simply does not work as-is. I will have to revert this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants