From 34607d96f1f40efae1dc83274c8b4d8eb632b790 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 6 Jul 2020 15:31:12 +0300 Subject: [PATCH] PathPattern caching with HandlerMappingIntrospector This commit evolves the solution from M1 by parsing and caching patterns with the target HandlerMapping's PathPatternParser. This makes it unnecessary for callers to be aware of pattern parsing. Closes gh-25312 --- .../handler/HandlerMappingIntrospector.java | 21 +++++- .../handler/MatchableHandlerMapping.java | 18 ----- .../PathPatternMatchableHandlerMapping.java | 73 +++++++++++++++++++ .../HandlerMappingIntrospectorTests.java | 32 ++++++-- 4 files changed, 117 insertions(+), 27 deletions(-) create mode 100644 spring-webmvc/src/main/java/org/springframework/web/servlet/handler/PathPatternMatchableHandlerMapping.java 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 043ac1fd204..ac7a2689b5d 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 @@ -22,6 +22,8 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequestWrapper; @@ -76,6 +78,10 @@ public class HandlerMappingIntrospector @Nullable private List handlerMappings; + @Nullable + private Map pathPatternMatchableHandlerMappings = + new ConcurrentHashMap<>(); + /** * Constructor for use with {@link ApplicationContextAware}. @@ -113,6 +119,7 @@ public class HandlerMappingIntrospector if (this.handlerMappings == null) { Assert.notNull(this.applicationContext, "No ApplicationContext"); this.handlerMappings = initHandlerMappings(this.applicationContext); + this.pathPatternMatchableHandlerMappings = initPathPatternMatchableHandlerMappings(this.handlerMappings); } } @@ -130,6 +137,7 @@ public class HandlerMappingIntrospector @Nullable public MatchableHandlerMapping getMatchableHandlerMapping(HttpServletRequest request) throws Exception { Assert.notNull(this.handlerMappings, "Handler mappings not initialized"); + Assert.notNull(this.pathPatternMatchableHandlerMappings, "Handler mappings with PathPatterns not initialized"); HttpServletRequest wrapper = new RequestAttributeChangeIgnoringWrapper(request); for (HandlerMapping handlerMapping : this.handlerMappings) { Object handler = handlerMapping.getHandler(wrapper); @@ -137,7 +145,8 @@ public class HandlerMappingIntrospector continue; } if (handlerMapping instanceof MatchableHandlerMapping) { - return ((MatchableHandlerMapping) handlerMapping); + return this.pathPatternMatchableHandlerMappings.getOrDefault( + handlerMapping, (MatchableHandlerMapping) handlerMapping); } throw new IllegalStateException("HandlerMapping is not a MatchableHandlerMapping"); } @@ -212,6 +221,16 @@ public class HandlerMappingIntrospector return result; } + private static Map initPathPatternMatchableHandlerMappings( + List mappings) { + + return mappings.stream() + .filter(mapping -> mapping instanceof MatchableHandlerMapping) + .map(mapping -> (MatchableHandlerMapping) mapping) + .filter(mapping -> mapping.getPatternParser() != null) + .collect(Collectors.toMap(mapping -> mapping, PathPatternMatchableHandlerMapping::new)); + } + /** * Request wrapper that ignores request attribute changes. diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MatchableHandlerMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MatchableHandlerMapping.java index ec779055242..eab017a4ac8 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MatchableHandlerMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MatchableHandlerMapping.java @@ -18,11 +18,8 @@ package org.springframework.web.servlet.handler; import javax.servlet.http.HttpServletRequest; -import org.springframework.http.server.PathContainer; import org.springframework.lang.Nullable; import org.springframework.web.servlet.HandlerMapping; -import org.springframework.web.util.ServletRequestPathUtils; -import org.springframework.web.util.pattern.PathPattern; import org.springframework.web.util.pattern.PathPatternParser; /** @@ -46,21 +43,6 @@ public interface MatchableHandlerMapping extends HandlerMapping { return null; } - /** - * Determine whether the request matches the given pattern. Use this method - * when {@link #getPatternParser()} is not {@code null} which means that the - * {@code HandlerMapping} has pre-parsed patterns enabled. - * @param request the current request - * @param pattern the pattern to match - * @return the result from request matching, or {@code null} if none - * @since 5.3 - */ - @Nullable - default RequestMatchResult match(HttpServletRequest request, PathPattern pattern) { - PathContainer path = ServletRequestPathUtils.getParsedRequestPath(request).pathWithinApplication(); - return (pattern.matches(path) ? new RequestMatchResult(pattern, path) : null); - } - /** * Determine whether the request matches the given pattern. Use this method * when {@link #getPatternParser()} returns {@code null} which means that the 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 new file mode 100644 index 00000000000..3a832b001d1 --- /dev/null +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/PathPatternMatchableHandlerMapping.java @@ -0,0 +1,73 @@ +/* + * Copyright 2002-2020 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.web.servlet.handler; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +import javax.servlet.http.HttpServletRequest; + +import org.springframework.http.server.PathContainer; +import org.springframework.lang.Nullable; +import org.springframework.util.Assert; +import org.springframework.web.servlet.HandlerExecutionChain; +import org.springframework.web.util.ServletRequestPathUtils; +import org.springframework.web.util.pattern.PathPattern; +import org.springframework.web.util.pattern.PathPatternParser; + +/** + * Wraps {@link MatchableHandlerMapping}s configured with a {@link PathPatternParser} + * in order to parse patterns lazily and cache them for re-ues. + * + * @author Rossen Stoyanchev + * @since 5.3 + */ +class PathPatternMatchableHandlerMapping implements MatchableHandlerMapping { + + private static final int MAX_PATTERNS = 1024; + + + private final MatchableHandlerMapping delegate; + + private final PathPatternParser parser; + + private final Map pathPatternCache = new ConcurrentHashMap<>(); + + + public PathPatternMatchableHandlerMapping(MatchableHandlerMapping delegate) { + Assert.notNull(delegate, "Delegate MatchableHandlerMapping is required."); + Assert.notNull(delegate.getPatternParser(), "PatternParser is required."); + this.delegate = delegate; + this.parser = delegate.getPatternParser(); + } + + @Nullable + @Override + public RequestMatchResult match(HttpServletRequest request, String pattern) { + PathPattern pathPattern = this.pathPatternCache.computeIfAbsent(pattern, value -> { + Assert.isTrue(this.pathPatternCache.size() < MAX_PATTERNS, "Max size for pattern cache exceeded."); + return this.parser.parse(pattern); + }); + PathContainer path = ServletRequestPathUtils.getParsedRequestPath(request).pathWithinApplication(); + return (pathPattern.matches(path) ? new RequestMatchResult(pathPattern, path) : null); + } + + @Nullable + @Override + public HandlerExecutionChain getHandler(HttpServletRequest request) throws Exception { + return this.delegate.getHandler(request); + } +} 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 3e476af444e..c6d03c054a3 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 @@ -16,6 +16,7 @@ package org.springframework.web.servlet.handler; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -44,6 +45,7 @@ import org.springframework.web.testfixture.servlet.MockHttpServletRequest; import org.springframework.web.util.ServletRequestPathUtils; import org.springframework.web.util.pattern.PathPattern; import org.springframework.web.util.pattern.PathPatternParser; +import org.springframework.web.util.pattern.PatternParseException; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; @@ -111,7 +113,7 @@ public class HandlerMappingIntrospectorTests { @ValueSource(booleans = {true, false}) void getMatchable(boolean usePathPatterns) throws Exception { - PathPatternParser parser = new PathPatternParser(); + TestPathPatternParser parser = new TestPathPatternParser(); GenericWebApplicationContext context = new GenericWebApplicationContext(); context.registerBean("mapping", SimpleUrlHandlerMapping.class, () -> { @@ -134,16 +136,14 @@ public class HandlerMappingIntrospectorTests { MatchableHandlerMapping mapping = initIntrospector(context).getMatchableHandlerMapping(request); assertThat(mapping).isNotNull(); - assertThat(mapping).isEqualTo(context.getBean("mapping")); assertThat(request.getAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE)).as("Attribute changes not ignored").isNull(); - String pattern = "/p*/*"; - PathPattern pathPattern = parser.parse(pattern); - assertThat(usePathPatterns ? mapping.match(request, pathPattern) : mapping.match(request, pattern)).isNotNull(); + assertThat(mapping.match(request, "/p*/*")).isNotNull(); + assertThat(mapping.match(request, "/b*/*")).isNull(); - pattern = "/b*/*"; - pathPattern = parser.parse(pattern); - assertThat(usePathPatterns ? mapping.match(request, pathPattern) : mapping.match(request, pattern)).isNull(); + if (usePathPatterns) { + assertThat(parser.getParsedPatterns()).containsExactly("/path/*", "/p*/*", "/b*/*"); + } } @Test @@ -230,4 +230,20 @@ public class HandlerMappingIntrospectorTests { } } + private static class TestPathPatternParser extends PathPatternParser { + + private final List parsedPatterns = new ArrayList<>(); + + + public List getParsedPatterns() { + return this.parsedPatterns; + } + + @Override + public PathPattern parse(String pathPattern) throws PatternParseException { + this.parsedPatterns.add(pathPattern); + return super.parse(pathPattern); + } + } + }