GH-3946: Revise Router channelKeyFallback option #3948
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
garyrussell
suggested changes
Nov 16, 2022
| * 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, to skip an attempt to resolve channel name. |
There was a problem hiding this comment.
Suggested change
| * if not called explicitly, to skip an attempt to resolve channel name. | |
| * if not called explicitly afterwards, to skip an attempt to resolve the channel name. |
| * 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 message |
There was a problem hiding this comment.
Suggested change
| * an attempt to resolve a channel from its key, but instead send message | |
| * an attempt to resolve a channel from its key, but instead send the message |
| * an attempt to resolve a channel from its key, but instead send message | ||
| * directly to this channel. | ||
| * If {@link #channelKeyFallback} is set explicitly to {@code true}, | ||
| * the further logic depend on a {@link #resolutionRequired} options |
There was a problem hiding this comment.
Suggested change
| * the further logic depend on a {@link #resolutionRequired} options | |
| * the logic depends on the {@link #resolutionRequired} option |
Comment on lines
153
to
156
| * The configuration where default output channel is present and | ||
| * {@link #resolutionRequired} and {@link #resolutionRequired} are set to {@code true} | ||
| * is rejected since it leads to ambiguity. |
There was a problem hiding this comment.
Suggested change
| * The configuration where default output channel is present and | |
| * {@link #resolutionRequired} and {@link #resolutionRequired} are set to {@code true} | |
| * is rejected since it leads to ambiguity. | |
| * 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. |
| * this default output channel may never happen. | ||
| * The configuration where default output channel is present and | ||
| * {@link #resolutionRequired} and {@link #resolutionRequired} are set to {@code true} | ||
| * is rejected since it leads to ambiguity. |
| "The 'defaultOutputChannel' cannot be reached " + | ||
| "when both 'channelKeyFallback' & 'resolutionRequired' are set to true. " + | ||
| "See their javadocs for more information."); | ||
| } |
There was a problem hiding this comment.
I don't think we can enforce this; what if both are true and channel resolution is successful.
There was a problem hiding this comment.
Correct. And what is the reason to have a defaultOutputChannel then?
You simply can set channelKeyFallback = true, but resolutionRequired = false.
So, if resolution is OK, we go there, if it is not we fallback to a default channel.
We probably can improve docs for this as well.
src/reference/asciidoc/router.adoc
Outdated
Comment on lines
352
to
353
| NOTE: Starting with version 6.0, setting a default output channel also resets `channelKeyFallback` option to `false`. | ||
| So, no attempts to resolve a channel from name, but rather fallback to this default output channel - similar to Java `switch` statement. |
There was a problem hiding this comment.
Suggested change
| NOTE: Starting with version 6.0, setting a default output channel also resets `channelKeyFallback` option to `false`. | |
| So, no attempts to resolve a channel from name, but rather fallback to this default output channel - similar to Java `switch` statement. | |
| 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. |
src/reference/asciidoc/router.adoc
Outdated
Comment on lines
354
to
355
| If `channelKeyFallback` is set to `true` explicitly, the further logic depends on the `resolutionRequired` option: the message 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. |
There was a problem hiding this comment.
We can't enforce this. See comment above.
Comment on lines
113
to
114
| Another convenient behaviour change has been done for an `AbstractMappingMessageRouter`. | ||
| Now setting a `defaultOutputChannel` also resets a `channelKeyFallback` to `false`, so no attempts to resolve channel from its key, but immediate fallback to `defaultOutputChannel`. |
There was a problem hiding this comment.
Suggested change
| Another convenient behaviour change has been done for an `AbstractMappingMessageRouter`. | |
| Now setting a `defaultOutputChannel` also resets a `channelKeyFallback` to `false`, so no attempts to resolve channel from its key, but immediate fallback to `defaultOutputChannel`. | |
| 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 mesage to the `defaultOutputChannel`. |
Fixes spring-projects#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
…ests-context.xml`
…OutputChannel` 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.
|
I don't think we need to say anything else.
|
|
My point with this change is that end-users expects a behavior a-la Java |
garyrussell
approved these changes
Nov 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3946
The
AbstractMappingMessageRouterhas bothresolutionRequiredandchannelKeyFallbackastrueby default.End-users expects them to back off when they set a
defaultOutputChannel. They really want something similar to JavaswitchstatementAbstractMappingMessageRouterto resetchannelKeyFallbacktofalsewhendefaultOutputChannelto avoid attempts to resolve channel from name, but rather fallback todefaultOutputChannelas it states from th mentioned Javaswitchstatement experienceRouterSpec.noChannelKeyFallback()in favor of newly introducedchannelKeyFallback(boolean)channelKeyFallback(false)from an overloadeddefaultOutputToParentFlow()to reflect the mentioned expected behavior in Java DSL as well.KotlinRouterSpec.noChannelKeyFallback()wrapper in favor of newly introducedchannelKeyFallback(channelKeyFallback: Boolean)noChannelKeyFallback()option in theNoFallbackAllowedTests