From 86123de8837d8650dd54182925ccddb33468ccc8 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 22 Apr 2021 17:21:03 +0100 Subject: [PATCH] HandlerMappingIntrospector handles attribute changes properly Closes gh-26833 --- .../handler/HandlerMappingIntrospector.java | 89 ++++++++++++++++--- .../PathPatternMatchableHandlerMapping.java | 35 -------- .../HandlerMappingIntrospectorTests.java | 48 +++++++--- 3 files changed, 114 insertions(+), 58 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/HandlerMappingIntrospector.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/HandlerMappingIntrospector.java index c696d43f669..d7363af4abf 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/HandlerMappingIntrospector.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/HandlerMappingIntrospector.java @@ -19,6 +19,8 @@ package org.springframework.web.servlet.handler; 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 class HandlerMappingIntrospector */ @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 class HandlerMappingIntrospector @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 @@ public class HandlerMappingIntrospector /** - * 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 attributes; + + AttributesPreservingRequest(HttpServletRequest request) { super(request); + this.attributes = initAttributes(request); + } + + private Map initAttributes(HttpServletRequest request) { + Map map = new HashMap<>(); + Enumeration 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 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); + } + } + } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/PathPatternMatchableHandlerMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/PathPatternMatchableHandlerMapping.java index 9d53ce0e09e..4b7a906732b 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/PathPatternMatchableHandlerMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/PathPatternMatchableHandlerMapping.java @@ -21,7 +21,6 @@ import java.util.concurrent.ConcurrentHashMap; 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 @@ class PathPatternMatchableHandlerMapping implements MatchableHandlerMapping { 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; - } - }; - } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMappingIntrospectorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMappingIntrospectorTests.java index f7437ef77d3..745d642b5ad 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMappingIntrospectorTests.java +++ b/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.context.support.StaticWebApplicationContext; 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 @@ public class HandlerMappingIntrospectorTests { assertThat(actual).isEqualTo(expected); } - void defaultHandlerMappings() { - StaticWebApplicationContext context = new StaticWebApplicationContext(); - context.refresh(); - List 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 @@ public class HandlerMappingIntrospectorTests { 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 class HandlerMappingIntrospectorTests { @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(); + } }