[
https://issues.apache.org/jira/browse/CAMEL-23735?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Thomas LE GUEN resolved CAMEL-23735.
------------------------------------
Fix Version/s: Future
Resolution: Fixed
> 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
> Fix For: Future
>
>
> 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)
