-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
HHH-19890 Add Jackson 3 FormatMapper support #11357
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
base: main
Are you sure you want to change the base?
HHH-19890 Add Jackson 3 FormatMapper support #11357
Conversation
| * @author Christian Beikov | ||
| * @author Yanming Zhou | ||
| * @author Nick Rayburn | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the previous author's need to be retained. This file was copied from the existing one that they were on. (I also don't need my name in here either unless it's the project standard, just tried to match what I copied from.)
hibernate-core/src/main/java/org/hibernate/type/format/jackson/Jackson3XmlFormatMapper.java
Outdated
Show resolved
Hide resolved
|
You should take care of backward compatibility. Jackson 3 does not have JSR 310 module since it is handling Here is one example that is showing the problem |
caa4ff0 to
b85e057
Compare
|
Thanks @cigaly, I added the tests and updated the implementation to pass. Are you suggesting that the new Jackson 3 support should match the existing Jackson 2 support as closely as possible or are you only concerned about the Jackson 3 changed default options for some features, so the new code here would just need to configure those back to the Jackson 2 values. I don't think there's a problem with this, but I wanted to check before I make that change. |
b85e057 to
f2a08e9
Compare
|
Main point is that one should be careful with backward compatibility. If I have project that was using Hibernate + Jackson 2, then I would expect that it will work with Jackson 3 as well without some major code changes. I am not 100% sure how many incompatibilities are introduced when switching from Jackson 2 to 3, One good thing with |
|
I think it’s a reasonable concern. Even if Jackson can serialize/deserialize the data if it isn’t the same format, there could still be other impact if the data being stored in the database is changed.
Not sure if any of that changes your opinion on how much we configure the Jackson 3 support to align with the existing Jackson 2 support. I can review the relevant Jackson features, but I think it will be 10+ that need to be explicitly configured to old defaults to be safe. |
|
Jackson offers a method that configures the mapper to use the Jackson 2 settings. I’ll adjust the PR to use that. This will resolve most of the concerns, but it’s probably still worth considering if Jackson 3 is part of the automatic detection or not. |
f2a08e9 to
fd2673f
Compare
|
Updated the code to use the Jackson methods to use the Jackson 2 defaults. I found a few discrepancies and opened PRs against Jackson to resolve those. |
|
|
||
| We also need to add Jackson or an implementation of JSONB—for example, Yasson—to our runtime classpath. | ||
| To use Jackson we could add this line to our Gradle build: | ||
| To use Jackson 2 we could add this line to our Gradle build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also mention what artifact is needed for Jackson 3?
| setting, | ||
| selector, | ||
| () -> { | ||
| final FormatMapper jackson3FormatMapper = getJsonJackson3FormatMapperOrNull( creationContext ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaulting to Jackson 3 if it's on the classpath might not be such a good idea since people might still want to keep using Jackson 2 for Hibernate ORM if it is also around. I would suggest to use Jackson 3 as fallback if Jackson 2 is not available.
| return jacksonFormatMapper != null | ||
| ? jacksonFormatMapper | ||
| : new JaxbXmlFormatMapper( legacyFormat ); | ||
| final FormatMapper jackson3FormatMapper = getXMLJackson3FormatMapperOrNull( creationContext ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, I would suggest to use Jackson 3 as fallback if Jackson 2 is not available.
| "{\"instant\": 1764576582.000000000, \"duration\": 11640.000000000, \"localDate\": [2025, 12, 1], \"localTime\": [9, 9, 42], \"localDateTime\": [2025, 12, 1, 9, 9, 42]}"; | ||
| // MySQL | ||
| String alternativeMySQLJson = | ||
| "\"{\\\"instant\\\": 1764576582.0, \\\"duration\\\": 11640.0, \\\"localDate\\\": [2025, 12, 1], \\\"localTime\\\": [9, 9, 42], \\\"localDateTime\\\": [2025, 12, 1, 9, 9, 42]}\""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit odd that JsonMappingTests works fine with MySQL without the double backslash quotation but this test requires the backslash quotation. Can you explain the reason for that?
This adds Jackson 3 support alongside the existing Jackson 2 support.
./gradlew clean build.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
https://hibernate.atlassian.net/browse/HHH-19890