GH-2873: Preserve mapping order in the router (#2877)

* GH-2873: Preserve mapping order in the router

Fixes #2873

Sometime it is important to map to most specific exception instead of
its super class.

* Use `LinkedHashMap` for mapping keys in the
`ErrorMessageExceptionTypeRouter`, as well as in its
`AbstractMappingMessageRouter` superclass.
Since we don't do that internal map modification, there is no reason to
worry about concurrent access: we just replace an internal instance
atomically with a new `LinkedHashMap` every time we modify a mapping
for router

**Cherry-pick to 5.1.x**

* * Fix `RouterSpec.RouterMappingProvider` to `LinkedHashMap` as well

* * Fix `RouterTests` for proper mapping order
* Polishing for `AbstractMappingMessageRouter` hierarchy, so we don't
use `protected channelMappings` field access any more

Author: artembilan

spring-integration-core/src/main/java/org/springframework/integration/dsl/RouterSpec.java

@@ -16,7 +16,7 @@
 
 package org.springframework.integration.dsl;
 
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
 
 import org.springframework.core.convert.ConversionService;
@@ -188,7 +188,7 @@ private static class RouterMappingProvider extends IntegrationObjectSupport {
 
 		private final MappingMessageRouterManagement router;
 
-		private final Map<Object, NamedComponent> mapping = new HashMap<>();
+		private final Map<Object, NamedComponent> mapping = new LinkedHashMap<>();
 
 		RouterMappingProvider(MappingMessageRouterManagement router) {
 			this.router = router;

spring-integration-core/src/main/java/org/springframework/integration/router/AbstractMappingMessageRouter.java

@@ -20,14 +20,12 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Properties;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 
 import org.springframework.integration.support.management.MappingMessageRouterManagement;
 import org.springframework.jmx.export.annotation.ManagedAttribute;
@@ -48,32 +46,35 @@
  * @author Gunnar Hillert
  * @author Gary Russell
  * @author Artem Bilan
+ *
  * @since 2.1
  */
-public abstract class AbstractMappingMessageRouter extends AbstractMessageRouter implements MappingMessageRouterManagement {
+public abstract class AbstractMappingMessageRouter extends AbstractMessageRouter
+		implements MappingMessageRouterManagement {
 
 	private static final int DEFAULT_DYNAMIC_CHANNEL_LIMIT = 100;
 
 	private int dynamicChannelLimit = DEFAULT_DYNAMIC_CHANNEL_LIMIT;
 
 	@SuppressWarnings("serial")
-	private final Map<String, MessageChannel> dynamicChannels = Collections.<String, MessageChannel>synchronizedMap(
-			new LinkedHashMap<String, MessageChannel>(DEFAULT_DYNAMIC_CHANNEL_LIMIT, 0.75f, true) {
+	private final Map<String, MessageChannel> dynamicChannels =
+			Collections.synchronizedMap(
+					new LinkedHashMap<String, MessageChannel>(DEFAULT_DYNAMIC_CHANNEL_LIMIT, 0.75f, true) {
 
-				@Override
-				protected boolean removeEldestEntry(Entry<String, MessageChannel> eldest) {
-					return this.size() > AbstractMappingMessageRouter.this.dynamicChannelLimit;
-				}
+						@Override
+						protected boolean removeEldestEntry(Entry<String, MessageChannel> eldest) {
+							return this.size() > AbstractMappingMessageRouter.this.dynamicChannelLimit;
+						}
 
-			});
+					});
 
-	protected volatile Map<String, String> channelMappings = new ConcurrentHashMap<String, String>();
+	private String prefix;
 
-	private volatile String prefix;
+	private String suffix;
 
-	private volatile String suffix;
+	private boolean resolutionRequired = true;
 
-	private volatile boolean resolutionRequired = true;
+	private volatile Map<String, String> channelMappings = new LinkedHashMap<>();
 
 
 	/**
@@ -86,7 +87,7 @@ protected boolean removeEldestEntry(Entry<String, MessageChannel> eldest) {
 	@ManagedAttribute
 	public void setChannelMappings(Map<String, String> channelMappings) {
 		Assert.notNull(channelMappings, "'channelMappings' must not be null");
-		Map<String, String> newChannelMappings = new ConcurrentHashMap<String, String>(channelMappings);
+		Map<String, String> newChannelMappings = new LinkedHashMap<>(channelMappings);
 		doSetChannelMappings(newChannelMappings);
 	}
 
@@ -135,7 +136,7 @@ public void setDynamicChannelLimit(int dynamicChannelLimit) {
 	@Override
 	@ManagedAttribute
 	public Map<String, String> getChannelMappings() {
-		return new HashMap<String, String>(this.channelMappings);
+		return new LinkedHashMap<>(this.channelMappings);
 	}
 
 	/**
@@ -146,7 +147,7 @@ public Map<String, String> getChannelMappings() {
 	@Override
 	@ManagedOperation
 	public void setChannelMapping(String key, String channelName) {
-		Map<String, String> newChannelMappings = new ConcurrentHashMap<String, String>(this.channelMappings);
+		Map<String, String> newChannelMappings = new LinkedHashMap<>(this.channelMappings);
 		newChannelMappings.put(key, channelName);
 		this.channelMappings = newChannelMappings;
 	}
@@ -158,7 +159,7 @@ public void setChannelMapping(String key, String channelName) {
 	@Override
 	@ManagedOperation
 	public void removeChannelMapping(String key) {
-		Map<String, String> newChannelMappings = new ConcurrentHashMap<String, String>(this.channelMappings);
+		Map<String, String> newChannelMappings = new LinkedHashMap<>(this.channelMappings);
 		newChannelMappings.remove(key);
 		this.channelMappings = newChannelMappings;
 	}
@@ -181,8 +182,8 @@ public Collection<String> getDynamicChannelNames() {
 
 	@Override
 	protected Collection<MessageChannel> determineTargetChannels(Message<?> message) {
-		Collection<MessageChannel> channels = new ArrayList<MessageChannel>();
-		Collection<Object> channelKeys = this.getChannelKeys(message);
+		Collection<MessageChannel> channels = new ArrayList<>();
+		Collection<Object> channelKeys = getChannelKeys(message);
 		addToCollection(channels, channelKeys, message);
 		return channels;
 	}
@@ -201,12 +202,12 @@ protected Collection<MessageChannel> determineTargetChannels(Message<?> message)
 	@ManagedOperation
 	public void replaceChannelMappings(Properties channelMappings) {
 		Assert.notNull(channelMappings, "'channelMappings' must not be null");
-		Map<String, String> newChannelMappings = new ConcurrentHashMap<String, String>();
+		Map<String, String> newChannelMappings = new LinkedHashMap<>();
 		Set<String> keys = channelMappings.stringPropertyNames();
 		for (String key : keys) {
 			newChannelMappings.put(key.trim(), channelMappings.getProperty(key).trim());
 		}
-		this.doSetChannelMappings(newChannelMappings);
+		doSetChannelMappings(newChannelMappings);
 	}
 
 	private void doSetChannelMappings(Map<String, String> newChannelMappings) {
@@ -227,9 +228,6 @@ private MessageChannel resolveChannelForName(String channelName, Message<?> mess
 				throw new MessagingException(message, "failed to resolve channel name '" + channelName + "'", e);
 			}
 		}
-		if (channel == null && this.resolutionRequired) {
-			throw new MessagingException(message, "failed to resolve channel name '" + channelName + "'");
-		}
 		return channel;
 	}
 
@@ -258,7 +256,7 @@ private void addChannelFromString(Collection<MessageChannel> channels, String ch
 		MessageChannel channel = resolveChannelForName(channelName, message);
 		if (channel != null) {
 			channels.add(channel);
-			if (!mapped && !(this.dynamicChannels.get(channelName) != null)) {
+			if (!mapped && this.dynamicChannels.get(channelName) == null) {
 				this.dynamicChannels.put(channelName, channel);
 			}
 		}

spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java

@@ -17,11 +17,11 @@
 package org.springframework.integration.router;
 
 import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 
 import org.springframework.jmx.export.annotation.ManagedAttribute;
 import org.springframework.jmx.export.annotation.ManagedOperation;
@@ -46,7 +46,7 @@
  */
 public class ErrorMessageExceptionTypeRouter extends AbstractMappingMessageRouter {
 
-	private volatile Map<String, Class<?>> classNameMappings = new ConcurrentHashMap<>();
+	private volatile Map<String, Class<?>> classNameMappings = new LinkedHashMap<>();
 
 	private volatile boolean initialized;
 
@@ -60,7 +60,7 @@ public void setChannelMappings(Map<String, String> channelMappings) {
 	}
 
 	private void populateClassNameMapping(Set<String> classNames) {
-		Map<String, Class<?>> newClassNameMappings = new ConcurrentHashMap<>();
+		Map<String, Class<?>> newClassNameMappings = new LinkedHashMap<>();
 		for (String className : classNames) {
 			newClassNameMappings.put(className, resolveClassFromName(className));
 		}
@@ -82,7 +82,7 @@ private Class<?> resolveClassFromName(String className) {
 	public void setChannelMapping(String key, String channelName) {
 		super.setChannelMapping(key, channelName);
 		if (this.initialized) {
-			Map<String, Class<?>> newClassNameMappings = new ConcurrentHashMap<>(this.classNameMappings);
+			Map<String, Class<?>> newClassNameMappings = new LinkedHashMap<>(this.classNameMappings);
 			newClassNameMappings.put(key, resolveClassFromName(key));
 			this.classNameMappings = newClassNameMappings;
 		}
@@ -92,7 +92,7 @@ public void setChannelMapping(String key, String channelName) {
 	@ManagedOperation
 	public void removeChannelMapping(String key) {
 		super.removeChannelMapping(key);
-		Map<String, Class<?>> newClassNameMappings = new ConcurrentHashMap<>(this.classNameMappings);
+		Map<String, Class<?>> newClassNameMappings = new LinkedHashMap<>(this.classNameMappings);
 		newClassNameMappings.remove(key);
 		this.classNameMappings = newClassNameMappings;
 	}
@@ -101,13 +101,13 @@ public void removeChannelMapping(String key) {
 	@ManagedOperation
 	public void replaceChannelMappings(Properties channelMappings) {
 		super.replaceChannelMappings(channelMappings);
-		populateClassNameMapping(this.channelMappings.keySet());
+		populateClassNameMapping(getChannelMappings().keySet());
 	}
 
 	@Override
 	protected void onInit() {
 		super.onInit();
-		populateClassNameMapping(this.channelMappings.keySet());
+		populateClassNameMapping(getChannelMappings().keySet());
 		this.initialized = true;
 	}
 

spring-integration-core/src/main/java/org/springframework/integration/router/PayloadTypeRouter.java

@@ -16,21 +16,21 @@
 
 package org.springframework.integration.router;
 
-import java.util.ArrayList;
 import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 
 import org.springframework.messaging.Message;
 import org.springframework.util.CollectionUtils;
 
 /**
  * A Message Router that resolves the {@link org.springframework.messaging.MessageChannel}
- * based on the
- * {@link Message Message's} payload type.
+ * based on the {@link Message Message's} payload type.
  *
  * @author Mark Fisher
  * @author Oleg Zhurakousky
  * @author Gary Russell
+ * @author Artem Bilan
  */
 public class PayloadTypeRouter extends AbstractMappingMessageRouter {
 
@@ -47,23 +47,22 @@ public class PayloadTypeRouter extends AbstractMappingMessageRouter {
 	 */
 	@Override
 	protected List<Object> getChannelKeys(Message<?> message) {
-		if (CollectionUtils.isEmpty(this.channelMappings)) {
+		if (CollectionUtils.isEmpty(getChannelMappings())) {
 			return null;
 		}
 		Class<?> type = message.getPayload().getClass();
 		boolean isArray = type.isArray();
 		if (isArray) {
 			type = type.getComponentType();
 		}
-		String closestMatch =  this.findClosestMatch(type, isArray);
-		return (closestMatch != null) ? Collections.<Object>singletonList(closestMatch) : null;
+		String closestMatch = findClosestMatch(type, isArray);
+		return (closestMatch != null) ? Collections.singletonList(closestMatch) : null;
 	}
 
-
 	private String findClosestMatch(Class<?> type, boolean isArray) {
 		int minTypeDiffWeight = Integer.MAX_VALUE;
-		List<String> matches = new ArrayList<String>();
-		for (String candidate : this.channelMappings.keySet()) {
+		List<String> matches = new LinkedList<>();
+		for (String candidate : getChannelMappings().keySet()) {
 			if (isArray) {
 				if (!candidate.endsWith(ARRAY_SUFFIX)) {
 					continue;
@@ -118,7 +117,7 @@ private int determineTypeDifferenceWeight(String candidate, Class<?> type, int l
 			// exhausted hierarchy and found no match
 			return Integer.MAX_VALUE;
 		}
-		return this.determineTypeDifferenceWeight(candidate, type.getSuperclass(), level + 2);
+		return determineTypeDifferenceWeight(candidate, type.getSuperclass(), level + 2);
 	}
 
 }

spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/RouterTests.java

@@ -743,8 +743,8 @@ public IntegrationFlow payloadTypeRouteFlow() {
 		public IntegrationFlow exceptionTypeRouteFlow() {
 			return f -> f
 					.routeByException(r -> r
-							.channelMapping(IllegalArgumentException.class, "illegalArgumentChannel")
 							.channelMapping(RuntimeException.class, "runtimeExceptionChannel")
+							.channelMapping(IllegalArgumentException.class, "illegalArgumentChannel")
 							.subFlowMapping(MessageHandlingException.class, sf ->
 									sf.channel("messageHandlingExceptionChannel"))
 							.defaultOutputChannel("exceptionRouterDefaultChannel"));

spring-integration-core/src/test/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouterTests.java

@@ -79,8 +79,8 @@ public void mostSpecificCause() {
 		ErrorMessageExceptionTypeRouter router = new ErrorMessageExceptionTypeRouter();
 		router.setBeanFactory(this.context);
 		router.setApplicationContext(this.context);
-		router.setChannelMapping(IllegalArgumentException.class.getName(), "illegalArgumentChannel");
 		router.setChannelMapping(RuntimeException.class.getName(), "runtimeExceptionChannel");
+		router.setChannelMapping(IllegalArgumentException.class.getName(), "illegalArgumentChannel");
 		router.setChannelMapping(MessageHandlingException.class.getName(), "messageHandlingExceptionChannel");
 		router.setDefaultOutputChannel(this.defaultChannel);
 		router.afterPropertiesSet();
@@ -104,7 +104,7 @@ public void fallbackToNextMostSpecificCause() {
 		router.setBeanFactory(this.context);
 		router.setApplicationContext(this.context);
 		router.setChannelMapping(RuntimeException.class.getName(), "runtimeExceptionChannel");
-		router.setChannelMapping(MessageHandlingException.class.getName(), "runtimeExceptionChannel");
+		router.setChannelMapping(MessageHandlingException.class.getName(), "messageHandlingExceptionChannel");
 		router.setDefaultOutputChannel(this.defaultChannel);
 		router.afterPropertiesSet();
 
@@ -192,8 +192,8 @@ public void exceptionPayloadButNotErrorMessage() {
 		ErrorMessageExceptionTypeRouter router = new ErrorMessageExceptionTypeRouter();
 		router.setBeanFactory(this.context);
 		router.setApplicationContext(this.context);
-		router.setChannelMapping(IllegalArgumentException.class.getName(), "illegalArgumentChannel");
 		router.setChannelMapping(RuntimeException.class.getName(), "runtimeExceptionChannel");
+		router.setChannelMapping(IllegalArgumentException.class.getName(), "illegalArgumentChannel");
 		router.setChannelMapping(MessageHandlingException.class.getName(), "messageHandlingExceptionChannel");
 		router.setDefaultOutputChannel(this.defaultChannel);
 		router.afterPropertiesSet();

src/reference/asciidoc/router.adoc

@@ -750,13 +750,12 @@ See <<xml-xpath-routing>>.
 Spring Integration also provides a special type-based router called `ErrorMessageExceptionTypeRouter` for routing error messages (defined as messages whose `payload` is a `Throwable` instance).
 `ErrorMessageExceptionTypeRouter` is similar to the `PayloadTypeRouter`.
 In fact, they are almost identical.
-The only difference is that, while `PayloadTypeRouter` navigates the instance hierarchy of a payload instance (for example, `payload.getClass().getSuperclass()`) to find the most specific type and channel mappings,
-the `ErrorMessageExceptionTypeRouter` navigates the hierarchy of 'exception causes' (for example, `payload.getCause()`)
-to find the most specific `Throwable` type or channel mappings and uses `mappingClass.isInstance(cause)` to match the
-`cause` to the class or any super class.
+The only difference is that, while `PayloadTypeRouter` navigates the instance hierarchy of a payload instance (for example, `payload.getClass().getSuperclass()`) to find the most specific type and channel mappings, the `ErrorMessageExceptionTypeRouter` navigates the hierarchy of 'exception causes' (for example, `payload.getCause()`) to find the most specific `Throwable` type or channel mappings and uses `mappingClass.isInstance(cause)` to match the `cause` to the class or any super class.
 
-NOTE: Since version 4.3 the `ErrorMessageExceptionTypeRouter` loads all mapping classes during the initialization
-phase to fail-fast for a `ClassNotFoundException`.
+IMPORTANT: The channel mapping order in this case matters.
+So, if there is a requirement to get mapping for an `IllegalArgumentException`, but not a `RuntimeException`, the last one must be configured on router first.
+
+NOTE: Since version 4.3 the `ErrorMessageExceptionTypeRouter` loads all mapping classes during the initialization phase to fail-fast for a `ClassNotFoundException`.
 
 The following example shows a sample configuration for `ErrorMessageExceptionTypeRouter`: