From e9257958fe657c6250b397c59fa38a8f2ea74d53 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Wed, 16 Nov 2022 14:19:26 -0500 Subject: [PATCH] GH-3946: Revise Router channelKeyFallback option (#3948) * GH-3946: Revise Router channelKeyFallback option Fixes https://github.com/spring-projects/spring-integration/issues/3946 The `AbstractMappingMessageRouter` has both `resolutionRequired` and `channelKeyFallback` as `true` by default. End-users expects them to back off when they set a `defaultOutputChannel`. They really want something similar to Java `switch` statement * Change the logic in the `AbstractMappingMessageRouter` to reset `channelKeyFallback` to `false` when `defaultOutputChannel` to avoid attempts to resolve channel from name, but rather fallback to `defaultOutputChannel` as it states from th mentioned Java `switch` statement experience * Deprecate `RouterSpec.noChannelKeyFallback()` in favor of newly introduced `channelKeyFallback(boolean)` * Call `channelKeyFallback(false)` from an overloaded `defaultOutputToParentFlow()` to reflect the mentioned expected behavior in Java DSL as well. * Respectively, deprecate `KotlinRouterSpec.noChannelKeyFallback()` wrapper in favor of newly introduced `channelKeyFallback(channelKeyFallback: Boolean)` * Remove redundant already `noChannelKeyFallback()` option in the `NoFallbackAllowedTests` * Document the change and new behavior * Fix `IntegrationGraphServerTests` to `setChannelKeyFallback(true)` explicitly * Remove not relevant `default-output-channel` from the `DynamicRouterTests-context.xml` * Reject an `AbstractMappingMessageRouter` configuration where `defaultOutputChannel` is provided and both `channelKeyFallback` & `resolutionRequired` are set to `true`. Such a state makes `defaultOutputChannel` as not reachable and may cause some confusions in target applications. * Remove `&` symbol from JavaDocs * Fix `boolean` expression for `AbstractMappingMessageRouter` configuration check * Fix `IntegrationGraphServerTests` for new router behavior * Improve language in docs --- .../integration/dsl/RouterSpec.java | 34 ++++++- .../router/AbstractMappingMessageRouter.java | 95 ++++++++++++++++--- .../integration/dsl/KotlinRouterSpec.kt | 9 +- .../dsl/routers/NoFallbackAllowedTests.java | 6 +- .../graph/IntegrationGraphServerTests.java | 10 +- .../jmx/config/DynamicRouterTests-context.xml | 3 +- src/reference/asciidoc/router.adoc | 5 +- src/reference/asciidoc/whats-new.adoc | 5 + 8 files changed, 139 insertions(+), 28 deletions(-) diff --git a/spring-integration-core/src/main/java/org/springframework/integration/dsl/RouterSpec.java b/spring-integration-core/src/main/java/org/springframework/integration/dsl/RouterSpec.java index d038f3ba5f..302150019f 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/dsl/RouterSpec.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/dsl/RouterSpec.java @@ -1,5 +1,5 @@ /* - * Copyright 2016-2021 the original author or authors. + * Copyright 2016-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,6 +23,7 @@ import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.integration.context.IntegrationObjectSupport; import org.springframework.integration.router.AbstractMappingMessageRouter; +import org.springframework.integration.router.AbstractMessageRouter; import org.springframework.integration.support.context.NamedComponent; import org.springframework.integration.support.management.MappingMessageRouterManagement; import org.springframework.messaging.MessageChannel; @@ -118,9 +119,24 @@ public final class RouterSpec * cause a stack overflow. * @return the router spec. * @since 5.2 + * @deprecated since 6.0 in favor of {@link #channelKeyFallback(boolean)} */ + @Deprecated(since = "6.0", forRemoval = true) public RouterSpec noChannelKeyFallback() { - this.handler.setChannelKeyFallback(false); + return channelKeyFallback(false); + } + + /** + * When true (default), if a resolved channel key does not exist in the channel map, + * the key itself is used as the channel name, which we will attempt to resolve to a + * channel. Set to {@code false} to disable this feature. + * @param channelKeyFallback false to disable the fallback. + * @return the router spec. + * @since 6.0 + * @see AbstractMappingMessageRouter#setChannelKeyFallback(boolean) + */ + public RouterSpec channelKeyFallback(boolean channelKeyFallback) { + this.handler.setChannelKeyFallback(channelKeyFallback); return _this(); } @@ -205,6 +221,20 @@ public final class RouterSpec return _this(); } + /** + * Make a default output mapping of the router to the parent flow. + * Use the next, after router, parent flow {@link MessageChannel} as a + * {@link AbstractMessageRouter#setDefaultOutputChannel(MessageChannel)} of this router. + * This option also disables {@link AbstractMappingMessageRouter#setChannelKeyFallback(boolean)}, + * if not called explicitly afterwards, to skip an attempt to resolve the channel name. + * @return the router spec. + * @since 6.0 + */ + public RouterSpec defaultOutputToParentFlow() { + return super.defaultOutputToParentFlow() + .channelKeyFallback(false); + } + @Override public Map getComponentsToRegister() { // The 'mappingProvider' must be added to the 'componentsToRegister' in the end to diff --git a/spring-integration-core/src/main/java/org/springframework/integration/router/AbstractMappingMessageRouter.java b/spring-integration-core/src/main/java/org/springframework/integration/router/AbstractMappingMessageRouter.java index 6b87000ec7..2860692e99 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/router/AbstractMappingMessageRouter.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/router/AbstractMappingMessageRouter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -62,7 +62,7 @@ public abstract class AbstractMappingMessageRouter extends AbstractMessageRouter @SuppressWarnings("serial") private final Map dynamicChannels = Collections.synchronizedMap( - new LinkedHashMap(DEFAULT_DYNAMIC_CHANNEL_LIMIT, 0.75f, true) { + new LinkedHashMap<>(DEFAULT_DYNAMIC_CHANNEL_LIMIT, 0.75f, true) { @Override protected boolean removeEldestEntry(Entry eldest) { @@ -79,6 +79,10 @@ public abstract class AbstractMappingMessageRouter extends AbstractMessageRouter private boolean channelKeyFallback = true; + private boolean defaultOutputChannelSet; + + private boolean channelKeyFallbackSetExplicitly; + private volatile Map channelMappings = new LinkedHashMap<>(); @@ -124,15 +128,72 @@ public abstract class AbstractMappingMessageRouter extends AbstractMessageRouter /** * When true (default), if a resolved channel key does not exist in the channel map, * the key itself is used as the channel name, which we will attempt to resolve to a - * channel. Set to false to disable this feature. This could be useful to prevent + * channel. Set to {@code false} to disable this feature. This could be useful to prevent * malicious actors from generating a message that could cause the message to be * routed to an unexpected channel, such as one upstream of the router, which would * cause a stack overflow. - * @param channelKeyFallback false to disable the fall back. + * @param channelKeyFallback false to disable the fallback. * @since 5.2 */ public void setChannelKeyFallback(boolean channelKeyFallback) { this.channelKeyFallback = channelKeyFallback; + this.channelKeyFallbackSetExplicitly = true; + } + + /** + * Set the default channel where Messages should be sent if channel resolution + * fails to return any channels. + * It also sets {@link #channelKeyFallback} to {@code false} to avoid + * an attempt to resolve a channel from its key, but instead send the message + * directly to this channel. + * If {@link #channelKeyFallback} is set explicitly to {@code true}, + * the logic depends on the {@link #resolutionRequired} option + * ({@code true} by default), and therefore a fallback to + * this default output channel may never happen. + * The configuration where a default output channel is present and + * {@link #setResolutionRequired resolutionRequired} and + * {@link #setChannelKeyFallback channelKeyFallback} are set to {@code true} + * is rejected since it leads to ambiguity. + * @param defaultOutputChannel The default output channel. + * @since 6.0 + * @see #setChannelKeyFallback(boolean) + * @see #setResolutionRequired(boolean) + */ + @Override + public void setDefaultOutputChannel(MessageChannel defaultOutputChannel) { + super.setDefaultOutputChannel(defaultOutputChannel); + if (!this.channelKeyFallbackSetExplicitly) { + this.channelKeyFallback = false; + } + this.defaultOutputChannelSet = true; + } + + /** + * Set the default channel where Messages should be sent if channel resolution + * fails to return any channels. + * It also sets {@link #channelKeyFallback} to {@code false} to avoid + * an attempt to resolve a channel from its key, but instead send the message + * directly to this channel. + * If {@link #channelKeyFallback} is set explicitly to {@code true}, + * the logic depends on the {@link #resolutionRequired} option + * ({@code true} by default), and therefore a fallback to + * this default output channel may never happen. + * The configuration where a default output channel is present and + * {@link #setResolutionRequired resolutionRequired} and + * {@link #setChannelKeyFallback channelKeyFallback} are set to {@code true} + * is rejected since it leads to ambiguity. + * @param defaultOutputChannelName the name of the channel bean for default output. + * @since 6.0 + * @see #setChannelKeyFallback(boolean) + * @see #setResolutionRequired(boolean) + */ + @Override + public void setDefaultOutputChannelName(String defaultOutputChannelName) { + super.setDefaultOutputChannelName(defaultOutputChannelName); + if (!this.channelKeyFallbackSetExplicitly) { + this.channelKeyFallback = false; + } + this.defaultOutputChannelSet = true; } /** @@ -189,15 +250,14 @@ public abstract class AbstractMappingMessageRouter extends AbstractMessageRouter return Collections.unmodifiableSet(this.dynamicChannels.keySet()); } - /** - * Subclasses must implement this method to return the channel keys. - * A "key" might be present in this router's "channelMappings", or it - * could be the channel's name or even the Message Channel instance itself. - * @param message The message. - * @return The channel keys. - */ - protected abstract List getChannelKeys(Message message); - + @Override + protected void onInit() { + super.onInit(); + Assert.state(!this.channelKeyFallback || !this.resolutionRequired || !this.defaultOutputChannelSet, + "The 'defaultOutputChannel' cannot be reached " + + "when both 'channelKeyFallback' & 'resolutionRequired' are set to true. " + + "See their javadocs for more information."); + } @Override protected Collection determineTargetChannels(Message message) { @@ -207,6 +267,15 @@ public abstract class AbstractMappingMessageRouter extends AbstractMessageRouter return channels; } + /** + * Subclasses must implement this method to return the channel keys. + * A "key" might be present in this router's "channelMappings", or it + * could be the channel's name or even the Message Channel instance itself. + * @param message The message. + * @return The channel keys. + */ + protected abstract List getChannelKeys(Message message); + /** * Convenience method allowing conversion of a list * of mappings in a control-bus message. diff --git a/spring-integration-core/src/main/kotlin/org/springframework/integration/dsl/KotlinRouterSpec.kt b/spring-integration-core/src/main/kotlin/org/springframework/integration/dsl/KotlinRouterSpec.kt index e7eb85dbd5..bc9ad9818c 100644 --- a/spring-integration-core/src/main/kotlin/org/springframework/integration/dsl/KotlinRouterSpec.kt +++ b/spring-integration-core/src/main/kotlin/org/springframework/integration/dsl/KotlinRouterSpec.kt @@ -1,5 +1,5 @@ /* - * Copyright 2020 the original author or authors. + * Copyright 2020-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -47,8 +47,13 @@ class KotlinRouterSpec(override val delegat this.delegate.suffix(suffix) } + @Deprecated(message = "Since 6.0", replaceWith = ReplaceWith("channelKeyFallback(false)")) fun noChannelKeyFallback() { - this.delegate.noChannelKeyFallback() + channelKeyFallback(false) + } + + fun channelKeyFallback(channelKeyFallback: Boolean) { + this.delegate.channelKeyFallback(channelKeyFallback) } fun channelMapping(key: K, channelName: String) { diff --git a/spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/NoFallbackAllowedTests.java b/spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/NoFallbackAllowedTests.java index b154c4b8cf..045a0db8ba 100644 --- a/spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/NoFallbackAllowedTests.java +++ b/spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/NoFallbackAllowedTests.java @@ -33,6 +33,8 @@ import static org.assertj.core.api.Assertions.assertThat; /** * @author Gary Russell + * @author Artem Bilan + * * @since 5.2 * */ @@ -53,9 +55,7 @@ public class NoFallbackAllowedTests { @Bean public IntegrationFlow flow() { - return f -> f.route("headers.whereTo", r -> r - .noChannelKeyFallback() - .defaultOutputChannel(queue())); + return f -> f.route("headers.whereTo", r -> r.defaultOutputChannel(queue())); } @Bean diff --git a/spring-integration-core/src/test/java/org/springframework/integration/graph/IntegrationGraphServerTests.java b/spring-integration-core/src/test/java/org/springframework/integration/graph/IntegrationGraphServerTests.java index e850da19c6..d24e683283 100644 --- a/spring-integration-core/src/test/java/org/springframework/integration/graph/IntegrationGraphServerTests.java +++ b/spring-integration-core/src/test/java/org/springframework/integration/graph/IntegrationGraphServerTests.java @@ -135,7 +135,7 @@ public class IntegrationGraphServerTests { List> links = (List>) map.get("links"); assertThat(links).isNotNull(); - assertThat(links.size()).isEqualTo(34); + assertThat(links.size()).isEqualTo(32); jsonArray = JsonPathUtils.evaluate(baos.toByteArray(), "$..nodes[?(@.name == 'expressionRouter.router')]"); @@ -169,7 +169,7 @@ public class IntegrationGraphServerTests { assertThat(nodes.size()).isEqualTo(34); links = (List>) map.get("links"); assertThat(links).isNotNull(); - assertThat(links.size()).isEqualTo(37); + assertThat(links.size()).isEqualTo(35); jsonArray = JsonPathUtils.evaluate(baos.toByteArray(), "$..nodes[?(@.name == 'router.router')]"); routerJson = jsonArray.toJSONString(); @@ -347,7 +347,7 @@ public class IntegrationGraphServerTests { HeaderValueRouter router = new HeaderValueRouter("foo"); router.setChannelMapping("bar", "barChannel"); router.setChannelMapping("baz", "bazChannel"); - router.setDefaultOutputChannel(discards()); + router.setChannelKeyFallback(true); return router; } @@ -365,7 +365,7 @@ public class IntegrationGraphServerTests { public ExpressionEvaluatingRouter expressionRouter() { ExpressionEvaluatingRouter router = new ExpressionEvaluatingRouter( new SpelExpressionParser().parseExpression("headers['foo']")); - router.setDefaultOutputChannel(discards()); + router.setChannelKeyFallback(true); return router; } @@ -404,7 +404,7 @@ public class IntegrationGraphServerTests { @Bean @InboundChannelAdapter(channel = "fizChannel", autoStartup = "false") public MessageSource testSource() { - return new AbstractMessageSource() { + return new AbstractMessageSource<>() { @Override public String getComponentType() { diff --git a/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/config/DynamicRouterTests-context.xml b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/config/DynamicRouterTests-context.xml index bf8ee68026..94e798c2dd 100644 --- a/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/config/DynamicRouterTests-context.xml +++ b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/config/DynamicRouterTests-context.xml @@ -19,8 +19,7 @@ object-name="org.springframework.integration.jmx.config:type=SimpleDynamicRouter,name=dynamicRouter" operation-name="addChannelMapping" /> - + diff --git a/src/reference/asciidoc/router.adoc b/src/reference/asciidoc/router.adoc index 731bae16f5..91454011f3 100644 --- a/src/reference/asciidoc/router.adoc +++ b/src/reference/asciidoc/router.adoc @@ -349,7 +349,10 @@ If set, this attribute provides a reference to the channel where messages should If no default output channel is provided, the router throws an exception. If you would like to silently drop those messages instead, set the default output channel attribute value to `nullChannel`. + -NOTE: A message is sent only to the `default-output-channel` if `resolution-required` is `false` and the channel is not resolved. +NOTE: Starting with version 6.0, setting a default output channel also resets the `channelKeyFallback` option to `false`. +So, no attempts will be made to resolve a channel from its name, but rather fallback to this default output channel - similar to a Java `switch` statement. +If `channelKeyFallback` is set to `true` explicitly, the further logic depends on the `resolutionRequired` option: the message to non-resolved channel from key can reach a `defaultOutputChannel` only if `resolutionRequired` is `false`. +Therefore, a configuration where `defaultOutputChannel` is provided and both `channelKeyFallback` & `resolutionRequired` are set to `true` is rejected by the `AbstractMappingMessageRouter` initialization phase. `resolution-required`:: This attribute specifies whether channel names must always be successfully resolved to channel instances that exist. diff --git a/src/reference/asciidoc/whats-new.adoc b/src/reference/asciidoc/whats-new.adoc index ec7befe072..18893ecdc7 100644 --- a/src/reference/asciidoc/whats-new.adoc +++ b/src/reference/asciidoc/whats-new.adoc @@ -110,6 +110,11 @@ For convenience, the XML and Java DSL for Scatter-Gather, based on the `Recipien See <<./scatter-gather.adoc#scatter-gather,Scatter-Gather>> for more information. +Another convenient behavior change has been made to the `AbstractMappingMessageRouter`. +Now, setting a `defaultOutputChannel` also resets the `channelKeyFallback` property to `false`, so no attempts will be made to resolve a channel from its key, but the logic immediately falls back to sending the message to the `defaultOutputChannel`. + +See <<./router.adoc#router-common-parameters-all,Router Options>> for more information. + The `AggregatingMessageHandler` now does not split a `Collection>` result of the `MessageGroupProcessor` (unless it is a `SimpleMessageGroupProcessor`) on the output, but emits a single message containing this whole collection as a payload. See <<./aggregator.adoc#aggregator,Aggregator>> for more information.