davsclaus commented on code in PR #27609:
URL: https://github.com/apache/camel/pull/27609#discussion_r4232654570
##########
core/camel-core-engine/src/main/java/org/apache/camel/impl/DefaultDumpRoutesStrategy.java:
##########
@@ -523,7 +523,9 @@ protected void doDumpRoutesAsYaml(CamelContext
camelContext) {
continue;
}
Resource res = route.getResource();
- if (res == null) {
+ if (res == null || outputFileName != null) {
Review Comment:
**Scope change when several files are dumped into one file.** A top-level
`onException` in a YAML file applies only to the routes of that file
(`YamlRoutesBuilderLoader` adds it to the `RoutesDefinition` of that file).
With the routes of all the files dumped together, it is written once at the top
and, when the output is loaded, applies to the routes of every file:
- `b.yaml` with `handled: true` for `java.lang.Exception` now also swallows
the errors of the routes of `a.yaml`;
- if `a.yaml` and `b.yaml` both handle `java.lang.Exception`, both end up at
the top with the same `ExceptionPolicyKey` (no route id, same class, no
`onWhen`), and `ErrorHandlerSupport.addExceptionPolicy` keeps the last one, so
the routes of `a.yaml` silently get the handler of `b.yaml`.
Before this PR the multi-file output failed the schema, but each copy stayed
in the routes of its own file. The PR description mentions the wider scope, but
nothing tells the user, and `camel validate normalize` always writes one file
(also for the documented `--output normalized/ routes/*.yaml`). Some options,
not necessarily all:
- a WARN when routes of more than one resource are merged into one file and
one of them has a top-level `onException` (it then applies to the routes of the
other files);
- a follow-up so that `camel validate normalize` with several files and an
output directory writes one file per input file (`dumpRoutesOutput` as a
directory already dumps one file per resource), which keeps the scope exactly;
- the upgrade-guide note mentioned in the review body.
Smaller point on the same line: with `outputFileName != null` every route
goes under `dummy`, so a dump into a file with `dumpRoutesLog=true` (the
default) no longer logs the `Source: foo.yaml` header, even when all the routes
come from one file. Merging into one group only when the routes come from more
than one resource would keep the single-file case as it was.
##########
core/camel-yaml-io/src/main/java/org/apache/camel/yaml/LwModelToYAMLDumper.java:
##########
@@ -178,6 +203,9 @@ String resolve(String value, Properties properties) {
List<JsonObject> roots = new ArrayList<>();
try {
+ for (OnExceptionDefinition oe : topLevel) {
Review Comment:
Question: for a single `RouteDefinition` (JMX
`ManagedRoute.dumpRouteAsYaml`, the `route-dump` dev console, the TUI source
view in YAML) the dump now starts with the top-level `onException` items before
the `- route:`, and the `onException` of a route configuration is no longer
shown at all, while the XML dump of the same route still shows it inside the
route. That looks like the right trade-off for valid YAML, but is it intended
for these views? This branch (`definition instanceof RouteDefinition`) has no
test; a small assertion on `dumpModelAsYaml(context,
context.getRouteDefinition("first"), ...)` would pin it.
##########
dsl/camel-yaml-dsl/camel-yaml-dsl/src/test/java/org/apache/camel/dsl/yaml/OnExceptionYamlDumpTest.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.yaml;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+
+import org.apache.camel.dsl.yaml.support.YamlTestSupport;
+import org.apache.camel.impl.DefaultDumpRoutesStrategy;
+import org.apache.camel.model.OnExceptionDefinition;
+import org.apache.camel.model.RouteDefinition;
+import org.apache.camel.model.RoutesDefinition;
+import org.apache.camel.yaml.LwModelToYAMLDumper;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class OnExceptionYamlDumpTest extends YamlTestSupport {
+
+ @ParameterizedTest
+ @ValueSource(booleans = { false, true })
+ void topLevelOnExceptionIsDumpedAsTopLevel(boolean started) throws
Exception {
+ loadRoutes("""
+ - onException:
+ exception:
+ - java.lang.Exception
+ handled:
+ constant: "true"
Review Comment:
Nit: new tests should use the canonical YAML DSL form rather than the
classic shorthand (the same applies to lines 92-93 and 129-130):
```suggestion
handled:
constant:
expression: "true"
```
##########
dsl/camel-yaml-dsl/camel-yaml-dsl/src/test/java/org/apache/camel/dsl/yaml/OnExceptionYamlDumpTest.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.yaml;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+
+import org.apache.camel.dsl.yaml.support.YamlTestSupport;
+import org.apache.camel.impl.DefaultDumpRoutesStrategy;
+import org.apache.camel.model.OnExceptionDefinition;
+import org.apache.camel.model.RouteDefinition;
+import org.apache.camel.model.RoutesDefinition;
+import org.apache.camel.yaml.LwModelToYAMLDumper;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class OnExceptionYamlDumpTest extends YamlTestSupport {
+
+ @ParameterizedTest
+ @ValueSource(booleans = { false, true })
+ void topLevelOnExceptionIsDumpedAsTopLevel(boolean started) throws
Exception {
+ loadRoutes("""
+ - onException:
+ exception:
+ - java.lang.Exception
+ handled:
+ constant: "true"
+ steps:
+ - to: mock:error
+ - route:
+ id: first
+ from:
+ uri: direct:first
+ steps:
+ - to: mock:first
+ - route:
+ id: second
+ from:
+ uri: direct:second
+ steps:
+ - to: mock:second
+ """);
+ if (started) {
+ context.start();
+ }
+ String yaml = dumpRoutes();
+ if (started) {
+ context.stop();
+ }
+
+ // written once, before the routes, and not in the steps of each route
+ assertThat(yaml).startsWith("- onException:");
+ assertThat(yaml.split("onException:", -1)).hasSize(2);
+
+ // the dump is valid for the YAML DSL schema, and the onException
still applies to all the routes
+
context.removeRouteDefinitions(List.copyOf(context.getRouteDefinitions()));
+ loadRoutes(yaml);
+ for (String id : List.of("first", "second")) {
+ OnExceptionDefinition oe = (OnExceptionDefinition)
context.getRouteDefinition(id).getOutputs().get(0);
+ assertThat(oe.isRouteScoped()).isFalse();
+
assertThat(oe.getExceptions()).containsExactly("java.lang.Exception");
+ }
+ }
+
+ @Test
+ void routeConfigurationOnExceptionIsNotDumpedInTheRoutes() throws
Exception {
+ loadRoutes("""
+ - routeConfiguration:
+ onException:
+ - onException:
+ exception:
+ - java.lang.Exception
+ handled:
+ constant: "true"
+ steps:
+ - to: mock:error
+ """);
+ loadRoutes("""
+ - route:
+ id: myRoute
+ from:
+ uri: direct:start
+ steps:
+ - to: mock:result
+ """);
+
assertThat(context.getRouteDefinition("myRoute").getOutputs().get(0)).isInstanceOf(OnExceptionDefinition.class);
+
+ // the onException is dumped with the route configuration
+ String yaml = dumpRoutes();
+ assertThat(yaml).doesNotContain("onException").contains("id: myRoute");
+ context.removeRouteDefinition(context.getRouteDefinition("myRoute"));
+ loadRoutes(yaml);
+ }
+
+ @Test
+ void onExceptionOfAnotherFileIsDumpedBeforeTheRoutesInOneFile(@TempDir
Path dir) throws Exception {
+ // two files normalized into one, as camel validate normalize a.yaml
b.yaml does: the onException of the
+ // second file must come before the route of the first one, or the
dump does not load
+ loadRoutes("""
+ - route:
+ id: first
+ from:
+ uri: direct:first
+ steps:
+ - to: mock:first
+ """, """
+ - onException:
+ exception:
+ - java.lang.Exception
+ handled:
+ constant: "true"
+ steps:
+ - to: mock:error
+ - route:
+ id: second
+ from:
+ uri: direct:second
+ steps:
+ - to: mock:second
+ """);
+
+ Path output = dir.resolve("normalized.yaml");
+ DefaultDumpRoutesStrategy dump = new DefaultDumpRoutesStrategy();
+ dump.setCamelContext(context);
+ dump.setInclude("routes");
+ dump.setLog(false);
+ dump.setUriAsParameters(true);
+ dump.setOutput(output.toString());
+ dump.dumpRoutes("yaml");
+ String yaml = Files.readString(output);
+
+ assertThat(yaml).startsWith("- onException:");
+ assertThat(yaml.split("onException:", -1)).hasSize(2);
+ assertThat(yaml).contains("id: first", "id: second");
+
+ // the combined dump is valid for the YAML DSL schema and loads
+
context.removeRouteDefinitions(List.copyOf(context.getRouteDefinitions()));
+ loadRoutes(yaml);
+ assertThat(context.getRouteDefinitions()).hasSize(2);
Review Comment:
If the wider scope stays, asserting it makes it explicit that the
`onException` of the second file now applies to the route of the first file too:
```suggestion
assertThat(context.getRouteDefinitions()).hasSize(2);
// the top-level onException of the second file now applies to the
route of the first file as well
assertThat(context.getRouteDefinition("first").getOutputs().get(0)).isInstanceOf(OnExceptionDefinition.class);
```
--
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]
