Skip to content
New issue

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

Sign up for GitHub

By clicking “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? Sign in to your account

Jump to bottom

RegexPathSpec documentation and MatchedPath improvements #8163

Merged
merged 7 commits into from Jun 16, 2022
Merged

RegexPathSpec documentation and MatchedPath improvements #8163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1eadfd7
RegexPathSpec documentation and MatchedPath improvements
joakime Jun 13, 2022
1c335f1
Fix javadoc
joakime Jun 13, 2022
9ea5ab6
Introduce RegexMatchedPath.getSplitIndex()
joakime Jun 14, 2022
52de789
Rework RegexMatchedPath to fit proposed algorithm
joakime Jun 15, 2022
89d9071
Update RegexMatchedPath bases on review
joakime Jun 15, 2022
54cce04
Fixing testcase assumption, now that splitting is done differently
joakime Jun 16, 2022
013f978
Fixing javadoc parse/build error
joakime Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
133 changes: 80 additions & 53 deletions jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java
View file
Open in desktop
Expand Up @@ -55,15 +55,23 @@
* <li>If there are no regex capturing groups,
* the entire path is returned in {@link MatchedPath#getPathMatch()},
* and a null returned for {@link MatchedPath#getPathInfo()}</li>
* <li>If the named regex capturing group {@code name} is present, that group
* is returned in {@link MatchedPath#getPathMatch()}</li>
* <li>If the named regex capturing group {@code info} is present, that group
* is returned in {@link MatchedPath#getPathInfo()}</li>
* <li>If the regex is of group type {@link PathSpecGroup#PREFIX_GLOB},
* the beginning of the regex is a literal, so it is split at the start of
* {@code java.util.regex.Matcher.group(1)}),
* taking care to handle trailing slash properly so that {@link MatchedPath#getPathMatch()}
* does not end in it, and {@link MatchedPath#getPathInfo()} starts with it.</li>
* <li>If both the named regex capturing groups {@code name} and {@code info} are present, then
* the {@code name} group is returned in {@link MatchedPath#getPathMatch()} and the
* {@code info} group is returned in {@link MatchedPath#getPathInfo()}</li>
* <li>
* If there is only 1 regex capturing group
* <ul>
* <li>If the named regex capturing group {@code name} is present, the
* input path up to the end of the capturing group is returned
* in {@link MatchedPath#getPathMatch()} and any following characters (or null)
* are returned in {@link MatchedPath#getPathInfo()} </li>
* <li>other wise the input path up to the start of the capturing group is returned
* in {@link MatchedPath#getPathMatch()} and any following characters (or null)
* are returned in {@link MatchedPath#getPathInfo()}</li>
* </ul>
* If the split on pathMatch ends with {@code /} AND the pathInfo doesn't start with {@code /}
* then the slash is moved from pathMatch to pathInfo.
* </li>
* <li>
* All other RegexPathSpec signatures will return the entire path
* in {@link MatchedPath#getPathMatch()}, and a null returned for {@link MatchedPath#getPathInfo()}
Expand Down Expand Up @@ -304,13 +312,19 @@ public int getPathDepth()
@Override
public String getPathInfo(String path)
{
return matched(path).getPathInfo();
MatchedPath matched = matched(path);
if (matched == null)
return null;
return matched.getPathInfo();
}

@Override
public String getPathMatch(String path)
{
return matched(path).getPathMatch();
MatchedPath matched = matched(path);
if (matched == null)
return "";
return matched.getPathMatch();
}

@Override
Expand Down Expand Up @@ -357,78 +371,92 @@ private class RegexMatchedPath implements MatchedPath
{
private final RegexPathSpec pathSpec;
private final String path;
private final Matcher matcher;

private final String pathMatch;
private final String pathInfo;
private String pathMatch;
private String pathInfo;

public RegexMatchedPath(RegexPathSpec regexPathSpec, String path, Matcher matcher)
{
this.pathSpec = regexPathSpec;
this.path = path;
this.matcher = matcher;

calcPathMatchInfo(matcher);
}

private void calcPathMatchInfo(Matcher matcher)
{
int groupCount = matcher.groupCount();

int idxNameStart = startOf(matcher, "name");
int idxNameEnd = endOf(matcher, "name");
int idxInfoStart = startOf(matcher, "info");
int idxInfoEnd = endOf(matcher, "info");

if (groupCount == 0)
{
pathMatch = path;
pathInfo = null;
return;
}
else if (groupCount == 1)

if (groupCount == 1)
{
if (idxNameStart >= 0)
// we know we are splitting
int idxNameEnd = endOf(matcher, "name");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately this lookup makes me itch! If they are not using the named group, then an exception is created and ignored for every lookup attempt.

For now, can we add a TODO that when we scan the pattern, we should look for the presence of "" and only do this lookup if we have seen it in the pattern.

if (idxNameEnd >= 0)
{
pathMatch = path.substring(idxNameStart, idxNameEnd);
pathMatch = path.substring(0, idxNameEnd);
pathInfo = path.substring(idxNameEnd);
}
else
{
pathMatch = path.substring(0, matcher.start(1));
pathInfo = matcher.group(1);
}
}
else
{
if (idxNameStart >= 0)
{
pathMatch = path.substring(idxNameStart, idxNameEnd);
if (idxInfoStart >= 0)
{
pathInfo = path.substring(idxInfoStart, idxInfoEnd);
}
else

// If split on pathMatch ends with '/'
// AND pathInfo doesn't have one, move the slash to pathInfo only move 1 level
if (pathMatch.length() > 0 && pathMatch.charAt(pathMatch.length() - 1) == '/' &&
!pathInfo.startsWith("/"))
{
pathInfo = path.substring(idxNameEnd);
pathMatch = pathMatch.substring(0, pathMatch.length() - 1);
pathInfo = '/' + pathInfo;
}
return;
}
else if (idxInfoStart >= 0)
{
pathMatch = path.substring(0, idxInfoStart);
pathInfo = path.substring(idxInfoStart, idxInfoEnd);
}
else

// Use start of anonymous group
int idx = matcher.start(1);
if (idx >= 0)
{
pathMatch = path;
pathInfo = null;
pathMatch = path.substring(0, idx);
pathInfo = path.substring(idx);

if (pathMatch.length() > 0 && pathMatch.charAt(pathMatch.length() - 1) == '/' &&
!pathInfo.startsWith("/"))
{
pathMatch = pathMatch.substring(0, pathMatch.length() - 1);
pathInfo = '/' + pathInfo;
}
return;
}
}

// Reach here we have 2+ groups

String gName = valueOf(matcher, "name");
String gInfo = valueOf(matcher, "info");
Comment on lines +394 to +395
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two exceptions may be created and ignored here. So again, I think we need a TODO to optimise that away in future.


// if both named groups exist
if (gName != null && gInfo != null)
{
pathMatch = gName;
pathInfo = gInfo;
return;
}

pathMatch = path;
pathInfo = null;
}

private int startOf(Matcher matcher, String groupName)
private String valueOf(Matcher matcher, String groupName)
{
try
{
return matcher.start(groupName);
return matcher.group(groupName);
}
catch (IllegalArgumentException notFound)
{
return -2;
return null;
}
}

Expand Down Expand Up @@ -462,7 +490,6 @@ public String toString()
return getClass().getSimpleName() + "[" +
"pathSpec=" + pathSpec +
", path=\"" + path + "\"" +
", matcher=" + matcher +
']';
}
}
Expand Down
25 changes: 15 additions & 10 deletions jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java
View file
Open in desktop
Expand Up @@ -81,23 +81,24 @@ public void testMiddleSpec()
assertNotMatches(spec, "/rest/admin/delete");
assertNotMatches(spec, "/rest/list");

assertThat(spec.getPathMatch("/rest/1.0/list"), equalTo("/rest/1.0/list"));
assertThat(spec.getPathInfo("/rest/1.0/list"), nullValue());
assertThat(spec.getPathMatch("/rest/1.0/list"), equalTo("/rest"));
assertThat(spec.getPathInfo("/rest/1.0/list"), equalTo("/1.0/list"));
}

public static Stream<Arguments> matchedPathCases()
{
return Stream.of(
// Suffix (with optional capture group)
Arguments.of("^/[Tt]est(/.*)?$", "/test/info", "/test", "/info"),
Arguments.of("^(.*).do$", "/a.do", "/a.do", null),
Arguments.of("^(.*).do$", "/a/b/c.do", "/a/b/c.do", null),
Arguments.of("^(.*).do$", "/abcde.do", "/abcde.do", null),
Arguments.of("^/[Tt]est(/.*)?$", "/test", "/test", null),
Arguments.of("^(.*).do$", "/a.do", "", "/a.do"),
Arguments.of("^(.*).do$", "/a/b/c.do", "", "/a/b/c.do"),
Arguments.of("^(.*).do$", "/abcde.do", "", "/abcde.do"),
// Exact (no capture group)
Arguments.of("^/test/info$", "/test/info", "/test/info", null),
// Middle (with one capture group)
Arguments.of("^/rest/([^/]*)/list$", "/rest/api/list", "/rest/api/list", null),
Arguments.of("^/rest/([^/]*)/list$", "/rest/1.0/list", "/rest/1.0/list", null),
Arguments.of("^/rest/([^/]*)/list$", "/rest/api/list", "/rest", "/api/list"),
Arguments.of("^/rest/([^/]*)/list$", "/rest/1.0/list", "/rest", "/1.0/list"),
// Middle (with two capture groups)
Arguments.of("^/t(.*)/i(.*)$", "/test/info", "/test/info", null),
// Prefix (with optional capture group)
Expand All @@ -116,7 +117,11 @@ public static Stream<Arguments> matchedPathCases()
// Middle (with 2 named capture groups)
// this is pretty much an all glob signature
Arguments.of("^(?<name>\\/.*)(?<info>\\/.*\\.action)$", "/test/info/code.action", "/test/info", "/code.action"),
Arguments.of("^(?<name>\\/.*)(?<info>\\/.*\\.action)$", "/a/b/c/d/e/f/g.action", "/a/b/c/d/e/f", "/g.action")
Arguments.of("^(?<name>\\/.*)(?<info>\\/.*\\.action)$", "/a/b/c/d/e/f/g.action", "/a/b/c/d/e/f", "/g.action"),
// Named groups with gap in the middle
Arguments.of("^(?<name>\\/.*)/x/(?<info>.*\\.action)$", "/a/b/c/x/e/f/g.action", "/a/b/c", "e/f/g.action"),
// Named groups in opposite order
Arguments.of("^(?<info>\\/.*)/x/(?<name>.*\\.action)$", "/a/b/c/x/e/f/g.action", "e/f/g.action", "/a/b/c")
);
}

Expand Down Expand Up @@ -196,8 +201,8 @@ public void testSuffixSpecTraditional()
assertNotMatches(spec, "/aa/bb");
assertNotMatches(spec, "/aa/bb.do/more");

assertThat(spec.getPathMatch("/a/b/c.do"), equalTo("/a/b/c.do"));
assertThat(spec.getPathInfo("/a/b/c.do"), nullValue());
assertThat(spec.getPathMatch("/a/b/c.do"), equalTo(""));
assertThat(spec.getPathInfo("/a/b/c.do"), equalTo("/a/b/c.do"));
}

/**
Expand Down