From 5130d51d066e515a9aada035f45f97401991d6ec Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Mon, 1 Apr 2019 13:06:10 -0400 Subject: [PATCH 1/3] GH-2873: Preserve mapping order in the router Fixes https://github.com/spring-projects/spring-integration/issues/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** --- .../router/AbstractMappingMessageRouter.java | 41 ++++++++++--------- .../ErrorMessageExceptionTypeRouter.java | 10 ++--- .../ErrorMessageExceptionTypeRouterTests.java | 6 +-- src/reference/asciidoc/router.adoc | 11 +++-- 4 files changed, 34 insertions(+), 34 deletions(-) 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 de3fbb695e6..50622a16e39 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 @@ -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,26 +46,29 @@ * @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 dynamicChannels = Collections.synchronizedMap( - new LinkedHashMap(DEFAULT_DYNAMIC_CHANNEL_LIMIT, 0.75f, true) { + private final Map dynamicChannels = + Collections.synchronizedMap( + new LinkedHashMap(DEFAULT_DYNAMIC_CHANNEL_LIMIT, 0.75f, true) { - @Override - protected boolean removeEldestEntry(Entry eldest) { - return this.size() > AbstractMappingMessageRouter.this.dynamicChannelLimit; - } + @Override + protected boolean removeEldestEntry(Entry eldest) { + return this.size() > AbstractMappingMessageRouter.this.dynamicChannelLimit; + } - }); + }); - protected volatile Map channelMappings = new ConcurrentHashMap(); + protected volatile Map channelMappings = new LinkedHashMap<>(); private volatile String prefix; @@ -86,7 +87,7 @@ protected boolean removeEldestEntry(Entry eldest) { @ManagedAttribute public void setChannelMappings(Map channelMappings) { Assert.notNull(channelMappings, "'channelMappings' must not be null"); - Map newChannelMappings = new ConcurrentHashMap(channelMappings); + Map newChannelMappings = new LinkedHashMap<>(channelMappings); doSetChannelMappings(newChannelMappings); } @@ -135,7 +136,7 @@ public void setDynamicChannelLimit(int dynamicChannelLimit) { @Override @ManagedAttribute public Map getChannelMappings() { - return new HashMap(this.channelMappings); + return new LinkedHashMap<>(this.channelMappings); } /** @@ -146,7 +147,7 @@ public Map getChannelMappings() { @Override @ManagedOperation public void setChannelMapping(String key, String channelName) { - Map newChannelMappings = new ConcurrentHashMap(this.channelMappings); + Map 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 newChannelMappings = new ConcurrentHashMap(this.channelMappings); + Map newChannelMappings = new LinkedHashMap<>(this.channelMappings); newChannelMappings.remove(key); this.channelMappings = newChannelMappings; } @@ -181,8 +182,8 @@ public Collection getDynamicChannelNames() { @Override protected Collection determineTargetChannels(Message message) { - Collection channels = new ArrayList(); - Collection channelKeys = this.getChannelKeys(message); + Collection channels = new ArrayList<>(); + Collection channelKeys = getChannelKeys(message); addToCollection(channels, channelKeys, message); return channels; } @@ -201,12 +202,12 @@ protected Collection determineTargetChannels(Message message) @ManagedOperation public void replaceChannelMappings(Properties channelMappings) { Assert.notNull(channelMappings, "'channelMappings' must not be null"); - Map newChannelMappings = new ConcurrentHashMap(); + Map newChannelMappings = new LinkedHashMap<>(); Set keys = channelMappings.stringPropertyNames(); for (String key : keys) { newChannelMappings.put(key.trim(), channelMappings.getProperty(key).trim()); } - this.doSetChannelMappings(newChannelMappings); + doSetChannelMappings(newChannelMappings); } private void doSetChannelMappings(Map newChannelMappings) { @@ -258,7 +259,7 @@ private void addChannelFromString(Collection 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); } } diff --git a/spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java b/spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java index 943005b038c..36375544026 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java +++ b/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> classNameMappings = new ConcurrentHashMap<>(); + private volatile Map> classNameMappings = new LinkedHashMap<>(); private volatile boolean initialized; @@ -60,7 +60,7 @@ public void setChannelMappings(Map channelMappings) { } private void populateClassNameMapping(Set classNames) { - Map> newClassNameMappings = new ConcurrentHashMap<>(); + Map> 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> newClassNameMappings = new ConcurrentHashMap<>(this.classNameMappings); + Map> 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> newClassNameMappings = new ConcurrentHashMap<>(this.classNameMappings); + Map> newClassNameMappings = new LinkedHashMap<>(this.classNameMappings); newClassNameMappings.remove(key); this.classNameMappings = newClassNameMappings; } diff --git a/spring-integration-core/src/test/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouterTests.java b/spring-integration-core/src/test/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouterTests.java index f207b344382..ed96ede88b0 100644 --- a/spring-integration-core/src/test/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouterTests.java +++ b/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(); diff --git a/src/reference/asciidoc/router.adoc b/src/reference/asciidoc/router.adoc index 4e5d124271a..d4150e07229 100644 --- a/src/reference/asciidoc/router.adoc +++ b/src/reference/asciidoc/router.adoc @@ -750,13 +750,12 @@ See <>. 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`: From 117dbc5097136f3f86bc676644853798f34c0bf8 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Mon, 1 Apr 2019 13:32:18 -0400 Subject: [PATCH 2/3] * Fix `RouterSpec.RouterMappingProvider` to `LinkedHashMap` as well --- .../java/org/springframework/integration/dsl/RouterSpec.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 7978f4b8ef8..cfd2aaafed3 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 @@ -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 mapping = new HashMap<>(); + private final Map mapping = new LinkedHashMap<>(); RouterMappingProvider(MappingMessageRouterManagement router) { this.router = router; From 5cdeb29abbb597430528291df57dd08ab8a2dbd8 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Mon, 1 Apr 2019 14:30:35 -0400 Subject: [PATCH 3/3] * Fix `RouterTests` for proper mapping order * Polishing for `AbstractMappingMessageRouter` hierarchy, so we don't use `protected channelMappings` field access any more --- .../router/AbstractMappingMessageRouter.java | 11 ++++------- .../ErrorMessageExceptionTypeRouter.java | 4 ++-- .../integration/router/PayloadTypeRouter.java | 19 +++++++++---------- .../integration/dsl/routers/RouterTests.java | 2 +- 4 files changed, 16 insertions(+), 20 deletions(-) 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 50622a16e39..10f8f919505 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 @@ -68,13 +68,13 @@ protected boolean removeEldestEntry(Entry eldest) { }); - protected volatile Map channelMappings = new LinkedHashMap<>(); + 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 channelMappings = new LinkedHashMap<>(); /** @@ -228,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; } diff --git a/spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java b/spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java index 36375544026..8982b1224ee 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/router/ErrorMessageExceptionTypeRouter.java @@ -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; } diff --git a/spring-integration-core/src/main/java/org/springframework/integration/router/PayloadTypeRouter.java b/spring-integration-core/src/main/java/org/springframework/integration/router/PayloadTypeRouter.java index e7ef537b06a..1d3224d4b53 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/router/PayloadTypeRouter.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/router/PayloadTypeRouter.java @@ -16,8 +16,8 @@ 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; @@ -25,12 +25,12 @@ /** * 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,7 +47,7 @@ public class PayloadTypeRouter extends AbstractMappingMessageRouter { */ @Override protected List getChannelKeys(Message message) { - if (CollectionUtils.isEmpty(this.channelMappings)) { + if (CollectionUtils.isEmpty(getChannelMappings())) { return null; } Class type = message.getPayload().getClass(); @@ -55,15 +55,14 @@ protected List getChannelKeys(Message message) { if (isArray) { type = type.getComponentType(); } - String closestMatch = this.findClosestMatch(type, isArray); - return (closestMatch != null) ? Collections.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 matches = new ArrayList(); - for (String candidate : this.channelMappings.keySet()) { + List 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); } } diff --git a/spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/RouterTests.java b/spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/RouterTests.java index 775eb449eae..32ffd476fb5 100644 --- a/spring-integration-core/src/test/java/org/springframework/integration/dsl/routers/RouterTests.java +++ b/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"));