gnodet-bot commented on code in PR #27535:
URL: https://github.com/apache/camel/pull/27535#discussion_r4220641785
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -479,7 +527,98 @@ private String emitReduce(ReduceExpr re) {
+ collection + ", " + init + ")";
}
}
- return "std.foldl(" + emitNode(re.lambda()) + ", " + collection + ",
null)";
+ // Shorthand form: payload.items reduce ($$ + $.price)
+ // $$ is the accumulator ($$ -> acc) and $ is the current item ($ ->
item).
+ // DataWeave shorthand without an explicit initial value uses the
first element
+ // as the starting accumulator: std.foldl(function(acc, item) body,
arr[1:], arr[0]).
+ String body = emitReduceShorthandBody(re.lambda());
+ return "local _arr = " + collection + ";\n"
+ + "std.foldl(function(acc, item) " + body + ", _arr[1:],
_arr[0])";
Review Comment:
⚠️ **Bug: `_arr[0]` crashes on empty arrays**
When the collection is empty at runtime, `_arr[0]` raises a Jsonnet `Array
index 0 out of bounds, not in [0, 0)` error. DataWeave's `reduce` without an
initial value on an empty list returns `null`. The generated code should match
that behaviour.
```suggestion
+ "if std.length(_arr) == 0 then null else
std.foldl(function(acc, item) " + body + ", _arr[1:], _arr[0])";
```
##########
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) or (a:
Array<Number>) -> skip to next param
+ if (check(TokenType.COLON)) {
+ advance(); // :
+ skipTypeExpression(); // type expression (simple,
generic, or union)
+ }
+ params.add(paramName);
+ if (check(TokenType.COMMA)) {
+ advance();
+ }
+ }
+ expect(TokenType.RPAREN);
+ // Optional return type annotation: fun f(a): Number = ...
+ if (check(TokenType.COLON)) {
+ advance(); // :
+ skipTypeExpression(); // return type
+ }
+ expect(TokenType.ASSIGN); // =
+ DataWeaveAst funBody = parseOr();
Review Comment:
⚠️ **Bug: header `fun` body uses `parseOr()` instead of `parseExpression()`**
`parseFunDecl()` (in-body) uses `parseExpression()` which handles `if/else`,
`var`, `fun`, `do`, and `using` before calling `parseOr()`. This header path
uses only `parseOr()`, so a header function like:
```
fun toUpper(x) = if (x != null) upper(x) else ""
```
would be silently misparsed — the `if` is not consumed as part of the body.
Same issue applies to the header `var` value at line 130: `DataWeaveAst
value = parseOr()` — complex var values with `if/else` would also be truncated.
```suggestion
DataWeaveAst funBody = parseExpression();
```
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -479,7 +527,98 @@ private String emitReduce(ReduceExpr re) {
+ collection + ", " + init + ")";
}
}
- return "std.foldl(" + emitNode(re.lambda()) + ", " + collection + ",
null)";
+ // Shorthand form: payload.items reduce ($$ + $.price)
+ // $$ is the accumulator ($$ -> acc) and $ is the current item ($ ->
item).
+ // DataWeave shorthand without an explicit initial value uses the
first element
+ // as the starting accumulator: std.foldl(function(acc, item) body,
arr[1:], arr[0]).
+ String body = emitReduceShorthandBody(re.lambda());
+ return "local _arr = " + collection + ";\n"
+ + "std.foldl(function(acc, item) " + body + ", _arr[1:],
_arr[0])";
+ }
+
+ /**
+ * Emit a reduce shorthand body, rewriting {@code $$} to {@code acc} and
{@code $} (optionally with field access) to
+ * {@code item} or {@code item.field}. Falls back to normal {@code
emitNode} for any sub-expression that doesn't
+ * contain shorthand references.
+ */
+ private String emitReduceShorthandBody(DataWeaveAst node) {
+ if (node instanceof DoubleDollar) {
+ return "acc";
+ }
+ if (node instanceof LambdaShorthand ls) {
+ if (ls.fields().isEmpty()) {
+ return "item";
+ }
+ return "item." + String.join(".", ls.fields());
+ }
+ if (node instanceof BinaryOp op) {
+ String left = emitReduceShorthandBody(op.left());
+ String right = emitReduceShorthandBody(op.right());
+ return switch (op.op()) {
+ case "++" -> left + " + " + right;
+ case "and" -> left + " && " + right;
+ case "or" -> left + " || " + right;
+ default -> left + " " + op.op() + " " + right;
+ };
+ }
+ if (node instanceof Parens p) {
+ return "(" + emitReduceShorthandBody(p.expr()) + ")";
+ }
+ if (node instanceof FieldAccess fa) {
+ return emitReduceShorthandBody(fa.object()) + "." + fa.field();
+ }
+ if (node instanceof UnaryOp op) {
+ return switch (op.op()) {
+ case "not" -> "!" + emitReduceShorthandBody(op.operand());
+ default -> op.op() + emitReduceShorthandBody(op.operand());
+ };
+ }
+ // For anything else: if the sub-expression contains a shorthand
reference ($ or $$)
+ // that emitNode cannot rewrite, emit a TODO to avoid silently
producing wrong code.
+ // Pure literals and identifiers without shorthand references are safe
to emit normally.
+ if (containsShorthand(node)) {
+ todoCount++;
+ return includeComments
+ ? "/* TODO: manual conversion needed -- reduce shorthand
in unsupported context: "
+ + node.getClass().getSimpleName() + "*/\nnull"
+ : "null";
+ }
+ return emitNode(node);
+ }
+
+ /**
+ * Returns true if the given AST node or any of its children contain a
LambdaShorthand ($) or DoubleDollar ($$) that
+ * would be emitted incorrectly by the normal emitNode path in a reduce
shorthand context.
+ */
+ private boolean containsShorthand(DataWeaveAst node) {
+ if (node == null) {
+ return false;
Review Comment:
⚠️ **Bug: `containsShorthand()` misses `IndexAccess` — shorthand inside
`$[n]` falls through to `emitNode` undetected**
If a reduce shorthand body contains `$[0]`, the parser produces
`IndexAccess(LambdaShorthand([]), NumberLit("0"))`. `containsShorthand` returns
`false` here (no `IndexAccess` branch), so `emitReduceShorthandBody` calls
`emitNode(IndexAccess)`, which emits `function(x) x[0]` — not `item[0]`.
Fix `containsShorthand` and add an `IndexAccess` handler in
`emitReduceShorthandBody`:
```suggestion
if (node instanceof IndexAccess ia) {
return containsShorthand(ia.object()) ||
containsShorthand(ia.index());
}
return false;
```
--
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]
