Skip to content

Spring DSL defaultOutputToParentFlow does not behave as described #3946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dobretony opened this issue Nov 15, 2022 · 4 comments · Fixed by #3948
Closed

Spring DSL defaultOutputToParentFlow does not behave as described #3946

dobretony opened this issue Nov 15, 2022 · 4 comments · Fixed by #3948

Comments

@dobretony
Copy link

In what version(s) of Spring Integration are you seeing this issue?
5.1.4.RELEASE

Describe the bug

When creating an IntegrationFlow bean using IntegrationFlows.from(CHANNEL), in the route(Function<S, T> router, Consumer<RouterSpec<T, MethodInvokingRouter>> routerConfigurer) method, when configuring the router mappings, the method defaultOutputToParentFlow() does not what the documentation says it does.

Example:

Given this Integration Flow definition:

 @Bean
    public IntegrationFlow orderIntegrationFlow() {
        return IntegrationFlows.from(CHANNEL)
                .<Order, OrderType>route(Order::getType,
                        r -> r.channelMapping(OrderType.DINE_IN, DineInOrderFlow.CHANNEL)
                                .channelMapping(OrderType.ONLINE, OnlineOrderFlow.CHANNEL)
                                .defaultOutputToParentFlow()
                )
                .log("It only reaches here if Order::getType is null.")
                .get();
    }

The log will only get printed if Order::getType is null. This is contrary to the javadoc which says that:

Make a default output mapping of the router to the parent flow. Use the next, after router, parent flow MessageChannel as a AbstractMessageRouter.setDefaultOutputChannel(MessageChannel) of this router.

To Reproduce

Create a Spring project in Java 11, using Spring 5.3.20 and Spring Integration 5.1.4.RELEASE .

Create an IntegrationFlow bean using the default IntegrationFlows.from() method:

    public static final String CHANNEL = "orderInputChannel";

    @Bean(name = CHANNEL)
    public MessageChannel getChannel() {
        DirectChannel dc = new DirectChannel();
        dc.setComponentName(CHANNEL);
        return dc;
    }


    @Bean
    public IntegrationFlow orderIntegrationFlow() {
        return IntegrationFlows.from(CHANNEL)
                .<Order, OrderType>route(Order::getType,
                        r -> r.channelMapping(OrderType.DINE_IN, DineInOrderFlow.CHANNEL)
                                .channelMapping(OrderType.ONLINE, OnlineOrderFlow.CHANNEL)
                                .defaultOutputToParentFlow()
                )
                .log("It only reaches here if Order::getType is null.")
                .get();
    }

Create a class called Order:

public class Order {

    private OrderType orderType;
    public OrderType getType() {
        return orderType;
    }

    public void setType(OrderType orderType) {
        this.orderType = orderType;
    }

}

And an enum called OrderType:

public enum OrderType {
    DINE_IN,
    ONLINE,
    TAKEAWAY;
}

And with everything setup correctly, send a message to CHANNEL of an Order with OrderType.TAKEAWAY.

Expected behavior

The log() at the end of the orderIntegrationFlow will be called because the defaultOutputToParent reached a default state.

** Actual behavior **

An Exception is thrown where TAKEAWAY is not found to be a proper CHANNEL name defined in the Spring Context. Only if Order::getOrderType returns null, then the log() function is called.

Sample

A link to a GitHub repository with a minimal, reproducible sample.

Reports that include a sample will take priority over reports that do not.
At times, we may require a sample, so it is good to try and include a sample up front.

@dobretony dobretony added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Nov 15, 2022
@garyrussell garyrussell added status: waiting-for-reporter Needs a feedback from the reporter and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Nov 15, 2022
@garyrussell
Copy link
Contributor

5.1.x is no longer supported: https://spring.io/projects/spring-integration#support

See .noChannelKeyFallback().

	/**
	 * By 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.
	 * Invoke this method 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.
	 * @return the router spec.
	 * @since 5.2
	 */
	public RouterSpec<K, R> noChannelKeyFallback() {
		this.handler.setChannelKeyFallback(false);
		return _this();
	}

@dobretony
Copy link
Author

All right, thank you for the information. Sorry to bother with an unsupported version.

@artembilan
Copy link
Member

The property for version you use is this:

	/**
	 * @param resolutionRequired the resolutionRequired.
	 * @return the router spec.
	 * @see AbstractMappingMessageRouter#setResolutionRequired(boolean)
	 */
	public RouterSpec<K, R> resolutionRequired(boolean resolutionRequired) {

More JavaDocs are here:

	/**
	 * Specify whether this router should ignore any failure to resolve a channel name to
	 * an actual MessageChannel instance when delegating to the ChannelResolver strategy.
	 * @param resolutionRequired true if resolution is required.
	 */
	public void setResolutionRequired(boolean resolutionRequired) {

And respective docs: https://docs.spring.io/spring-integration/docs/current/reference/html/message-routing.html#router-common-parameters-all.

Since this defaultOutputToParentFlow() is a bit misleading, I believe we can improve end-user experience at least with more JavaDocs on this method.

Reopening...

@artembilan artembilan reopened this Nov 15, 2022
@artembilan artembilan added this to the 6.0.0 milestone Nov 15, 2022
@garyrussell
Copy link
Contributor

Good point @artembilan I had forgotten about that property; while polishing the javadocs, we should note that noChannelKeyFallback is a little more efficient because it doesn't attempt resolution at all.

artembilan added a commit to artembilan/spring-integration that referenced this issue Nov 15, 2022
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
artembilan added a commit to artembilan/spring-integration that referenced this issue Nov 16, 2022
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
garyrussell pushed a commit that referenced this issue Nov 16, 2022
* GH-3946: Revise Router channelKeyFallback option

Fixes #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants