davsclaus commented on code in PR #27535:
URL: https://github.com/apache/camel/pull/27535#discussion_r4221883077
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/resources/tui/web/vendor/xterm.js:
##########
@@ -1,2 +1,18 @@
+/*
Review Comment:
This is a vendored third-party file under the MIT license (see
`LICENSE-xterm.txt` next to it). Adding the ASF header misstates its license.
Please revert this file, and the other three xterm vendor files,
`camel-sbom/camel-sbom.xml` and `.mvn/extensions.xml`. None of them relate to
this PR.
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -479,7 +527,104 @@ 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"
+ + "if std.length(_arr) == 0 then null else
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 IndexAccess ia) {
+ return emitReduceShorthandBody(ia.object()) + "[" +
emitReduceShorthandBody(ia.index()) + "]";
+ }
+ 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;
+ }
+ if (node instanceof LambdaShorthand || node instanceof DoubleDollar) {
+ return true;
+ }
+ if (node instanceof BinaryOp op) {
+ return containsShorthand(op.left()) ||
containsShorthand(op.right());
+ }
+ if (node instanceof UnaryOp op) {
+ return containsShorthand(op.operand());
+ }
+ if (node instanceof Parens p) {
+ return containsShorthand(p.expr());
+ }
+ if (node instanceof FieldAccess fa) {
+ return containsShorthand(fa.object());
+ }
+ if (node instanceof FunctionCall fc) {
+ return fc.args().stream().anyMatch(this::containsShorthand);
+ }
+ if (node instanceof DefaultExpr def) {
+ return containsShorthand(def.expr()) ||
containsShorthand(def.fallback());
+ }
+ if (node instanceof IfElse ie) {
+ return containsShorthand(ie.condition()) ||
containsShorthand(ie.thenExpr()) || containsShorthand(ie.elseExpr());
+ }
+ if (node instanceof IndexAccess ia) {
+ return containsShorthand(ia.object()) ||
containsShorthand(ia.index());
+ }
+ return false;
Review Comment:
Returning `false` for any node type not listed means unknown shapes fall
through to `emitNode`, which writes `$` as `function(x) x...`. For example:
- `$$ + ($.price as Number)` becomes `cml.toDecimal(function(x) x.price)`;
- `$$ ++ [$.name]` becomes `[function(x) x.name]`.
Both are wrong output with no TODO counted. It would be safer to default to
`true` here and list only the leaves known to be safe (literals, `Identifier`).
Alternatively, set a flag that `emitLambdaShorthand` and `DoubleDollar` check.
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -149,6 +149,12 @@ private String emitNode(DataWeaveAst node) {
return emitNode(ia.object()) + "[" + emitNode(ia.index()) + "]";
} else if (node instanceof MultiValueSelector mv) {
return emitMultiValueSelector(mv);
+ } else if (node instanceof AttributeAccess aa) {
+ return emitAttributeAccess(aa);
+ } else if (node instanceof ExistenceCheck ec) {
+ return emitExistenceCheck(ec);
+ } else if (node instanceof DoubleDollar) {
+ return "acc"; // $$ is the reduce accumulator; use the 'acc' name
convention
Review Comment:
`$$` means the accumulator only inside `reduce`. In `map`/`filter` it is the
index, and in `mapObject`/`pluck` it is the key. So `payload map { i: $$ }`
emits an unbound `acc` and fails at runtime with "Unknown variable acc", with
no TODO counted. Outside the reduce-shorthand path this should be counted as a
TODO.
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -214,8 +220,23 @@ 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("\"", "\\\"") + "\"";
+ String value = s.value();
+ // Jsonnet has no in-string interpolation. DataWeave uses "Hello
$(expr)" but there is
+ // no faithful automated conversion (the sub-expression must be
re-emitted through the AST).
+ // Emit as a TODO comment and a placeholder so the converter output is
syntactically valid.
+ if (value.contains("$(")) {
+ todoCount++;
+ return includeComments
+ ? "/* TODO: manual conversion needed -- string
interpolation: \"" + value + "\"*/\n\""
+ + value.replace("\"", "\\\"") + "\""
+ : "\"" + value.replace("\"", "\\\"") + "\"";
Review Comment:
This branch still uses `value.replace("\"", "\\\"")`, the old double-escape.
`"say \"$(x)\""` therefore emits invalid Jsonnet. It should reuse the same
escaping as the `escaped` value below.
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -479,7 +527,104 @@ 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"
+ + "if std.length(_arr) == 0 then null else
std.foldl(function(acc, item) " + body + ", _arr[1:], _arr[0])";
Review Comment:
The `local _arr = ...; if ... else std.foldl(...)` expression is not
parenthesised. Used as the left operand of a binary operator, the trailing
operator is absorbed into the `else` branch. Wrapping the whole result in `(
... )` avoids that.
##########
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); // =
Review Comment:
A header `var` with a type annotation (`var rate: Number = 0.08`) reaches
`expect(ASSIGN)` while looking at `:`. On this PR alone that produces a garbled
AST. Once #27552 lands, `expect()` throws, so the whole script fails. The
header `fun` path already calls `skipTypeExpression()`, and the same is needed
here. The body-level `parseVarDecl` has the same gap, but that one predates
this PR.
--
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]
