GH-139: Allow pointing to array elements via a reference field #203
Conversation
…rence field. Example path `/array/id=123/data`
WalkthroughThe changes introduce extended JSON Pointer functionality to support referencing array elements by key-value pairs, update documentation to describe this feature, and add comprehensive tests and test data for validation. Parsing and evaluation logic are enhanced to handle new escape sequences and array key references, with supporting updates to the version and formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JsonPointer
participant JsonDocument
User->>JsonPointer: Parse pointer (e.g., /array/id=123/data)
JsonPointer->>JsonPointer: Tokenize and detect key reference (id=123)
User->>JsonPointer: Evaluate pointer against JsonDocument
JsonPointer->>JsonDocument: Locate array node
JsonPointer->>JsonDocument: Search array for object with id=123
JsonDocument-->>JsonPointer: Return matching object
JsonPointer->>JsonDocument: Access 'data' field of matched object
JsonDocument-->>User: Return value of 'data'
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (1)
383-396:⚠️ Potential issueEquals and hashCode methods don't consider all fields.
The
equalsandhashCodemethods only consider thedecodedTokenfield, ignoringindexandkeyRef. This can lead to unexpected behavior, as two tokens with the same decoded text but different array indices or key references would be considered equal.Update the equals and hashCode methods to consider all fields that define the token's identity:
@Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; RefToken refToken = (RefToken) o; - return decodedToken.equals(refToken.decodedToken); + return decodedToken.equals(refToken.decodedToken) && + Objects.equals(index, refToken.index) && + Objects.equals(keyRef, refToken.keyRef); } @Override public int hashCode() { - return decodedToken.hashCode(); + return Objects.hash(decodedToken, index, keyRef); }
🧹 Nitpick comments (3)
README.md (1)
3-3: Fix grammar and markdown heading style.There's a missing article and a trailing period in the heading.
-# This is an implementation of [RFC 6902 JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) written in Java with extended JSON pointer. +# This is an implementation of [RFC 6902 JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) written in Java with an extended JSON pointer🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “an” here.
Context: .../doc/html/rfc6902) written in Java with extended JSON pointer. ## Description & Use-Cas...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
🪛 markdownlint-cli2 (0.17.2)
3-3: Trailing punctuation in heading
Punctuation: '.'(MD026, no-trailing-punctuation)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (2)
236-256: Performance concern in array key reference evaluation.The implementation correctly handles array key references, but could be inefficient for large arrays as it performs a linear search through all elements. For frequently accessed paths, this could become a performance bottleneck.
Consider optimizing this with one of the following approaches:
- Add a disclaimer in the documentation about performance implications for large arrays
- Add caching for frequently accessed paths
- For advanced use cases, consider adding support for creating an index structure when the same JSON document is evaluated multiple times
// Example documentation note that could be added to the class comments * <p>Note on array key references: The evaluation of key references in arrays * requires a linear search through all array elements, which may impact * performance for large arrays or frequently accessed paths.
399-422: Well-implemented KeyRef class.The KeyRef class has a clean implementation with proper equals and hashCode methods using Objects utilities.
Consider making the fields final to enforce immutability, which aligns with the immutability claims in the class documentation:
- private String key; - private String value; + private final String key; + private final String value;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
README.md(3 hunks)pom.xml(1 hunks)src/main/java/com/flipkart/zjsonpatch/JsonDiff.java(1 hunks)src/main/java/com/flipkart/zjsonpatch/JsonPointer.java(7 hunks)src/test/java/com/flipkart/zjsonpatch/JsonPointerTest.java(7 hunks)src/test/resources/testdata/json-pointer-key-refs.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/flipkart/zjsonpatch/JsonDiff.java (1)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (2)
JsonPointer(42-432)RefToken(287-397)
🪛 LanguageTool
README.md
[uncategorized] ~3-~3: You might be missing the article “an” here.
Context: .../doc/html/rfc6902) written in Java with extended JSON pointer. ## Description & Use-Cas...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~8-~8: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...if data (in JSON format) is required to send across multiple systems over network or...
(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
[uncategorized] ~8-~8: Possible missing article found.
Context: ...ed to send across multiple systems over network or in case of multi DC transfer. - When...
(AI_HYDRA_LEO_MISSING_A)
🪛 markdownlint-cli2 (0.17.2)
README.md
3-3: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🔇 Additional comments (14)
pom.xml (1)
7-7: Appropriate version update for the new feature.The version bump from 0.4.17-SNAPSHOT to 0.5.0-SNAPSHOT correctly follows semantic versioning principles for adding a significant new feature (extended JSON pointer functionality).
src/main/java/com/flipkart/zjsonpatch/JsonDiff.java (1)
272-272: Good use of the enhanced RefToken parsing.The change correctly utilizes the new
parsemethod to ensure consistent token processing with the extended JSON Pointer functionality. This enables support for key-based array element references while maintaining compatibility with existing functionality.src/test/resources/testdata/json-pointer-key-refs.json (1)
1-26: Well-structured test data for the new feature.The test data provides a good variety of objects with different structures and ID values, which is appropriate for validating the key-reference functionality. Each object has unique identifiers and various properties to test different access patterns.
README.md (2)
9-10: Good documentation of the new feature.The addition of the extended JSON pointer functionality is well-documented with a clear explanation of the syntax format.
83-108: Excellent example demonstrating the new functionality.The added example clearly shows how to use the key-reference syntax and what results to expect, which is very helpful for users adopting this new feature.
src/test/java/com/flipkart/zjsonpatch/JsonPointerTest.java (6)
39-46: Test coverage for new array key reference functionality looks good!The test effectively verifies that array key references are correctly parsed and the key-value pair is properly extracted and stored in the KeyRef object.
105-122: Great comprehensive test for mixed reference types!The test has been properly extended to validate the new array key reference functionality within a mixed path context. The assertions verify all expected behaviors, including proper extraction of the key-value pair.
143-150: Good test for the new escape sequence handling.This test properly validates that the new
~2escape sequence for the equals sign (=) is correctly parsed and decoded.
161-161: Correctly updated invalid escape test.The test has been properly updated to reflect that
~2is now a valid escape sequence, and~3is now the first invalid one.
192-211: Thorough test cases for key reference evaluation.The test coverage for key reference evaluation is comprehensive, including:
- Basic key reference resolution
- Nested path traversal after key reference
- Multiple different keys for matching
- Numeric values
- Error cases for non-existent keys and out-of-bounds indices
This provides good validation of the new functionality.
233-234: Good symmetry tests for new token formats.These tests ensure that both the regular equals sign in paths and the escaped equals sign (
~2) are correctly handled in both parsing and rendering operations.src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (3)
86-91: Properly extended escape sequence handling.The escape sequence processing has been correctly extended to support the new
~2escape for equals sign, maintaining backward compatibility with existing escape sequences.
132-132: Field initialization consistent with new design.The token creation consistently uses the new constructor pattern with explicit null values for unused fields.
144-145: Array index token creation properly updated.The append method for array indices now correctly initializes the token with both the string representation and the parsed index value.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (1)
396-409: 🛠️ Refactor suggestionPotential inconsistency between equals/hashCode and class fields.
The
equalsandhashCodemethods only consider thedecodedTokenfield, not the newindexandkeyReffields. This could lead to unexpected behavior when comparing tokens with the same decoded token but different array index or key reference values.Update the equals and hashCode methods to consider all fields:
@Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; RefToken refToken = (RefToken) o; - return decodedToken.equals(refToken.decodedToken); + if (!decodedToken.equals(refToken.decodedToken)) return false; + if (!Objects.equals(index, refToken.index)) return false; + return Objects.equals(keyRef, refToken.keyRef); } @Override public int hashCode() { - return decodedToken.hashCode(); + return Objects.hash(decodedToken, index, keyRef); }
♻️ Duplicate comments (2)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (2)
270-281: Good implementation of value type handling.The
matchesmethod correctly handles different JSON value types (text, numbers, booleans) when comparing the key reference value.
301-304: Consistency in field declarations.The
decodedTokenfield is now correctly declared as final, aligning with the class documentation stating that RefTokens are immutable.
🧹 Nitpick comments (1)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (1)
242-254: Consider performance optimization for large arrays.The current implementation searches sequentially through all array elements to find a matching key-value pair, which has O(n) complexity. For large arrays, this could be inefficient.
Consider creating a temporary map-based index for faster lookups when the array size exceeds a certain threshold. This would trade memory for speed:
if (current.size() > LARGE_ARRAY_THRESHOLD) { Map<String, JsonNode> keyToNodeMap = new HashMap<>(); for (int arrayIdx = 0; arrayIdx < current.size(); ++arrayIdx) { JsonNode arrayNode = current.get(arrayIdx); if (arrayNode.has(keyRef.key)) { keyToNodeMap.put(arrayNode.get(keyRef.key).toString(), arrayNode); } } JsonNode foundArrayNode = keyToNodeMap.get(keyRef.value); if (foundArrayNode == null) { error(idx, "Array has no matching object for key reference " + token, document); } current = foundArrayNode; } else { // Existing sequential search code }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java(8 hunks)
🔇 Additional comments (10)
src/main/java/com/flipkart/zjsonpatch/JsonPointer.java (10)
86-91: Good implementation of the new escape sequence.You've correctly extended the escape sequence handling to include
~2for the equals sign, maintaining consistency with the existing escape sequences~0and~1.
132-132: LGTM!You've correctly updated the
append(String)method to use the new RefToken constructor signature with explicit null values for index and keyRef.
143-145: LGTM!The
append(int)method has been properly updated to use the new RefToken constructor signature with the array index value and null for keyRef.
236-256: Well-structured array handling logic.The array handling logic is now comprehensive with three distinct cases:
- Numeric index access with proper bounds checking
- Key-value reference search
- Error handling for invalid tokens
Good job implementing the array key reference search logic.
314-314: Consistent pattern naming.The naming of the new patterns for handling equals signs follows the established convention for tilde and slash patterns.
Also applies to: 325-325
319-320: Comprehensive path encoding/decoding.The path encoding and decoding methods have been correctly updated to handle equals signs, maintaining consistency with the existing escape sequence handling.
Also applies to: 330-331
340-362: Well-structured token parsing.The
parsemethod now properly detects and handles three token types:
- Array indices (including the special "-" for last index)
- Key-value references of the form "key=value"
- Regular string tokens
This provides a solid foundation for the new array key reference functionality.
369-371: Good encapsulation with appropriate error handling.The
isArrayKeyRef()andgetKeyRef()methods provide clean API access to the key reference functionality with appropriate error handling for invalid operations.Also applies to: 378-381
389-393: Consistent string representation.The
toString()method has been updated to properly handle key references, ensuring that key-value pairs are correctly encoded in the JSON Pointer string.
336-336:❌ Incorrect review comment
Ensure key-value pairs without equals signs are handled correctly.
The regex pattern
([^=]+)=([^=]+)will match strings with one equals sign, ensuring each side has at least one character. However, it prevents equals signs in either the key or value.
🏁 Script executed:
#!/bin/bash # Check if the implementation allows encoding equals signs within keys or values ast-grep --pattern $'public static RefToken parse(String rawToken) { $$$ Matcher arrayKeyRefMatcher = VALID_ARRAY_KEY_REF.matcher(rawToken); if (arrayKeyRefMatcher.matches()) { $$$ } $$$ }'Length of output: 218
🏁 Script executed:
#!/bin/bash file="src/main/java/com/flipkart/zjsonpatch/JsonPointer.java" # Locate the VALID_ARRAY_KEY_REF declaration rg -n "VALID_ARRAY_KEY_REF" "$file" # Find the parse method signature rg -n "public static RefToken parse" -n "$file" # Extract the parse method around the array key handling sed -n '330,380p' "$file"Length of output: 2555
No change needed: the regex correctly matches only the
=delimiter and relies on~2escapes for literal=in keys/values.
VALID_ARRAY_KEY_REFat src/main/java/com/flipkart/zjsonpatch/JsonPointer.java:336 is intended to split on a single unescaped=.- Any literal
=in a key or value must be encoded as~2(per the library’s escape rules) and is restored bydecodePath().Likely an incorrect or invalid review comment.
| static class KeyRef { | ||
| private String key; | ||
| private String value; | ||
|
|
||
| public KeyRef(String key, String value) { | ||
| this.key = key; | ||
| this.value = value; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
|
|
||
| KeyRef keyRef = (KeyRef) o; | ||
|
|
||
| return Objects.equals(key, keyRef.key) && Objects.equals(value, keyRef.value); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(key, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make KeyRef fields final for immutability.
The key and value fields in the KeyRef class should be declared as final to ensure immutability, consistent with the immutability documentation of the RefToken class.
static class KeyRef {
- private String key;
- private String value;
+ private final String key;
+ private final String value;
public KeyRef(String key, String value) {
this.key = key;
this.value = value;
}
// ...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static class KeyRef { | |
| private String key; | |
| private String value; | |
| public KeyRef(String key, String value) { | |
| this.key = key; | |
| this.value = value; | |
| } | |
| @Override | |
| public boolean equals(Object o) { | |
| if (this == o) return true; | |
| if (o == null || getClass() != o.getClass()) return false; | |
| KeyRef keyRef = (KeyRef) o; | |
| return Objects.equals(key, keyRef.key) && Objects.equals(value, keyRef.value); | |
| } | |
| @Override | |
| public int hashCode() { | |
| return Objects.hash(key, value); | |
| } | |
| } | |
| static class KeyRef { | |
| private final String key; | |
| private final String value; | |
| public KeyRef(String key, String value) { | |
| this.key = key; | |
| this.value = value; | |
| } | |
| @Override | |
| public boolean equals(Object o) { | |
| if (this == o) return true; | |
| if (o == null || getClass() != o.getClass()) return false; | |
| KeyRef keyRef = (KeyRef) o; | |
| return Objects.equals(key, keyRef.key) && Objects.equals(value, keyRef.value); | |
| } | |
| @Override | |
| public int hashCode() { | |
| return Objects.hash(key, value); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/flipkart/zjsonpatch/JsonPointer.java between lines 412 and
435, the KeyRef class fields key and value are not declared final, which
compromises immutability. To fix this, declare both key and value fields as
final to ensure they cannot be modified after construction, aligning with the
immutability principle stated for RefToken.
|
Thanks @KaiStapel for the PR. I really appreciate it. Please find my general comments as below -
I will look into more details in the files during this week. |
|
@vishwakarma Thanks for your review. Regarding your remarks:
|
#139:
Allow pointing to array elements via a reference field
Example path
/array/id=123/dataSummary by CodeRabbit
Summary by CodeRabbit
/array/id=123/data).~2as a literal equals sign.0.5.0-SNAPSHOT.