[
https://issues.apache.org/jira/browse/CAMEL-23735?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Thomas LE GUEN updated CAMEL-23735:
-----------------------------------
Description:
h2. Summary
Since the CAMEL-20302 refactoring (Camel 4.4.0), the {{dynamic-router}}
component uses a private {{NoopAggregationStrategy}} as its default aggregation
strategy, which keeps the *first* recipient's result. Both the user
documentation and the component's own javadoc still state that the *last* reply
is used by default (the pre-4.4 behavior, {{{}UseLatestAggregationStrategy{}}}).
In {{{}recipientMode=allMatch{}}}, any modification made by a recipient other
than the first one (body, exchange properties) is silently discarded when the
exchange returns to the calling route. Whether a downstream {{choice()}} sees a
property set by a subscriber now depends on subscription priorities, which is
surprising and undocumented.
h2. Expected behavior (as documented)
The component documentation for the {{aggregationStrategy}} option states:
{quote}Refers to an AggregationStrategy to be used to assemble the replies from
the multicasts, into a single outgoing message from the Multicast. By default,
Camel will use the last reply as the outgoing message.
{quote}
The javadoc of {{DynamicRouterRecipientListHelper#createAggregationStrategy}}
also states:
{quote}If all else fails, then a {{UseLatestAggregationStrategy}} is used.
{quote}
h2. Actual behavior
{{DynamicRouterRecipientListHelper.createAggregationStrategy}} falls back to a
private {{{}NoopAggregationStrategy{}}}:
{code:java}
.orElse(new NoopAggregationStrategy());
...
public static class NoopAggregationStrategy implements AggregationStrategy {
public Exchange aggregate(Exchange oldExchange, Exchange newExchange) {
return oldExchange == null ? newExchange : oldExchange; // keeps the
FIRST result
}
}
{code}
The aggregated (first) copy is then written back onto the original exchange by
{{MulticastProcessor.doDone}} via {{{}ExchangeHelper.copyResults{}}}, so the
caller route only ever sees the first recipient's outcome.
Note that the unused import of {{UseLatestAggregationStrategy}} is still
present in {{{}DynamicRouterRecipientListHelper{}}}, which suggests the
behavior change was unintentional.
h2. Regression evidence
* Camel 4.3.0, {{{}DynamicRouterEndpoint#determineAggregationStrategy{}}}:
{code:java}
return cfgStrategy != null
? CamelContextHelper.mandatoryLookup(camelContext, cfgStrategy,
AggregationStrategy.class)
: new UseLatestAggregationStrategy();
{code}
* Camel 4.4.0 (commit {{{}c4711c97{}}}, CAMEL-20302) introduced
{{DynamicRouterRecipientListHelper}} with the {{NoopAggregationStrategy}}
fallback shown above.
* Still present and verified on 4.14.1 (LTS) and 4.20.0.
h2. Reproduction (JUnit 5, camel-test-junit5, verified on 4.14.1 and 4.20.0)
{code:java}
public class DynamicRouterDefaultAggregationTest extends CamelTestSupport {
@Override
protected RoutesBuilder createRouteBuilder() {
return new RouteBuilder() {
@Override
public void configure() {
from("direct:start")
.to("dynamic-router:demo?recipientMode=allMatch")
.to("mock:result");
// first recipient (priority 1): leaves its copy untouched
from("direct:first").log("first recipient");
// second recipient (priority 2): modifies its copy
from("direct:second")
.setProperty("enrichedBy", constant("second"))
.setBody(constant("modified by second"));
}
};
}
@Test
void defaultAggregationShouldKeepLastReplyAsDocumented() throws Exception {
subscribe("first-sub", "direct:first", 1);
subscribe("second-sub", "direct:second", 2);
getMockEndpoint("mock:result").expectedMessageCount(1);
template.sendBody("direct:start", "original");
MockEndpoint.assertIsSatisfied(context);
Exchange result = getMockEndpoint("mock:result").getExchanges().get(0);
// Documentation: "By default, Camel will use the last reply as the
outgoing message."
// Both assertions FAIL: the body is still "original" and the property
is null,
// because the first recipient's (untouched) copy wins.
assertEquals("modified by second",
result.getMessage().getBody(String.class));
assertEquals("second", result.getProperty("enrichedBy", String.class));
}
private void subscribe(String id, String uri, int priority) {
template.sendBody("dynamic-router-control:subscribe",
DynamicRouterControlMessage.Builder.newBuilder()
.subscribeChannel("demo")
.subscriptionId(id)
.destinationUri(uri)
.priority(priority)
.predicate("${body} != null")
.expressionLanguage("simple")
.build());
}
}
{code}
h2. Suggested resolution
Either:
# restore {{UseLatestAggregationStrategy}} as the default, matching the
documentation and the pre-4.4 behavior, or
# keep {{NoopAggregationStrategy}} but update the option description, the
javadoc of {{createAggregationStrategy}} (and remove the stale
{{UseLatestAggregationStrategy}} import), clearly stating that the first
matching recipient's result is kept by default.
Option 1 seems preferable since the current behavior looks unintentional (stale
import, contradicting javadoc) and silently drops data.
> camel-dynamic-router: default aggregation keeps the FIRST reply,
> contradicting the documented "last reply" behavior (regression since 4.4.0)
> --------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-23735
> URL: https://issues.apache.org/jira/browse/CAMEL-23735
> Project: Camel
> Issue Type: Bug
> Components: camel-core
> Affects Versions: 4.4.0
> Reporter: Thomas LE GUEN
> Priority: Minor
> Labels: dynamic-router, dynamic-router-eip
>
> h2. Summary
> Since the CAMEL-20302 refactoring (Camel 4.4.0), the {{dynamic-router}}
> component uses a private {{NoopAggregationStrategy}} as its default
> aggregation strategy, which keeps the *first* recipient's result. Both the
> user documentation and the component's own javadoc still state that the
> *last* reply is used by default (the pre-4.4 behavior,
> {{{}UseLatestAggregationStrategy{}}}).
> In {{{}recipientMode=allMatch{}}}, any modification made by a recipient other
> than the first one (body, exchange properties) is silently discarded when the
> exchange returns to the calling route. Whether a downstream {{choice()}} sees
> a property set by a subscriber now depends on subscription priorities, which
> is surprising and undocumented.
> h2. Expected behavior (as documented)
> The component documentation for the {{aggregationStrategy}} option states:
> {quote}Refers to an AggregationStrategy to be used to assemble the replies
> from the multicasts, into a single outgoing message from the Multicast. By
> default, Camel will use the last reply as the outgoing message.
> {quote}
> The javadoc of {{DynamicRouterRecipientListHelper#createAggregationStrategy}}
> also states:
> {quote}If all else fails, then a {{UseLatestAggregationStrategy}} is used.
> {quote}
> h2. Actual behavior
> {{DynamicRouterRecipientListHelper.createAggregationStrategy}} falls back to
> a private {{{}NoopAggregationStrategy{}}}:
> {code:java}
> .orElse(new NoopAggregationStrategy());
> ...
> public static class NoopAggregationStrategy implements AggregationStrategy {
> public Exchange aggregate(Exchange oldExchange, Exchange newExchange) {
> return oldExchange == null ? newExchange : oldExchange; // keeps the
> FIRST result
> }
> }
> {code}
> The aggregated (first) copy is then written back onto the original exchange
> by {{MulticastProcessor.doDone}} via {{{}ExchangeHelper.copyResults{}}}, so
> the caller route only ever sees the first recipient's outcome.
> Note that the unused import of {{UseLatestAggregationStrategy}} is still
> present in {{{}DynamicRouterRecipientListHelper{}}}, which suggests the
> behavior change was unintentional.
> h2. Regression evidence
> * Camel 4.3.0, {{{}DynamicRouterEndpoint#determineAggregationStrategy{}}}:
> {code:java}
> return cfgStrategy != null
> ? CamelContextHelper.mandatoryLookup(camelContext, cfgStrategy,
> AggregationStrategy.class)
> : new UseLatestAggregationStrategy();
> {code}
> * Camel 4.4.0 (commit {{{}c4711c97{}}}, CAMEL-20302) introduced
> {{DynamicRouterRecipientListHelper}} with the {{NoopAggregationStrategy}}
> fallback shown above.
> * Still present and verified on 4.14.1 (LTS) and 4.20.0.
> h2. Reproduction (JUnit 5, camel-test-junit5, verified on 4.14.1 and 4.20.0)
> {code:java}
> public class DynamicRouterDefaultAggregationTest extends CamelTestSupport {
> @Override
> protected RoutesBuilder createRouteBuilder() {
> return new RouteBuilder() {
> @Override
> public void configure() {
> from("direct:start")
> .to("dynamic-router:demo?recipientMode=allMatch")
> .to("mock:result");
> // first recipient (priority 1): leaves its copy untouched
> from("direct:first").log("first recipient");
> // second recipient (priority 2): modifies its copy
> from("direct:second")
> .setProperty("enrichedBy", constant("second"))
> .setBody(constant("modified by second"));
> }
> };
> }
> @Test
> void defaultAggregationShouldKeepLastReplyAsDocumented() throws Exception
> {
> subscribe("first-sub", "direct:first", 1);
> subscribe("second-sub", "direct:second", 2);
> getMockEndpoint("mock:result").expectedMessageCount(1);
> template.sendBody("direct:start", "original");
> MockEndpoint.assertIsSatisfied(context);
> Exchange result =
> getMockEndpoint("mock:result").getExchanges().get(0);
> // Documentation: "By default, Camel will use the last reply as the
> outgoing message."
> // Both assertions FAIL: the body is still "original" and the
> property is null,
> // because the first recipient's (untouched) copy wins.
> assertEquals("modified by second",
> result.getMessage().getBody(String.class));
> assertEquals("second", result.getProperty("enrichedBy",
> String.class));
> }
> private void subscribe(String id, String uri, int priority) {
> template.sendBody("dynamic-router-control:subscribe",
> DynamicRouterControlMessage.Builder.newBuilder()
> .subscribeChannel("demo")
> .subscriptionId(id)
> .destinationUri(uri)
> .priority(priority)
> .predicate("${body} != null")
> .expressionLanguage("simple")
> .build());
> }
> }
> {code}
> h2. Suggested resolution
> Either:
> # restore {{UseLatestAggregationStrategy}} as the default, matching the
> documentation and the pre-4.4 behavior, or
> # keep {{NoopAggregationStrategy}} but update the option description, the
> javadoc of {{createAggregationStrategy}} (and remove the stale
> {{UseLatestAggregationStrategy}} import), clearly stating that the first
> matching recipient's result is kept by default.
> Option 1 seems preferable since the current behavior looks unintentional
> (stale import, contradicting javadoc) and silently drops data.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
