Improve MappingRegistry tests and polish

Issue: SPR-11541

spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java

@@ -148,6 +148,13 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 		return this.mappingRegistry.getHandlerMethodsByMappingName(mappingName);
 	}
 
+	/**
+	 * Return the internal mapping registry. Provided for testing purposes.
+	 */
+	MappingRegistry getMappingRegistry() {
+		return this.mappingRegistry;
+	}
+
 
 	/**
 	 * Detects handler methods at initialization.
@@ -341,7 +348,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 	 */
 	protected HandlerMethod lookupHandlerMethod(String lookupPath, HttpServletRequest request) throws Exception {
 		List<Match> matches = new ArrayList<Match>();
-		List<T> directPathMatches = this.mappingRegistry.getMappingKeysByUrl(lookupPath);
+		List<T> directPathMatches = this.mappingRegistry.getMappingsByUrl(lookupPath);
 		if (directPathMatches != null) {
 			addMatchingMappings(directPathMatches, matches, request);
 		}
@@ -382,7 +389,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 		for (T mapping : mappings) {
 			T match = getMatchingMapping(mapping, request);
 			if (match != null) {
-				matches.add(new Match(match, this.mappingRegistry.getHandlerMethod(mapping)));
+				matches.add(new Match(match, this.mappingRegistry.getMappings().get(mapping)));
 			}
 		}
 	}
