Skip to content

Avoid using regex matching for static patterns #24887

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? No Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
* @author Arjen Poutsma
* @author Rossen Stoyanchev
* @author Sam Brannen
* @author Vladislav Kisel
* @since 16.07.2003
*/
public class AntPathMatcher implements PathMatcher {
Expand Down Expand Up @@ -647,6 +648,18 @@ protected static class AntPathStringMatcher {

private final Pattern pattern;

private final String originalPattern;

/**
* True if this matcher accepts any value (e.g. regex <code>.*</code>)
*/
private final boolean acceptAny;

/**
* True if given pattern is a literal pattern, thus candidate string may be tested by strings comparison for better perf.
*/
private final boolean allowStringComparison;

private final List<String> variableNames = new LinkedList<>();

public AntPathStringMatcher(String pattern) {
Expand Down Expand Up @@ -686,6 +699,10 @@ else if (match.startsWith("{") && match.endsWith("}")) {
patternBuilder.append(quote(pattern, end, pattern.length()));
this.pattern = (caseSensitive ? Pattern.compile(patternBuilder.toString()) :
Pattern.compile(patternBuilder.toString(), Pattern.CASE_INSENSITIVE));

this.originalPattern = pattern;
this.acceptAny = this.pattern.pattern().equals(DEFAULT_VARIABLE_PATTERN);
this.allowStringComparison = end == 0;
}

private String quote(String s, int start, int end) {
Expand All @@ -700,16 +717,22 @@ private String quote(String s, int start, int end) {
* @return {@code true} if the string matches against the pattern, or {@code false} otherwise.
*/
public boolean matchStrings(String str, @Nullable Map<String, String> uriTemplateVariables) {
// Check whether current pattern accepts any value or given string is exact match
if (this.acceptAny) {
if (uriTemplateVariables != null) {
assertUrlVariablesCount(1);
uriTemplateVariables.put(this.variableNames.get(0), str);
}
return true;
}
else if (this.allowStringComparison && str.equals(this.originalPattern)) {
return true;
}

Matcher matcher = this.pattern.matcher(str);
if (matcher.matches()) {
if (uriTemplateVariables != null) {
// SPR-8455
if (this.variableNames.size() != matcher.groupCount()) {
throw new IllegalArgumentException("The number of capturing groups in the pattern segment " +
this.pattern + " does not match the number of URI template variables it defines, " +
"which can occur if capturing groups are used in a URI template regex. " +
"Use non-capturing groups instead.");
}
assertUrlVariablesCount(matcher.groupCount());
for (int i = 1; i <= matcher.groupCount(); i++) {
String name = this.variableNames.get(i - 1);
String value = matcher.group(i);
Expand All @@ -722,6 +745,17 @@ public boolean matchStrings(String str, @Nullable Map<String, String> uriTemplat
return false;
}
}

// SPR-8455
private void assertUrlVariablesCount(int expected) {
if (this.variableNames.size() != expected) {
throw new IllegalArgumentException("The number of capturing groups in the pattern segment " +
this.pattern + " does not match the number of URI template variables it defines, " +
"which can occur if capturing groups are used in a URI template regex. " +
"Use non-capturing groups instead.");
}
}

}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ void match() {
assertThat(pathMatcher.match("", "")).isTrue();

assertThat(pathMatcher.match("/{bla}.*", "/testing.html")).isTrue();

// Test that sending the same pattern will not match (gh #24887)
assertThat(pathMatcher.match("/bla/{foo:[0-9]}", "/bla/{foo:[0-9]}")).isFalse();
}

@Test
Expand Down