feat(jackson3): add validated document reader with pointer diagnostics - #27
Conversation
Decode JSON:API into core documents via explicit PrimaryDataKind, then aggregate-validate before return. Retain token locations by pointer so local and aggregate failures report exact or nearest enclosing source positions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdded Jackson 3 JSON:API document reading with explicit primary-data selection, token-driven decoding, validation, categorized diagnostics, source locations, reader factories, tests, and updated project documentation. ChangesJackson 3 document reader
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant JsonApiDocumentReader
participant JsonApiWireReader
participant DocumentWireReader
participant JsonApiDocumentValidator
Caller->>JsonApiDocumentReader: readValue(input)
JsonApiDocumentReader->>JsonApiWireReader: readDocument(parser, primaryDataKind, locations)
JsonApiWireReader->>DocumentWireReader: decode document
DocumentWireReader-->>JsonApiWireReader: JsonApiDocument
JsonApiWireReader-->>JsonApiDocumentReader: JsonApiDocument
JsonApiDocumentReader->>JsonApiDocumentValidator: validate(document, context)
JsonApiDocumentValidator-->>JsonApiDocumentReader: validation result
JsonApiDocumentReader-->>Caller: document or categorized exception
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
jsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/model/Relationships.java (1)
139-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass
additionalCopydirectly torequireNoCollisions.Remove
castRelationships. The method acceptsMap<K, ?>and only reads keys, so the unchecked cast is unnecessary and introduces an incorrect non-nullRelationshiptype.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/model/Relationships.java` around lines 139 - 148, Update the relationship-copy flow in Relationships to pass additionalCopy directly to requireNoCollisions, relying on its Map<K, ?> parameter. Remove the castRelationships helper and all references to it, preserving the existing collision validation behavior without introducing a non-null Relationship cast.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.java (1)
143-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable
default-arm self-checks duplicated across five drafts.Each draft declares a member-name
Setand tests it in thedefaultarm of the member switch. Every name in each set already has an explicitcase, so thedefaultarm can never see one of those names and theWireTokens.unexpected(...)throw is unreachable. The sets exist only to catch a future edit that adds a constant to the set without adding acase. The same five-line idiom, the set field, and the throw are copied five times.Replace the runtime self-check with a compile-time or test-time guarantee. One option: drop the sets and the
default-arm check, then add a Spock test per draft that feeds every reserved member name and asserts it is decoded rather than placed inadditional. That removes the duplication and keeps the protection.
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.java#L143-L149: remove theDOCUMENT_MEMBERS.contains(name)check and theDOCUMENT_MEMBERSfield at Lines 26-33; keep theadditionalpass-through.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.java#L188-L194: remove theJSONAPI_MEMBERS.contains(name)check and theJSONAPI_MEMBERSfield at Lines 35-37.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ErrorWireReader.java#L84-L90: remove theERROR_MEMBERS.contains(name)check and theERROR_MEMBERSfield at Lines 19-28.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ErrorWireReader.java#L132-L138: remove theERROR_SOURCE_MEMBERS.contains(name)check and theERROR_SOURCE_MEMBERSfield at Lines 30-31.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/LinkWireReader.java#L101-L107: remove theLINK_OBJECT_MEMBERS.contains(name)check and theLINK_OBJECT_MEMBERSfield at Lines 19-27.Add the covering tests under each module's
src/test/groovy/directory, mirroring the main package structure. Based on coding guidelines: "Use Groovy and Spock for tests under each module'ssrc/test/groovy/directory, mirroring the main package structure."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.java` around lines 143 - 149, Remove the unreachable reserved-member self-checks and their set fields from DocumentWireReader in jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.java at lines 143-149 and 188-194, ErrorWireReader in jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ErrorWireReader.java at lines 84-90 and 132-138, and LinkWireReader in jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/LinkWireReader.java at lines 101-107; retain each default arm’s additional-member pass-through. Add Groovy/Spock coverage under each module’s src/test/groovy directory, mirroring the main package structure, verifying every reserved member is decoded rather than added to additional.Source: Coding guidelines
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireTokens.java (2)
64-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the immutability policy for open containers, and record why
List.copyOfis unsafe here.
readOpenArrayreturnsCollections.unmodifiableList(...), whilereadOpenObjectreturns the mutableLinkedHashMapto the caller. Make the policy consistent.Keep
Collections.unmodifiableListinreadOpenArray.List.copyOfrejects null elements, so a valid document such as{"meta":{"a":[null]}}would throw aNullPointerException. Add a short comment so a later consistency refactor does not switch it toList.copyOflikereadStringArrayat Line 34.♻️ Proposed clarification
while (parser.nextToken() != JsonToken.END_ARRAY) { pointer.pushIndex(index); pointer.capture(parser); values.add(readOpenValue(parser, pointer)); pointer.pop(); index++; } + // Open arrays may contain JSON null; List.copyOf would reject null elements. return Collections.unmodifiableList(values); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireTokens.java` around lines 64 - 84, Update readOpenObject to return an unmodifiable map, matching readOpenArray while preserving null-valued members. Keep Collections.unmodifiableList in readOpenArray and add a brief comment explaining that List.copyOf must not replace it because it rejects valid null elements.
53-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
readNumberagainst invalid tokens and enum values.Pass
pointertoreadNumber, reject non-numeric tokens withUNEXPECTED_TOKEN, and add acase null, defaultarm to prevent uncategorized failures whengetNumberType()returnsnullor adds a new constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireTokens.java` around lines 53 - 62, Update WireTokens.readNumber to accept the parser pointer, validate that the current token is numeric, and reject invalid tokens with UNEXPECTED_TOKEN. Extend the getNumberType() switch with a case null, default arm that also produces the established unexpected-token error, preserving the existing numeric conversions.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireObjectMembers.java (1)
18-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the handler consumption contract.
The loop depends on an invariant: each
MemberHandlermust consume the complete member value before it returns. If a handler leaves the parser inside a value, the nextparser.nextToken()reads a token inside that value andrequireFieldNamereports a misleadingUNEXPECTED_TOKENat the wrong pointer. State this invariant in the javadoc so future readers keep it.♻️ Proposed javadoc addition
/** * Expects {`@link` JsonToken#START_OBJECT}, captures the current pointer location, then invokes * {`@code` handler} once per member with the parser positioned on that member's value token. + * + * <p>Each handler must consume the complete member value, including nested arrays and objects, + * so the parser rests on the last token of that value when the handler returns. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireObjectMembers.java` around lines 18 - 36, Update the Javadoc for WireObjectMembers.forEachMember to state that each MemberHandler invocation must consume the complete member value and return with the parser positioned after that value, before the loop advances to the next member. Keep the existing parsing behavior unchanged.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiJackson3.java (1)
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
basedirectly toJsonApiDocumentReader. The reader uses the mapper only to create parsers.JsonApiWireReaderperforms token-driven decoding, andJsonApiDocumentModuleregisters only a serializer. This avoids the unnecessaryrebuild()and module registration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiJackson3.java` around lines 56 - 61, Update the reader method in JsonApiJackson3 to pass the validated base JsonMapper directly to JsonApiDocumentReader instead of calling documentMapper(base); leave the existing null checks and read context wiring unchanged.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiDocumentReader.java (1)
113-126: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdvance the parser for any completed prior root value.
readValue(JsonParser)supports sequential root values. After a scalar root value, the parser sits on a value token such asVALUE_STRING, not onEND_OBJECTorEND_ARRAY. Line 116 then keeps that stale token, and decoding starts on the previous value. Advance whenever the current token is not a structural start token.♻️ Proposed condition
private static void ensureCurrentToken(JsonParser parser) { JsonToken token = parser.currentToken(); - // After a prior root value, Jackson leaves the parser on END_OBJECT/END_ARRAY. - if (token == null || token == JsonToken.END_OBJECT || token == JsonToken.END_ARRAY) { + // After a prior root value, the parser rests on that value's last token. + if (token != JsonToken.START_OBJECT && token != JsonToken.START_ARRAY) { token = parser.nextToken(); if (token == null) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiDocumentReader.java` around lines 113 - 126, Update ensureCurrentToken to advance the parser whenever currentToken() is not a structural start token, rather than only after END_OBJECT or END_ARRAY. Preserve the existing null-token handling and malformed-document exception, while allowing readValue(JsonParser) to correctly move past completed scalar and composite root values.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ValidationPointers.java (1)
90-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated RFC 6901 escaping in the internal package. Two classes define the same
escapeimplementation. One shared helper prevents the two copies from diverging.
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ValidationPointers.java#L90-L92: moveescapeandunescapeinto a shared internal pointer utility and call it fromjoin.jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/JsonPointerAccumulator.java#L51-L53: delete the localescapeand call the shared utility frompush.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ValidationPointers.java` around lines 90 - 92, Extract the duplicated RFC 6901 escaping logic into one shared internal pointer utility. In jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ValidationPointers.java:90-92, move both escape and unescape there and update join to use the utility; in jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/JsonPointerAccumulator.java:51-53, remove the local escape method and update push to call the shared utility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ReadLocations.java`:
- Around line 23-32: Update ReadLocations.from to compare the unknown location
using TokenStreamLocation.NA.equals(location) rather than identity comparison,
and return SourceLocation.UNKNOWN when all four location coordinates are
unavailable. Keep the existing Jackson 3 accessors and normal coordinate
conversion unchanged for valid locations.
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiDocumentReadException.java`:
- Around line 15-19: Update JsonApiDocumentReadException so its sourceLocation
field is either excluded from serialization with transient or its SourceLocation
type is made serializable, ensuring serializing the exception no longer throws
NotSerializableException while preserving source-location behavior during normal
use.
---
Nitpick comments:
In
`@jsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/model/Relationships.java`:
- Around line 139-148: Update the relationship-copy flow in Relationships to
pass additionalCopy directly to requireNoCollisions, relying on its Map<K, ?>
parameter. Remove the castRelationships helper and all references to it,
preserving the existing collision validation behavior without introducing a
non-null Relationship cast.
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.java`:
- Around line 143-149: Remove the unreachable reserved-member self-checks and
their set fields from DocumentWireReader in
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.java
at lines 143-149 and 188-194, ErrorWireReader in
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ErrorWireReader.java
at lines 84-90 and 132-138, and LinkWireReader in
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/LinkWireReader.java
at lines 101-107; retain each default arm’s additional-member pass-through. Add
Groovy/Spock coverage under each module’s src/test/groovy directory, mirroring
the main package structure, verifying every reserved member is decoded rather
than added to additional.
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ValidationPointers.java`:
- Around line 90-92: Extract the duplicated RFC 6901 escaping logic into one
shared internal pointer utility. In
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ValidationPointers.java:90-92,
move both escape and unescape there and update join to use the utility; in
jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/JsonPointerAccumulator.java:51-53,
remove the local escape method and update push to call the shared utility.
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireObjectMembers.java`:
- Around line 18-36: Update the Javadoc for WireObjectMembers.forEachMember to
state that each MemberHandler invocation must consume the complete member value
and return with the parser positioned after that value, before the loop advances
to the next member. Keep the existing parsing behavior unchanged.
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireTokens.java`:
- Around line 64-84: Update readOpenObject to return an unmodifiable map,
matching readOpenArray while preserving null-valued members. Keep
Collections.unmodifiableList in readOpenArray and add a brief comment explaining
that List.copyOf must not replace it because it rejects valid null elements.
- Around line 53-62: Update WireTokens.readNumber to accept the parser pointer,
validate that the current token is numeric, and reject invalid tokens with
UNEXPECTED_TOKEN. Extend the getNumberType() switch with a case null, default
arm that also produces the established unexpected-token error, preserving the
existing numeric conversions.
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiDocumentReader.java`:
- Around line 113-126: Update ensureCurrentToken to advance the parser whenever
currentToken() is not a structural start token, rather than only after
END_OBJECT or END_ARRAY. Preserve the existing null-token handling and
malformed-document exception, while allowing readValue(JsonParser) to correctly
move past completed scalar and composite root values.
In
`@jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiJackson3.java`:
- Around line 56-61: Update the reader method in JsonApiJackson3 to pass the
validated base JsonMapper directly to JsonApiDocumentReader instead of calling
documentMapper(base); leave the existing null checks and read context wiring
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 97aa321b-45a7-4926-806a-1a5fbc8cd4c8
📒 Files selected for processing (37)
.agentWork/milestones/README.md.agentWork/milestones/phase-2-4-document-reads.mdREADME.mddocs/adr/README.mddocs/conformance.mddocs/vision.mdjsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/internal/AdditionalMembers.javajsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/internal/OrderedMaps.javajsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/model/Attributes.javajsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/model/Links.javajsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/model/Meta.javajsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/model/Relationships.javajsonapi-java-core/src/main/java/io/github/kazemek/jsonapi/core/validation/JsonApiDocumentValidator.javajsonapi-java-jackson3/README.mdjsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/CodecFailureCategory.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/DocumentReadContext.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiDocumentReadException.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiDocumentReader.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/JsonApiJackson3.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/PrimaryDataKind.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/SourceLocation.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DocumentWireReader.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ErrorWireReader.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/JsonApiWireReader.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/JsonPointerAccumulator.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/LinkWireReader.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/MemberClassifier.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ReadLocationIndex.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ReadLocations.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ResourceWireReader.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/ValidationPointers.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireObjectMembers.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/WireTokens.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/package-info.javajsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/package-info.javajsonapi-java-jackson3/src/test/groovy/io/github/kazemek/jsonapi/jackson3/DocumentReaderIsolationSpec.groovyjsonapi-java-jackson3/src/test/groovy/io/github/kazemek/jsonapi/jackson3/DocumentReaderSpec.groovy
Clear the ValidationPointers cast Sonar issue, harden read diagnostics and parser sequencing, and apply the high-signal CodeRabbit cleanups.
|
Pushed 3a7062b with the PR review + Sonar follow-ups: Actionable
Sonar
Nitpicks applied
Skipped (as planned)
|
|



Decode JSON:API into core documents via explicit PrimaryDataKind, then aggregate-validate before return. Retain token locations by pointer so local and aggregate failures report exact or nearest enclosing source positions.
Summary by CodeRabbit