Repository navigation
CAMEL-25490: camel-yaml-io - dump a top-level onException as top-level, not as a step of every route - #27609
Open
Croway wants to merge 3 commits into
Open
CAMEL-25490: camel-yaml-io - dump a top-level onException as top-level, not as a step of every route#27609Croway wants to merge 3 commits into
Croway wants to merge 3 commits into
Conversation
…l, not as a step of every route When routes are prepared, the context scoped onExceptions (the top-level onException of a YAML file, the onException of a RouteBuilder, and those of the route configurations that apply) are added to the outputs of each route. The YAML dumper wrote them as steps of every route, which the YAML DSL schema rejects, so camel validate normalize turned a valid file into an invalid one. The YAML dumper now leaves the context scoped onExceptions out of the route steps, and writes those that do not belong to a route configuration once, as top-level onException entries before the routes.
oscerd
approved these changes
Oct 9, 2026
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
reviewed
Oct 9, 2026
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Solid fix. Context-scoped onExceptions are correctly separated from route-scoped ones, and the identity-based filtering in doWriteOutputs is the right approach given how prepareRoute injects shared references. One low-severity robustness nit below.
Pre-fix test validation ✅: topLevelOnExceptionIsDumpedAsTopLevel — without the fix, a 2-route setup would produce 3 onException: occurrences (1 top-level + 1 per route), so hasSize(2) would fail. routeConfigurationOnExceptionIsNotDumpedInTheRoutes — the test first asserts the onException was injected into route outputs (proving prepareRoute fired), then asserts the dump omits it; without the fix that assertion fails.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 497 of 704 tested, 27 compile-only — current: 497 all testedMaveniverse Scalpel detected 497 affected modules (current approach: 497). Skip-tests mode would test 497 modules (3 direct + 495 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (497)
Modules with tests skipped (27)
Build reactor — dependencies compiled but only changed modules were tested (3 modules, 36.0s total)Total reactor time: 36.0s
Top 20 slowest modules:
|
…ether when they go into one YAML file When the routes are dumped into a single file (dumpRoutesOutput is a file, as camel validate normalize does), the routes of each source file were dumped separately and appended, so the top-level onException of a second file came after the routes of the first file, which the YAML DSL loader rejects. The routes of all the files are now dumped together, so every top-level onException comes before the first route.
gnodet-bot
reviewed
Oct 9, 2026
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the new YAML dump normalization logic — the approach is clean and the test coverage looks good. One null-safety concern in the newly added helper methods.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
… a route configuration Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gnodet-bot
reviewed
Oct 9, 2026
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review after latest commits: both prior findings (null guard in containsInstance) are fully addressed. Deep trace of the context-scoped OE lifecycle confirms the collection, filtering, and top-level emission logic is correct across all paths — started/not-started, route-configuration OEs, and multi-file dump. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
reviewed
Oct 9, 2026
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks @Croway, this is a careful fix with a good write-up, and the round-trip test with schema validation is exactly the right way to pin it. The single-file case reported in CAMEL-25490 looks correct to me:
- Filtering by identity is safe:
RoutesDefinition.prepareRouteadds theOnExceptionDefinitioninstances of theRouteBuilderand of the route configurations themselves to each route (oe.addAll(g.getOnExceptions())), asdesign/cross-cutting-binding.adocalso notes ("the lists are new, the definitions are not"). routeScopeddefaults totrue, so route scoped and not-yet-preparedonExceptions are still written as steps.
A few points, none blocking on their own; the first one is a design question worth settling before the backport (#27610) follows:
-
Several files into one output widens the scope of a top-level
onException(inline onDefaultDumpRoutesStrategy).camel validate normalizealways writes one file, also for the documented--output normalized/ routes/*.yaml, so the normalized output now changes the error handling of the routes of the other files without telling the user. -
Upgrade guide. This changes what YAML route dumps look like (
camel.main.dumpRoutes=yaml, JMXdumpRouteAsYaml/dumpRoutesAsYaml, theroute-dumpdev console) and howdumpRoutesOutput=<file>groups the routes.camel-4x-upgrade-guide-4_23.adocalready has a Route dumps as YAML and Java section (CAMEL-25255) where a short paragraph would fit, for example: a top-levelonException(of a YAML file or aRouteBuilder) is written once as a top-levelonExceptionbefore the routes instead of as a step of every route; theonExceptionof a route configuration is no longer written in the routes; when the routes are dumped into one file, the routes of all the source files are dumped together, so a top-levelonExceptionapplies to all of them when the dump is loaded. If the backport also gets a note for 4.22.2, it goes intocamel-4x-upgrade-guide-4_22.adoconmain. -
Per-route YAML views (inline on
LwModelToYAMLDumper): question about the JMX / dev console / TUI output, and a missing test for the single-route path. -
Test YAML (inline): canonical form for the new test input, and an assertion that makes the scope change in the multi-file test explicit.
-
Attribution (CLAUDE.md, "Attribution"): the last commit carries a
Co-Authored-By: Claudetrailer and the replies are signed as Claude Code, but the PR description has no AI attribution line; please add one, and keep theCo-Authored-Bytrailer in the squash commit (the first two commits do not have it).
CI: only the three changed modules were tested (dependents skipped, over the threshold). I looked at the YAML dump tests of camel-core, camel-management, camel-console, camel-kamelet, camel-jbang and camel-yaml-dsl-validator: none dumps a prepared route with a context scoped onException, so I do not expect breakage there.
Claude Code on behalf of @davsclaus. This is a rules-and-conventions review; it does not replace CodeRabbit, Sourcery or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| } | ||
| Resource res = route.getResource(); | ||
| if (res == null) { | ||
| if (res == null || outputFileName != null) { |
Contributor
There was a problem hiding this 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.yamlwithhandled: trueforjava.lang.Exceptionnow also swallows the errors of the routes ofa.yaml;- if
a.yamlandb.yamlboth handlejava.lang.Exception, both end up at the top with the sameExceptionPolicyKey(no route id, same class, noonWhen), andErrorHandlerSupport.addExceptionPolicykeeps the last one, so the routes ofa.yamlsilently get the handler ofb.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 normalizewith several files and an output directory writes one file per input file (dumpRoutesOutputas 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.
|
|
||
| List<JsonObject> roots = new ArrayList<>(); | ||
| try { | ||
| for (OnExceptionDefinition oe : topLevel) { |
Contributor
There was a problem hiding this 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.
| // 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); |
Contributor
There was a problem hiding this 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:
Suggested change
| assertThat(context.getRouteDefinitions()).hasSize(2); | |
| 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); |
Comment on lines
+45
to
+46
| handled: | ||
| constant: "true" |
Contributor
There was a problem hiding this 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):
Suggested change
| handled: | |
| constant: "true" | |
| handled: | |
| constant: | |
| expression: "true" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA: https://issues.apache.org/jira/browse/CAMEL-25490
Problem
camel validate normalizeturns a valid YAML file with a top-levelonExceptioninto a file thatcamel validate yamlrejects: theonExceptionis written as the first step of every route.camel validate normalize in.yaml --output=out.yaml(4.22.1) writes theonExceptionunderroute.from.steps, and then:The
onExceptionof arouteConfigurationloaded before the routes ends up in the steps of each route as well (and is also dumped in therouteConfiguration).camel validate yamland the compact notation warning recommendcamel validate normalize, so users and AI agents run it and copy its output over valid routes.Cause
When a route is prepared (
RouteDefinitionHelper.prepareRoute/initOnExceptions), the context scoped onExceptions (top-level ones, those of aRouteBuilder, and those of the route configurations that apply) are added to the outputs of the route withrouteScoped=false.LwModelToYAMLDumper.dumpModelAsYamlwrote all the route outputs as steps, but the YAML DSL has noonExceptionamong the steps of a route (see CAMEL-25207).Fix
LwModelToYAMLDumperleaves the context scoped onExceptions out of the route steps, and writes the ones that do not belong to a route configuration once, as top-levelonExceptionentries before the routes. Route scoped onExceptions, such asfrom(...).onException(...)in the Java DSL, are written as before.Several files normalized into one
camel validate normalize a.yaml b.yamlwrites one file:DefaultDumpRoutesStrategydumped the routes of each source file separately and appended them to the same output file. With the fix above, a top-levelonExceptionofb.yamlwould then come after the routes ofa.yaml, which the YAML DSL loader rejects (onException must be defined before any routes). The second commit changesDefaultDumpRoutesStrategy(camel-core-engine): when the routes are dumped into a single file (dumpRoutesOutputis a file name), the routes of all the files are dumped together, so every top-levelonExceptioncomes before the first route. This is outside camel-yaml-io because the dumper only sees one group of routes at a time, and the grouping per file is done by the dump strategy; the change is limited to the YAML routes dump, and dumps into a directory still write one file per source file. As the files are merged into one, a top-levelonExceptionthen applies to the routes of all of them (a single YAML file has no narrower scope for it).Test
OnExceptionYamlDumpTest(camel-yaml-dsl) loads YAML routes, dumps them ascamel.main.dumpRoutesdoes, and loads the dump back with the YAML DSL JSON schema validation ofYamlTestSupport:onExceptionwith two routes, before and after the context is started: dumped once, as top-level, and it still applies to both routes after the reload;routeConfigurationonExceptionloaded before the route: not dumped in the route;onException, dumped withDefaultDumpRoutesStrategyinto one output file ascamel validate normalizedoes: theonExceptioncomes first, and the combined file passes the schema validation and loads both routes.The tests fail without the fixes and pass with them. The camel-yaml-io and camel-yaml-dsl tests pass, and so do the dump tests of camel-core (
DefaultDumpRoutesStrategy*,DumpModel*).Other top-level elements show related but separate problems with
camel validate normalize; they are listed in the JIRA issue and not changed here.