davsclaus commented on code in PR #26636:
URL: https://github.com/apache/camel/pull/26636#discussion_r4057776734
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc:
##########
@@ -782,6 +782,12 @@ cost of a slower startup. If you passed `--runtime=main`
explicitly and want the
run, use `--runtime=jbang` (or leave the option out). The options
`--background`, `--code`, `--open-api`,
`--empty` and `--mcp-stdio` are not supported with `--runtime=main`.
+When a YAML route file does not load, `camel run` now prints the report of
`camel validate yaml` for the route
Review Comment:
This paragraph lands between the `--runtime=main` paragraph and its
continuation ("Running an existing Maven project (`camel run pom.xml`) is
unchanged…"), so the `--runtime` explanation is now split in two. Suggest
moving it after that continuation paragraph, or to the end of the `===
camel-jbang` section.
Also, CLAUDE.md keeps the upgrade guide migration-only. The part that
matters for upgraders is the last sentence: the file watcher (camel-support)
now emits `CamelContextReloadFailure` per failed file reload, so an existing
listener will fire for a single-file failure that is not a full context reload.
The description of the `camel run` report is a feature and could be trimmed to
a short mention.
##########
dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/commands/ai/YamlLoadFailureReportTest.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.ai;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.camel.RuntimeCamelException;
+import org.apache.camel.dsl.yaml.common.exception.YamlDeserializationException;
+import org.apache.camel.impl.engine.SimpleCamelContext;
+import org.apache.camel.impl.event.CamelContextReloadFailureEvent;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** CAMEL-24851: a YAML file that did not load gets the validator's report,
which says what to write. */
+class YamlLoadFailureReportTest {
+
+ @TempDir
+ Path dir;
+
+ private static final String BAD_ROUTE = """
+ - route:
+ from:
+ uri: timer:tick
+ steps:
+ - pollEnrich:
+ uri: file:./order.json
+ - to:
+ uri: log:done
+ """;
+
+ @Test
+ void aDeserializationErrorIsAYamlLoadFailure() {
+ Exception loader
+ = new YamlDeserializationException("Error constructing YAML
node id: pollEnrich: unsupported field: uri");
+ assertThat(YamlLoadFailureReport.isYamlLoadFailure(new
RuntimeCamelException("Error starting Camel", loader))).isTrue();
+ assertThat(
+ YamlLoadFailureReport.isYamlLoadFailure(new
RuntimeCamelException("Error pre-parsing resource: file:x.yaml")))
+ .isTrue();
+ assertThat(YamlLoadFailureReport.isYamlLoadFailure(new
IllegalArgumentException("Invalid directory: archived/${x}")))
+ .isFalse();
+ }
+
+ @Test
+ void theReportSaysWhatToWrite() throws Exception {
+ Path route = dir.resolve("orders.camel.yaml");
+ Files.writeString(route, BAD_ROUTE);
+ List<String> lines = YamlLoadFailureReport.report(List.of(route));
+ assertThat(lines).hasSize(4);
+ assertThat(lines.get(0)).isEqualTo("The route file did not load. camel
validate yaml says what to write:");
+ assertThat(lines.get(1)).isEqualTo(" orders.camel.yaml:");
+ // the schema message (with the CAMEL-24850 hint once it is in: "write
pollEnrich: {expression: ...}")
+ assertThat(lines.get(2)).startsWith("
").contains("pollEnrich").contains("'uri'");
+ assertThat(lines.get(3)).startsWith(" (camel validate yaml <file> for
the full report");
+ }
+
+ @Test
+ void aFileTheValidatorAcceptsGivesNoReport() throws Exception {
+ Path route = dir.resolve("ok.camel.yaml");
+ Files.writeString(route, BAD_ROUTE.replace("uri: file:./order.json",
+ "expression:\n constant:\n
expression: \"file:./order.json\""));
+ assertThat(YamlLoadFailureReport.report(List.of(route))).isEmpty();
+ }
+
+ @Test
+ void theRunFilesAreFilteredToLocalYaml() throws Exception {
+ Path route = dir.resolve("a.yaml");
+ Files.writeString(route, BAD_ROUTE);
+ List<Path> files
+ = YamlLoadFailureReport.yamlFiles(List.of("file:" + route,
route.toString(), dir.resolve("Foo.java").toString(),
+ "github:apache:camel:x.yaml",
dir.resolve("missing.yaml").toString()));
+ assertThat(files).containsExactly(route, route);
Review Comment:
This asserts that the same file listed twice (`file:` form and plain) is
returned twice, which would print the report block twice. `Run` de-duplicates
`files` (line 842) so it does not happen in practice, but a `distinct()` in
`yamlFiles` would make the helper self-contained and let this assert
`containsExactly(route)`, which reads as the intent.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/YamlLoadFailureReport.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.ai;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.Consumer;
+
+import org.apache.camel.catalog.CamelCatalog;
+import org.apache.camel.catalog.DefaultCamelCatalog;
+import org.apache.camel.dsl.yaml.common.exception.YamlDeserializationException;
+import org.apache.camel.impl.event.CamelContextReloadFailureEvent;
+import org.apache.camel.spi.CamelEvent;
+import org.apache.camel.support.SimpleEventNotifierSupport;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When a YAML route file fails to load, the loader's message says which node
is wrong but not what to write; the schema
+ * validator has the message that does, with its hints. This runs the
validator on the route files of a failed start (or
+ * of a failed reload in dev mode) and prints its report, so the hints reach
everyone who runs, whether they validated
+ * first or not (CAMEL-24851).
+ */
+public final class YamlLoadFailureReport {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(YamlLoadFailureReport.class);
+
+ private static volatile CamelCatalog catalog;
+
+ private YamlLoadFailureReport() {
+ }
+
+ /** Whether the failure comes from loading a YAML route file: a
deserialization error or a pre-parse error. */
+ public static boolean isYamlLoadFailure(Throwable failure) {
+ for (Throwable t = failure; t != null; t = t.getCause() == t ? null :
t.getCause()) {
+ if (t instanceof YamlDeserializationException) {
+ return true;
+ }
+ String msg = t.getMessage();
+ if (msg != null && (msg.startsWith("Error pre-parsing resource")
|| msg.contains("Error constructing YAML node")
+ || msg.contains("Error parsing YAML"))) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Logs the validator's report for the YAML files of a run whose start
failed on loading one of them; nothing for
+ * any other failure. Logged (not printed) so that it is in the console
and in the run's log file alike, where the
+ * camel-jbang-mcp tools read it, and before the loader's own error.
+ */
+ public static void logIfYamlLoadFailure(Throwable failure, List<String>
files) {
+ if (!isYamlLoadFailure(failure)) {
+ return;
+ }
+ List<String> lines = report(yamlFiles(files));
+ if (!lines.isEmpty()) {
+ LOG.error(String.join(System.lineSeparator(), lines));
+ }
+ }
+
+ /**
+ * The validator's report for the YAML files, as lines to print: a header,
then per file with problems its name and
+ * the messages, then a footer; empty when the validator finds nothing
(then the loader's message is all there is).
+ */
+ public static List<String> report(List<Path> yamlFiles) {
+ List<String> lines = new ArrayList<>();
+ for (Path file : yamlFiles) {
+ if (!Files.isRegularFile(file)) {
+ continue;
+ }
+ List<String> errors;
+ try {
+ String content = Files.readString(file);
+ Path dir = file.toAbsolutePath().getParent();
+ errors =
SourceValidator.validate(file.getFileName().toString(), content, catalog(),
null, dir);
+ } catch (Exception e) {
Review Comment:
Correct that a validator problem must never hide the loader's error, but a
`LOG.debug("Cannot validate {}", file, e)` here would make a broken validator
diagnosable (same for the `catch (Exception ignore)` in `KameletMain.doFail`).
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/YamlLoadFailureReport.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.ai;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.Consumer;
+
+import org.apache.camel.catalog.CamelCatalog;
+import org.apache.camel.catalog.DefaultCamelCatalog;
+import org.apache.camel.dsl.yaml.common.exception.YamlDeserializationException;
+import org.apache.camel.impl.event.CamelContextReloadFailureEvent;
+import org.apache.camel.spi.CamelEvent;
+import org.apache.camel.support.SimpleEventNotifierSupport;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When a YAML route file fails to load, the loader's message says which node
is wrong but not what to write; the schema
+ * validator has the message that does, with its hints. This runs the
validator on the route files of a failed start (or
+ * of a failed reload in dev mode) and prints its report, so the hints reach
everyone who runs, whether they validated
+ * first or not (CAMEL-24851).
+ */
+public final class YamlLoadFailureReport {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(YamlLoadFailureReport.class);
+
+ private static volatile CamelCatalog catalog;
+
+ private YamlLoadFailureReport() {
+ }
+
+ /** Whether the failure comes from loading a YAML route file: a
deserialization error or a pre-parse error. */
+ public static boolean isYamlLoadFailure(Throwable failure) {
+ for (Throwable t = failure; t != null; t = t.getCause() == t ? null :
t.getCause()) {
+ if (t instanceof YamlDeserializationException) {
+ return true;
+ }
+ String msg = t.getMessage();
+ if (msg != null && (msg.startsWith("Error pre-parsing resource")
|| msg.contains("Error constructing YAML node")
+ || msg.contains("Error parsing YAML"))) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Logs the validator's report for the YAML files of a run whose start
failed on loading one of them; nothing for
+ * any other failure. Logged (not printed) so that it is in the console
and in the run's log file alike, where the
+ * camel-jbang-mcp tools read it, and before the loader's own error.
+ */
+ public static void logIfYamlLoadFailure(Throwable failure, List<String>
files) {
+ if (!isYamlLoadFailure(failure)) {
+ return;
+ }
+ List<String> lines = report(yamlFiles(files));
+ if (!lines.isEmpty()) {
+ LOG.error(String.join(System.lineSeparator(), lines));
+ }
+ }
+
+ /**
+ * The validator's report for the YAML files, as lines to print: a header,
then per file with problems its name and
+ * the messages, then a footer; empty when the validator finds nothing
(then the loader's message is all there is).
+ */
+ public static List<String> report(List<Path> yamlFiles) {
+ List<String> lines = new ArrayList<>();
+ for (Path file : yamlFiles) {
+ if (!Files.isRegularFile(file)) {
+ continue;
+ }
+ List<String> errors;
+ try {
+ String content = Files.readString(file);
+ Path dir = file.toAbsolutePath().getParent();
+ errors =
SourceValidator.validate(file.getFileName().toString(), content, catalog(),
null, dir);
+ } catch (Exception e) {
+ continue;
+ }
+ if (!errors.isEmpty()) {
+ if (lines.isEmpty()) {
+ lines.add("The route file did not load. camel validate
yaml says what to write:");
+ }
+ lines.add(" " + file.getFileName() + ":");
+ for (String error : errors) {
+ lines.add(" " + error);
+ }
+ }
+ }
+ if (!lines.isEmpty()) {
+ lines.add(" (camel validate yaml <file> for the full report; the
loader's error follows)");
+ }
+ return lines;
+ }
+
+ /** The YAML route files among the files of a run: local files ending in
.yaml or .yml. */
+ public static List<Path> yamlFiles(List<String> files) {
+ List<Path> answer = new ArrayList<>();
+ if (files == null) {
+ return answer;
+ }
+ for (String f : files) {
+ if (f == null) {
+ continue;
+ }
+ if (f.startsWith("file:")) {
+ f = f.substring(5);
+ }
+ if (f.contains(":")) {
Review Comment:
`f.contains(":")` also skips Windows absolute paths (`C:\routes\a.yaml`), so
those users silently get no report. `Run.java` (around line 1136) makes the
same decision with `ResourceHelper.hasScheme(file) ||
file.startsWith("github:")`, which is drive-letter safe; reusing it keeps the
two in sync (needs the `org.apache.camel.support.ResourceHelper` import).
```suggestion
if (ResourceHelper.hasScheme(f) || f.startsWith("github:")) {
```
##########
dsl/camel-kamelet-main/src/main/java/org/apache/camel/main/KameletMain.java:
##########
@@ -173,8 +176,23 @@ public static void main(String... args) throws Exception {
System.exit(code);
}
+ /**
+ * Called with the exception when Camel fails to start, before the error
is logged: the CLI uses it to report what
+ * the schema validator says about a YAML route file that did not load
(CAMEL-24851).
+ */
+ public void setStartFailureListener(Consumer<Exception>
startFailureListener) {
+ this.startFailureListener = startFailureListener;
+ }
+
@Override
protected void doFail(Exception e) {
+ if (startFailureListener != null) {
+ try {
+ startFailureListener.accept(e);
+ } catch (Exception ignore) {
Review Comment:
Same as in `YamlLoadFailureReport.report`: swallowing is right, but a
`LOG.debug` with the exception would help when the listener itself misbehaves.
--
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]
