From 71683c5f8db19e3795f27d0c7ad7b4dbaa3083fe Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 4 May 2015 17:24:49 -0400 Subject: [PATCH] Fix regressions from 8f558e7 The change to provide public register/unregister methods in AbstractHandlerMethodMapping assumed that a single method cannot be mapped more than once. This is not the case with the MvcEndpoints and EndpointHandlerMapping from Spring Boot which wrap one or more non-web Endpoint types with an MvcEndpointAdapter in order to expose them for use over the web. In effect Spring MVC sees a single handler method mapped many times. This change removes that assumption so rather than unregistering with a HandlerMethod, which is not necessarily unique, the unregister method now takes the actual mapping, which is the only thing that should actually be unique. Issue: SPR-11541 --- .../handler/AbstractHandlerMethodMapping.java | 70 ++++++++++++------- .../handler/HandlerMethodMappingTests.java | 19 ++--- 2 files changed, 53 insertions(+), 36 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java index 4050407f9d..9a27045305 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java @@ -21,6 +21,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.List; @@ -227,26 +228,21 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap protected abstract T getMappingForMethod(Method method, Class handlerType); /** - * Register a handler method and its unique mapping. - *

Invoked at startup for each detected handler method. May also be - * invoked at runtime after initialization is complete. + * Register a handler method and its unique mapping. Invoked at startup for + * each detected handler method. * @param handler the bean name of the handler or the handler instance * @param method the method to register * @param mapping the mapping conditions associated with the handler method * @throws IllegalStateException if another method was already registered * under the same mapping + * @deprecated as of 4.2 you can invoke the public methods + * {@link #registerMapping(Object, Object, Method)} and + * {@link #unregisterMapping(Object)} during initialization or at runtime, + * i.e. after initialization is complete. */ - public void registerHandlerMethod(Object handler, Method method, T mapping) { - this.mappingRegistry.register(handler, method, mapping); - } - - /** - * Un-register a handler method. - *

This method may be invoked at runtime after initialization has completed. - * @param handlerMethod the handler method to be unregistered - */ - public void unregisterHandlerMethod(HandlerMethod handlerMethod) { - this.mappingRegistry.unregister(handlerMethod); + @Deprecated + protected void registerHandlerMethod(Object handler, Method method, T mapping) { + this.mappingRegistry.register(mapping, handler, method); } /** @@ -287,6 +283,25 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap protected void handlerMethodsInitialized(Map handlerMethods) { } + /** + * Register the given mapping. + *

This method may be invoked at runtime after initialization has completed. + * @param mapping the mapping for the handler method + * @param handler the handler + * @param method the method + */ + public void registerMapping(T mapping, Object handler, Method method) { + this.mappingRegistry.register(mapping, handler, method); + } + + /** + * Un-register the given mapping. + *

This method may be invoked at runtime after initialization has completed. + * @param mapping the mapping to unregister + */ + public void unregisterMapping(T mapping) { + this.mappingRegistry.unregister(mapping); + } /** * Look up a handler method for the given request. @@ -437,8 +452,10 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap private final Map> mappingNameLookup = new ConcurrentHashMap>(); - private final Map> methodLookup = - new ConcurrentHashMap>(); + private final Map> definitionMap = new HashMap>(); + + private final Map corsLookup = + new ConcurrentHashMap(); private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); @@ -480,8 +497,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap */ public CorsConfiguration getCorsConfiguration(HandlerMethod handlerMethod) { Method method = handlerMethod.getMethod(); - MappingDefinition definition = this.methodLookup.get(method); - return (definition != null ? definition.getCorsConfiguration() : null); + return this.corsLookup.get(method); } /** @@ -499,7 +515,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } - public void register(Object handler, Method method, T mapping) { + public void register(T mapping, Object handler, Method method) { this.readWriteLock.writeLock().lock(); try { @@ -523,8 +539,11 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } CorsConfiguration corsConfig = initCorsConfiguration(handler, method, mapping); + if (corsConfig != null) { + this.corsLookup.put(method, corsConfig); + } - this.methodLookup.put(method, + this.definitionMap.put(mapping, new MappingDefinition(mapping, handlerMethod, directUrls, name, corsConfig)); } finally { @@ -540,11 +559,6 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap newHandlerMethod + "\nto " + mapping + ": There is already '" + handlerMethod.getBean() + "' bean method\n" + handlerMethod + " mapped."); } - MappingDefinition definition = this.methodLookup.get(newHandlerMethod.getMethod()); - if (definition != null) { - throw new IllegalStateException("Cannot map " + newHandlerMethod.getMethod() + - "\nto " + mapping + ".\n It is already mapped to " + definition.getMapping()); - } } private List getDirectUrls(T mapping) { @@ -585,10 +599,10 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } } - public void unregister(HandlerMethod handlerMethod) { + public void unregister(T mapping) { this.readWriteLock.writeLock().lock(); try { - MappingDefinition definition = this.methodLookup.remove(handlerMethod.getMethod()); + MappingDefinition definition = this.definitionMap.remove(mapping); if (definition == null) { return; } @@ -606,6 +620,8 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } removeMappingName(definition); + + this.corsLookup.remove(definition.getHandlerMethod().getMethod()); } finally { this.readWriteLock.writeLock().unlock(); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java index 61d5b18bd1..6a1c577aba 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java @@ -62,14 +62,14 @@ public class HandlerMethodMappingTests { @Test(expected = IllegalStateException.class) public void registerDuplicates() { - mapping.registerHandlerMethod(handler, method1, "foo"); - mapping.registerHandlerMethod(handler, method2, "foo"); + mapping.registerMapping("foo", handler, method1); + mapping.registerMapping("foo", handler, method2); } @Test public void directMatch() throws Exception { String key = "foo"; - mapping.registerHandlerMethod(handler, method1, key); + mapping.registerMapping(key, handler, method1); HandlerMethod result = mapping.getHandlerInternal(new MockHttpServletRequest("GET", key)); assertEquals(method1, result.getMethod()); @@ -77,8 +77,8 @@ public class HandlerMethodMappingTests { @Test public void patternMatch() throws Exception { - mapping.registerHandlerMethod(handler, method1, "/fo*"); - mapping.registerHandlerMethod(handler, method2, "/f*"); + mapping.registerMapping("/fo*", handler, method1); + mapping.registerMapping("/f*", handler, method2); HandlerMethod result = mapping.getHandlerInternal(new MockHttpServletRequest("GET", "/foo")); assertEquals(method1, result.getMethod()); @@ -86,8 +86,8 @@ public class HandlerMethodMappingTests { @Test(expected = IllegalStateException.class) public void ambiguousMatch() throws Exception { - mapping.registerHandlerMethod(handler, method1, "/f?o"); - mapping.registerHandlerMethod(handler, method2, "/fo?"); + mapping.registerMapping("/f?o", handler, method1); + mapping.registerMapping("/fo?", handler, method2); mapping.getHandlerInternal(new MockHttpServletRequest("GET", "/foo")); } @@ -114,12 +114,12 @@ public class HandlerMethodMappingTests { @Test public void unregister() throws Exception { String key = "foo"; - mapping.registerHandlerMethod(handler, method1, key); + mapping.registerMapping(key, handler, method1); HandlerMethod handlerMethod = mapping.getHandlerInternal(new MockHttpServletRequest("GET", key)); assertEquals(method1, handlerMethod.getMethod()); - mapping.unregisterHandlerMethod(handlerMethod); + mapping.unregisterMapping(key); assertNull(mapping.getHandlerInternal(new MockHttpServletRequest("GET", key))); } @@ -159,6 +159,7 @@ public class HandlerMethodMappingTests { } } + @SuppressWarnings("unused") @Controller static class MyHandler {