Improve separator support in PathContainer

To make the switching of separators complete, it is also important to
know whether the decoding of path segment values and the parsing of
path param should be done as those are applied transparently.

This commit replaces the recently added separator argument to
PathContainer.parsePath with an Options type with two predefined
constants. One for HTTP URLs with automatic decoding and parsing of
path params, and another for "." separated message routes without
decoding except for encoded sequences of the separator itself.

See gh-23310
This commit is contained in:
Rossen Stoyanchev 2019-07-22 11:25:19 +01:00
parent fbe697061c
commit 358a6d6f10
10 changed files with 225 additions and 86 deletions

View File

@ -20,7 +20,9 @@ import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import org.springframework.lang.Nullable;
@ -38,11 +40,16 @@ import org.springframework.util.StringUtils;
*/
final class DefaultPathContainer implements PathContainer {
private static final MultiValueMap<String, String> EMPTY_MAP = new LinkedMultiValueMap<>();
private static final MultiValueMap<String, String> EMPTY_PARAMS = new LinkedMultiValueMap<>();
private static final PathContainer EMPTY_PATH = new DefaultPathContainer("", Collections.emptyList());
private static final PathContainer.Separator SEPARATOR = () -> "/";
private static final Map<Character, DefaultSeparator> SEPARATORS = new HashMap<>(2);
static {
SEPARATORS.put('/', new DefaultSeparator('/', "%2F"));
SEPARATORS.put('.', new DefaultSeparator('.', "%2E"));
}
private final String path;
@ -72,10 +79,10 @@ final class DefaultPathContainer implements PathContainer {
if (this == other) {
return true;
}
if (other == null || getClass() != other.getClass()) {
if (!(other instanceof PathContainer)) {
return false;
}
return this.path.equals(((DefaultPathContainer) other).path);
return value().equals(((PathContainer) other).value());
}
@Override
@ -89,18 +96,19 @@ final class DefaultPathContainer implements PathContainer {
}
static PathContainer createFromUrlPath(String path, String separator) {
static PathContainer createFromUrlPath(String path, Options options) {
if (path.equals("")) {
return EMPTY_PATH;
}
if (separator.length() == 0) {
throw new IllegalArgumentException("separator should not be empty");
char separator = options.separator();
DefaultSeparator separatorElement = SEPARATORS.get(separator);
if (separatorElement == null) {
throw new IllegalArgumentException("Unexpected separator: '" + separator + "'");
}
Separator separatorElement = separator.equals(SEPARATOR.value()) ? SEPARATOR : () -> separator;
List<Element> elements = new ArrayList<>();
int begin;
if (path.length() > 0 && path.startsWith(separator)) {
begin = separator.length();
if (path.length() > 0 && path.charAt(0) == separator) {
begin = 1;
elements.add(separatorElement);
}
else {
@ -110,23 +118,25 @@ final class DefaultPathContainer implements PathContainer {
int end = path.indexOf(separator, begin);
String segment = (end != -1 ? path.substring(begin, end) : path.substring(begin));
if (!segment.equals("")) {
elements.add(parsePathSegment(segment));
elements.add(options.shouldDecodeAndParseSegments() ?
decodeAndParsePathSegment(segment) :
new DefaultPathSegment(segment, separatorElement));
}
if (end == -1) {
break;
}
elements.add(separatorElement);
begin = end + separator.length();
begin = end + 1;
}
return new DefaultPathContainer(path, elements);
}
private static PathSegment parsePathSegment(String segment) {
private static PathSegment decodeAndParsePathSegment(String segment) {
Charset charset = StandardCharsets.UTF_8;
int index = segment.indexOf(';');
if (index == -1) {
String valueToMatch = StringUtils.uriDecode(segment, charset);
return new DefaultPathSegment(segment, valueToMatch, EMPTY_MAP);
return new DefaultPathSegment(segment, valueToMatch, EMPTY_PARAMS);
}
else {
String valueToMatch = StringUtils.uriDecode(segment.substring(0, index), charset);
@ -192,6 +202,30 @@ final class DefaultPathContainer implements PathContainer {
}
private static class DefaultSeparator implements Separator {
private final String separator;
private final String encodedSequence;
DefaultSeparator(char separator, String encodedSequence) {
this.separator = String.valueOf(separator);
this.encodedSequence = encodedSequence;
}
@Override
public String value() {
return this.separator;
}
public String encodedSequence() {
return this.encodedSequence;
}
}
private static class DefaultPathSegment implements PathSegment {
private final String value;
@ -202,14 +236,29 @@ final class DefaultPathContainer implements PathContainer {
private final MultiValueMap<String, String> parameters;
public DefaultPathSegment(String value, String valueToMatch, MultiValueMap<String, String> params) {
Assert.isTrue(!value.contains("/"), () -> "Invalid path segment value: " + value);
/**
* Constructor for decoded and parsed segments.
*/
DefaultPathSegment(String value, String valueToMatch, MultiValueMap<String, String> params) {
this.value = value;
this.valueToMatch = valueToMatch;
this.valueToMatchAsChars = valueToMatch.toCharArray();
this.parameters = CollectionUtils.unmodifiableMultiValueMap(params);
}
/**
* Constructor for segments without decoding and parsing.
*/
DefaultPathSegment(String value, DefaultSeparator separator) {
this.value = value;
this.valueToMatch = value.contains(separator.encodedSequence()) ?
value.replaceAll(separator.encodedSequence(), separator.value()) : value;
this.valueToMatchAsChars = this.valueToMatch.toCharArray();
this.parameters = EMPTY_PARAMS;
}
@Override
public String value() {
return this.value;
@ -235,10 +284,10 @@ final class DefaultPathContainer implements PathContainer {
if (this == other) {
return true;
}
if (other == null || getClass() != other.getClass()) {
if (!(other instanceof PathSegment)) {
return false;
}
return this.value.equals(((DefaultPathSegment) other).value);
return value().equals(((PathSegment) other).value());
}
@Override

View File

@ -72,19 +72,19 @@ public interface PathContainer {
* @return the parsed path
*/
static PathContainer parsePath(String path) {
return DefaultPathContainer.createFromUrlPath(path, "/");
return DefaultPathContainer.createFromUrlPath(path, Options.HTTP_PATH);
}
/**
* Parse the path value into a sequence of {@link Separator Separator} and
* {@link PathSegment PathSegment} elements.
* @param path the encoded, raw path value to parse
* @param separator the decoded separator for parsing patterns
* @param options to customize parsing
* @return the parsed path
* @since 5.2
*/
static PathContainer parsePath(String path, String separator) {
return DefaultPathContainer.createFromUrlPath(path, separator);
static PathContainer parsePath(String path, Options options) {
return DefaultPathContainer.createFromUrlPath(path, options);
}
@ -128,4 +128,58 @@ public interface PathContainer {
MultiValueMap<String, String> parameters();
}
/**
* Options to customize parsing based on the type of input path.
* @since 5.2
*/
class Options {
/**
* Options for HTTP URL paths:
* <p>Separator '/' with URL decoding and parsing of path params.
*/
public final static Options HTTP_PATH = Options.create('/', true);
/**
* Options for a message route:
* <p>Separator '.' without URL decoding nor parsing of params. Escape
* sequences for the separator char in segment values are still decoded.
*/
public final static Options MESSAGE_ROUTE = Options.create('.', false);
private final char separator;
private final boolean decodeAndParseSegments;
private Options(char separator, boolean decodeAndParseSegments) {
this.separator = separator;
this.decodeAndParseSegments = decodeAndParseSegments;
}
public char separator() {
return this.separator;
}
public boolean shouldDecodeAndParseSegments() {
return this.decodeAndParseSegments;
}
/**
* Create an {@link Options} instance with the given settings.
* @param separator the separator for parsing the path into segments;
* currently this must be slash or dot.
* @param decodeAndParseSegments whether to URL decode path segment
* values and parse path parameters. If set to false, only escape
* sequences for the separator char are decoded.
*/
public static Options create(char separator, boolean decodeAndParseSegments) {
return new Options(separator, decodeAndParseSegments);
}
}
}

View File

@ -105,16 +105,17 @@ class InternalPathPatternParser {
while (this.pos < this.pathPatternLength) {
char ch = this.pathPatternData[this.pos];
if (ch == this.parser.getSeparator()) {
char separator = this.parser.getPathOptions().separator();
if (ch == separator) {
if (this.pathElementStart != -1) {
pushPathElement(createPathElement());
}
if (peekDoubleWildcard()) {
pushPathElement(new WildcardTheRestPathElement(this.pos, this.parser.getSeparator()));
pushPathElement(new WildcardTheRestPathElement(this.pos, separator));
this.pos += 2;
}
else {
pushPathElement(new SeparatorPathElement(this.pos, this.parser.getSeparator()));
pushPathElement(new SeparatorPathElement(this.pos, separator));
}
}
else {
@ -221,7 +222,7 @@ class InternalPathPatternParser {
}
curlyBracketDepth--;
}
if (ch == this.parser.getSeparator() && !previousBackslash) {
if (ch == this.parser.getPathOptions().separator() && !previousBackslash) {
throw new PatternParseException(this.pos, this.pathPatternData,
PatternMessage.MISSING_CLOSE_CAPTURE);
}
@ -309,6 +310,7 @@ class InternalPathPatternParser {
}
PathElement newPE = null;
char separator = this.parser.getPathOptions().separator();
if (this.variableCaptureCount > 0) {
if (this.variableCaptureCount == 1 && this.pathElementStart == this.variableCaptureStart &&
@ -316,13 +318,13 @@ class InternalPathPatternParser {
if (this.isCaptureTheRestVariable) {
// It is {*....}
newPE = new CaptureTheRestPathElement(
this.pathElementStart, getPathElementText(), this.parser.getSeparator());
this.pathElementStart, getPathElementText(), separator);
}
else {
// It is a full capture of this element (possibly with constraint), for example: /foo/{abc}/
try {
newPE = new CaptureVariablePathElement(this.pathElementStart, getPathElementText(),
this.parser.isCaseSensitive(), this.parser.getSeparator());
this.parser.isCaseSensitive(), separator);
}
catch (PatternSyntaxException pse) {
throw new PatternParseException(pse,
@ -340,7 +342,7 @@ class InternalPathPatternParser {
}
RegexPathElement newRegexSection = new RegexPathElement(this.pathElementStart,
getPathElementText(), this.parser.isCaseSensitive(),
this.pathPatternData, this.parser.getSeparator());
this.pathPatternData, separator);
for (String variableName : newRegexSection.getVariableNames()) {
recordCapturedVariable(this.pathElementStart, variableName);
}
@ -350,20 +352,20 @@ class InternalPathPatternParser {
else {
if (this.wildcard) {
if (this.pos - 1 == this.pathElementStart) {
newPE = new WildcardPathElement(this.pathElementStart, this.parser.getSeparator());
newPE = new WildcardPathElement(this.pathElementStart, separator);
}
else {
newPE = new RegexPathElement(this.pathElementStart, getPathElementText(),
this.parser.isCaseSensitive(), this.pathPatternData, this.parser.getSeparator());
this.parser.isCaseSensitive(), this.pathPatternData, separator);
}
}
else if (this.singleCharWildcardCount != 0) {
newPE = new SingleCharWildcardedPathElement(this.pathElementStart, getPathElementText(),
this.singleCharWildcardCount, this.parser.isCaseSensitive(), this.parser.getSeparator());
this.singleCharWildcardCount, this.parser.isCaseSensitive(), separator);
}
else {
newPE = new LiteralPathElement(this.pathElementStart, getPathElementText(),
this.parser.isCaseSensitive(), this.parser.getSeparator());
this.parser.isCaseSensitive(), separator);
}
}

View File

@ -100,8 +100,8 @@ public class PathPattern implements Comparable<PathPattern> {
/** The parser used to construct this pattern. */
private final PathPatternParser parser;
/** The separator used when parsing the pattern. */
private final char separator;
/** The options to use to parse a pattern. */
private final PathContainer.Options pathOptions;
/** If this pattern has no trailing slash, allow candidates to include one and still match successfully. */
private final boolean matchOptionalTrailingSeparator;
@ -146,7 +146,7 @@ public class PathPattern implements Comparable<PathPattern> {
PathPattern(String patternText, PathPatternParser parser, @Nullable PathElement head) {
this.patternString = patternText;
this.parser = parser;
this.separator = parser.getSeparator();
this.pathOptions = parser.getPathOptions();
this.matchOptionalTrailingSeparator = parser.isMatchOptionalTrailingSeparator();
this.caseSensitive = parser.isCaseSensitive();
this.head = head;
@ -340,7 +340,7 @@ public class PathPattern implements Comparable<PathPattern> {
}
}
}
resultPath = PathContainer.parsePath(buf.toString(), String.valueOf(this.separator));
resultPath = PathContainer.parsePath(buf.toString(), this.pathOptions);
}
else if (startIndex >= endIndex) {
resultPath = PathContainer.parsePath("");
@ -401,7 +401,7 @@ public class PathPattern implements Comparable<PathPattern> {
// /hotels + /booking => /hotels/booking
// /hotels + booking => /hotels/booking
int starDotPos1 = this.patternString.indexOf("*."); // Are there any file prefix/suffix things to consider?
if (this.capturedVariableCount != 0 || starDotPos1 == -1 || this.separator == '.') {
if (this.capturedVariableCount != 0 || starDotPos1 == -1 || getSeparator() == '.') {
return this.parser.parse(concat(this.patternString, pattern2string.patternString));
}
@ -428,13 +428,13 @@ public class PathPattern implements Comparable<PathPattern> {
}
PathPattern otherPattern = (PathPattern) other;
return (this.patternString.equals(otherPattern.getPatternString()) &&
this.separator == otherPattern.getSeparator() &&
getSeparator() == otherPattern.getSeparator() &&
this.caseSensitive == otherPattern.caseSensitive);
}
@Override
public int hashCode() {
return (this.patternString.hashCode() + this.separator) * 17 + (this.caseSensitive ? 1 : 0);
return (this.patternString.hashCode() + getSeparator()) * 17 + (this.caseSensitive ? 1 : 0);
}
@Override
@ -461,7 +461,7 @@ public class PathPattern implements Comparable<PathPattern> {
}
char getSeparator() {
return this.separator;
return this.pathOptions.separator();
}
int getCapturedVariableCount() {
@ -506,8 +506,8 @@ public class PathPattern implements Comparable<PathPattern> {
* @return joined path that may include separator if necessary
*/
private String concat(String path1, String path2) {
boolean path1EndsWithSeparator = (path1.charAt(path1.length() - 1) == this.separator);
boolean path2StartsWithSeparator = (path2.charAt(0) == this.separator);
boolean path1EndsWithSeparator = (path1.charAt(path1.length() - 1) == getSeparator());
boolean path2StartsWithSeparator = (path2.charAt(0) == getSeparator());
if (path1EndsWithSeparator && path2StartsWithSeparator) {
return path1 + path2.substring(1);
}
@ -515,7 +515,7 @@ public class PathPattern implements Comparable<PathPattern> {
return path1 + path2;
}
else {
return path1 + this.separator + path2;
return path1 + getSeparator() + path2;
}
}
@ -534,7 +534,7 @@ public class PathPattern implements Comparable<PathPattern> {
private boolean pathContainerIsJustSeparator(PathContainer pathContainer) {
return pathContainer.value().length() == 1 &&
pathContainer.value().charAt(0) == this.separator;
pathContainer.value().charAt(0) == getSeparator();
}
/**

View File

@ -16,6 +16,8 @@
package org.springframework.web.util.pattern;
import org.springframework.http.server.PathContainer;
/**
* Parser for URI path patterns producing {@link PathPattern} instances that can
* then be matched to requests.
@ -36,7 +38,7 @@ public class PathPatternParser {
private boolean caseSensitive = true;
private char separator = '/';
private PathContainer.Options pathOptions = PathContainer.Options.HTTP_PATH;
/**
@ -77,20 +79,21 @@ public class PathPatternParser {
}
/**
* Char that represents the separator to use in the patterns.
* <p>The default is {@code '/'}.
* Set options for parsing patterns. These should be the same as the
* options used to parse input paths.
* <p>{@link PathContainer.Options#HTTP_PATH} is used by default.
* @since 5.2
*/
public void setSeparator(char separator) {
this.separator = separator;
public void setPathOptions(PathContainer.Options pathOptions) {
this.pathOptions = pathOptions;
}
/**
* Char that represents the separator to use in the patterns.
* Return the {@link #setPathOptions configured} pattern parsing options.
* @since 5.2
*/
public char getSeparator() {
return this.separator;
public PathContainer.Options getPathOptions() {
return this.pathOptions;
}

View File

@ -37,21 +37,32 @@ public class PathPatternRouteMatcher implements RouteMatcher {
private final PathPatternParser parser;
private final String separator;
private final Map<String, PathPattern> pathPatternCache = new ConcurrentHashMap<>();
/**
* Default constructor with {@link PathPatternParser} customized for
* {@link PathContainer.Options#MESSAGE_ROUTE MESSAGE_ROUTE} and without
* matching of trailing separator.
*/
public PathPatternRouteMatcher() {
this.parser = new PathPatternParser();
this.parser.setPathOptions(PathContainer.Options.MESSAGE_ROUTE);
this.parser.setMatchOptionalTrailingSeparator(false);
}
/**
* Constructor with given {@link PathPatternParser}.
*/
public PathPatternRouteMatcher(PathPatternParser parser) {
Assert.notNull(parser, "PathPatternParser must not be null");
this.parser = parser;
this.separator = String.valueOf(parser.getSeparator());
}
@Override
public Route parseRoute(String routeValue) {
return new PathContainerRoute(PathContainer.parsePath(routeValue, this.separator));
return new PathContainerRoute(PathContainer.parsePath(routeValue, this.parser.getPathOptions()));
}
@Override

View File

@ -27,7 +27,6 @@ import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
/**
* Unit tests for {@link DefaultPathContainer}.
@ -36,7 +35,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException
public class DefaultPathContainerTests {
@Test
public void pathSegment() throws Exception {
public void pathSegment() {
// basic
testPathSegment("cars", "cars", new LinkedMultiValueMap<>());
@ -92,7 +91,7 @@ public class DefaultPathContainerTests {
}
@Test
public void path() throws Exception {
public void path() {
// basic
testPath("/a/b/c", "/a/b/c", Arrays.asList("/", "a", "/", "b", "/", "c"));
@ -112,20 +111,20 @@ public class DefaultPathContainerTests {
testPath("//%20/%20", "//%20/%20", Arrays.asList("/", "/", "%20", "/", "%20"));
}
private void testPath(String input, String separator, String value, List<String> expectedElements) {
PathContainer path = PathContainer.parsePath(input, separator);
private void testPath(String input, PathContainer.Options options, String value, List<String> expectedElements) {
PathContainer path = PathContainer.parsePath(input, options);
assertThat(path.value()).as("value: '" + input + "'").isEqualTo(value);
assertThat(path.elements().stream()
.map(PathContainer.Element::value).collect(Collectors.toList())).as("elements: " + input).isEqualTo(expectedElements);
assertThat(path.elements().stream().map(PathContainer.Element::value).collect(Collectors.toList()))
.as("elements: " + input).isEqualTo(expectedElements);
}
private void testPath(String input, String value, List<String> expectedElements) {
testPath(input, "/", value, expectedElements);
testPath(input, PathContainer.Options.HTTP_PATH, value, expectedElements);
}
@Test
public void subPath() throws Exception {
public void subPath() {
// basic
PathContainer path = PathContainer.parsePath("/a/b/c");
assertThat(path.subPath(0)).isSameAs(path);
@ -141,14 +140,16 @@ public class DefaultPathContainerTests {
assertThat(path.subPath(2).value()).isEqualTo("/b/");
}
@Test
public void pathWithCustomSeparator() throws Exception {
testPath("a.b.c", ".", "a.b.c", Arrays.asList("a", ".", "b", ".", "c"));
}
@Test // gh-23310
public void pathWithCustomSeparator() {
PathContainer path = PathContainer.parsePath("a.b%2Eb.c", PathContainer.Options.MESSAGE_ROUTE);
@Test
public void emptySeparator() {
assertThatIllegalArgumentException().isThrownBy(() -> PathContainer.parsePath("path", ""));
List<String> decodedSegments = path.elements().stream()
.filter(e -> e instanceof PathSegment)
.map(e -> ((PathSegment) e).valueToMatch())
.collect(Collectors.toList());
assertThat(decodedSegments).isEqualTo(Arrays.asList("a", "b.b", "c"));
}
}

View File

@ -411,7 +411,7 @@ public class PathPatternParserTests {
@Test
public void separatorTests() {
PathPatternParser parser = new PathPatternParser();
parser.setSeparator('.');
parser.setPathOptions(PathContainer.Options.HTTP_PATH);
String rawPattern = "first.second.{last}";
PathPattern pattern = parser.parse(rawPattern);
assertThat(pattern.computePatternString()).isEqualTo(rawPattern);

View File

@ -16,8 +16,11 @@
package org.springframework.web.util.pattern;
import java.util.Map;
import org.junit.Test;
import org.springframework.http.server.PathContainer;
import org.springframework.util.RouteMatcher;
import static org.assertj.core.api.Assertions.assertThat;
@ -32,18 +35,33 @@ public class PathPatternRouteMatcherTests {
@Test
public void matchRoute() {
PathPatternRouteMatcher routeMatcher = new PathPatternRouteMatcher(new PathPatternParser());
RouteMatcher.Route route = routeMatcher.parseRoute("/projects/spring-framework");
assertThat(routeMatcher.match("/projects/{name}", route)).isTrue();
}
@Test
public void matchRouteWithCustomSeparator() {
PathPatternParser pathPatternParser = new PathPatternParser();
pathPatternParser.setSeparator('.');
PathPatternRouteMatcher routeMatcher = new PathPatternRouteMatcher(pathPatternParser);
PathPatternRouteMatcher routeMatcher = new PathPatternRouteMatcher();
RouteMatcher.Route route = routeMatcher.parseRoute("projects.spring-framework");
assertThat(routeMatcher.match("projects.{name}", route)).isTrue();
}
@Test
public void matchRouteWithCustomSeparator() {
PathPatternParser parser = new PathPatternParser();
parser.setPathOptions(PathContainer.Options.create('/', false));
PathPatternRouteMatcher routeMatcher = new PathPatternRouteMatcher(parser);
RouteMatcher.Route route = routeMatcher.parseRoute("/projects/spring-framework");
assertThat(routeMatcher.match("/projects/{name}", route)).isTrue();
}
@Test // gh-23310
public void noDecodingAndNoParamParsing() {
PathPatternRouteMatcher routeMatcher = new PathPatternRouteMatcher();
RouteMatcher.Route route = routeMatcher.parseRoute("projects.spring%20framework;p=1");
assertThat(routeMatcher.match("projects.spring%20framework;p=1", route)).isTrue();
}
@Test // gh-23310
public void separatorOnlyDecoded() {
PathPatternRouteMatcher routeMatcher = new PathPatternRouteMatcher();
RouteMatcher.Route route = routeMatcher.parseRoute("projects.spring%2Eframework");
Map<String, String> vars = routeMatcher.matchAndExtract("projects.{project}", route);
assertThat(vars).containsEntry("project", "spring.framework");
}
}

View File

@ -710,9 +710,10 @@ public class PathPatternTests {
@Test
public void extractPathWithinPatternCustomSeparator() {
PathPatternParser ppp = new PathPatternParser();
ppp.setSeparator('.');
ppp.setPathOptions(PathContainer.Options.create('.', true));
PathPattern pp = ppp.parse("test.**");
PathContainer pathContainer = PathContainer.parsePath("test.projects..spring-framework", ".");
PathContainer pathContainer = PathContainer.parsePath(
"test.projects..spring-framework", PathContainer.Options.create('.', true));
PathContainer result = pp.extractPathWithinPattern(pathContainer);
assertThat(result.value()).isEqualTo("projects.spring-framework");
assertThat(result.elements()).hasSize(3);