JRef+JsonPointer implementation as ObjectMapper module. #5920 - #6045
JRef+JsonPointer implementation as ObjectMapper module. #5920#6045scottslewis wants to merge 11 commits into
Conversation
as a single ObjectMapper module...called JRefModule, in JRefModule.java. No changes to existing Jackson 3.0 classes, or other code additions are necessary. The tools.jackson.databind.JRefModule class has JRefValueSerializer and JRefValueSerializer implementations, used in appropriate circumstances during ser/des via the valueserializermodifier and the valuedeserializermodifier. Also included in this commit are initial versions (incomplete) test code for the JRefModule in new jref test package. Signed-off-by: Scott Lewis <scottslewis@gmail.com>
names for clarity.
|
Some review and forward motion for this pr would be appreciated. |
|
@scottslewis Sorry, I started a new job and am bit overloaded -- I will try to get back to this soon; looks like there has been great progress. |
I think things have come together well as a Jackson module. NP on overloaded/delays, etc. It just means you are an OSS maintainer. Congrats on the job. fyi, I've also got a Gson pr that uses Gson type adapter factory...analogous to the Jackson *modifier api, and includes JsonPointer and TokenStreamContext classes from Jackson...to convert JSONPath references (used by Gson from years ago) to JsonPointers. There is also now a javascript impl of JREF.stringify and JREF.parse here. Trying to figure out how to contribute that. |
|
@scottslewis Ok, so, great -- I really like the idea of opt-in module, given performance implications. Now... the immediate follow-up part is that as https://github.com/FasterXML/jackson-modules-base repo. This is convenient for publishing; it's fine to refer to modules from databind README.md and/or Wiki but just keep module along with other foundational modules (like Afterburner, Blackbird, MrBean). |
|
On correctness, Claude pointed out a few possible concerns; will include just one for now: JSON Pointer prefix check uses string startsWith — latent path bug String prefix matching over pointers is wrong at segment boundaries: /items/10.startsWith(/items/1) is true, and |
Ok. How would this structural comparison be done with JsonPointer? |
As is, I don't think Maybe by custom |
I wouldn't rule out implementing Comparator/Comparable, but I don't think the semantics of my startsWith use case matches C/C very well, and I don't think it would be necessary to support all of C/C for JsonPointers (i.e. it's not needed for sorting, for example). How would you feel about: public boolean startsWith(JsonPointer other);in JsonPointer class? If good with this I'll give it a shot. Since in core though...should I open a new issue in that repo and submit pr there? |
|
@cowtowncoder How about this: /**
* Method to check whether this pointer starts with the given other pointer.
*
* This implementation compares logical segments rather than raw string prefix:
* it iterates through segments of 'other' and ensures corresponding segments
* of 'this' match exactly (either same property name or same element index).
*
* @param other Pointer to check as prefix
* @return true if this pointer starts with the given other pointer
*/
public boolean startsWith(JsonPointer other) {
if (other == null) {
return false;
}
if (other == EMPTY) {
return true;
}
JsonPointer a = this;
JsonPointer b = other;
while (b != EMPTY) {
if (a == EMPTY) {
// 'other' has more segments than 'this'
return false;
}
// Compare element index if present in 'b'
if (b._matchingElementIndex >= 0) {
if (a._matchingElementIndex != b._matchingElementIndex) {
return false;
}
} else {
// Compare property names (may be empty string)
if (a._matchingPropertyName == null) {
return false;
}
if (!a._matchingPropertyName.equals(b._matchingPropertyName)) {
return false;
}
}
a = a._nextSegment;
b = b._nextSegment;
}
return true;
} |
|
Yes, issue and pr need to go in Addition itself sounds reasonable. |
frm line 206-207 of JRefModule discussed starting with this comment: FasterXML#6045 (comment) The fix is to use the newly merged JsonPointer.startsWith(JsonPointer other) method added in jackson-core via pr this pr: FasterXML/jackson-core#1636 This means that the latest of Jackson-core 3.x branch will be required to compile the updated JRefModule.
Ok. How do developers install and use the jackson-modules-base? Is it by installing a separate jar dependency? (I'm not familiar with the Jackson releng/maven publishing conventions). I would prefer that the number of dependencies be small...as possible...and the JRefModule already depends upon both jackson core and databind. Also...fyi, we are soon going to release a Jref-lib for javascript https://github.com/OpenMCPTools/jref-lib/tree/main/javascript that should (will) interoperate with Jackson JRefModule...as the wire format is the same (json pointer + jref spec...i.e. '$ref'). |
Yes, they are all separate jars, only grouped together in multi-Maven project for convenient release publishing -- modules do not typically depend on each other. That is, there is no
Neat! |
How/who is bug fix, ci/releng/release process done for these separate modules? It's unlikely that I will be able to maintain/releng another library, especially without commit rights...which, fwiw I'm not wanting or looking to get for Jackson. I have other maintenance commitments and adding on another one without support will currently be difficult for me. From looking at jackson-modules-base, the modules there seem to be more about integrations with other frameworks (e.g. jaxb, osgi)...many of them legacy. This is not the case (imo) wrt jref/json improved support). Jref has always seemed to me more like a new interoperability+efficiency optional feature. I implemented as JRefModule for separation and because you suggested this...and this clearly makes separation of concerns/modularity sense.
Finally, I think that given the size of JRefModule (one top-level class with a small number of inner classes) is seems overkill to have this be in a separate jar (with separate releng/meta-data, versioning, etc). And it puts an extra burden on the dev consumer, which I think will make adoption more difficult. Respectfully, I would suggest tools.jackson.databind package, or tools.jackson.databind.util package or tools.jackson.databind.jref package. If more source-level reworking (e.g. JacksonFeature) is desired or required for this, and/or feature, or usage docs/examples desired and needed, I will commit to doing that with your guidance. But I don't think I can commit to doing ongoing ci, releng and maintainance as a separate repo and jar given my other commitments. |
Tested Implementation to address #5920 with a new ObjectMapper module. This new SimpleModule is called JRefModule, currently in tools.jackson.databind package. No changes to existing Jackson 3.0 classes, or any other code changes are necessary to support jref+jsonpointer serialization or deserialization.
The
tools.jackson.databind.JRefModule class has inner classes: JRefValueSerializer and JRefValueSerializer, used in appropriate circumstances during ser/des via the valueserializermodifier and the valuedeserializermodifier creation.
Also included in this commit are initial versions of units tests in a new test package in src/test/java named: tools.jackson.databind.jref