gnodet-bot commented on code in PR #27552: URL: https://github.com/apache/camel/pull/27552#discussion_r4222108085
########## dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/commands/TransformDataWeaveTest.java: ########## @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.dsl.jbang.core.commands; + +import java.nio.file.Files; +import java.nio.file.Path; + +import org.apache.camel.dsl.jbang.core.common.StringPrinter; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import picocli.CommandLine; + +import static org.assertj.core.api.Assertions.assertThat; + +class TransformDataWeaveTest { + + @TempDir + Path dir; + + /** + * A conditional object element ({ (a: 1) if cond }) is valid DataWeave that the parser does not model yet; it + * causes expect() to throw DataWeaveConversionException. In directory mode the CLI must report the failure for that + * file, continue with the remaining files, and exit non-zero. + */ + @Test + void directoryModeReportsParseErrorPerFileAndContinues() throws Exception { + // valid DataWeave that converts fine + Path good = Files.writeString(dir.resolve("good.dwl"), """ + %dw 2.0 + output application/json + --- + payload + """); + // conditional object element — not yet modelled by the parser, expect() throws + Path bad = Files.writeString(dir.resolve("bad.dwl"), """ + %dw 2.0 + output application/json + --- + { (a: 1) if payload.flag } + """); + + StringPrinter printer = new StringPrinter(); + TransformDataWeave cmd = new TransformDataWeave(new CamelJBangMain().withPrinter(printer)); + CommandLine.populateCommand(cmd, "--input=" + dir.toString()); + int exit = cmd.doCall(); + + assertThat(exit).isEqualTo(1); + String out = printer.getOutput(); + // the bad file's name should be reported + assertThat(out).contains("bad.dwl"); + // the good file should still have been processed (its output lands in the summary) + assertThat(out).contains("Converted:"); Review Comment: ⚠️ **Weak assertion — `contains("Converted:")` passes even when all files fail.** `printSummary()` is called unconditionally after the loop and always emits `"Converted: N expressions"` (via `printer().printErr(...)`). Because no `--output` flag is set in this test, the per-file `"Converted: " + dwlFile + " -> " + outputPath` line is never printed. So the only `"Converted:"` in the output is the summary, which would appear as `"Converted: 0 expressions"` even if _both_ files failed — and this assertion would still pass. A stronger assertion that actually proves `good.dwl` was processed: ```suggestion assertThat(out).contains("// === good.dwl ==="); ``` The multi-file header `// === <filename> ===` is only emitted for successfully converted files, so this directly proves the good file was processed while the bad one was skipped. ########## components/camel-dataweave/src/main/java/org/apache/camel/component/dataweave/DataWeaveParser.java: ########## @@ -745,7 +751,12 @@ private void advance() { private void expect(TokenType type) { if (check(type)) { advance(); + } else { + Token cur = current(); + throw new DataWeaveConversionException( + "DataWeave parse error at " + cur.line() + ":" + cur.col() + + ": expected " + type + " but found " + cur.type() + + " ('" + cur.value() + "')"); Review Comment: ⚠️ **Behavioral interaction: `expect()` now throws, but `parseLambdaOrShorthand()` silently swallows it.** Now that `expect()` throws `DataWeaveConversionException` (a `RuntimeException`) on a token mismatch, the existing `catch (Exception e)` block in `parseLambdaOrShorthand()` (line 483) will catch it. For any lambda-like construct where `parseLambda()` internally calls `expect()` and fails (e.g. `(a +` — an unclosed lambda attempt), the exception is swallowed and `parsePrimary()` is returned instead. The caller sees a best-effort AST fragment rather than the parse error. This is a behavioural change worth pinning with a test in `DataWeaveParserTest`. davsclaus asked for this in the prior review cycle and it was not addressed. A minimal test: ```java @Test void shouldHandleLambdaLikeExpressionGracefully() { // An unclosed lambda attempt '(a +' — parseLambda() calls expect(RPAREN) which now throws. // parseLambdaOrShorthand() catches and falls back to parsePrimary(). // Pin the actual behaviour so future changes to the catch block are intentional. DataWeaveAst ast = parseExpr("(a +"); // Verify we get some AST (not an exception propagated to the caller) assertNotNull(ast); } ``` Without such a test, it's impossible to tell whether the fallback in `parseLambdaOrShorthand()` is intentional or accidental for this specific failure mode. -- 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]
