Conversation
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
…h locations Co-authored-by: simbo1905 <simbo1905@60hertz.com>
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
…e path normalization 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 significantly expands the project's capabilities by introducing a new 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
|
![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-transforms module, which provides a Java implementation of Microsoft's JSON Document Transforms specification. This is a substantial addition, including the core transformation logic, a new queryMatches capability in the jsonpath module to support it, and extensive test coverage.
My review focuses on the new implementation. I've identified a major maintainability issue in JsonPath.java due to significant code duplication, which should be addressed. I've also found opportunities to improve the robustness and clarity of the new transformation logic in TransformRunner.java and TransformPatch.java. The overall structure of the new module is well-designed, and the addition of comprehensive tests is commendable.
| private static void evaluateSegmentsMatches( | ||
| List<JsonPathAst.Segment> segments, | ||
| int index, | ||
| JsonValue current, | ||
| JsonValue root, | ||
| ArrayList<JsonPathLocationStep> location, | ||
| Consumer<JsonPathMatch> sink) { | ||
|
|
||
| if (index >= segments.size()) { | ||
| sink.accept(new JsonPathMatch(location, current)); | ||
| return; | ||
| } | ||
|
|
||
| final var segment = segments.get(index); | ||
| LOG.finer(() -> "Evaluating segment (matches) " + index + ": " + segment); | ||
|
|
||
| switch (segment) { | ||
| case JsonPathAst.PropertyAccess prop -> | ||
| evaluatePropertyAccessMatches(prop, segments, index, current, root, location, sink); | ||
| case JsonPathAst.ArrayIndex arr -> | ||
| evaluateArrayIndexMatches(arr, segments, index, current, root, location, sink); | ||
| case JsonPathAst.ArraySlice slice -> | ||
| evaluateArraySliceMatches(slice, segments, index, current, root, location, sink); | ||
| case JsonPathAst.Wildcard ignored -> | ||
| evaluateWildcardMatches(segments, index, current, root, location, sink); | ||
| case JsonPathAst.RecursiveDescent desc -> | ||
| evaluateRecursiveDescentMatches(desc, segments, index, current, root, location, sink); | ||
| case JsonPathAst.Filter filter -> | ||
| evaluateFilterMatches(filter, segments, index, current, root, location, sink); | ||
| case JsonPathAst.Union union -> | ||
| evaluateUnionMatches(union, segments, index, current, root, location, sink); | ||
| case JsonPathAst.ScriptExpression script -> | ||
| evaluateScriptExpressionMatches(script, segments, index, current, root, location, sink); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between the new ...Matches methods (like this one) and the original evaluation methods (e.g., evaluateSegments). The entire logic for traversing the JSON document based on the AST segments is repeated, with the only difference being whether to collect just the value or the value with its location.
This duplication creates a significant maintenance burden, as any bug fixes or improvements to the traversal logic will need to be applied in two places.
Consider refactoring to a single, unified evaluation implementation. This could be achieved by parameterizing the evaluation methods with a strategy or functional interface that defines how to handle a match and how to track the path. This would eliminate the duplicated code and make the implementation more robust and easier to maintain.
| private static JsonValue resolve(JsonValue root, List<JsonPathLocationStep> location) { | ||
| JsonValue cur = root; | ||
| for (final var step : location) { | ||
| cur = switch (step) { | ||
| case JsonPathLocationStep.Property p -> cur instanceof JsonObject obj ? obj.members().get(p.name()) : null; | ||
| case JsonPathLocationStep.Index idx -> cur instanceof JsonArray arr ? arr.elements().get(idx.index()) : null; | ||
| }; | ||
| if (cur == null) { | ||
| return null; | ||
| } | ||
| } | ||
| return cur; | ||
| } |
There was a problem hiding this comment.
The resolve method is not robust against an invalid array index in the location path. Accessing arr.elements().get(idx.index()) could throw an IndexOutOfBoundsException if the index is out of bounds. While locations generated from queryMatches should be valid, modifications to the document during a transform could invalidate them for subsequent operations within the same verb.
Adding bounds checking for array access will make this method safer and prevent potential runtime exceptions.
private static JsonValue resolve(JsonValue root, List<JsonPathLocationStep> location) {
JsonValue cur = root;
for (final var step : location) {
cur = switch (step) {
case JsonPathLocationStep.Property p -> cur instanceof JsonObject obj ? obj.members().get(p.name()) : null;
case JsonPathLocationStep.Index idx -> {
if (cur instanceof JsonArray arr) {
final int i = idx.index();
if (i >= 0 && i < arr.elements().size()) {
yield arr.elements().get(i);
}
}
yield null;
}
};
if (cur == null) {
return null;
}
}
return cur;
}| return switch (step) { | ||
| case JsonPathLocationStep.Property p -> { | ||
| if (!(current instanceof JsonObject obj)) yield current; | ||
| if (!obj.members().containsKey(p.name())) yield current; | ||
| final var out = new LinkedHashMap<String, JsonValue>(obj.members().size()); | ||
| for (final var entry : obj.members().entrySet()) { | ||
| if (!entry.getKey().equals(p.name())) { | ||
| out.put(entry.getKey(), entry.getValue()); | ||
| } | ||
| } | ||
| yield JsonObject.of(out); | ||
| } | ||
| case JsonPathLocationStep.Index idx -> { | ||
| if (!(current instanceof JsonArray arr)) yield current; | ||
| final int i = idx.index(); | ||
| if (i < 0 || i >= arr.elements().size()) yield current; | ||
| final var out = new ArrayList<JsonValue>(Math.max(0, arr.elements().size() - 1)); | ||
| for (int j = 0; j < arr.elements().size(); j++) { | ||
| if (j != i) out.add(arr.elements().get(j)); | ||
| } | ||
| yield JsonArray.of(out); | ||
| } |
There was a problem hiding this comment.
The logic for removing an element from a JsonObject or JsonArray can be made more concise and readable. Instead of manually iterating to build a new collection, you can create a mutable copy of the original collection and use the standard remove() method.
return switch (step) {
case JsonPathLocationStep.Property p -> {
if (!(current instanceof JsonObject obj)) yield current;
if (!obj.members().containsKey(p.name())) yield current;
final var out = new LinkedHashMap<String, JsonValue>(obj.members());
out.remove(p.name());
yield JsonObject.of(out);
}
case JsonPathLocationStep.Index idx -> {
if (!(current instanceof JsonArray arr)) yield current;
final int i = idx.index();
if (i < 0 || i >= arr.elements().size()) yield current;
final var out = new ArrayList<JsonValue>(arr.elements());
out.remove(i);
yield JsonArray.of(out);
}
};![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: d9e59644b2
ℹ️ 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".
| // Apply removals; remove from arrays in descending index order for stability. | ||
| final var sorted = sortForRemoval(matches); | ||
| for (final var match : sorted) { | ||
| final var location = match.location(); | ||
| if (location.isEmpty()) { |
There was a problem hiding this comment.
The remove-by-path flow applies every match returned by queryMatches in sequence. If the path expression can produce duplicate locations (e.g., a union with repeated selectors like $.A[0,0]), the second removal will act on the already-shifted array/object and delete an additional element that didn’t originally match. This makes @jdt.remove non-idempotent with duplicate matches and can remove unintended data. Consider de-duplicating locations before removal (or collapsing duplicates during queryMatches) so each location is removed at most once.
Useful? React with 👍 / 👎.
| if (!current.members().containsKey(oldName)) { | ||
| LOG.fine(() -> "Rename mapping skipped; missing key: " + oldName); | ||
| continue; | ||
| } | ||
| current = renameKey(current, oldName, newName); |
There was a problem hiding this comment.
Rename mappings are applied sequentially to a mutating current object. If a mapping contains overlapping keys (e.g., { "A": "B", "B": "C" }), the first rename can overwrite the second key, and the second rename then operates on a modified object, potentially losing data. Since JSON object key order is not semantically meaningful, outcomes become order-dependent. A safer approach is to compute a new object from the original members using the full mapping in one pass so renames are effectively simultaneous.
Useful? React with 👍 / 👎.
2 participants
This pull request contains changes generated by a Cursor Cloud Agent