@@ -443,17 +450,23 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 	}
 
 
-	private class MappingRegistry {
+	/**
+	 * A registry that maintains all mappings to handler methods, exposing methods
+	 * to perform lookups and providing concurrent access.
+	 *
+	 * <p>Package-private for testing purposes.
+	 */
+	class MappingRegistry {
+
+		private final Map<T, MappingRegistration<T>> registry = new HashMap<T, MappingRegistration<T>>();
 
 		private final Map<T, HandlerMethod> mappingLookup = new LinkedHashMap<T, HandlerMethod>();
 
 		private final MultiValueMap<String, T> urlLookup = new LinkedMultiValueMap<String, T>();
 
-		private final Map<String, List<HandlerMethod>> mappingNameLookup =
+		private final Map<String, List<HandlerMethod>> nameLookup =
 				new ConcurrentHashMap<String, List<HandlerMethod>>();
 
-		private final Map<T, MappingDefinition<T>> definitionMap = new HashMap<T, MappingDefinition<T>>();
-
 		private final Map<Method, CorsConfiguration> corsLookup =
 				new ConcurrentHashMap<Method, CorsConfiguration>();
 
@@ -473,23 +486,15 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 		 * Return matches for the given URL path. Not thread-safe.
 		 * @see #acquireReadLock()
 		 */
-		public List<T> getMappingKeysByUrl(String urlPath) {
+		public List<T> getMappingsByUrl(String urlPath) {
 			return this.urlLookup.get(urlPath);
 		}
 
-		/**
-		 * Return the handler method for the mapping key. Not thread-safe.
-		 * @see #acquireReadLock()
-		 */
-		public HandlerMethod getHandlerMethod(T mapping) {
-			return this.mappingLookup.get(mapping);
-		}
-
 		/**
 		 * Return handler methods by mapping name. Thread-safe for concurrent use.
 		 */
 		public List<HandlerMethod> getHandlerMethodsByMappingName(String mappingName) {
-			return this.mappingNameLookup.get(mappingName);
+			return this.nameLookup.get(mappingName);
 		}
 
 		/**
@@ -501,14 +506,14 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 		}
 
 		/**
-		 * Acquire the read lock when using getMappings and getMappingKeysByUrl.
+		 * Acquire the read lock when using getMappings and getMappingsByUrl.
 		 */
 		public void acquireReadLock() {
 			this.readWriteLock.readLock().lock();
 		}
 
 		/**
-		 * Release the read lock after using getMappings and getMappingKeysByUrl.
+		 * Release the read lock after using getMappings and getMappingsByUrl.
 		 */
 		public void releaseReadLock() {
 			this.readWriteLock.readLock().unlock();
@@ -543,8 +548,8 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 					this.corsLookup.put(method, corsConfig);
 				}
 
-				this.definitionMap.put(mapping,
+				this.registry.put(mapping,
-						new MappingDefinition<T>(mapping, handlerMethod, directUrls, name, corsConfig));
+						new MappingRegistration<T>(mapping, handlerMethod, directUrls, name, corsConfig));
 			}
 			finally {
 				this.readWriteLock.writeLock().unlock();
@@ -573,8 +578,8 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 
 		private void addMappingName(String name, HandlerMethod handlerMethod) {
 
-			List<HandlerMethod> oldList = this.mappingNameLookup.containsKey(name) ?
+			List<HandlerMethod> oldList = this.nameLookup.containsKey(name) ?
-					this.mappingNameLookup.get(name) : Collections.<HandlerMethod>emptyList();
+					this.nameLookup.get(name) : Collections.<HandlerMethod>emptyList();
 
 			for (HandlerMethod current : oldList) {
 				if (handlerMethod.getMethod().equals(current.getMethod())) {
@@ -589,7 +594,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 			List<HandlerMethod> newList = new ArrayList<HandlerMethod>(oldList.size() + 1);
 			newList.addAll(oldList);
 			newList.add(handlerMethod);
-			this.mappingNameLookup.put(name, newList);
+			this.nameLookup.put(name, newList);
 
 			if (newList.size() > 1) {
 				if (logger.isDebugEnabled()) {
@@ -602,7 +607,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 		public void unregister(T mapping) {
 			this.readWriteLock.writeLock().lock();
 			try {
-				MappingDefinition<T> definition = this.definitionMap.remove(mapping);
+				MappingRegistration<T> definition = this.registry.remove(mapping);
 				if (definition == null) {
 					return;
 				}
@@ -628,18 +633,18 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 			}
 		}
 
-		private void removeMappingName(MappingDefinition<T> definition) {
+		private void removeMappingName(MappingRegistration<T> definition) {
-			String name = definition.getName();
+			String name = definition.getMappingName();
 			if (name == null) {
 				return;
 			}
 			HandlerMethod handlerMethod = definition.getHandlerMethod();
-			List<HandlerMethod> oldList = this.mappingNameLookup.get(name);
+			List<HandlerMethod> oldList = this.nameLookup.get(name);
 			if (oldList == null) {
 				return;
 			}
 			if (oldList.size() <= 1) {
-				this.mappingNameLookup.remove(name);
+				this.nameLookup.remove(name);
 				return;
 			}
 			List<HandlerMethod> newList = new ArrayList<HandlerMethod>(oldList.size() - 1);
@@ -648,12 +653,12 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 					newList.add(current);
 				}
 			}
-			this.mappingNameLookup.put(name, newList);
+			this.nameLookup.put(name, newList);
 		}
 
 	}
 
-	private static class MappingDefinition<T> {
+	private static class MappingRegistration<T> {
 
 		private final T mapping;
 
@@ -661,13 +666,13 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 
 		private final List<String> directUrls;
 
-		private final String name;
+		private final String mappingName;
 
 		private final CorsConfiguration corsConfiguration;
 
 
-		public MappingDefinition(T mapping, HandlerMethod handlerMethod, List<String> directUrls,
+		public MappingRegistration(T mapping, HandlerMethod handlerMethod, List<String> directUrls,
-				String name, CorsConfiguration corsConfiguration) {
+				String mappingName, CorsConfiguration corsConfiguration) {
 
 			Assert.notNull(mapping);
 			Assert.notNull(handlerMethod);
@@ -675,7 +680,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 			this.mapping = mapping;
 			this.handlerMethod = handlerMethod;
 			this.directUrls = (directUrls != null ? directUrls : Collections.<String>emptyList());
-			this.name = name;
+			this.mappingName = mappingName;
 			this.corsConfiguration = corsConfiguration;
 		}
 
@@ -692,8 +697,8 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 			return this.directUrls;
 		}
 
-		public String getName() {
+		public String getMappingName() {
-			return this.name;
+			return this.mappingName;
 		}
 
 		public CorsConfiguration getCorsConfiguration() {

spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java

@@ -19,8 +19,9 @@ package org.springframework.web.servlet.handler;
 import static org.junit.Assert.*;
 
 import java.lang.reflect.Method;
+import java.util.Collections;
 import java.util.Comparator;
-import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 import javax.servlet.http.HttpServletRequest;
@@ -34,14 +35,17 @@ import org.springframework.stereotype.Controller;
 import org.springframework.util.AntPathMatcher;
 import org.springframework.util.PathMatcher;
 import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.cors.CorsConfiguration;
 import org.springframework.web.method.HandlerMethod;
 import org.springframework.web.util.UrlPathHelper;
 
+
 /**
  * Test for {@link AbstractHandlerMethodMapping}.
  *
  * @author Arjen Poutsma
  */
+@SuppressWarnings("unused")
 public class HandlerMethodMappingTests {
 
 	private AbstractHandlerMethodMapping<String> mapping;
@@ -52,44 +56,46 @@ public class HandlerMethodMappingTests {
 
 	private Method method2;
 
+
 	@Before
 	public void setUp() throws Exception {
-		mapping = new MyHandlerMethodMapping();
+		this.mapping = new MyHandlerMethodMapping();
-		handler = new MyHandler();
+		this.handler = new MyHandler();
-		method1 = handler.getClass().getMethod("handlerMethod1");
+		this.method1 = handler.getClass().getMethod("handlerMethod1");
-		method2 = handler.getClass().getMethod("handlerMethod2");
+		this.method2 = handler.getClass().getMethod("handlerMethod2");
 	}
 
+
 	@Test(expected = IllegalStateException.class)
 	public void registerDuplicates() {
-		mapping.registerMapping("foo", handler, method1);
+		this.mapping.registerMapping("foo", this.handler, this.method1);
-		mapping.registerMapping("foo", handler, method2);
+		this.mapping.registerMapping("foo", this.handler, this.method2);
 	}
 
 	@Test
 	public void directMatch() throws Exception {
 		String key = "foo";
-		mapping.registerMapping(key, handler, method1);
+		this.mapping.registerMapping(key, this.handler, this.method1);
 
-		HandlerMethod result = mapping.getHandlerInternal(new MockHttpServletRequest("GET", key));
+		HandlerMethod result = this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", key));
 		assertEquals(method1, result.getMethod());
 	}
 
 	@Test
 	public void patternMatch() throws Exception {
-		mapping.registerMapping("/fo*", handler, method1);
+		this.mapping.registerMapping("/fo*", this.handler, this.method1);
-		mapping.registerMapping("/f*", handler, method2);
+		this.mapping.registerMapping("/f*", this.handler, this.method2);
 
-		HandlerMethod result = mapping.getHandlerInternal(new MockHttpServletRequest("GET", "/foo"));
+		HandlerMethod result = this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", "/foo"));
 		assertEquals(method1, result.getMethod());
 	}
 
 	@Test(expected = IllegalStateException.class)
 	public void ambiguousMatch() throws Exception {
-		mapping.registerMapping("/f?o", handler, method1);
+		this.mapping.registerMapping("/f?o", this.handler, this.method1);
-		mapping.registerMapping("/fo?", handler, method2);
+		this.mapping.registerMapping("/fo?", this.handler, this.method2);
 
-		mapping.getHandlerInternal(new MockHttpServletRequest("GET", "/foo"));
+		this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", "/foo"));
 	}
 
 	@Test
@@ -112,15 +118,62 @@ public class HandlerMethodMappingTests {
 	}
 
 	@Test
-	public void unregister() throws Exception {
+	public void registerMapping() throws Exception {
+
+		String key1 = "/foo";
+		String key2 = "/foo*";
+		this.mapping.registerMapping(key1, this.handler, this.method1);
+		this.mapping.registerMapping(key2, this.handler, this.method2);
+
+		// Direct URL lookup
+
+		List directUrlMatches = this.mapping.getMappingRegistry().getMappingsByUrl(key1);
+		assertNotNull(directUrlMatches);
+		assertEquals(1, directUrlMatches.size());
+		assertEquals(key1, directUrlMatches.get(0));
+
+		// Mapping name lookup
+
+		HandlerMethod handlerMethod1 = new HandlerMethod(this.handler, this.method1);
+		HandlerMethod handlerMethod2 = new HandlerMethod(this.handler, this.method2);
+
+		String name1 = this.method1.getName();
+		List<HandlerMethod> handlerMethods = this.mapping.getMappingRegistry().getHandlerMethodsByMappingName(name1);
+		assertNotNull(handlerMethods);
+		assertEquals(1, handlerMethods.size());
+		assertEquals(handlerMethod1, handlerMethods.get(0));
+
+		String name2 = this.method2.getName();
+		handlerMethods = this.mapping.getMappingRegistry().getHandlerMethodsByMappingName(name2);
+		assertNotNull(handlerMethods);
+		assertEquals(1, handlerMethods.size());
+		assertEquals(handlerMethod2, handlerMethods.get(0));
+
+		// CORS lookup
+
+		CorsConfiguration config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod1);
+		assertNotNull(config);
+		assertEquals("http://" + name1, config.getAllowedOrigins().get(0));
+
+		config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod2);
+		assertNotNull(config);
+		assertEquals("http://" + name2, config.getAllowedOrigins().get(0));
+	}
+
+	@Test
+	public void unregisterMapping() throws Exception {
+
 		String key = "foo";
-		mapping.registerMapping(key, handler, method1);
+		HandlerMethod handlerMethod = new HandlerMethod(this.handler, this.method1);
 
-		HandlerMethod handlerMethod = mapping.getHandlerInternal(new MockHttpServletRequest("GET", key));
+		this.mapping.registerMapping(key, this.handler, this.method1);
-		assertEquals(method1, handlerMethod.getMethod());
+		assertNotNull(this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", key)));
 
-		mapping.unregisterMapping(key);
+		this.mapping.unregisterMapping(key);
 		assertNull(mapping.getHandlerInternal(new MockHttpServletRequest("GET", key)));
+		assertNull(this.mapping.getMappingRegistry().getMappingsByUrl(key));
+		assertNull(this.mapping.getMappingRegistry().getHandlerMethodsByMappingName(this.method1.getName()));
+		assertNull(this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod));
 	}
 
 
@@ -130,10 +183,14 @@ public class HandlerMethodMappingTests {
 
 		private PathMatcher pathMatcher = new AntPathMatcher();
 
+
+		public MyHandlerMethodMapping() {
+			setHandlerMethodMappingNamingStrategy(new SimpleMappingNamingStrategy());
+		}
+
 		@Override
-		protected String getMatchingMapping(String pattern, HttpServletRequest request) {
+		protected boolean isHandler(Class<?> beanType) {
-			String lookupPath = pathHelper.getLookupPathForRequest(request);
+			return true;
-			return pathMatcher.match(pattern, lookupPath) ? pattern : null;
 		}
 
 		@Override
@@ -142,24 +199,40 @@ public class HandlerMethodMappingTests {
 			return methodName.startsWith("handler") ? methodName : null;
 		}
 
-		@Override
-		protected Comparator<String> getMappingComparator(HttpServletRequest request) {
-			String lookupPath = pathHelper.getLookupPathForRequest(request);
-			return pathMatcher.getPatternComparator(lookupPath);
-		}
-
-		@Override
-		protected boolean isHandler(Class<?> beanType) {
-			return true;
-		}
-
 		@Override
 		protected Set<String> getMappingPathPatterns(String key) {
-			return new HashSet<String>();
+			return (this.pathMatcher.isPattern(key) ? Collections.<String>emptySet() : Collections.singleton(key));
+		}
+
+		@Override
+		protected CorsConfiguration initCorsConfiguration(Object handler, Method method, String mapping) {
+			CorsConfiguration corsConfig = new CorsConfiguration();
+			corsConfig.setAllowedOrigins(Collections.singletonList("http://" + method.getName()));
+			return corsConfig;
+		}
+
+		@Override
+		protected String getMatchingMapping(String pattern, HttpServletRequest request) {
+			String lookupPath = this.pathHelper.getLookupPathForRequest(request);
+			return this.pathMatcher.match(pattern, lookupPath) ? pattern : null;
+		}
+
+		@Override
+		protected Comparator<String> getMappingComparator(HttpServletRequest request) {
+			String lookupPath = this.pathHelper.getLookupPathForRequest(request);
+			return this.pathMatcher.getPatternComparator(lookupPath);
+		}
+
+	}
+
+	private static class SimpleMappingNamingStrategy implements HandlerMethodMappingNamingStrategy<String> {
+
+		@Override
+		public String getName(HandlerMethod handlerMethod, String mapping) {
+			return handlerMethod.getMethod().getName();
 		}
 	}
 
-	@SuppressWarnings("unused")
 	@Controller
 	static class MyHandler {