gnodet-bot commented on code in PR #27535:
URL: https://github.com/apache/camel/pull/27535#discussion_r4220007670
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -510,9 +605,12 @@ 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 + ")";
+ // Jsonnet object keys must be strings. DataWeave allows any type
as a groupBy key,
+ // so we wrap with std.toString() unconditionally to ensure valid
Jsonnet output.
+ // If the key expression is already a string, std.toString() is a
no-op.
+ 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:
🐛 **Bug — `std.toString()` wraps a function value, not its return value**
When `gbe.lambda()` is not a `Lambda` instance, the emitted Jsonnet is:
```jsonnet
c.groupBy(collection, function(x) std.toString(<emittedFunction>))
```
Here `<emittedFunction>` is itself a function expression, so `std.toString`
receives a function object — not the key produced by calling it on `x`. The
correct form is:
```suggestion
return "c.groupBy(" + collection + ", function(x) std.toString(" +
emitNode(gbe.lambda()) + "(x)))";
```
This calls the emitted function on `x` first, then stringifies the result.
##########
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:
🐛 **Bug — `//` line comment in return value breaks sub-expression usage**
When `includeComments` is true, this method returns a string starting with
`// TODO...\n"..."`. That is fine when the call site assigns the result to a
standalone statement, but if the result is embedded in a larger expression —
e.g. as an argument or operator operand — the `//` comment swallows everything
that follows it on the generated line, producing invalid Jsonnet.
Use a block comment (`/* ... */`) instead, which is safe in any position:
```suggestion
return includeComments
? "/* TODO: manual conversion needed -- string
interpolation: \"" + value + "\"*/\n\""
+ value.replace("\"", "\\\"") + "\""
: "\"" + value.replace("\"", "\\\"") + "\"";
```
##########
components/camel-dataweave/src/test/java/org/apache/camel/component/dataweave/DataWeaveConverterTest.java:
##########
@@ -483,8 +484,174 @@ void testStringEscapesPreserved() {
assertTrue(result.contains("\"\\n\""), "Newline escape should be
preserved, got: " + result);
}
+ // -- CAMEL-25324 fixes --
+
+ @Test
+ void testDoubleQuoteNoDoubleEscape() {
+ // DW: "say \"hi\"" -- the lexer stores the backslash-quote verbatim;
do NOT double-escape on emit
+ String result = converter.convertExpression("\"say \\\"hi\\\"\"");
+ assertEquals("\"say \\\"hi\\\"\"", result);
+ }
+
+ @Test
+ void testStringInterpolation() {
+ // DW: "Hello $(payload.name)" -- Jsonnet has no string interpolation,
emit as TODO
+ String result = converter.convertExpression("\"Hello
$(payload.name)\"");
+ assertTrue(result.contains("TODO"), "String interpolation should be a
TODO, got: " + result);
+ assertTrue(converter.getTodoCount() > 0);
+ }
+
+ @Test
+ void testAttributeAccess() {
+ // DW: payload.Order.@id -> DS: body.Order["@id"]
+ String result = converter.convertExpression("payload.Order.@id");
+ assertEquals("body.Order[\"@id\"]", result);
+ }
+
+ @Test
+ void testExistenceCheck() {
+ // DW: payload.a? -> DS: std.objectHas(body, "a") -- using
std.objectHas for FieldAccess
+ String result = converter.convertExpression("payload.a?");
+ assertEquals("std.objectHas(body, \"a\")", result);
+ assertFalse(converter.needsCamelLib());
+ }
+
+ @Test
+ void testDoubleDollarInReduce() {
+ // DW: payload.items reduce ((item, acc = 0) -> acc + item.price) --
explicit lambda
+ String result = converter.convertExpression("payload.items reduce
((item, acc = 0) -> acc + item.price)");
+ assertTrue(result.contains("std.foldl"), "Should use std.foldl, got: "
+ result);
+ assertTrue(result.contains("function(acc, item)"), "acc and item
should be swapped for foldl, got: " + result);
+ }
+
+ @Test
+ void testDoubleDollarLexedCorrectly() {
+ // $$ must lex as DOLLAR_DOLLAR and emit as 'acc' in the converter
+ String result = converter.convertExpression("$$");
+ assertEquals("acc", result);
+ }
+
+ @Test
+ void testVarDeclarationInHeader() {
+ // DW header var declarations must survive as local bindings in the
body
+ String dw = """
+ %dw 2.0
+ output application/json
+ var rate = 0.08
+ ---
+ payload.price * rate
+ """;
+ String result = converter.convert(dw);
+ assertTrue(result.contains("local rate = 0.08"), "var rate must emit
as local rate, got: " + result);
+ assertTrue(result.contains("body.price * rate"), "body expression must
reference rate, got: " + result);
+ }
+
+ @Test
+ void testFunDeclarationInHeader() {
+ // DW header fun declarations must survive as local functions in the
body
+ String dw = """
+ %dw 2.0
+ output application/json
+ fun double(x) = x * 2
+ ---
+ double(payload.value)
+ """;
+ String result = converter.convert(dw);
+ assertTrue(result.contains("local double(x) ="), "fun double must emit
as local function, got: " + result);
+ assertTrue(result.contains("double(body.value)"), "body expression
must call double, got: " + result);
+ }
+
+ @Test
+ void testTypedFunParams() {
+ // DW: fun f(a: Number): Number = a * 2 -- type annotations must be
stripped
+ String result = converter.convertExpression("fun f(a: Number) = a *
2\nf(payload.x)");
+ assertTrue(result.contains("local f(a) ="), "typed param should be
stripped, got: " + result);
+ assertFalse(result.contains("Number"), "type annotation must not
appear in output, got: " + result);
+ }
+
+ @Test
+ void testGroupByKeyStringified() {
+ // DW: payload.items groupBy ((i) -> i.qty) -- groupBy key must be
stringified
+ String result = converter.convertExpression("payload.items groupBy
((i) -> i.qty)");
+ assertTrue(result.contains("c.groupBy("), "Should use c.groupBy, got:
" + result);
+ assertTrue(result.contains("std.toString("), "groupBy key must be
stringified, got: " + result);
+ }
+
+ @Test
+ void testMultiValueSelectorXmlChildren() {
+ // DW: payload.Order.Items.*Item -> DS: std.map(function(x) x.Item,
body.Order.Items)
+ String result =
converter.convertExpression("payload.Order.Items.*Item");
+ assertEquals("std.map(function(x) x.Item, body.Order.Items)", result);
+ assertFalse(converter.needsCamelLib());
+ }
+
// -- Helpers --
+ @Test
+ void testSingleQuotedStringWithDoubleQuote() {
Review Comment:
⚠️ **Test name/intent mismatch**
The method is called `testSingleQuotedStringWithDoubleQuote` but it never
constructs a single-quoted DataWeave string. The input passed to
`convertExpression` is `"\"say \\\"hi\\\"\""` — a double-quoted DW string
literal. A test of the single-quoted path would need to feed a value that the
lexer would produce from a `'say "hi"'` token (i.e. the raw string `say "hi"`
with no surrounding quotes).
Either rename the method to `testDoubleQuotedStringWithEscapedQuote`, or add
a genuinely single-quoted scenario (constructing a `StringLit` directly with
the unescaped value and verifying the re-escaping).
##########
components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveConverter.java:
##########
@@ -254,9 +275,36 @@ private String emitFieldAccess(FieldAccess fa) {
private String emitMultiValueSelector(MultiValueSelector mv) {
String collection = emitNode(mv.object());
+ // DataWeave .*field collects all values for key 'field' from each
element of the collection.
+ // In DataSonnet/Jsonnet: std.map(function(x) x.<field>, collection).
+ // Note: camel.libsonnet has no multiValue helper, so we emit directly.
return "std.map(function(x) x." + mv.field() + ", " + collection + ")";
}
+ private String emitAttributeAccess(AttributeAccess aa) {
+ // DataWeave .@attr accesses an XML attribute; in DataSonnet XML
attributes are exposed
+ // as object keys prefixed with '@', e.g. body.Order['@id'].
+ return emitNode(aa.object()) + "[\"@" + aa.attribute() + "\"]";
+ }
+
+ private String emitExistenceCheck(ExistenceCheck ec) {
+ // DataWeave expr? (key-present selector) returns true/false.
+ // In Jsonnet, accessing a missing field raises an error, so we cannot
simply wrap the expression.
+ // For FieldAccess and AttributeAccess we can use std.objectHas(obj,
"key") safely.
+ // For other shapes the semantics cannot be faithfully reproduced
without a helper; emit as TODO.
+ if (ec.expr() instanceof FieldAccess fa) {
+ return "std.objectHas(" + emitNode(fa.object()) + ", \"" +
fa.field() + "\")";
+ }
+ if (ec.expr() instanceof AttributeAccess aa) {
+ return "std.objectHas(" + emitNode(aa.object()) + ", \"@" +
aa.attribute() + "\")";
+ }
+ todoCount++;
+ return includeComments
+ ? "// TODO: manual conversion needed -- existence check on
non-field expression: "
+ + emitNode(ec.expr()) + "\nfalse"
+ : "false";
Review Comment:
🐛 **Same `//`-comment-in-return-value bug as in `emitStringLit`**
Same fix — use `/* ... */` for the TODO annotation:
```suggestion
return includeComments
? "/* TODO: manual conversion needed -- existence check on
non-field expression: "
+ emitNode(ec.expr()) + "*/\nfalse"
: "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]
