From 4a8baebf5908c0734a1297bb20e72268fa48979a Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 5 May 2015 10:49:01 -0400 Subject: [PATCH] Improve MappingRegistry tests and polish Issue: SPR-11541 --- .../handler/AbstractHandlerMethodMapping.java | 77 +++++----- .../handler/HandlerMethodMappingTests.java | 135 ++++++++++++++---- 2 files changed, 145 insertions(+), 67 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 9a270453056..29803e9f59d 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 @@ -148,6 +148,13 @@ public abstract class AbstractHandlerMethodMapping 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 extends AbstractHandlerMap */ protected HandlerMethod lookupHandlerMethod(String lookupPath, HttpServletRequest request) throws Exception { List matches = new ArrayList(); - List directPathMatches = this.mappingRegistry.getMappingKeysByUrl(lookupPath); + List directPathMatches = this.mappingRegistry.getMappingsByUrl(lookupPath); if (directPathMatches != null) { addMatchingMappings(directPathMatches, matches, request); } @@ -382,7 +389,7 @@ public abstract class AbstractHandlerMethodMapping 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 extends AbstractHandlerMap } - private class MappingRegistry { + /** + * A registry that maintains all mappings to handler methods, exposing methods + * to perform lookups and providing concurrent access. + * + *

Package-private for testing purposes. + */ + class MappingRegistry { + + private final Map> registry = new HashMap>(); private final Map mappingLookup = new LinkedHashMap(); private final MultiValueMap urlLookup = new LinkedMultiValueMap(); - private final Map> mappingNameLookup = + private final Map> nameLookup = new ConcurrentHashMap>(); - private final Map> definitionMap = new HashMap>(); - private final Map corsLookup = new ConcurrentHashMap(); @@ -473,23 +486,15 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap * Return matches for the given URL path. Not thread-safe. * @see #acquireReadLock() */ - public List getMappingKeysByUrl(String urlPath) { + public List 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 getHandlerMethodsByMappingName(String mappingName) { - return this.mappingNameLookup.get(mappingName); + return this.nameLookup.get(mappingName); } /** @@ -501,14 +506,14 @@ public abstract class AbstractHandlerMethodMapping 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 extends AbstractHandlerMap this.corsLookup.put(method, corsConfig); } - this.definitionMap.put(mapping, - new MappingDefinition(mapping, handlerMethod, directUrls, name, corsConfig)); + this.registry.put(mapping, + new MappingRegistration(mapping, handlerMethod, directUrls, name, corsConfig)); } finally { this.readWriteLock.writeLock().unlock(); @@ -573,8 +578,8 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap private void addMappingName(String name, HandlerMethod handlerMethod) { - List oldList = this.mappingNameLookup.containsKey(name) ? - this.mappingNameLookup.get(name) : Collections.emptyList(); + List oldList = this.nameLookup.containsKey(name) ? + this.nameLookup.get(name) : Collections.emptyList(); for (HandlerMethod current : oldList) { if (handlerMethod.getMethod().equals(current.getMethod())) { @@ -589,7 +594,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap List newList = new ArrayList(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 extends AbstractHandlerMap public void unregister(T mapping) { this.readWriteLock.writeLock().lock(); try { - MappingDefinition definition = this.definitionMap.remove(mapping); + MappingRegistration definition = this.registry.remove(mapping); if (definition == null) { return; } @@ -628,18 +633,18 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } } - private void removeMappingName(MappingDefinition definition) { - String name = definition.getName(); + private void removeMappingName(MappingRegistration definition) { + String name = definition.getMappingName(); if (name == null) { return; } HandlerMethod handlerMethod = definition.getHandlerMethod(); - List oldList = this.mappingNameLookup.get(name); + List oldList = this.nameLookup.get(name); if (oldList == null) { return; } if (oldList.size() <= 1) { - this.mappingNameLookup.remove(name); + this.nameLookup.remove(name); return; } List newList = new ArrayList(oldList.size() - 1); @@ -648,12 +653,12 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap newList.add(current); } } - this.mappingNameLookup.put(name, newList); + this.nameLookup.put(name, newList); } } - private static class MappingDefinition { + private static class MappingRegistration { private final T mapping; @@ -661,13 +666,13 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap private final List directUrls; - private final String name; + private final String mappingName; private final CorsConfiguration corsConfiguration; - public MappingDefinition(T mapping, HandlerMethod handlerMethod, List directUrls, - String name, CorsConfiguration corsConfiguration) { + public MappingRegistration(T mapping, HandlerMethod handlerMethod, List directUrls, + String mappingName, CorsConfiguration corsConfiguration) { Assert.notNull(mapping); Assert.notNull(handlerMethod); @@ -675,7 +680,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap this.mapping = mapping; this.handlerMethod = handlerMethod; this.directUrls = (directUrls != null ? directUrls : Collections.emptyList()); - this.name = name; + this.mappingName = mappingName; this.corsConfiguration = corsConfiguration; } @@ -692,8 +697,8 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap return this.directUrls; } - public String getName() { - return this.name; + public String getMappingName() { + return this.mappingName; } public CorsConfiguration getCorsConfiguration() { 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 6a1c577aba9..044920e8c8b 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 @@ -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 mapping; @@ -52,44 +56,46 @@ public class HandlerMethodMappingTests { private Method method2; + @Before public void setUp() throws Exception { - mapping = new MyHandlerMethodMapping(); - handler = new MyHandler(); - method1 = handler.getClass().getMethod("handlerMethod1"); - method2 = handler.getClass().getMethod("handlerMethod2"); + this.mapping = new MyHandlerMethodMapping(); + this.handler = new MyHandler(); + this.method1 = handler.getClass().getMethod("handlerMethod1"); + this.method2 = handler.getClass().getMethod("handlerMethod2"); } + @Test(expected = IllegalStateException.class) public void registerDuplicates() { - mapping.registerMapping("foo", handler, method1); - mapping.registerMapping("foo", handler, method2); + this.mapping.registerMapping("foo", this.handler, this.method1); + 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); - mapping.registerMapping("/f*", handler, method2); + this.mapping.registerMapping("/fo*", this.handler, this.method1); + 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); - mapping.registerMapping("/fo?", handler, method2); + this.mapping.registerMapping("/f?o", this.handler, this.method1); + 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 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)); - assertEquals(method1, handlerMethod.getMethod()); + this.mapping.registerMapping(key, this.handler, this.method1); + 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) { - String lookupPath = pathHelper.getLookupPathForRequest(request); - return pathMatcher.match(pattern, lookupPath) ? pattern : null; + protected boolean isHandler(Class beanType) { + return true; } @Override @@ -143,23 +200,39 @@ public class HandlerMethodMappingTests { } @Override - protected Comparator getMappingComparator(HttpServletRequest request) { - String lookupPath = pathHelper.getLookupPathForRequest(request); - return pathMatcher.getPatternComparator(lookupPath); + protected Set getMappingPathPatterns(String key) { + return (this.pathMatcher.isPattern(key) ? Collections.emptySet() : Collections.singleton(key)); } @Override - protected boolean isHandler(Class beanType) { - return true; + protected CorsConfiguration initCorsConfiguration(Object handler, Method method, String mapping) { + CorsConfiguration corsConfig = new CorsConfiguration(); + corsConfig.setAllowedOrigins(Collections.singletonList("http://" + method.getName())); + return corsConfig; } @Override - protected Set getMappingPathPatterns(String key) { - return new HashSet(); + protected String getMatchingMapping(String pattern, HttpServletRequest request) { + String lookupPath = this.pathHelper.getLookupPathForRequest(request); + return this.pathMatcher.match(pattern, lookupPath) ? pattern : null; + } + + @Override + protected Comparator getMappingComparator(HttpServletRequest request) { + String lookupPath = this.pathHelper.getLookupPathForRequest(request); + return this.pathMatcher.getPatternComparator(lookupPath); + } + + } + + private static class SimpleMappingNamingStrategy implements HandlerMethodMappingNamingStrategy { + + @Override + public String getName(HandlerMethod handlerMethod, String mapping) { + return handlerMethod.getMethod().getName(); } } - @SuppressWarnings("unused") @Controller static class MyHandler {