From 8853107f76e318ce1595c58f36a05716db5ce1c7 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 5 May 2015 12:40:47 -0400 Subject: [PATCH] Find CORS config by HandlerMethod Before this change AbstractHandlerMethodMapping used a map from Method to CorsConfiguration. That works for regular @RequestMapping methods. However frameworks like Spring Boot and Spring Integration may programmatically register the same Method under multiple mappings, i.e. adapter/gateway type classes. This change ensures that CorsConfiguraiton is indexed by HandlerMethod so that we can store CorsConfiguration for different handler instances even when the method is the same. In order for to make this work, HandlerMethod now provides an additional field called resolvedFromHandlerMethod that returns the original HandlerMethod (with the String bean name). This makes it possible to perform reliable lookups. Issue: SPR-11541 --- .../web/method/HandlerMethod.java | 15 ++++ .../handler/AbstractHandlerMethodMapping.java | 32 ++++----- .../handler/HandlerMethodMappingTests.java | 71 ++++++++++++++++++- 3 files changed, 95 insertions(+), 23 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java b/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java index cdbf7315a5..6905dce959 100644 --- a/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java +++ b/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java @@ -60,6 +60,8 @@ public class HandlerMethod { private final MethodParameter[] parameters; + private final HandlerMethod resolvedFromHandlerMethod; + /** * Create an instance from a bean instance and a method. @@ -73,6 +75,7 @@ public class HandlerMethod { this.method = method; this.bridgedMethod = BridgeMethodResolver.findBridgedMethod(method); this.parameters = initMethodParameters(); + this.resolvedFromHandlerMethod = null; } /** @@ -88,6 +91,7 @@ public class HandlerMethod { this.method = bean.getClass().getMethod(methodName, parameterTypes); this.bridgedMethod = BridgeMethodResolver.findBridgedMethod(this.method); this.parameters = initMethodParameters(); + this.resolvedFromHandlerMethod = null; } /** @@ -105,6 +109,7 @@ public class HandlerMethod { this.method = method; this.bridgedMethod = BridgeMethodResolver.findBridgedMethod(method); this.parameters = initMethodParameters(); + this.resolvedFromHandlerMethod = null; } /** @@ -118,6 +123,7 @@ public class HandlerMethod { this.method = handlerMethod.method; this.bridgedMethod = handlerMethod.bridgedMethod; this.parameters = handlerMethod.parameters; + this.resolvedFromHandlerMethod = handlerMethod.resolvedFromHandlerMethod; } /** @@ -132,6 +138,7 @@ public class HandlerMethod { this.method = handlerMethod.method; this.bridgedMethod = handlerMethod.bridgedMethod; this.parameters = handlerMethod.parameters; + this.resolvedFromHandlerMethod = handlerMethod; } @@ -182,6 +189,14 @@ public class HandlerMethod { return this.parameters; } + /** + * Return the HandlerMethod from which this HandlerMethod instance was + * resolved via {@link #createWithResolvedBean()}. + */ + public HandlerMethod getResolvedFromHandlerMethod() { + return this.resolvedFromHandlerMethod; + } + /** * Return the HandlerMethod return type. */ 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 29803e9f59..2acb0f83ff 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 @@ -467,8 +467,8 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap private final Map> nameLookup = new ConcurrentHashMap>(); - private final Map corsLookup = - new ConcurrentHashMap(); + private final Map corsLookup = + new ConcurrentHashMap(); private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); @@ -501,8 +501,8 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap * Return CORS configuration. Thread-safe for concurrent use. */ public CorsConfiguration getCorsConfiguration(HandlerMethod handlerMethod) { - Method method = handlerMethod.getMethod(); - return this.corsLookup.get(method); + HandlerMethod original = handlerMethod.getResolvedFromHandlerMethod(); + return this.corsLookup.get(original != null ? original : handlerMethod); } /** @@ -545,11 +545,10 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap CorsConfiguration corsConfig = initCorsConfiguration(handler, method, mapping); if (corsConfig != null) { - this.corsLookup.put(method, corsConfig); + this.corsLookup.put(handlerMethod, corsConfig); } - this.registry.put(mapping, - new MappingRegistration(mapping, handlerMethod, directUrls, name, corsConfig)); + this.registry.put(mapping, new MappingRegistration(mapping, handlerMethod, directUrls, name)); } finally { this.readWriteLock.writeLock().unlock(); @@ -582,7 +581,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap this.nameLookup.get(name) : Collections.emptyList(); for (HandlerMethod current : oldList) { - if (handlerMethod.getMethod().equals(current.getMethod())) { + if (handlerMethod.equals(current)) { return; } } @@ -597,8 +596,8 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap this.nameLookup.put(name, newList); if (newList.size() > 1) { - if (logger.isDebugEnabled()) { - logger.debug("Mapping name clash for handlerMethods=" + newList + + if (logger.isTraceEnabled()) { + logger.trace("Mapping name clash for handlerMethods=" + newList + ". Consider assigning explicit names."); } } @@ -626,7 +625,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap removeMappingName(definition); - this.corsLookup.remove(definition.getHandlerMethod().getMethod()); + this.corsLookup.remove(definition.getHandlerMethod()); } finally { this.readWriteLock.writeLock().unlock(); @@ -668,11 +667,9 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap private final String mappingName; - private final CorsConfiguration corsConfiguration; - - public MappingRegistration(T mapping, HandlerMethod handlerMethod, List directUrls, - String mappingName, CorsConfiguration corsConfiguration) { + public MappingRegistration(T mapping, HandlerMethod handlerMethod, + List directUrls, String mappingName) { Assert.notNull(mapping); Assert.notNull(handlerMethod); @@ -681,7 +678,6 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap this.handlerMethod = handlerMethod; this.directUrls = (directUrls != null ? directUrls : Collections.emptyList()); this.mappingName = mappingName; - this.corsConfiguration = corsConfiguration; } @@ -700,10 +696,6 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap public String getMappingName() { return this.mappingName; } - - public CorsConfiguration getCorsConfiguration() { - return this.corsConfiguration; - } } 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 044920e8c8..74c057b462 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 @@ -29,12 +29,15 @@ import javax.servlet.http.HttpServletRequest; import org.junit.Before; import org.junit.Test; +import org.springframework.beans.factory.support.StaticListableBeanFactory; +import org.springframework.context.support.GenericApplicationContext; import org.springframework.context.support.StaticApplicationContext; import org.springframework.mock.web.test.MockHttpServletRequest; 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.context.support.StaticWebApplicationContext; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.method.HandlerMethod; import org.springframework.web.util.UrlPathHelper; @@ -44,6 +47,7 @@ import org.springframework.web.util.UrlPathHelper; * Test for {@link AbstractHandlerMethodMapping}. * * @author Arjen Poutsma + * @author Rossen Stoyanchev */ @SuppressWarnings("unused") public class HandlerMethodMappingTests { @@ -153,11 +157,53 @@ public class HandlerMethodMappingTests { CorsConfiguration config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod1); assertNotNull(config); - assertEquals("http://" + name1, config.getAllowedOrigins().get(0)); + assertEquals("http://" + handler.hashCode() + name1, config.getAllowedOrigins().get(0)); config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod2); assertNotNull(config); - assertEquals("http://" + name2, config.getAllowedOrigins().get(0)); + assertEquals("http://" + handler.hashCode() + name2, config.getAllowedOrigins().get(0)); + } + + @Test + public void registerMappingWithSameMethodAndTwoHandlerInstances() throws Exception { + + String key1 = "foo"; + String key2 = "bar"; + + MyHandler handler1 = new MyHandler(); + MyHandler handler2 = new MyHandler(); + + HandlerMethod handlerMethod1 = new HandlerMethod(handler1, this.method1); + HandlerMethod handlerMethod2 = new HandlerMethod(handler2, this.method1); + + this.mapping.registerMapping(key1, handler1, this.method1); + this.mapping.registerMapping(key2, handler2, this.method1); + + // Direct URL lookup + + List directUrlMatches = this.mapping.getMappingRegistry().getMappingsByUrl(key1); + assertNotNull(directUrlMatches); + assertEquals(1, directUrlMatches.size()); + assertEquals(key1, directUrlMatches.get(0)); + + // Mapping name lookup + + String name = this.method1.getName(); + List handlerMethods = this.mapping.getMappingRegistry().getHandlerMethodsByMappingName(name); + assertNotNull(handlerMethods); + assertEquals(2, handlerMethods.size()); + assertEquals(handlerMethod1, handlerMethods.get(0)); + assertEquals(handlerMethod2, handlerMethods.get(1)); + + // CORS lookup + + CorsConfiguration config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod1); + assertNotNull(config); + assertEquals("http://" + handler1.hashCode() + name, config.getAllowedOrigins().get(0)); + + config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod2); + assertNotNull(config); + assertEquals("http://" + handler2.hashCode() + name, config.getAllowedOrigins().get(0)); } @Test @@ -176,6 +222,25 @@ public class HandlerMethodMappingTests { assertNull(this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod)); } + @Test + public void getCorsConfigWithBeanNameHandler() throws Exception { + + String key = "foo"; + String beanName = "handler1"; + + StaticWebApplicationContext context = new StaticWebApplicationContext(); + context.registerSingleton(beanName, MyHandler.class); + + this.mapping.setApplicationContext(context); + this.mapping.registerMapping(key, beanName, this.method1); + HandlerMethod handlerMethod = this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", key)); + + CorsConfiguration config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod); + assertNotNull(config); + assertEquals("http://" + beanName.hashCode() + this.method1.getName(), config.getAllowedOrigins().get(0)); + } + + private static class MyHandlerMethodMapping extends AbstractHandlerMethodMapping { @@ -207,7 +272,7 @@ public class HandlerMethodMappingTests { @Override protected CorsConfiguration initCorsConfiguration(Object handler, Method method, String mapping) { CorsConfiguration corsConfig = new CorsConfiguration(); - corsConfig.setAllowedOrigins(Collections.singletonList("http://" + method.getName())); + corsConfig.setAllowedOrigins(Collections.singletonList("http://" + handler.hashCode() + method.getName())); return corsConfig; }