oscerd commented on code in PR #27430:
URL: https://github.com/apache/camel/pull/27430#discussion_r4193704426
##########
core/camel-core/src/test/java/org/apache/camel/model/RoutesConfigurationOnCompletionTest.java:
##########
@@ -122,4 +122,36 @@ public void configure() {
assertMockEndpointsSatisfied();
}
+ @Test
+ public void testLocalConfigurationWithIndirectConsumer() throws Exception {
+ // CAMEL-25135: onCompletion from a named routeConfiguration must fire
even when
+ // the opted-in route is not itself the consumer route (e.g. REST DSL
→ direct:).
+ context.addRoutes(new RouteConfigurationBuilder() {
+ @Override
+ public void configuration() {
+
routeConfiguration("myconfig").onCompletion().to("mock:completion");
Review Comment:
**BeforeConsumer mode, which the Jira reproducer uses, is not covered**
The CAMEL-25135 reproducer uses `mode: BeforeConsumer`, which takes a
separate code path (`OnCompletionSynchronizationBeforeConsumer.onAfterRoute()`,
whose global branch rejects the indirect route via
`!exchange.getFromRouteId().equals(routeId)`), not `shouldSkip()`. This test
only exercises the default AfterConsumer path. A case such as
`routeConfiguration("myconfig").onCompletion().modeBeforeConsumer().setHeader("done",
constant("yes"))` with `direct:consumer` -> `direct:processor`, asserting the
header is in the reply of `template.request("direct:consumer", ...)`, next to
the double-firing cases davsclaus asked for, would pin the BeforeConsumer count
too.
##########
core/camel-core-model/src/main/java/org/apache/camel/model/RouteConfigurationDefinition.java:
##########
@@ -233,8 +233,17 @@ public final OnExceptionDefinition onException(Class<?
extends Throwable>... exc
public OnCompletionDefinition onCompletion() {
OnCompletionDefinition answer = new OnCompletionDefinition();
answer.setRouteConfiguration(this);
- // is global scoped by default
- answer.setRouteScoped(false);
+ if (getId() == null) {
Review Comment:
**`getId() == null` misses the other global form, a configuration with id
`*`**
`RouteDefinitionHelper.routesByIdOrPattern()` applies a configuration to
every route without a `routeConfigurationId` when its id is null **or `*`**, so
`routeConfiguration("*")` is global as well; with this change it becomes
route-scoped and fires once per visited route. If the id-based split stays
(here or moved to the merge helper), `*` should be treated like no id:
```suggestion
if (getId() == null || "*".equals(getId())) {
```
(Nit on the comment above: a nameless configuration is the fallback for
routes *without* a `routeConfigurationId`, not "all routes".)
--
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]
