Conversation
Implements JsonPath query language for the java.util.json backport based on Stefan Goessner's specification (https://goessner.net/articles/JsonPath/). Features: - Parses JsonPath expressions to AST (sealed interface + records) - Evaluates AST against JsonValue documents from core library - No external dependencies (java.base only) - Java 21 functional style (immutable records, pattern matching) - Pure TDD development with 76 comprehensive tests Supported operators: - Root ($), property access (.name, ['name']) - Array index ([n], [-1]), slice ([:2], [0:10:2]) - Wildcard ([*], .*), recursive descent (..) - Union ([0,1], ['a','b']) - Filter expressions ([?(@.isbn)], [?(@.price<10)]) - Script expressions ([(@.length-1)]) All examples from Goessner article implemented as tests. To verify: mvn test -pl json-java21-jsonpath -Djava.util.logging.ConsoleHandler.level=INFO Co-authored-by: simbo1905 <simbo1905@60hertz.com>
|
Cursor Agent can help with this pull request. Just |
Summary of ChangesHello @simbo1905, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new capability by integrating a native JsonPath query engine into the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Adds alternative API style for JsonPath queries: - JsonPath.parse(expr) returns a compiled JsonPath - .select(json) evaluates against a document - Compiled paths are reusable across multiple documents - Retains original JsonPath.query(expr, json) as convenience method Tests verify: - parse(...).select(...) returns same results as query(...) - Compiled paths can be reused on different documents - expression() accessor returns original path string To verify: mvn test -pl json-java21-jsonpath Co-authored-by: simbo1905 <simbo1905@60hertz.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 262dadaf32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (properties.isEmpty()) { | ||
| // Just @ with no properties | ||
| return new JsonPathAst.PropertyPath(List.of("@")); |
There was a problem hiding this comment.
The parser maps a bare @ to PropertyPath(List.of("@")), which makes filters like [?(@)] or comparisons such as [?(@ == 1)] resolve a property literally named @ instead of the current node. Since resolvePropertyPath only follows object members, these expressions will never match unless the JSON object actually has an "@" key. The AST already has CurrentNode, so this is a behavioral bug where valid JsonPath filters against the current node evaluate as false.
Useful? React with 👍 / 👎.
| default -> { | ||
| // Other segment types in recursive descent context | ||
| LOG.finer(() -> "Unsupported target in recursive descent: " + target); |
There was a problem hiding this comment.
The recursive descent evaluator only handles PropertyAccess, Wildcard, and ArrayIndex targets, and falls through to the default case for others. However, the parser allows recursive descent with bracket notation (e.g. $..['author','title'] or $..[?(@.price<10)]), producing Union, Filter, ArraySlice, or ScriptExpression targets. These are silently ignored here, so those valid expressions yield no matches. Either the parser should reject those forms or evaluateTargetSegment should support them.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new json-java21-jsonpath module, which is a significant and well-structured addition. The use of modern Java features like records and sealed interfaces for the AST is excellent, and the TDD approach with comprehensive tests against the Goessner specification provides a solid foundation. The documentation in AGENTS.md and ARCHITECTURE.md is thorough and very helpful.
My review has identified a few issues, primarily related to edge cases and completeness in the parser and evaluator. Specifically, I've found areas for improvement in handling recursive descent targets, parsing the current node selector (@), evaluating literal values in filters, and a discrepancy between documented and implemented support for logical operators in filters. Addressing these points will enhance the robustness and correctness of this new JsonPath engine.
| if (current instanceof JsonObject obj) { | ||
| for (final var value : obj.members().values()) { | ||
| evaluateSegments(segments, index + 1, value, root, results); | ||
| } | ||
| } else if (current instanceof JsonArray array) { | ||
| for (final var element : array.elements()) { | ||
| evaluateSegments(segments, index + 1, element, root, results); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static void evaluateRecursiveDescent( | ||
| JsonPathAst.RecursiveDescent desc, | ||
| List<JsonPathAst.Segment> segments, | ||
| int index, | ||
| JsonValue current, | ||
| JsonValue root, | ||
| List<JsonValue> results) { | ||
|
|
||
| // First, try matching the target at current level | ||
| evaluateTargetSegment(desc.target(), segments, index, current, root, results); | ||
|
|
||
| // Then recurse into children | ||
| if (current instanceof JsonObject obj) { | ||
| for (final var value : obj.members().values()) { | ||
| evaluateRecursiveDescent(desc, segments, index, value, root, results); | ||
| } | ||
| } else if (current instanceof JsonArray array) { | ||
| for (final var element : array.elements()) { | ||
| evaluateRecursiveDescent(desc, segments, index, element, root, results); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static void evaluateTargetSegment( | ||
| JsonPathAst.Segment target, | ||
| List<JsonPathAst.Segment> segments, | ||
| int index, | ||
| JsonValue current, | ||
| JsonValue root, | ||
| List<JsonValue> results) { | ||
|
|
||
| switch (target) { | ||
| case JsonPathAst.PropertyAccess prop -> { |
There was a problem hiding this comment.
The evaluateTargetSegment method is incomplete. It only handles PropertyAccess, Wildcard, and ArrayIndex as targets for a recursive descent (..). Other valid segments like Filter, ArraySlice, etc., are not supported. This will cause queries like $..[?(@.price<10)] to fail silently, as the default case in the switch statement just logs a message without performing any evaluation.
To fix this, you should add cases for the other Segment types. This will likely introduce some code duplication from the main evaluate* methods, which could be addressed in a separate refactoring. I'd also recommend adding a test case for a path like $..[?(@.price<10)] to verify the fix.
| private JsonPathAst.PropertyPath parseCurrentNodePath() { | ||
| pos++; // skip @ | ||
|
|
||
| final var properties = new ArrayList<String>(); | ||
|
|
||
| while (pos < path.length() && path.charAt(pos) == '.') { | ||
| pos++; // skip . | ||
| final int start = pos; | ||
|
|
||
| while (pos < path.length()) { | ||
| final char c = path.charAt(pos); | ||
| if (!Character.isLetterOrDigit(c) && c != '_') { | ||
| break; | ||
| } | ||
| pos++; | ||
| } | ||
|
|
||
| if (pos == start) { | ||
| throw new JsonPathParseException("Expected property name after '.'", path, pos); | ||
| } | ||
|
|
||
| properties.add(path.substring(start, pos)); | ||
| } | ||
|
|
||
| if (properties.isEmpty()) { | ||
| // Just @ with no properties | ||
| return new JsonPathAst.PropertyPath(List.of("@")); | ||
| } | ||
|
|
||
| return new JsonPathAst.PropertyPath(properties); | ||
| } |
There was a problem hiding this comment.
The parser incorrectly represents the current node selector @ as a PropertyPath with the property name "@". This causes incorrect behavior during filter evaluation, as the evaluator will then try to find a property named "@" instead of using the current context node.
The fix is to modify parseCurrentNodePath to return a JsonPathAst.CurrentNode when it parses a standalone @. This will require changing the method's return type to the more general JsonPathAst.FilterExpression.
private JsonPathAst.FilterExpression parseCurrentNodePath() {
pos++; // skip @
if (pos >= path.length() || path.charAt(pos) != '.') {
return new JsonPathAst.CurrentNode();
}
final var properties = new ArrayList<String>();
while (pos < path.length() && path.charAt(pos) == '.') {
pos++; // skip .
final int start = pos;
while (pos < path.length()) {
final char c = path.charAt(pos);
if (!Character.isLetterOrDigit(c) && c != '_') {
break;
}
pos++;
}
if (pos == start) {
throw new JsonPathParseException("Expected property name after '.'", path, pos);
}
properties.add(path.substring(start, pos));
}
return new JsonPathAst.PropertyPath(properties);
}| <version>0.1.9</version> | ||
| </parent> | ||
|
|
||
| <artifactId>java.util.json.jsonpath</artifactId> |
There was a problem hiding this comment.
The artifactId java.util.json.jsonpath is inconsistent with the module name json-java21-jsonpath defined in the parent pom.xml and the project's directory structure. For better project clarity, consistency, and ease of navigation, it's recommended to align the artifactId with the module name.
| <artifactId>java.util.json.jsonpath</artifactId> | |
| <artifactId>json-java21-jsonpath</artifactId> |
| JsonPathAst.Filter filter, | ||
| List<JsonPathAst.Segment> segments, | ||
| int index, | ||
| JsonValue current, |
There was a problem hiding this comment.
The matchesFilter method currently returns true for any LiteralValue. This means a filter expression like [?(false)] or [?(0)] would incorrectly match all elements. While the JSONPath specification is not explicit about literal truthiness, this behavior is likely unintended and can lead to incorrect query results.
A safer implementation would be to handle boolean literals correctly and either throw an exception or return false for other literal types.
case JsonPathAst.LiteralValue lv -> {
if (lv.value() instanceof Boolean b) {
yield b;
}
// Non-boolean literals in a filter context don't have a clear
// "truthy" definition in JSONPath. Returning false is safer than true.
yield false;
}| private JsonPathAst.FilterExpression parseFilterContent() { | ||
| if (pos >= path.length()) { | ||
| throw new JsonPathParseException("Unexpected end of filter expression", path, pos); | ||
| } | ||
|
|
||
| // Parse the left side (usually @.property) | ||
| final var left = parseFilterAtom(); | ||
|
|
||
| skipWhitespace(); | ||
|
|
||
| // Check if there's a comparison operator | ||
| if (pos < path.length() && path.charAt(pos) != ')') { | ||
| final var op = parseComparisonOp(); | ||
| skipWhitespace(); | ||
| final var right = parseFilterAtom(); | ||
| return new JsonPathAst.ComparisonFilter(left, op, right); | ||
| } | ||
|
|
||
| // No operator means existence check | ||
| if (left instanceof JsonPathAst.PropertyPath pp) { | ||
| return new JsonPathAst.ExistsFilter(pp); | ||
| } | ||
|
|
||
| throw new JsonPathParseException("Invalid filter expression - expected property path", path, pos); | ||
| } |
There was a problem hiding this comment.
The parseFilterContent method currently only parses a single filter condition (either an existence check or a single comparison). It does not support logical operators like && and || to combine multiple conditions, e.g., [?(@.price < 10 && @.isbn)].
However, the AST (LogicalFilter) and evaluator (matchesFilter) include support for these operators, and ARCHITECTURE.md mentions them as "implemented but not extensively tested".
To align the implementation with the documentation, the parser should be enhanced to handle logical expressions, including operator precedence. If supporting logical operators is out of scope for now, the documentation in ARCHITECTURE.md should be updated to reflect that they are not yet implemented in the parser.
2 participants
What changed
A new Maven module
json-java21-jsonpathhas been added, providing a JsonPath query engine for thejava.util.jsonJava 21 backport.This module includes:
JsonValuedocuments from the core library.AGENTS.mdandARCHITECTURE.mdfiles for the module.Why this change is needed
This change addresses the need for a native JsonPath implementation within the
java.util.jsonecosystem. The primary motivations were:java.baseonly constraint.java.util.jsonAPI.How were these changes tested
The changes were developed using a Pure TDD approach.
JsonPathAstTest(17 tests) verifies the correctness of the AST structure.JsonPathParserTest(32 tests) ensures the parser correctly converts various JsonPath expressions into the expected AST.JsonPathGoessnerTest(27 tests) covers all examples from the official JsonPath specification by Stefan Goessner, validating the end-to-end query evaluation against a sample JSON document.Checklist
CODING_STYLE_LLM.mdconvensionsAGENTS.mdupdated if appropriate