allthingssecurity commented on PR #27745:
URL: https://github.com/apache/camel/pull/27745#issuecomment-6142684533
A late note, as this was merged before I posted it. I looked at
111b1e387bd2; `RouteBuilder.updateTemplatedRoutesToCamelContext` on main today
has the same logic, but I have not re-run the test below on main. It is one
observation for the `routesReloadRemoveAllRoutes=false` mode, in case it is
useful for a follow-up or a doc note.
Thanks for this, the two-pass order in `doUpdateRoutes` (templates of all
builders first, templated routes last) looks
right to me, and the three tests fail without the change (I ran them on the
merge-base).
One thing I ran into, in the `routesReloadRemoveAllRoutes=false` mode only:
**A `templatedRoute` without a `routeId` is now added again on every
reload** (`RouteBuilder.java:841`,
`populateTemplatedRoutes` from `updateTemplatedRoutesToCamelContext`). The
"same template replaces the existing route"
rule in `DefaultModel.doAddRouteFromTemplate` only applies when the new
route has an id, so with a generated id the old
route stays and a new one is created next to it. On main the reload left the
existing route alone (with the stale
parameter).
Reproducer, next to `RouteReloadTemplatedRouteTest` (same
`TestableStrategy`, `setRemoveAllRoutes(false)`), with the
`routeId` line removed from the `templatedRoute`:
```java
@Test
void reloadKeepingTheOtherRoutesTemplatedRouteWithoutId() throws Exception {
// file: routeTemplate "greeting" (from
seda:greet?multipleConsumers=true), a templatedRoute
// WITHOUT routeId, and the route "plain"
assertThat(context.getRoutes()).hasSize(2);
for (int i = 0; i < 3; i++) {
Files.writeString(file, routes("Hi" + i));
strategy.getResourceReload().onReload(file.toString(),
ResourceHelper.resolveResource(context, "file:" + file));
}
assertThat(context.getRoutes()).hasSize(2);
}
```
On this branch: `routes=[route6, plain, route8]`, then `[route6, route8,
plain, route10]`, then
`[route6, route8, route10, plain, route12]` (expected size 2, was 5). On
main the same test passes
(`[route4, plain]` after each reload).
With the template on `direct:greet` (as in the existing test) the reload now
fails instead:
```
FailedToStartRouteException: Failed to start route: route4 because:
Multiple consumers for the same endpoint is not allowed: direct://greet
```
where on main it succeeded.
To be fair, a plain route without an id behaves the same way in this mode
today (I checked: it is duplicated on each
reload on main too), and the "Routes with no id's detected" warning covers
it. So this may simply be the documented
limit of `removeAllRoutes=false`. But it is a change for existing files (a
reload that worked now fails or duplicates),
so maybe worth either a line in the PR description / upgrade guide, or
skipping templated routes that have no
`routeId` on the update path. Your call.
_Claude Code on behalf of allthingssecurity_
--
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]
