From 43700302c6a34924f727dc3594f074ca75298e24 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Mon, 27 Nov 2023 10:27:56 +0000 Subject: [PATCH] RequestMappingInfo defaults to PathPatternParser This changes ensures RequestMappingInfo uses PathPatternParser by default as all AbstractHandlerMapping implementations do as of 6.0. RequestMappingInfo instances are typically created internally and aligned with the RequestMappingHandlerMapping in terms of path mapping options. If a RequestMappingInfo is registered programmatically, the caller needs to also ensure they are aligned. However, if the two should be aligned by default. Closes gh-31662 --- .../mvc/method/RequestMappingInfo.java | 32 +++++++++++-------- ...RequestMappingInfoHandlerMappingTests.java | 29 ++++++++++++----- .../mvc/method/RequestMappingInfoTests.java | 28 ++++++++++------ .../method/annotation/CrossOriginTests.java | 7 ++-- 4 files changed, 61 insertions(+), 35 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java index 33711146934a..4caeef3e71d7 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java @@ -705,27 +705,28 @@ public Builder options(BuilderConfiguration options) { @SuppressWarnings("deprecation") public RequestMappingInfo build() { - PathPatternsRequestCondition pathPatterns = null; - PatternsRequestCondition patterns = null; + PathPatternsRequestCondition pathPatternsCondition = null; + PatternsRequestCondition patternsCondition = null; - if (this.options.patternParser != null) { - pathPatterns = (ObjectUtils.isEmpty(this.paths) ? - EMPTY_PATH_PATTERNS : - new PathPatternsRequestCondition(this.options.patternParser, this.paths)); - } - else { - patterns = (ObjectUtils.isEmpty(this.paths) ? + if (this.options.getPathMatcher() != null) { + patternsCondition = (ObjectUtils.isEmpty(this.paths) ? EMPTY_PATTERNS : new PatternsRequestCondition( this.paths, null, this.options.getPathMatcher(), this.options.useSuffixPatternMatch(), this.options.useTrailingSlashMatch(), this.options.getFileExtensions())); } + else { + PathPatternParser parser = (this.options.getPatternParser() != null ? + this.options.getPatternParser() : new PathPatternParser()); + pathPatternsCondition = (ObjectUtils.isEmpty(this.paths) ? + EMPTY_PATH_PATTERNS : new PathPatternsRequestCondition(parser, this.paths)); + } ContentNegotiationManager manager = this.options.getContentNegotiationManager(); return new RequestMappingInfo( - this.mappingName, pathPatterns, patterns, + this.mappingName, pathPatternsCondition, patternsCondition, ObjectUtils.isEmpty(this.methods) ? EMPTY_REQUEST_METHODS : new RequestMethodsRequestCondition(this.methods), ObjectUtils.isEmpty(this.params) ? @@ -737,8 +738,7 @@ public RequestMappingInfo build() { ObjectUtils.isEmpty(this.produces) && !this.hasAccept ? EMPTY_PRODUCES : new ProducesRequestCondition(this.produces, this.headers, manager), this.customCondition != null ? - new RequestConditionHolder(this.customCondition) : EMPTY_CUSTOM, - this.options); + new RequestConditionHolder(this.customCondition) : EMPTY_CUSTOM, this.options); } } @@ -894,7 +894,9 @@ public static class BuilderConfiguration { * {@link AbstractHandlerMapping#setPatternParser(PathPatternParser)}. *

Note: This property is mutually exclusive with * {@link #setPathMatcher(PathMatcher)}. - *

By default this is not enabled. + *

By default this is not set, but {@link RequestMappingInfo.Builder} + * defaults to using {@link PathPatternParser} unless + * {@link #setPathMatcher(PathMatcher)} is explicitly set. * @since 5.3 */ public void setPatternParser(@Nullable PathPatternParser patternParser) { @@ -936,7 +938,9 @@ public UrlPathHelper getUrlPathHelper() { /** * Set a custom PathMatcher to use for the PatternsRequestCondition. - *

By default this is not set. + *

By default this is not set. You must set it explicitly if you want + * {@link PathMatcher} to be used, or otherwise {@link RequestMappingInfo} + * defaults to using {@link PathPatternParser}. */ public void setPathMatcher(@Nullable PathMatcher pathMatcher) { this.pathMatcher = pathMatcher; diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java index bca7f56f1741..1d7cc8e64f38 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java @@ -38,6 +38,7 @@ import org.springframework.http.server.observation.ServerRequestObservationContext; import org.springframework.lang.Nullable; import org.springframework.stereotype.Controller; +import org.springframework.util.AntPathMatcher; import org.springframework.util.MultiValueMap; import org.springframework.web.HttpMediaTypeNotAcceptableException; import org.springframework.web.HttpMediaTypeNotSupportedException; @@ -258,10 +259,13 @@ void getHandlerMappedInterceptors() throws Exception { @SuppressWarnings("unchecked") @PathPatternsParameterizedTest void handleMatchUriTemplateVariables(TestRequestMappingInfoHandlerMapping mapping) { - RequestMappingInfo key = RequestMappingInfo.paths("/{path1}/{path2}").build(); + RequestMappingInfo.BuilderConfiguration config = new RequestMappingInfo.BuilderConfiguration(); + config.setPathMatcher(new AntPathMatcher()); + + RequestMappingInfo info = RequestMappingInfo.paths("/{path1}/{path2}").options(config).build(); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2"); String lookupPath = new UrlPathHelper().getLookupPathForRequest(request); - mapping.handleMatch(key, lookupPath, request); + mapping.handleMatch(info, lookupPath, request); String name = HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE; Map uriVariables = (Map) request.getAttribute(name); @@ -274,7 +278,10 @@ void handleMatchUriTemplateVariables(TestRequestMappingInfoHandlerMapping mappin @SuppressWarnings("unchecked") @PathPatternsParameterizedTest // SPR-9098 void handleMatchUriTemplateVariablesDecode(TestRequestMappingInfoHandlerMapping mapping) { - RequestMappingInfo key = RequestMappingInfo.paths("/{group}/{identifier}").build(); + RequestMappingInfo.BuilderConfiguration config = new RequestMappingInfo.BuilderConfiguration(); + config.setPathMatcher(new AntPathMatcher()); + + RequestMappingInfo info = RequestMappingInfo.paths("/{group}/{identifier}").options(config).build(); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/group/a%2Fb"); UrlPathHelper pathHelper = new UrlPathHelper(); @@ -282,7 +289,7 @@ void handleMatchUriTemplateVariablesDecode(TestRequestMappingInfoHandlerMapping String lookupPath = pathHelper.getLookupPathForRequest(request); mapping.setUrlPathHelper(pathHelper); - mapping.handleMatch(key, lookupPath, request); + mapping.handleMatch(info, lookupPath, request); String name = HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE; Map uriVariables = (Map) request.getAttribute(name); @@ -294,20 +301,26 @@ void handleMatchUriTemplateVariablesDecode(TestRequestMappingInfoHandlerMapping @PathPatternsParameterizedTest void handleMatchBestMatchingPatternAttribute(TestRequestMappingInfoHandlerMapping mapping) { - RequestMappingInfo key = RequestMappingInfo.paths("/{path1}/2", "/**").build(); + RequestMappingInfo.BuilderConfiguration config = new RequestMappingInfo.BuilderConfiguration(); + config.setPathMatcher(new AntPathMatcher()); + + RequestMappingInfo info = RequestMappingInfo.paths("/{path1}/2", "/**").options(config).build(); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2"); - mapping.handleMatch(key, "/1/2", request); + mapping.handleMatch(info, "/1/2", request); assertThat(request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE)).isEqualTo("/{path1}/2"); } @PathPatternsParameterizedTest void handleMatchBestMatchingPatternAttributeInObservationContext(TestRequestMappingInfoHandlerMapping mapping) { - RequestMappingInfo key = RequestMappingInfo.paths("/{path1}/2", "/**").build(); + RequestMappingInfo.BuilderConfiguration config = new RequestMappingInfo.BuilderConfiguration(); + config.setPathMatcher(new AntPathMatcher()); + + RequestMappingInfo info = RequestMappingInfo.paths("/{path1}/2", "/**").options(config).build(); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2"); ServerRequestObservationContext observationContext = new ServerRequestObservationContext(request, new MockHttpServletResponse()); request.setAttribute(ServerHttpObservationFilter.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext); - mapping.handleMatch(key, "/1/2", request); + mapping.handleMatch(info, "/1/2", request); assertThat(request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE)).isEqualTo("/{path1}/2"); assertThat(observationContext.getPathPattern()).isEqualTo("/{path1}/2"); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java index 1e2914959cef..3192621b1797 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java @@ -26,6 +26,7 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; +import org.springframework.util.AntPathMatcher; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.servlet.handler.PathPatternsParameterizedTest; import org.springframework.web.servlet.handler.PathPatternsTestUtils; @@ -48,8 +49,8 @@ class RequestMappingInfoTests { @SuppressWarnings("unused") static Stream pathPatternsArguments() { RequestMappingInfo.BuilderConfiguration config = new RequestMappingInfo.BuilderConfiguration(); - config.setPatternParser(new PathPatternParser()); - return Stream.of(RequestMappingInfo.paths().options(config), RequestMappingInfo.paths()); + config.setPathMatcher(new AntPathMatcher()); + return Stream.of(RequestMappingInfo.paths(), RequestMappingInfo.paths().options(config)); } @@ -86,6 +87,13 @@ void createEmpty(RequestMappingInfo.Builder infoBuilder) { assertThat(info.getCustomCondition()).isSameAs(result.getCustomCondition()); } + @Test // gh-31662 + void pathPatternByDefault() { + RequestMappingInfo info = RequestMappingInfo.paths().build(); + assertThat(info.getPathPatternsCondition()).isNotNull(); + assertThat(info.getPatternsCondition()).isNull(); + } + @PathPatternsParameterizedTest void matchPatternsCondition(RequestMappingInfo.Builder builder) { @@ -105,7 +113,7 @@ void matchPatternsCondition(RequestMappingInfo.Builder builder) { @Test void matchParamsCondition() { - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", true); request.setParameter("foo", "bar"); RequestMappingInfo info = RequestMappingInfo.paths("/foo").params("foo=bar").build(); @@ -121,7 +129,7 @@ void matchParamsCondition() { @Test void matchHeadersCondition() { - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", true); request.addHeader("foo", "bar"); RequestMappingInfo info = RequestMappingInfo.paths("/foo").headers("foo=bar").build(); @@ -137,7 +145,7 @@ void matchHeadersCondition() { @Test void matchConsumesCondition() { - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", true); request.setContentType("text/plain"); RequestMappingInfo info = RequestMappingInfo.paths("/foo").consumes("text/plain").build(); @@ -153,7 +161,7 @@ void matchConsumesCondition() { @Test void matchProducesCondition() { - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", true); request.addHeader("Accept", "text/plain"); RequestMappingInfo info = RequestMappingInfo.paths("/foo").produces("text/plain").build(); @@ -169,7 +177,7 @@ void matchProducesCondition() { @Test void matchCustomCondition() { - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/foo", true); request.setParameter("foo", "bar"); RequestMappingInfo info = RequestMappingInfo.paths("/foo").params("foo=bar").build(); @@ -189,7 +197,7 @@ void compareToWithImplicitVsExplicitHttpMethodDeclaration() { RequestMappingInfo oneMethod = RequestMappingInfo.paths().methods(GET).build(); RequestMappingInfo oneMethodOneParam = RequestMappingInfo.paths().methods(GET).params("foo").build(); - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/", true); Comparator comparator = (info, otherInfo) -> info.compareTo(otherInfo, request); List list = asList(noMethods, oneMethod, oneMethodOneParam); @@ -204,7 +212,7 @@ void compareToWithImplicitVsExplicitHttpMethodDeclaration() { @Test // SPR-14383 void compareToWithHttpHeadMapping() { - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("GET", "/", true); request.setMethod("HEAD"); request.addHeader("Accept", "application/json"); @@ -297,7 +305,7 @@ void equalsMethod(RequestMappingInfo.Builder infoBuilder) { @Test void preFlightRequest() { - MockHttpServletRequest request = PathPatternsTestUtils.initRequest("OPTIONS", "/foo", false); + MockHttpServletRequest request = PathPatternsTestUtils.initRequest("OPTIONS", "/foo", true); request.addHeader(HttpHeaders.ORIGIN, "https://domain.com"); request.addHeader(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "POST"); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/CrossOriginTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/CrossOriginTests.java index 72dffa046ff0..872f501bfd9b 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/CrossOriginTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/CrossOriginTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -38,6 +38,7 @@ import org.springframework.http.server.RequestPath; import org.springframework.lang.Nullable; import org.springframework.stereotype.Controller; +import org.springframework.util.AntPathMatcher; import org.springframework.util.CollectionUtils; import org.springframework.web.bind.annotation.CrossOrigin; import org.springframework.web.bind.annotation.GetMapping; @@ -595,8 +596,8 @@ protected RequestMappingInfo getMappingForMethod(Method method, Class handler RequestMapping annotation = AnnotatedElementUtils.findMergedAnnotation(method, RequestMapping.class); if (annotation != null) { RequestMappingInfo.BuilderConfiguration options = new RequestMappingInfo.BuilderConfiguration(); - if (getPatternParser() != null) { - options.setPatternParser(getPatternParser()); + if (getPatternParser() == null) { + options.setPathMatcher(new AntPathMatcher()); } return RequestMappingInfo.paths(annotation.value()) .methods(annotation.method())