#1810 Add Jackson3JsonpMapper#1879
Conversation
- add Jackson3JsonpMapper and companion classes - add Jackson3 test - add jackson3 dependencies Signed-off-by: Claas Thiele <cthiele@ct42.de>
cthiele42
requested review from
Bukhtawar,
VachaShah,
Xtansia,
madhusudhankonda,
reta,
saratvemulapalli and
szczepanczykd
as code owners
| @@ -217,6 +218,8 @@ dependencies { | |||
| // Apache 2.0 | |||
| implementation("com.fasterxml.jackson.core", "jackson-core", jacksonVersion) | |||
There was a problem hiding this comment.
Thank you @cthiele42 , I would suggest to have 3.x line as a default for 4.x major release (we should not bring Jackson 2.x and 3.x dependencies at the same time):
| implementation("com.fasterxml.jackson.core", "jackson-core", jacksonVersion) | |
| compileOnly ("com.fasterxml.jackson.core", "jackson-core", jacksonVersion) | |
| compileOnly ("com.fasterxml.jackson.core", "jackson-databind", jacksonDatabindVersion) |
There was a problem hiding this comment.
The need for a certain jackson version in opensearch is driven by the client side. If a client is enforced to use jackson version 2 (by its dependencies), jackson version 2 is needed in opensearch as well for making use of enhanced serialization features.
By making a hard cut towards jackson3 with release 4.x, clients enforced to use jackson2 are effectively locked out.
Therefore I would suggest to support both jackson versions for a while. The jackson library is designed for being used in different major versions in parallel, see: https://github.com/FasterXML/jackson/tree/main?tab=readme-ov-file#jackson-major-versions.
Making Jackson3Mapper the default replacing the Jackson2Mapper default with 4.x is a different thing and I would appreciate it.
There was a problem hiding this comment.
There are two issues to solve here (with 4.x):
- promote Jackson 3.x
- optionally, support Jackson 2.x
The clients are free to pick 3.x or 2.x, this is not our decision as you rightly said, but our decision is what the default should be - so most of the clients would work out of the box. We should not depend / bring Jackson 3.x and Jackson 2.x at the same time, hope it makes sense.
There was a problem hiding this comment.
Probably, I missed something here. If 4.x comes with Jackson 3 only, how is the optional support for Jackson2 working in that case?
There was a problem hiding this comment.
Perhaps this is the misunderstanding: Jackson3 is not backward compatible, it cannot serialize/deserialize Jackson2 objects. So a client side Jackson2 cannot communicate with an opensearch Jackson3, it needs Jackson2 support baked into opensearch.
There was a problem hiding this comment.
Probably, I missed something here. If 4.x comes with Jackson 3 only, how is the optional support for Jackson2 working in that case?
It all comes down to JsonProvider impl: add Jackson 2.x deps, exclude Jackson 3.x deps, use JacksonJsonProvider
Perhaps this is the misunderstanding: Jackson3 is not backward compatible, it cannot serialize/deserialize Jackson2 objects.
This is not a concern here I believe
There was a problem hiding this comment.
This pull request is not about upgrading jackson deps from 2 to 3, it is providing an implementation of the JsonpMapper interface for jackson3. Implementations of the JsonpMapper can be used for highly efficient communication with Opensearch.
Typical communication flow:
(1) client gets a JSON String i.e. via. HTTP REST interface
(2) the String is parsed to JSON processing events
the client is doing something senseful with this events in for instance stream processing
(3) processing events are mapped to a Java object
(4) The Java object is passed to the Opensearch client interface, for instance as document in an indexing request
(5) the Opensearch client is serializing the object to JSON processing events
(6) the processing events are serialized to JSON String for sending it out as part of a request. That's done by an implementation of JsonpMapper.
For the case the processing events in (2) and (6) are of the same type, Opensearch is allowing to shortcut steps (3) to (5).
The Mapper used in (6) is configurable, defaulting to JacksonJsonpMapper dealing with jackson2 Json processing event objects. As long as the client is using jackson2 in step (2), the shortcut can be used.
If the client comes up with jackson3, a jackson3 compatible Mapper is needed in (6) to be able to shortcut. -- that's what this pull request is providing.
The new Jackson3JsonpMapper is not the default one, but can be configured for use, for instance with the Opensearch Client instantiation.
Why should the JacksonJsonpMapper (requiring jackson2 deps) stay in place beside the new Jackson3JsonpMapper?
Because there might be users of Opensearch being enforced to use jackson2 on their side and other users being enforced to use jackson3. Both want's to have a compatible Mapper available in Opensearch for using the shortcut.
There was a problem hiding this comment.
The new Jackson3JsonpMapper is not the default one, but can be configured for use, for instance with the Opensearch Client instantiation.
This is what I am suggesting in above #1879 (comment) for main / 4.x, the 3.x release line will preserve Jackson 2.x as default
| implementation("com.fasterxml.jackson.core", "jackson-databind", jacksonDatabindVersion) | ||
| implementation("tools.jackson.core", "jackson-core", jackson3Version) | ||
| implementation("tools.jackson.core", "jackson-databind", jackson3Version) | ||
| testImplementation("com.fasterxml.jackson.datatype", "jackson-datatype-jakarta-jsonp", jacksonVersion) |
There was a problem hiding this comment.
Probably needed:
testImplementation("tools.jackson.datatype", "jackson-datatype-jakarta-jsonp", jackson3Version)
|
@cthiele42 wondering if you would have time to finish up this pull request, thank you. |
2 participants
Description
Provide a Jackson3JsonpMapper implementation
Issues Resolved
#1810
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.