Skip to content

Commit

Permalink
RequestMappingInfo defaults to PathPatternParser
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@rstoyanchev
rstoyanchev committed Nov 27, 2023
1 parent 54ecbb2 commit 4370030
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 35 deletions.
32 changes: 18 additions & 14 deletions ...g-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) ?
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -894,7 +894,9 @@ public static class BuilderConfiguration {
* {@link AbstractHandlerMapping#setPatternParser(PathPatternParser)}.
* <p><strong>Note:</strong> This property is mutually exclusive with
* {@link #setPathMatcher(PathMatcher)}.
* <p>By default this is not enabled.
* <p>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) {
Expand Down Expand Up @@ -936,7 +938,9 @@ public UrlPathHelper getUrlPathHelper() {

/**
* Set a custom PathMatcher to use for the PatternsRequestCondition.
* <p>By default this is not set.
* <p>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;
Expand Down
29 changes: 21 additions & 8 deletions ...ava/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String, String> uriVariables = (Map<String, String>) request.getAttribute(name);
Expand All @@ -274,15 +278,18 @@ 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();
pathHelper.setUrlDecode(false);
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<String, String> uriVariables = (Map<String, String>) request.getAttribute(name);
Expand All @@ -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");
Expand Down
28 changes: 18 additions & 10 deletions ...mvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -48,8 +49,8 @@ class RequestMappingInfoTests {
@SuppressWarnings("unused")
static Stream<RequestMappingInfo.Builder> 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));
}


Expand Down Expand Up @@ -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) {

Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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<RequestMappingInfo> comparator = (info, otherInfo) -> info.compareTo(otherInfo, request);

List<RequestMappingInfo> list = asList(noMethods, oneMethod, oneMethodOneParam);
Expand All @@ -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");

Expand Down Expand Up @@ -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");

Expand Down
7 changes: 4 additions & 3 deletions ...src/test/java/org/springframework/web/servlet/mvc/method/annotation/CrossOriginTests.java
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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())
Expand Down

0 comments on commit 4370030

Please sign in to comment.