HandlerMappingIntrospector handles attribute changes properly

Closes gh-26833

Author: rstoyanchev

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

@@ -19,6 +19,8 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
@@ -138,16 +140,17 @@ public void afterPropertiesSet() {
 	 */
 	@Nullable
 	public MatchableHandlerMapping getMatchableHandlerMapping(HttpServletRequest request) throws Exception {
-		HttpServletRequest wrappedRequest = new RequestAttributeChangeIgnoringWrapper(request);
+		HttpServletRequest wrappedRequest = new AttributesPreservingRequest(request);
 		return doWithMatchingMapping(wrappedRequest, false, (matchedMapping, executionChain) -> {
 			if (matchedMapping instanceof MatchableHandlerMapping) {
 				PathPatternMatchableHandlerMapping mapping = this.pathPatternHandlerMappings.get(matchedMapping);
 				if (mapping != null) {
-					RequestPath requestPath = ServletRequestPathUtils.getParsedRequestPath(request);
-					return mapping.decorate(requestPath);
+					RequestPath requestPath = ServletRequestPathUtils.getParsedRequestPath(wrappedRequest);
+					return new PathSettingHandlerMapping(mapping, requestPath);
 				}
 				else {
-					return (MatchableHandlerMapping) matchedMapping;
+					String lookupPath = (String) wrappedRequest.getAttribute(UrlPathHelper.PATH_ATTRIBUTE);
+					return new PathSettingHandlerMapping((MatchableHandlerMapping) matchedMapping, lookupPath);
 				}
 			}
 			throw new IllegalStateException("HandlerMapping is not a MatchableHandlerMapping");
@@ -157,7 +160,7 @@ public MatchableHandlerMapping getMatchableHandlerMapping(HttpServletRequest req
 	@Override
 	@Nullable
 	public CorsConfiguration getCorsConfiguration(HttpServletRequest request) {
-		RequestAttributeChangeIgnoringWrapper wrappedRequest = new RequestAttributeChangeIgnoringWrapper(request);
+		AttributesPreservingRequest wrappedRequest = new AttributesPreservingRequest(request);
 		return doWithMatchingMappingIgnoringException(wrappedRequest, (handlerMapping, executionChain) -> {
 			for (HandlerInterceptor interceptor : executionChain.getInterceptorList()) {
 				if (interceptor instanceof CorsConfigurationSource) {
@@ -272,19 +275,83 @@ private static Map<HandlerMapping, PathPatternMatchableHandlerMapping> initPathP
 
 
 	/**
-	 * Request wrapper that ignores request attribute changes.
+	 * Request wrapper that buffers request attributes in order protect the
+	 * underlying request from attribute changes.
 	 */
-	private static class RequestAttributeChangeIgnoringWrapper extends HttpServletRequestWrapper {
+	private static class AttributesPreservingRequest extends HttpServletRequestWrapper {
 
-		RequestAttributeChangeIgnoringWrapper(HttpServletRequest request) {
+		private final Map<String, Object> attributes;
+
+		AttributesPreservingRequest(HttpServletRequest request) {
 			super(request);
+			this.attributes = initAttributes(request);
+		}
+
+		private Map<String, Object> initAttributes(HttpServletRequest request) {
+			Map<String, Object> map = new HashMap<>();
+			Enumeration<String> names = request.getAttributeNames();
+			while (names.hasMoreElements()) {
+				String name = names.nextElement();
+				map.put(name, request.getAttribute(name));
+			}
+			return map;
 		}
 
 		@Override
 		public void setAttribute(String name, Object value) {
-			if (name.equals(ServletRequestPathUtils.PATH_ATTRIBUTE) || name.equals(UrlPathHelper.PATH_ATTRIBUTE)) {
-				super.setAttribute(name, value);
+			this.attributes.put(name, value);
+		}
+
+		@Override
+		public Object getAttribute(String name) {
+			return this.attributes.get(name);
+		}
+
+		@Override
+		public Enumeration<String> getAttributeNames() {
+			return Collections.enumeration(this.attributes.keySet());
+		}
+
+		@Override
+		public void removeAttribute(String name) {
+			this.attributes.remove(name);
+		}
+	}
+
+
+	private static class PathSettingHandlerMapping implements MatchableHandlerMapping {
+
+		private final MatchableHandlerMapping delegate;
+
+		private final Object path;
+
+		private final String pathAttributeName;
+
+		PathSettingHandlerMapping(MatchableHandlerMapping delegate, Object path) {
+			this.delegate = delegate;
+			this.path = path;
+			this.pathAttributeName = (path instanceof RequestPath ?
+					ServletRequestPathUtils.PATH_ATTRIBUTE : UrlPathHelper.PATH_ATTRIBUTE);
+		}
+
+		@Nullable
+		@Override
+		public RequestMatchResult match(HttpServletRequest request, String pattern) {
+			Object previousPath = request.getAttribute(this.pathAttributeName);
+			request.setAttribute(this.pathAttributeName, this.path);
+			try {
+				return this.delegate.match(request, pattern);
+			}
+			finally {
+				request.setAttribute(this.pathAttributeName, previousPath);
 			}
 		}
+
+		@Nullable
+		@Override
+		public HandlerExecutionChain getHandler(HttpServletRequest request) throws Exception {
+			return this.delegate.getHandler(request);
+		}
 	}
+
 }

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

@@ -21,7 +21,6 @@
 import javax.servlet.http.HttpServletRequest;
 
 import org.springframework.http.server.PathContainer;
-import org.springframework.http.server.RequestPath;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.web.servlet.HandlerExecutionChain;
@@ -72,38 +71,4 @@ public HandlerExecutionChain getHandler(HttpServletRequest request) throws Excep
 		return this.delegate.getHandler(request);
 	}
 
-	MatchableHandlerMapping decorate(RequestPath requestPath) {
-		return new MatchableHandlerMapping() {
-
-			@Nullable
-			@Override
-			public RequestMatchResult match(HttpServletRequest request, String pattern) {
-				RequestPath previousPath = setRequestPathAttribute(request);
-				try {
-					return PathPatternMatchableHandlerMapping.this.match(request, pattern);
-				}
-				finally {
-					ServletRequestPathUtils.setParsedRequestPath(previousPath, request);
-				}
-			}
-
-			@Nullable
-			@Override
-			public HandlerExecutionChain getHandler(HttpServletRequest request) throws Exception {
-				RequestPath previousPath = setRequestPathAttribute(request);
-				try {
-					return PathPatternMatchableHandlerMapping.this.getHandler(request);
-				}
-				finally {
-					ServletRequestPathUtils.setParsedRequestPath(previousPath, request);
-				}
-			}
-
-			private RequestPath setRequestPathAttribute(HttpServletRequest request) {
-				RequestPath previous = (RequestPath) request.getAttribute(ServletRequestPathUtils.PATH_ATTRIBUTE);
-				ServletRequestPathUtils.setParsedRequestPath(requestPath, request);
-				return previous;
-			}
-		};
-	}
 }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2020 the original author or authors.
+ * Copyright 2002-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -40,6 +40,10 @@
 import org.springframework.web.cors.CorsConfiguration;
 import org.springframework.web.servlet.HandlerExecutionChain;
 import org.springframework.web.servlet.HandlerMapping;
+import org.springframework.web.servlet.function.RouterFunction;
+import org.springframework.web.servlet.function.RouterFunctions;
+import org.springframework.web.servlet.function.ServerResponse;
+import org.springframework.web.servlet.function.support.RouterFunctionMapping;
 import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
 import org.springframework.web.testfixture.servlet.MockHttpServletRequest;
 import org.springframework.web.util.ServletRequestPathUtils;
@@ -99,16 +103,6 @@ void detectHandlerMappingsOrdered() {
 		assertThat(actual).isEqualTo(expected);
 	}
 
-	void defaultHandlerMappings() {
-		StaticWebApplicationContext context = new StaticWebApplicationContext();
-		context.refresh();
-		List<HandlerMapping> actual = initIntrospector(context).getHandlerMappings();
-
-		assertThat(actual.size()).isEqualTo(2);
-		assertThat(actual.get(0).getClass()).isEqualTo(BeanNameUrlHandlerMapping.class);
-		assertThat(actual.get(1).getClass()).isEqualTo(RequestMappingHandlerMapping.class);
-	}
-
 	@ParameterizedTest
 	@ValueSource(booleans = {true, false})
 	void getMatchable(boolean usePathPatterns) throws Exception {
@@ -151,6 +145,22 @@ void getMatchableWhereHandlerMappingDoesNotImplementMatchableInterface() {
 		assertThatIllegalStateException().isThrownBy(() -> initIntrospector(cxt).getMatchableHandlerMapping(request));
 	}
 
+	@Test // gh-26833
+	void getMatchablePreservesRequestAttributes() throws Exception {
+		AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext();
+		context.register(TestConfig.class);
+		context.refresh();
+
+		MockHttpServletRequest request = new MockHttpServletRequest("POST", "/path");
+		request.setAttribute("name", "value");
+
+		MatchableHandlerMapping matchable = initIntrospector(context).getMatchableHandlerMapping(request);
+		assertThat(matchable).isNotNull();
+
+		// RequestPredicates.restoreAttributes clears and re-adds attributes
+		assertThat(request.getAttribute("name")).isEqualTo("value");
+	}
+
 	@Test
 	void getCorsConfigurationPreFlight() {
 		AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext();
@@ -204,15 +214,29 @@ public HandlerExecutionChain getHandler(HttpServletRequest request) {
 	@Configuration
 	static class TestConfig {
 
+		@Bean
+		public RouterFunctionMapping routerFunctionMapping() {
+			RouterFunctionMapping mapping = new RouterFunctionMapping();
+			mapping.setOrder(1);
+			return mapping;
+		}
+
 		@Bean
 		public RequestMappingHandlerMapping handlerMapping() {
-			return new RequestMappingHandlerMapping();
+			RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping();
+			mapping.setOrder(2);
+			return mapping;
 		}
 
 		@Bean
 		public TestController testController() {
 			return new TestController();
 		}
+
+		@Bean
+		public RouterFunction<?> routerFunction() {
+			return RouterFunctions.route().GET("/fn-path", request -> ServerResponse.ok().build()).build();
+		}
 	}