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

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<T> extends AbstractHandlerMap
 	protected abstract T getMappingForMethod(Method method, Class<?> handlerType);
 
 	/**
-	 * Register a handler method and its unique mapping.
-	 * <p>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.
-	 * <p>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<T> extends AbstractHandlerMap
 	protected void handlerMethodsInitialized(Map<T, HandlerMethod> handlerMethods) {
 	}
 
+	/**
+	 * Register the given mapping.
+	 * <p>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.
+	 * <p>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<T> extends AbstractHandlerMap
 		private final Map<String, List<HandlerMethod>> mappingNameLookup =
 				new ConcurrentHashMap<String, List<HandlerMethod>>();
 
-		private final Map<Method, MappingDefinition<T>> methodLookup =
-				new ConcurrentHashMap<Method, MappingDefinition<T>>();
+		private final Map<T, MappingDefinition<T>> definitionMap = new HashMap<T, MappingDefinition<T>>();
+
+		private final Map<Method, CorsConfiguration> corsLookup =
+				new ConcurrentHashMap<Method, CorsConfiguration>();
 
 
 		private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();
@@ -480,8 +497,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 		 */
 		public CorsConfiguration getCorsConfiguration(HandlerMethod handlerMethod) {
 			Method method = handlerMethod.getMethod();
-			MappingDefinition<T> definition = this.methodLookup.get(method);
-			return (definition != null ? definition.getCorsConfiguration() : null);
+			return this.corsLookup.get(method);
 		}
 
 		/**
@@ -499,7 +515,7 @@ public abstract class AbstractHandlerMethodMapping<T> 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<T> 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<T>(mapping, handlerMethod, directUrls, name, corsConfig));
 			}
 			finally {
@@ -540,11 +559,6 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 								newHandlerMethod + "\nto " +	mapping + ": There is already '" +
 								handlerMethod.getBean() + "' bean method\n" + handlerMethod + " mapped.");
 			}
-			MappingDefinition<T> 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<String> getDirectUrls(T mapping) {
@@ -585,10 +599,10 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 			}
 		}
 
-		public void unregister(HandlerMethod handlerMethod) {
+		public void unregister(T mapping) {
 			this.readWriteLock.writeLock().lock();
 			try {
-				MappingDefinition<T> definition = this.methodLookup.remove(handlerMethod.getMethod());
+				MappingDefinition<T> definition = this.definitionMap.remove(mapping);
 				if (definition == null) {
 					return;
 				}
@@ -606,6 +620,8 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 				}
 
 				removeMappingName(definition);
+
+				this.corsLookup.remove(definition.getHandlerMethod().getMethod());
 			}
 			finally {
 				this.readWriteLock.writeLock().unlock();

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 {