Repository navigation
CAMEL-26484: camel-jbang - AI tools: a JsonPath or SpEL filter in Simple, a jsonpath text that is not a path, {id} for a path parameter and json as a language are caught at write time - #28849
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔁 2 tests passed only after a retry on JDK 21 (2 retried attempts) Recovered flaky tests on JDK 21 (2)
🔬 Scalpel shadow comparison — Scalpel: 7 of 713 tested, 8 compile-only — current: 7 all testedMaveniverse Scalpel detected 7 affected modules (current approach: 7). Skip-tests mode would test 7 modules (2 direct + 6 downstream), skip tests for 8 (generated code, meta-modules) Modules Scalpel would test (7)
Modules with tests skipped (8)
💡 Manual integration tests recommended:
All tested modules (16 modules, 2m 20s total)Total reactor time: 2m 20s
Top 20 slowest modules:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
✅ LGTM — well-structured enhancement adding write-time validation for four AI-model failure modes: REST verbs, path parameters, OpenAPI verbs, and URI JSON path. Thorough test coverage and defensive null handling throughout.
Minor notes (non-blocking):
REST_VERBSandPATH_PARAMETERconstants are defined identically in three files (RouteGraph,ValidatorFacts,OpenApiVerbs). Worth extracting to a shared constants class to avoid drift.- The
Walkrecord now has 15 parameters — approaching builder-pattern territory for readability. - In
ValidatorFacts,uri.indexOf('?')is computed twice in the same expression — assign to a local variable first.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…ple, a jsonpath text that is not a path, {id} for a path parameter and json as a language are caught at write time
In the r25 run of the local-model benchmark, rest-code-first step 1 (answer
one order by its id, or 404) passed in 2 of 7 rounds, against 5 of 7 in r24.
The validator accepted routes that then failed on every request:
- ${bodyAs(List).?[orderId == '${header.id}']} and
${body[?(@.orderId == '${header.id}')]}: a SpEL or JsonPath filter inside
Simple, which Simple reads as a method or a key.
- jsonpath "$[?(...)] {resultType: java.util.Map}": the words of the List
message copied into the text of the path (InvalidPathException).
- jsonpath "$[?(@.orderId == '{id}')]": the text {id}, not the path
parameter, so every id got 404.
- The JSON answer of the 404 under json:, told "did you mean 'js'?" 4 times,
then written as jsonpath, which failed on a null body.
What changed, at write time:
- A JsonPath or SpEL filter inside a Simple function is reported, with the
jsonpath step that picks the element, as YAML lines.
- The text of a jsonpath expression is read as a path. Text after the path, a
bracket or a quote that is not closed, and a JSON text are reported, with
the lines to write. The JsonPath library is not added: with the JSON parser
it needs, it is four more jars in the CLI. So the check is light, and it
reports only what the JsonPath compiler refuses.
- {id} in a simple, constant or jsonpath text, or in a log message, is
reported when a REST operation with the path parameter id reaches the
route: it is the literal text, and the path parameter is the header
${header.id}. The REST operations of the other files of the project, and of
an OpenAPI specification a file binds to, count too.
- json: under expression: says that json is a data format, not a language,
and gives the JSON text with constant (simple when it has ${...}) in place
of "did you mean 'js'?".
- The List message gives the jsonpath step as YAML lines, with expression:
and resultType: as two keys, and with the header of the route's own path
parameter when its name matches the field.
The new checks report nothing on the reference files of the benchmark steps,
on the YAML examples of the simple, jsonpath and REST DSL docs, or on the
1238 right final versions of the r24 and r25 runs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016Y4dDi5iQNgtHrDdz2NpwF
876164a to
2703855
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Nice addition to camel-jbang AI tooling — the write-time validation catches several easy-to-miss mistakes (JsonPath text vs path, SpEL/Simple confusion, path-parameter hygiene) that would otherwise silently produce bad routes. Two minor style observations:
Double indexOf('?') call (ValidatorFacts.java:174)
return uri == null ? null : uri.indexOf('?') > 0 ? uri.substring(0, uri.indexOf('?')) : uri.trim();uri.indexOf('?') is computed twice. Extract it:
if (uri == null) return null;
int q = uri.indexOf('?');
return q > 0 ? uri.substring(0, q) : uri.trim();Duplicated constants across three files
PATH_PARAMETER (Pattern.compile("\\{([A-Za-z_][\\w.-]*)}") ) appears identically in OpenApiVerbs, ValidatorFacts, and RouteGraph. REST_VERBS (List.of("get","post","put","delete","patch","head")) appears in both ValidatorFacts and RouteGraph. Worth extracting to a small shared constants class (or promoting to the existing RouteGraph as package-private statics) to avoid the three definitions drifting independently.
These are low-severity nits — the logic is sound.
…e ai package, and the ? of a uri found once
From the review: OpenApiVerbs reads the PATH_PARAMETER of ValidatorFacts
instead of a copy of its own, and the uri and directory helpers compute
indexOf('?') once.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016Y4dDi5iQNgtHrDdz2NpwF
|
Thanks, both done in the last commit:
Validator tests: 345 pass. The AI tool tests of camel-jbang-core: 837 pass. Claude Code on behalf of davsclaus |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Source-based (the JDK21 build (21, false) check is still queued in CI, not failed — worth letting it finish before merge). This continues the same static-analysis approach as the sibling authoring-tool PRs and keeps to it: JsonPathText, SimpleChecks, the path-parameter and OpenAPI-verb checks all parse with snakeyaml SafeConstructor and otherwise work over the parsed structure plus regex, with no Runtime/ProcessBuilder, Class.forName/loadClass, URL/Socket/HTTP, CamelContext or expression evaluation. The "jsonpath evaluates before it compiles" / "constant vs simple evaluates it" lines are the checks describing the runtime's behavior to the author, not performing it — so this stays a read-only source analysis.
Coverage is solid and again tests both directions: a JsonPath/SpEL filter written inside a Simple function is reported and tied back to the right step (aFilterOfAnotherLanguageInAFunctionIsReportedWithTheJsonPathStep), while whatIsNotAFilterOfAnotherLanguageIsNotReported guards against over-flagging; json used as a language is identified as a data format; and the {id} path-parameter handling is covered for literal, REST and project-wide cases. Consistent and safe. LGTM.
Claude Code on behalf of oscerd
…m source (#28886) camel-jbang-core calls the API of camel-yaml-dsl-validator. When a change on main touches both, the workflow compiled camel-jbang-core from source against the published validator snapshot and failed until the next deploy (seen on #28885 after #28849). Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
https://issues.apache.org/jira/browse/CAMEL-26484
In the r25 run of the local-model benchmark (qwen3.6 35B on Ollama), contracts-rest-code-first step 1 passed in 2 of 7 rounds, against 5 of 7 in r24. The step asks to answer one order by its id, or 404. The validator accepted routes that then failed on every request:
${bodyAs(List).?[orderId == '${header.id}']}and${body[?(@.orderId == '${header.id}')]}: a SpEL or JsonPath filter inside Simple, which Simple reads as a method or a key.$[?(...)] {resultType: java.util.Map}: the model copied the words of the List message into the text of the path (InvalidPathException).$[?(@.orderId == '{id}')]: the text{id}, not the path parameter, so every id got 404.json:: it was told "did you mean 'js'?" 4 times, then wrote it as jsonpath, which failed on a null body.What changes, at write time
{id}is reported when a REST operation with the path parameter id reaches the route. This covers a simple, constant or jsonpath text, and a log message. The message says it is the literal text, and that the path parameter is the header${header.id}.json:underexpression:gets the answer that json is a data format, not a language. It gives the JSON text with constant, or with simple when it has${...}. This replaces "did you mean 'js'?".expression:andresultType:as two keys. When the route's own path parameter matches the field, it uses that header.Tests
🤖 Generated with Claude Code
https://claude.ai/code/session_016Y4dDi5iQNgtHrDdz2NpwF