gnodet-bot commented on code in PR #27535:
URL: https://github.com/apache/camel/pull/27535#discussion_r4215704036
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveParser.java:
##########
@@ -85,9 +94,47 @@ private DataWeaveAst.Header parseHeader() {
} else if (checkIdentifier("import")) {
// Skip import directives
while (!check(TokenType.EOF) && !checkIdentifier("output") &&
!checkIdentifier("input")
+ && !checkIdentifier("var") && !checkIdentifier("fun")
&& !check(TokenType.HEADER_SEPARATOR)) {
advance();
}
+ } else if (checkIdentifier("var")) {
+ // var declaration in header: emit as local binding in body
+ advance(); // var
+ String name = current().value();
+ advance(); // name
+ expect(TokenType.ASSIGN); // =
+ DataWeaveAst value = parseOr();
+ declarations.add(new DataWeaveAst.VarDecl(name, value, null));
+ } else if (checkIdentifier("fun")) {
+ // fun declaration in header: emit as local function in body
+ advance(); // fun
+ String name = current().value();
+ advance(); // name
+ expect(TokenType.LPAREN);
+ List<String> params = new ArrayList<>();
+ while (!check(TokenType.RPAREN) && !check(TokenType.EOF)) {
+ String paramName = current().value();
+ advance();
+ // Skip type annotation: fun f(a: Number) -> skip ":
Number"
+ if (check(TokenType.COLON)) {
+ advance(); // :
+ advance(); // type name
Review Comment:
⚠️ **Bug (medium): type-annotation skip consumes exactly one token —
compound types like `Array<String>` will corrupt parsing**
The `advance(); // type name` call skips only one token after `:`. A generic
type such as `Array<String>` produces the token stream `Array`, `<`, `String`,
`>` — so the parser would stop after `Array`, leaving `<String>` as live input
and misidentifying the next comma or closing paren.
Same pattern is repeated in the return-type skip at line 131–133 in this
same `parseHeader` fun block, in `parseFunDecl` (lines 212–225), and in
`parseLambda` (lines 585–587). A shared helper that loop-advances over a
balanced type expression would fix all three sites at once.
```java
// Suggested helper — handles Array<String>, Union<A, B>, etc.
private void skipTypeAnnotation() {
advance(); // consume the base type token
// consume a balanced generic/union suffix: <...>, |T, etc.
if (check(TokenType.LT)) {
int depth = 1;
advance();
while (depth > 0 && !check(TokenType.EOF)) {
if (check(TokenType.LT)) depth++;
else if (check(TokenType.GT)) depth--;
advance();
}
}
}
```
*(Same fix applies to lines 131–133, 212–214, 223–225, and 585–587.)*
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -214,8 +220,12 @@ private String emitNode(DataWeaveAst node) {
}
private String emitStringLit(StringLit s) {
- // The lexer preserves escape sequences as-is, so don't double-escape
- return "\"" + s.value().replace("\"", "\\\"") + "\"";
+ // The lexer preserves escape sequences verbatim (e.g. \" is stored as
\").
+ // Wrapping in double-quotes directly is correct; do NOT replace \"
with \\\"
+ // as that would double-escape an already-escaped sequence.
+ // DataWeave string interpolation "Hello $(expr)" -> DataSonnet "Hello
%(expr)"
+ String value = s.value().replace("$(", "%(");
Review Comment:
💡 **Note (low): string interpolation `$(expr)→%(expr)` is text-level only —
identifiers inside are never rewritten**
`s.value().replace("$(", "%(" )` is a pure text substitution. If the
interpolated expression references `payload` (e.g. `"Hello $(payload.name)"`),
the output will be `"Hello %(payload.name)"` — but `payload` won't be rewritten
to `body` because the interpolated sub-expression is stored as raw text and
never visited by `emitNode()`.
This means the emitted DataSonnet string will still contain `payload`
instead of `body` for interpolated expressions, producing a runtime error or
wrong result.
A proper fix would parse the content inside `$(…)` as a sub-expression, run
`emitNode()` on it, and reassemble. Even a targeted `value.replace("%(payload",
"%(body")` band-aid would be safer than silently emitting broken code.
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -510,9 +535,10 @@ private String emitGroupBy(GroupByExpr gbe) {
if (gbe.lambda() instanceof Lambda lam) {
List<String> paramNames = lambdaParamNames(lam);
String body = emitNode(lam.body());
- return "c.groupBy(" + collection + ", function(" +
paramNames.get(0) + ") " + body + ")";
+ // DataSonnet groupBy requires string keys; wrap with
std.toString() for safety.
+ return "c.groupBy(" + collection + ", function(" +
paramNames.get(0) + ") std.toString(" + body + "))";
}
- return "c.groupBy(" + collection + ", " + emitNode(gbe.lambda()) + ")";
+ return "c.groupBy(" + collection + ", function(x) std.toString(" +
emitNode(gbe.lambda()) + "))";
Review Comment:
💡 **Note (low): `std.toString()` on `groupBy` keys is unconditional**
Wrapping every key in `std.toString()` is safe (DataSonnet's `std.toString`
is a no-op for strings), but adds a small overhead for the common case of
string keys, and makes the emitted code slightly harder to read/debug.
If the DataSonnet `c.groupBy` implementation already coerces keys to strings
internally, this wrapper is redundant. Otherwise it's fine as-is — just worth a
comment explaining why.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
