diff --git a/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java b/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java index 7b6cd7d9c2e5..084d720dff25 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java @@ -20,6 +20,7 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.TreeMap; import java.util.function.Consumer; import jakarta.servlet.DispatcherType; @@ -33,11 +34,13 @@ import org.jspecify.annotations.Nullable; import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; +import org.springframework.http.HttpStatusCode; import org.springframework.http.server.PathContainer; import org.springframework.http.server.RequestPath; +import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; +import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; import org.springframework.web.util.ServletRequestPathUtils; import org.springframework.web.util.pattern.PathPattern; @@ -59,7 +62,7 @@ *

This {@code Filter} should be ordered after {@link ForwardedHeaderFilter}, * before {@link ServletRequestPathFilter}, and before security filters. * - * @author Rossen Stoyanchev + * @author Rossen Stoyanchev, James Missen * @since 6.2 */ public final class UrlHandlerFilter extends OncePerRequestFilter { @@ -67,11 +70,14 @@ public final class UrlHandlerFilter extends OncePerRequestFilter { private static final Log logger = LogFactory.getLog(UrlHandlerFilter.class); - private final MultiValueMap handlers; + private final HandlerRegistry handlerRegistry; + private final boolean excludeContextPath; - private UrlHandlerFilter(MultiValueMap handlers) { - this.handlers = new LinkedMultiValueMap<>(handlers); + + private UrlHandlerFilter(HandlerRegistry handlerRegistry, boolean excludeContextPath) { + this.handlerRegistry = handlerRegistry; + this.excludeContextPath = excludeContextPath; } @@ -82,19 +88,12 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse RequestPath path = (ServletRequestPathUtils.hasParsedRequestPath(request) ? ServletRequestPathUtils.getParsedRequestPath(request) : ServletRequestPathUtils.parse(request)); - - for (Map.Entry> entry : this.handlers.entrySet()) { - if (!entry.getKey().supports(request, path)) { - continue; - } - for (PathPattern pattern : entry.getValue()) { - if (pattern.matches(path)) { - entry.getKey().handle(request, response, chain); - return; - } - } + PathContainer lookupPath = excludeContextPath ? path.subPath(path.contextPath().elements().size()) : path; + Handler handler = handlerRegistry.lookupHandler(path, lookupPath, request); + if (handler != null) { + handler.handle(request, response, chain); + return; } - chain.doFilter(request, response); } @@ -110,13 +109,14 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse * a trailing slash to a type-level prefix mapping, and therefore would never * match to a URL with the trailing slash removed. Use {@code @RequestMapping} * without a path instead to avoid the trailing slash in the mapping. - * @param pathPatterns patterns to map the handler to, e.g. - * "/path/*", "/path/**", "/path/foo/" + * @param patterns path patterns to map the handler to, for example, + * "/path/*", "/path/**", + * "/path/foo/". * @return a spec to configure the trailing slash handler with * @see Builder#trailingSlashHandler(String...) */ - public static Builder.TrailingSlashSpec trailingSlashHandler(String... pathPatterns) { - return new DefaultBuilder().trailingSlashHandler(pathPatterns); + public static Builder.TrailingSlashSpec trailingSlashHandler(String... patterns) { + return new DefaultBuilder().trailingSlashHandler(patterns); } @@ -126,13 +126,30 @@ public static Builder.TrailingSlashSpec trailingSlashHandler(String... pathPatte public interface Builder { /** - * Add a handler for URL's with a trailing slash. - * @param pathPatterns path patterns to map the handler to, for example, + * Add a handler for URLs with a trailing slash. + * @param patterns path patterns to map the handler to, for example, * "/path/*", "/path/**", * "/path/foo/". * @return a spec to configure the handler with */ - TrailingSlashSpec trailingSlashHandler(String... pathPatterns); + TrailingSlashSpec trailingSlashHandler(String... patterns); + + /** + * Specify whether to use path pattern specificity for matching handlers, + * with more specific patterns taking precedence. + *

The default value is {@code false}. + * @return the {@link Builder}, which allows adding more + * handlers and then building the Filter instance. + */ + Builder sortPatternsBySpecificity(boolean sortPatternsBySpecificity); + + /** + * Specify whether to exclude the context path when matching paths. + *

The default value is {@code false}. + * @return the {@link Builder}, which allows adding more + * handlers and then building the Filter instance. + */ + Builder excludeContextPath(boolean excludeContextPath); /** * Build the {@link UrlHandlerFilter} instance. @@ -154,11 +171,11 @@ interface TrailingSlashSpec { /** * Handle requests by sending a redirect to the same URL but the * trailing slash trimmed. - * @param status the redirect status to use + * @param statusCode the redirect status to use * @return the top level {@link Builder}, which allows adding more * handlers and then building the Filter instance. */ - Builder redirect(HttpStatus status); + Builder redirect(HttpStatusCode statusCode); /** * Handle the request by wrapping it in order to trim the trailing @@ -176,36 +193,64 @@ interface TrailingSlashSpec { */ private static final class DefaultBuilder implements Builder { - private final PathPatternParser patternParser = new PathPatternParser(); + private final MultiValueMap handlers = new LinkedMultiValueMap<>(); + + private boolean sortPatternsBySpecificity = false; - private final MultiValueMap handlers = new LinkedMultiValueMap<>(); + private boolean excludeContextPath = false; @Override public TrailingSlashSpec trailingSlashHandler(String... patterns) { return new DefaultTrailingSlashSpec(patterns); } - private DefaultBuilder addHandler(List pathPatterns, Handler handler) { - pathPatterns.forEach(pattern -> this.handlers.add(handler, pattern)); + @Override + public Builder sortPatternsBySpecificity(boolean sortPatternsBySpecificity) { + this.sortPatternsBySpecificity = sortPatternsBySpecificity; + return this; + } + + @Override + public Builder excludeContextPath(boolean excludeContextPath) { + this.excludeContextPath = excludeContextPath; + return this; + } + + private Builder addHandler(Handler handler, String... patterns) { + if (!ObjectUtils.isEmpty(patterns)) { + this.handlers.addAll(handler, Arrays.stream(patterns).toList()); + } return this; } @Override public UrlHandlerFilter build() { - return new UrlHandlerFilter(this.handlers); + HandlerRegistry handlerRegistry; + if (this.sortPatternsBySpecificity) { + handlerRegistry = new OrderedHandlerRegistry(); + } + else { + handlerRegistry = new DefaultHandlerRegistry(); + } + for (Map.Entry> entry : this.handlers.entrySet()) { + for (String pattern : entry.getValue()) { + handlerRegistry.registerHandler(pattern, entry.getKey()); + } + } + + return new UrlHandlerFilter(handlerRegistry, this.excludeContextPath); } private final class DefaultTrailingSlashSpec implements TrailingSlashSpec { - private final List pathPatterns; + private final String[] pathPatterns; private @Nullable Consumer interceptor; private DefaultTrailingSlashSpec(String[] patterns) { this.pathPatterns = Arrays.stream(patterns) .map(pattern -> pattern.endsWith("**") || pattern.endsWith("/") ? pattern : pattern + "/") - .map(patternParser::parse) - .toList(); + .toArray(String[]::new); } @Override @@ -215,21 +260,20 @@ public TrailingSlashSpec intercept(Consumer consumer) { } @Override - public Builder redirect(HttpStatus status) { - Handler handler = new RedirectTrailingSlashHandler(status, this.interceptor); - return DefaultBuilder.this.addHandler(this.pathPatterns, handler); + public Builder redirect(HttpStatusCode statusCode) { + Handler handler = new RedirectTrailingSlashHandler(statusCode, this.interceptor); + return DefaultBuilder.this.addHandler(handler, this.pathPatterns); } @Override public Builder wrapRequest() { Handler handler = new RequestWrappingTrailingSlashHandler(this.interceptor); - return DefaultBuilder.this.addHandler(this.pathPatterns, handler); + return DefaultBuilder.this.addHandler(handler, this.pathPatterns); } } } - /** * Internal handler to encapsulate different ways to handle a request. */ @@ -288,6 +332,11 @@ protected String trimTrailingSlash(String path) { int index = (StringUtils.hasLength(path) ? path.lastIndexOf('/') : -1); return (index != -1 ? path.substring(0, index) : path); } + + @Override + public String toString() { + return getClass().getSimpleName(); + } } @@ -296,11 +345,13 @@ protected String trimTrailingSlash(String path) { */ private static final class RedirectTrailingSlashHandler extends AbstractTrailingSlashHandler { - private final HttpStatus httpStatus; + private final HttpStatusCode statusCode; - RedirectTrailingSlashHandler(HttpStatus httpStatus, @Nullable Consumer interceptor) { + RedirectTrailingSlashHandler(HttpStatusCode statusCode, @Nullable Consumer interceptor) { super(interceptor); - this.httpStatus = httpStatus; + Assert.isTrue(statusCode.is3xxRedirection(), "HTTP status code for redirect handlers " + + "must be in the Redirection class (3xx)"); + this.statusCode = statusCode; } @Override @@ -313,10 +364,15 @@ public void handleInternal(HttpServletRequest request, HttpServletResponse respo } response.resetBuffer(); - response.setStatus(this.httpStatus.value()); + response.setStatus(this.statusCode.value()); response.setHeader(HttpHeaders.LOCATION, location); response.flushBuffer(); } + + @Override + public String toString() { + return getClass().getSimpleName() + " {statusCode=" + this.statusCode.value() + "}"; + } } @@ -410,4 +466,122 @@ private HttpServletRequest getDelegate() { } } + + /** + * Internal registry to encapsulate different ways to select a handler for a request. + */ + private interface HandlerRegistry { + + /** + * Register the specified handler for the given path pattern. + * @param pattern the path pattern the handler should be mapped to + * @param handler the handler instance to register + * @throws IllegalStateException if there is a conflicting handler registered + */ + void registerHandler(String pattern, Handler handler); + + /** + * Look up a handler instance for the given URL lookup path. + * @param path the parsed RequestPath + * @param lookupPath the URL path the handler is mapped to + * @param request the current request + * @return the associated handler instance, or {@code null} if not found + * @see org.springframework.web.util.pattern.PathPattern + */ + @Nullable Handler lookupHandler(RequestPath path, PathContainer lookupPath, HttpServletRequest request); + } + + + /** + * Base class for {@link HandlerRegistry} implementations. + */ + private static abstract class AbstractHandlerRegistry implements HandlerRegistry { + + private final PathPatternParser patternParser = new PathPatternParser(); + + @Override + public final void registerHandler(String pattern, Handler handler) { + Assert.notNull(pattern, "Pattern must not be null"); + Assert.notNull(handler, "Handler must not be null"); + + // Parse path pattern + pattern = patternParser.initFullPathPattern(pattern); + PathPattern pathPattern = patternParser.parse(pattern); + + // Register handler + registerHandlerInternal(pathPattern, handler); + if (logger.isTraceEnabled()) { + logger.trace("Mapped [" + pattern + "] onto " + handler); + } + } + + protected abstract void registerHandlerInternal(PathPattern pathPattern, Handler handler); + } + + + /** + * Default {@link HandlerRegistry} implementation. + */ + private static final class DefaultHandlerRegistry extends AbstractHandlerRegistry { + + private final MultiValueMap handlerMap = new LinkedMultiValueMap<>(); + + @Override + protected void registerHandlerInternal(PathPattern pathPattern, Handler handler) { + this.handlerMap.add(handler, pathPattern); + } + + @Override + public @Nullable Handler lookupHandler(RequestPath path, PathContainer lookupPath, HttpServletRequest request) { + for (Map.Entry> entry : this.handlerMap.entrySet()) { + if (!entry.getKey().supports(request, path)) { + continue; + } + for (PathPattern pattern : entry.getValue()) { + if (pattern.matches(lookupPath)) { + return entry.getKey(); + } + } + } + return null; + } + } + + + /** + * Handler registry that selects the handler mapped to the best-matching + * (i.e. most specific) path pattern. + */ + private static final class OrderedHandlerRegistry extends AbstractHandlerRegistry { + + private final Map handlerMap = new TreeMap<>(); + + @Override + protected void registerHandlerInternal(PathPattern pathPattern, Handler handler) { + if (this.handlerMap.containsKey(pathPattern)) { + Handler existingHandler = this.handlerMap.get(pathPattern); + if (existingHandler != null && existingHandler != handler) { + throw new IllegalStateException( + "Cannot map " + handler + " to [" + pathPattern + "]: there is already " + + existingHandler + " mapped."); + } + } + this.handlerMap.put(pathPattern, handler); + } + + @Override + public @Nullable Handler lookupHandler(RequestPath path, PathContainer lookupPath, HttpServletRequest request) { + for (Map.Entry entry : this.handlerMap.entrySet()) { + if (!entry.getKey().matches(lookupPath)) { + continue; + } + if (entry.getValue().supports(request, path)) { + return entry.getValue(); + } + return null; // only match one path pattern + } + return null; + } + } + } diff --git a/spring-web/src/main/java/org/springframework/web/filter/reactive/UrlHandlerFilter.java b/spring-web/src/main/java/org/springframework/web/filter/reactive/UrlHandlerFilter.java index ca332edc1b33..6f7688029c2d 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/reactive/UrlHandlerFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/reactive/UrlHandlerFilter.java @@ -20,6 +20,7 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.TreeMap; import java.util.function.Function; import org.apache.commons.logging.Log; @@ -34,8 +35,10 @@ import org.springframework.http.server.RequestPath; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; +import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilter; @@ -44,7 +47,7 @@ import org.springframework.web.util.pattern.PathPatternParser; /** - * {@link org.springframework.web.server.WebFilter} that modifies the URL, and + * {@link WebFilter} that modifies the URL, and * then redirects or wraps the request to apply the change. * *

To create an instance, you can use the following: @@ -58,7 +61,7 @@ * *

This {@code WebFilter} should be ordered ahead of security filters. * - * @author Rossen Stoyanchev + * @author Rossen Stoyanchev, James Missen * @since 6.2 */ public final class UrlHandlerFilter implements WebFilter { @@ -66,30 +69,29 @@ public final class UrlHandlerFilter implements WebFilter { private static final Log logger = LogFactory.getLog(UrlHandlerFilter.class); - private final MultiValueMap handlers; + private final HandlerRegistry handlerRegistry; + private final boolean excludeContextPath; - private UrlHandlerFilter(MultiValueMap handlers) { - this.handlers = new LinkedMultiValueMap<>(handlers); + + private UrlHandlerFilter(HandlerRegistry handlerRegistry, boolean excludeContextPath) { + this.handlerRegistry = handlerRegistry; + this.excludeContextPath = excludeContextPath; } @Override public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { RequestPath path = exchange.getRequest().getPath(); - for (Map.Entry> entry : this.handlers.entrySet()) { - if (!entry.getKey().supports(exchange)) { - continue; - } - for (PathPattern pattern : entry.getValue()) { - if (pattern.matches(path)) { - return entry.getKey().handle(exchange, chain); - } - } + PathContainer lookupPath = excludeContextPath ? path.pathWithinApplication() : path; + Handler handler = handlerRegistry.lookupHandler(lookupPath, exchange); + if (handler != null) { + return handler.handle(exchange, chain); } return chain.filter(exchange); } + /** * Add a handler that removes the trailing slash from URL paths to ensure * consistent interpretation of paths with or without a trailing slash for @@ -101,13 +103,14 @@ public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { * a trailing slash to a type-level prefix mapping, and therefore would never * match to a URL with the trailing slash removed. Use {@code @RequestMapping} * without a path instead to avoid the trailing slash in the mapping. - * @param pathPatterns patterns to map the handler to, e.g. - * "/path/*", "/path/**", "/path/foo/" + * @param patterns path patterns to map the handler to, e.g. + * "/path/*", "/path/**", + * "/path/foo/". * @return a spec to configure the trailing slash handler with * @see Builder#trailingSlashHandler(String...) */ - public static Builder.TrailingSlashSpec trailingSlashHandler(String... pathPatterns) { - return new DefaultBuilder().trailingSlashHandler(pathPatterns); + public static Builder.TrailingSlashSpec trailingSlashHandler(String... patterns) { + return new DefaultBuilder().trailingSlashHandler(patterns); } @@ -117,13 +120,30 @@ public static Builder.TrailingSlashSpec trailingSlashHandler(String... pathPatte public interface Builder { /** - * Add a handler for URL's with a trailing slash. - * @param pathPatterns path patterns to map the handler to, e.g. + * Add a handler for URLs with a trailing slash. + * @param patterns path patterns to map the handler to, e.g. * "/path/*", "/path/**", * "/path/foo/". * @return a spec to configure the handler with */ - TrailingSlashSpec trailingSlashHandler(String... pathPatterns); + TrailingSlashSpec trailingSlashHandler(String... patterns); + + /** + * Specify whether to use path pattern specificity for matching handlers, + * with more specific patterns taking precedence. + *

The default value is {@code false}. + * @return the {@link Builder}, which allows adding more + * handlers and then building the Filter instance. + */ + Builder sortPatternsBySpecificity(boolean sortPatternsBySpecificity); + + /** + * Specify whether to exclude the context path when matching paths. + *

The default value is {@code false}. + * @return the {@link Builder}, which allows adding more + * handlers and then building the Filter instance. + */ + Builder excludeContextPath(boolean excludeContextPath); /** * Build the {@link UrlHandlerFilter} instance. @@ -167,37 +187,65 @@ interface TrailingSlashSpec { */ private static final class DefaultBuilder implements Builder { - private final PathPatternParser patternParser = new PathPatternParser(); + private final MultiValueMap handlers = new LinkedMultiValueMap<>(); + + private boolean sortPatternsBySpecificity = false; - private final MultiValueMap handlers = new LinkedMultiValueMap<>(); + private boolean excludeContextPath = false; @Override public TrailingSlashSpec trailingSlashHandler(String... patterns) { return new DefaultTrailingSlashSpec(patterns); } - private DefaultBuilder addHandler(List pathPatterns, Handler handler) { - pathPatterns.forEach(pattern -> this.handlers.add(handler, pattern)); + @Override + public Builder sortPatternsBySpecificity(boolean sortPatternsBySpecificity) { + this.sortPatternsBySpecificity = sortPatternsBySpecificity; + return this; + } + + @Override + public Builder excludeContextPath(boolean excludeContextPath) { + this.excludeContextPath = excludeContextPath; + return this; + } + + private Builder addHandler(Handler handler, String... patterns) { + if (!ObjectUtils.isEmpty(patterns)) { + this.handlers.addAll(handler, Arrays.stream(patterns).toList()); + } return this; } @Override public UrlHandlerFilter build() { - return new UrlHandlerFilter(this.handlers); + HandlerRegistry handlerRegistry; + if (this.sortPatternsBySpecificity) { + handlerRegistry = new OrderedHandlerRegistry(); + } + else { + handlerRegistry = new DefaultHandlerRegistry(); + } + for (Map.Entry> entry : this.handlers.entrySet()) { + for (String pattern : entry.getValue()) { + handlerRegistry.registerHandler(pattern, entry.getKey()); + } + } + + return new UrlHandlerFilter(handlerRegistry, this.excludeContextPath); } private final class DefaultTrailingSlashSpec implements TrailingSlashSpec { - private final List pathPatterns; + private final String[] patterns; private @Nullable List>> interceptors; private DefaultTrailingSlashSpec(String[] patterns) { - this.pathPatterns = Arrays.stream(patterns) + this.patterns = Arrays.stream(patterns) .map(pattern -> pattern.endsWith("**") || pattern.endsWith("/") ? pattern : pattern + "/") - .map(patternParser::parse) - .toList(); + .toArray(String[]::new); } @Override @@ -210,13 +258,13 @@ public TrailingSlashSpec intercept(Function> inter @Override public Builder redirect(HttpStatusCode statusCode) { Handler handler = new RedirectTrailingSlashHandler(statusCode, this.interceptors); - return DefaultBuilder.this.addHandler(this.pathPatterns, handler); + return DefaultBuilder.this.addHandler(handler, this.patterns); } @Override public Builder mutateRequest() { Handler handler = new RequestWrappingTrailingSlashHandler(this.interceptors); - return DefaultBuilder.this.addHandler(this.pathPatterns, handler); + return DefaultBuilder.this.addHandler(handler, this.patterns); } } } @@ -279,6 +327,11 @@ protected String trimTrailingSlash(ServerHttpRequest request) { int index = (StringUtils.hasLength(path) ? path.lastIndexOf('/') : -1); return (index != -1 ? path.substring(0, index) : path); } + + @Override + public String toString() { + return getClass().getSimpleName(); + } } @@ -293,6 +346,8 @@ private static final class RedirectTrailingSlashHandler extends AbstractTrailing HttpStatusCode statusCode, @Nullable List>> interceptors) { super(interceptors); + Assert.isTrue(statusCode.is3xxRedirection(), "HTTP status code for redirect handlers " + + "must be in the Redirection class (3xx)"); this.statusCode = statusCode; } @@ -310,6 +365,11 @@ public Mono handleInternal(ServerWebExchange exchange, WebFilterChain chai response.getHeaders().set(HttpHeaders.LOCATION, location); return Mono.empty(); } + + @Override + public String toString() { + return getClass().getSimpleName() + " {statusCode=" + this.statusCode.value() + "}"; + } } @@ -330,4 +390,118 @@ public Mono handleInternal(ServerWebExchange exchange, WebFilterChain chai } } + + /** + * Internal registry to encapsulate different ways to select a handler for a request. + */ + private interface HandlerRegistry { + + /** + * Register the specified handler for the given path pattern. + * @param pattern the path pattern the handler should be mapped to + * @param handler the handler instance to register + * @throws IllegalStateException if there is a conflicting handler registered + */ + void registerHandler(String pattern, Handler handler); + + /** + * Look up a handler instance for the given URL lookup path. + * @param lookupPath the URL path the handler is mapped to + * @param exchange the current exchange + * @return the associated handler instance, or {@code null} if not found + * @see org.springframework.web.util.pattern.PathPattern + */ + @Nullable Handler lookupHandler(PathContainer lookupPath, ServerWebExchange exchange); + } + + + /** + * Base class for {@link HandlerRegistry} implementations. + */ + private static abstract class AbstractHandlerRegistry implements HandlerRegistry { + + private final PathPatternParser patternParser = new PathPatternParser(); + + @Override + public final void registerHandler(String pattern, Handler handler) { + Assert.notNull(pattern, "Pattern must not be null"); + Assert.notNull(handler, "Handler must not be null"); + + // Parse path pattern + pattern = patternParser.initFullPathPattern(pattern); + PathPattern pathPattern = patternParser.parse(pattern); + + // Register handler + registerHandlerInternal(pathPattern, handler); + if (logger.isTraceEnabled()) { + logger.trace("Mapped [" + pattern + "] onto " + handler); + } + } + + protected abstract void registerHandlerInternal(PathPattern pathPattern, Handler handler); + } + + + /** + * Default {@link HandlerRegistry} implementation. + */ + private static final class DefaultHandlerRegistry extends AbstractHandlerRegistry { + + private final MultiValueMap handlerMap = new LinkedMultiValueMap<>(); + + @Override + protected void registerHandlerInternal(PathPattern pathPattern, Handler handler) { + this.handlerMap.add(handler, pathPattern); + } + + @Override + public @Nullable Handler lookupHandler(PathContainer lookupPath, ServerWebExchange exchange) { + for (Map.Entry> entry : this.handlerMap.entrySet()) { + if (!entry.getKey().supports(exchange)) { + continue; + } + for (PathPattern pattern : entry.getValue()) { + if (pattern.matches(lookupPath)) { + return entry.getKey(); + } + } + } + return null; + } + } + + + /** + * Handler registry that selects the handler mapped to the best-matching + * (i.e. most specific) path pattern. + */ + private static final class OrderedHandlerRegistry extends AbstractHandlerRegistry { + + private final Map handlerMap = new TreeMap<>(); + + @Override + protected void registerHandlerInternal(PathPattern pathPattern, Handler handler) { + Handler existingHandler = this.handlerMap.put(pathPattern, handler); + if (existingHandler != null && existingHandler != handler) { + throw new IllegalStateException( + "Cannot map " + handler + " to [" + pathPattern + "]: there is already " + + existingHandler + " mapped."); + } + } + + @Override + public @Nullable Handler lookupHandler(PathContainer lookupPath, ServerWebExchange exchange) { + for (Map.Entry entry : this.handlerMap.entrySet()) { + if (!entry.getKey().matches(lookupPath)) { + continue; + } + if (entry.getValue().supports(exchange)) { + return entry.getValue(); + } + return null; // only match one path pattern + } + return null; + } + } + } diff --git a/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java index e020527fb1f2..2822fe6974b9 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java @@ -33,31 +33,38 @@ import org.springframework.web.testfixture.servlet.MockHttpServletRequest; import org.springframework.web.testfixture.servlet.MockHttpServletResponse; import org.springframework.web.util.ServletRequestPathUtils; - -import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.*; /** * Unit tests for {@link UrlHandlerFilter}. * - * @author Rossen Stoyanchev + * @author Rossen Stoyanchev, James Missen */ public class UrlHandlerFilterTests { @Test void requestWrapping() throws Exception { - testRequestWrapping("/path/**", "/path/123", null); - testRequestWrapping("/path/*", "/path", "/123"); - testRequestWrapping("/path/*", "", "/path/123"); + testRequestWrapping("/path/**", "", "/path/123", null); + testRequestWrapping("/path/*", "", "/path", "/123"); + testRequestWrapping("/path/*", "", "", "/path/123"); + testRequestWrapping("/path/**", "/myApp", "/path/123", null); // gh-35975 + testRequestWrapping("/path/*", "/myApp", "/path", "/123"); // gh-35975 + testRequestWrapping("/path/*", "/myApp", "", "/path/123"); // gh-35975 } - void testRequestWrapping(String pattern, String servletPath, @Nullable String pathInfo) throws Exception { + void testRequestWrapping(String pattern, String contextPath, String servletPath, + @Nullable String pathInfo) throws Exception { - UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler(pattern).wrapRequest().build(); + UrlHandlerFilter filter = UrlHandlerFilter + .trailingSlashHandler(pattern).wrapRequest() + .excludeContextPath(true) // gh-35975 + .build(); boolean hasPathInfo = StringUtils.hasLength(pathInfo); - String requestURI = servletPath + (hasPathInfo ? pathInfo : ""); + String requestURI = contextPath + servletPath + (hasPathInfo ? pathInfo : ""); MockHttpServletRequest request = new MockHttpServletRequest("GET", requestURI + "/"); + request.setContextPath(contextPath); request.setServletPath(hasPathInfo ? servletPath : servletPath + "/"); request.setPathInfo(hasPathInfo ? pathInfo + "/" : pathInfo); @@ -93,12 +100,55 @@ void redirect() throws Exception { assertThat(response.isCommitted()).isTrue(); } + @Test // gh-35882 + void orderedUrlHandling() throws Exception { + String path = "/path/123"; + MockHttpServletRequest request = new MockHttpServletRequest("GET", path + "/"); + + HttpStatus status = HttpStatus.PERMANENT_REDIRECT; + + // Request wrapping + UrlHandlerFilter filter = UrlHandlerFilter + .trailingSlashHandler("/path/**").redirect(status) + .trailingSlashHandler("/path/123/").wrapRequest() // most specific pattern + .trailingSlashHandler("/path/123/**").redirect(status) + .sortPatternsBySpecificity(true) + .build(); + + MockFilterChain chain = new MockFilterChain(); + filter.doFilterInternal(request, new MockHttpServletResponse(), chain); + + HttpServletRequest actual = (HttpServletRequest) chain.getRequest(); + assertThat(actual).isNotNull().isNotSameAs(request); + assertThat(actual.getRequestURL().toString()).isEqualTo("http://localhost" + path); + + // Redirect + filter = UrlHandlerFilter + .trailingSlashHandler("/path/**").wrapRequest() + .trailingSlashHandler("/path/123/").redirect(status) // most specific pattern + .trailingSlashHandler("/path/123/**").wrapRequest() + .sortPatternsBySpecificity(true) + .build(); + + MockHttpServletResponse response = new MockHttpServletResponse(); + + chain = new MockFilterChain(); + filter.doFilterInternal(request, response, chain); + + assertThat(chain.getRequest()).isNull(); + assertThat(response.getStatus()).isEqualTo(status.value()); + assertThat(response.getHeader(HttpHeaders.LOCATION)).isEqualTo(path); + assertThat(response.isCommitted()).isTrue(); + } + @Test void noUrlHandling() throws Exception { testNoUrlHandling("/path/**", "", "/path/123"); testNoUrlHandling("/path/*", "", "/path/123"); testNoUrlHandling("/**", "", "/"); // gh-33444 testNoUrlHandling("/**", "/myApp", "/myApp/"); // gh-33565 + testNoUrlHandling("/path/**", "/myApp", "/myApp/path/123/"); // gh-35975 + testNoUrlHandling("/path/*", "/myApp", "/myApp/path/123/"); // gh-35975 } private static void testNoUrlHandling(String pattern, String contextPath, String requestURI) diff --git a/spring-web/src/test/java/org/springframework/web/filter/reactive/UrlHandlerFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/reactive/UrlHandlerFilterTests.java index 14a8e40440bf..c6b5b13b6406 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/reactive/UrlHandlerFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/reactive/UrlHandlerFilterTests.java @@ -40,16 +40,25 @@ /** * Unit tests for {@link UrlHandlerFilter}. * - * @author Rossen Stoyanchev + * @author Rossen Stoyanchev, James Missen */ public class UrlHandlerFilterTests { @Test void requestMutation() { - UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/**").mutateRequest().build(); + testRequestMutation("/path/**", "", "/path/123"); + testRequestMutation("/path/*", "", "/path/123"); + testRequestMutation("/path/**", "/myApp", "/myApp/path/123"); // gh-35975 + testRequestMutation("/path/*", "/myApp", "/myApp/path/123"); // gh-35975 + } - String path = "/path/123"; - MockServerHttpRequest original = MockServerHttpRequest.get(path + "/").build(); + void testRequestMutation(String pattern, String contextPath, String path) { + UrlHandlerFilter filter = UrlHandlerFilter + .trailingSlashHandler(pattern).mutateRequest() + .excludeContextPath(true) // gh-35975 + .build(); + + MockServerHttpRequest original = MockServerHttpRequest.get(path + "/").contextPath(contextPath).build(); ServerWebExchange exchange = MockServerWebExchange.from(original); ServerHttpRequest actual = invokeFilter(filter, exchange); @@ -75,12 +84,51 @@ void redirect() { assertThat(exchange.getResponse().getHeaders().getLocation()).isEqualTo(URI.create(path + "?" + queryString)); } + @Test // gh-35882 + void orderedUrlHandling() { + String path = "/path/123"; + MockServerHttpRequest original = MockServerHttpRequest.get(path + "/").build(); + ServerWebExchange exchange = MockServerWebExchange.from(original); + + HttpStatus status = HttpStatus.PERMANENT_REDIRECT; + + // Request mutation + UrlHandlerFilter filter = UrlHandlerFilter + .trailingSlashHandler("/path/**").redirect(status) + .trailingSlashHandler("/path/123/").mutateRequest() // most specific pattern + .trailingSlashHandler("/path/123/**").redirect(status) + .sortPatternsBySpecificity(true) + .build(); + + ServerHttpRequest actual = invokeFilter(filter, exchange); + + assertThat(actual).isNotNull().isNotSameAs(original); + assertThat(actual.getPath().value()).isEqualTo(path); + + // Redirect + filter = UrlHandlerFilter + .trailingSlashHandler("/path/**").mutateRequest() + .trailingSlashHandler("/path/123/").redirect(status) // most specific pattern + .trailingSlashHandler("/path/123/**").mutateRequest() + .sortPatternsBySpecificity(true) + .build(); + + UrlHandlerFilter finalFilter = filter; + assertThatThrownBy(() -> invokeFilter(finalFilter, exchange)) + .hasMessageContaining("No argument value was captured"); + + assertThat(exchange.getResponse().getStatusCode()).isEqualTo(status); + assertThat(exchange.getResponse().getHeaders().getLocation()).isEqualTo(URI.create(path)); + } + @Test void noUrlHandling() { testNoUrlHandling("/path/**", "", "/path/123"); testNoUrlHandling("/path/*", "", "/path/123"); testNoUrlHandling("/**", "", "/"); // gh-33444 testNoUrlHandling("/**", "/myApp", "/myApp/"); // gh-33565 + testNoUrlHandling("/path/**", "/myApp", "/myApp/path/123/"); // gh-35975 + testNoUrlHandling("/path/*", "/myApp", "/myApp/path/123/"); // gh-35975 } private static void testNoUrlHandling(String pattern, String contextPath, String path) {