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

PathMappings optimizations #9055

Merged
merged 7 commits into from
Dec 20, 2022
Merged

PathMappings optimizations #9055

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
976ca77
Optimize PathMappings
gregw Dec 14, 2022
496b6b8
Optimize PathMappings
gregw Dec 14, 2022
91da887
Optimize PathMappings
gregw Dec 16, 2022
943aa3e
Optimize PathMappings
gregw Dec 16, 2022
b047af2
More test coverage
gregw Dec 19, 2022
8a1dc8e
More test coverage
gregw Dec 19, 2022
411e9ca
No need for this change now.
gregw Dec 19, 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
156 changes: 137 additions & 19 deletions jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -42,6 +43,7 @@ public class PathMappings<E> implements Iterable<MappedResource<E>>, Dumpable
private static final Logger LOG = LoggerFactory.getLogger(PathMappings.class);
private final Set<MappedResource<E>> _mappings = new TreeSet<>(Comparator.comparing(MappedResource::getPathSpec));

private boolean _nonServletPathSpecs;
private boolean _optimizedExact = true;
private final Index.Mutable<MappedResource<E>> _exactMap = new Index.Builder<MappedResource<E>>()
.caseSensitive(true)
Expand All @@ -58,6 +60,9 @@ public class PathMappings<E> implements Iterable<MappedResource<E>>, Dumpable
.mutable()
.build();

private MappedResource<E> _servletRoot;
private MappedResource<E> _servletDefault;

@Override
public String dump()
{
Expand Down Expand Up @@ -86,6 +91,12 @@ public void reset()
_mappings.clear();
_prefixMap.clear();
_suffixMap.clear();
_optimizedExact = true;
_optimizedPrefix = true;
_optimizedSuffix = true;
_nonServletPathSpecs = false;
_servletRoot = null;
_servletDefault = null;
}

public void removeIf(Predicate<MappedResource<E>> predicate)
Expand Down Expand Up @@ -123,29 +134,97 @@ public List<MappedResource<E>> getMatches(String path)
{
boolean isRootPath = "/".equals(path);

List<MappedResource<E>> ret = new ArrayList<>();
if (_mappings.isEmpty())
joakime marked this conversation as resolved.
Show resolved Hide resolved
return Collections.emptyList();

List<MappedResource<E>> matches = null;
for (MappedResource<E> mr : _mappings)
{
switch (mr.getPathSpec().getGroup())
{
case ROOT:
if (isRootPath)
ret.add(mr);
{
if (matches == null)
matches = new ArrayList<>();
matches.add(mr);
}
break;
case DEFAULT:
if (isRootPath || mr.getPathSpec().matched(path) != null)
ret.add(mr);
{
if (matches == null)
matches = new ArrayList<>();
matches.add(mr);
}
break;
default:
if (mr.getPathSpec().matched(path) != null)
ret.add(mr);
{
if (matches == null)
matches = new ArrayList<>();
matches.add(mr);
}
break;
}
}
return ret;
return matches == null ? Collections.emptyList() : matches;
}

public MatchedResource<E> getMatched(String path)
{
if (_mappings.isEmpty())
return null;

if (_nonServletPathSpecs)
return getMatchedMixed(path);

if (_servletRoot != null && "/".equals(path))
return new MatchedResource<>(_servletRoot.getResource(), _servletRoot.getPathSpec(), _servletRoot.getPathSpec().matched(path));

MappedResource<E> candidate = _exactMap.getBest(path);
if (candidate != null)
{
MatchedPath matchedPath = candidate.getPathSpec().matched(path);
if (matchedPath != null)
return new MatchedResource<>(candidate.getResource(), candidate.getPathSpec(), matchedPath);
}

candidate = _prefixMap.getBest(path);
if (candidate != null)
{
MatchedPath matchedPath = candidate.getPathSpec().matched(path);
if (matchedPath != null)
return new MatchedResource<>(candidate.getResource(), candidate.getPathSpec(), matchedPath);
}

if (!_suffixMap.isEmpty())
{
int i = Math.max(0, path.lastIndexOf("/"));
// Loop through each suffix mark
// Input is "/a.b.c.foo"
joakime marked this conversation as resolved.
Show resolved Hide resolved
// Loop 1: "b.c.foo"
// Loop 2: "c.foo"
// Loop 3: "foo"
while ((i = path.indexOf('.', i + 1)) > 0)
{
candidate = _suffixMap.get(path, i + 1, path.length() - i - 1);
if (candidate == null)
continue;

MatchedPath matchedPath = candidate.getPathSpec().matched(path);
if (matchedPath != null)
return new MatchedResource<>(candidate.getResource(), candidate.getPathSpec(), matchedPath);
}
}

if (_servletDefault != null)
return new MatchedResource<>(_servletDefault.getResource(), _servletDefault.getPathSpec(), _servletDefault.getPathSpec().matched(path));

return null;
}

private MatchedResource<E> getMatchedMixed(String path)
{
MatchedPath matchedPath;
PathSpecGroup lastGroup = null;
Expand Down Expand Up @@ -173,18 +252,12 @@ public MatchedResource<E> getMatched(String path)
{
if (_optimizedExact)
{
int i = path.length();
while (i >= 0)
MappedResource<E> candidate = _exactMap.getBest(path);
if (candidate != null)
{
MappedResource<E> candidate = _exactMap.getBest(path, 0, i--);
if (candidate == null)
continue;

matchedPath = candidate.getPathSpec().matched(path);
if (matchedPath != null)
{
return new MatchedResource<>(candidate.getResource(), candidate.getPathSpec(), matchedPath);
}
}
// If we reached here, there's NO optimized EXACT Match possible, skip simple match below
skipRestOfGroup = true;
Expand All @@ -196,17 +269,14 @@ public MatchedResource<E> getMatched(String path)
{
if (_optimizedPrefix)
{
int i = path.length();
while (i >= 0)
MappedResource<E> candidate = _prefixMap.getBest(path);
if (candidate != null)
{
MappedResource<E> candidate = _prefixMap.getBest(path, 0, i--);
if (candidate == null)
continue;

matchedPath = candidate.getPathSpec().matched(path);
if (matchedPath != null)
return new MatchedResource<>(candidate.getResource(), candidate.getPathSpec(), matchedPath);
}

// If we reached here, there's NO optimized PREFIX Match possible, skip simple match below
skipRestOfGroup = true;
}
Expand Down Expand Up @@ -317,6 +387,7 @@ public boolean put(PathSpec pathSpec, E resource)
// Note: Example exact in Regex that can cause problems `^/a\Q/b\E/` (which is only ever matching `/a/b/`)
// Note: UriTemplate can handle exact easily enough
_optimizedExact = false;
_nonServletPathSpecs = true;
}
break;
case PREFIX_GLOB:
Expand All @@ -333,6 +404,7 @@ public boolean put(PathSpec pathSpec, E resource)
// Note: Example Prefix in Regex that can cause problems `^/a/b+` or `^/a/bb*` ('b' one or more times)
// Note: Example Prefix in UriTemplate that might cause problems `/a/{b}/{c}`
_optimizedPrefix = false;
_nonServletPathSpecs = true;
}
break;
case SUFFIX_GLOB:
Expand All @@ -349,6 +421,29 @@ public boolean put(PathSpec pathSpec, E resource)
// Note: Example suffix in Regex that can cause problems `^.*/path/name.ext` or `^/a/.*(ending)`
// Note: Example suffix in UriTemplate that can cause problems `/{a}/name.ext`
_optimizedSuffix = false;
_nonServletPathSpecs = true;
}
break;
case ROOT:
if (pathSpec instanceof ServletPathSpec)
{
if (_servletRoot == null)
_servletRoot = entry;
}
else
{
_nonServletPathSpecs = true;
}
break;
case DEFAULT:
if (pathSpec instanceof ServletPathSpec)
{
if (_servletDefault == null)
_servletDefault = entry;
}
else
{
_nonServletPathSpecs = true;
}
break;
default:
Expand Down Expand Up @@ -387,6 +482,7 @@ public boolean remove(PathSpec pathSpec)
_exactMap.remove(exact);
// Recalculate _optimizeExact
_optimizedExact = canBeOptimized(PathSpecGroup.EXACT);
_nonServletPathSpecs = nonServletPathSpec();
}
break;
case PREFIX_GLOB:
Expand All @@ -396,6 +492,7 @@ public boolean remove(PathSpec pathSpec)
_prefixMap.remove(prefix);
// Recalculate _optimizePrefix
_optimizedPrefix = canBeOptimized(PathSpecGroup.PREFIX_GLOB);
_nonServletPathSpecs = nonServletPathSpec();
}
break;
case SUFFIX_GLOB:
Expand All @@ -405,8 +502,23 @@ public boolean remove(PathSpec pathSpec)
_suffixMap.remove(suffix);
// Recalculate _optimizeSuffix
_optimizedSuffix = canBeOptimized(PathSpecGroup.SUFFIX_GLOB);
_nonServletPathSpecs = nonServletPathSpec();
}
break;
case ROOT:
_servletRoot = _mappings.stream()
.filter(mapping -> mapping.getPathSpec().getGroup() == PathSpecGroup.ROOT)
.filter(mapping -> mapping.getPathSpec() instanceof ServletPathSpec)
.findFirst().orElse(null);
_nonServletPathSpecs = nonServletPathSpec();
break;
case DEFAULT:
_servletDefault = _mappings.stream()
.filter(mapping -> mapping.getPathSpec().getGroup() == PathSpecGroup.DEFAULT)
.filter(mapping -> mapping.getPathSpec() instanceof ServletPathSpec)
.findFirst().orElse(null);
_nonServletPathSpecs = nonServletPathSpec();
break;
}
}

Expand All @@ -420,6 +532,12 @@ private boolean canBeOptimized(PathSpecGroup suffixGlob)
.allMatch((mapping) -> mapping.getPathSpec() instanceof ServletPathSpec);
}

private boolean nonServletPathSpec()
{
return _mappings.stream()
.allMatch((mapping) -> mapping.getPathSpec() instanceof ServletPathSpec);
}

@Override
public String toString()
{
Expand Down
51 changes: 33 additions & 18 deletions jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@

package org.eclipse.jetty.http.pathmap;

import java.util.stream.Stream;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
Expand Down Expand Up @@ -319,30 +324,40 @@ public void testGetServletPathSpec()
assertThat(p.get(new RegexPathSpec("/a/b/c")), nullValue());
}

@Test
public void testServletMultipleSuffixMappings()
public static Stream<Arguments> suffixMatches()
{
return Stream.of(
Arguments.of("/a.b.c.foo", "resourceFoo"),
Arguments.of("/a.b.c.bar", "resourceBar"),
Arguments.of("/a.b.c.foo.bar", "resourceFooBar"),
Arguments.of("/a.b/c.d.foo", "resourceFoo"),
Arguments.of("/a.b/c.d.bar", "resourceBar"),
Arguments.of("/a.b/c.d.foo.bar", "resourceFooBar"),
Arguments.of("/a.b.c.pop", null),
Arguments.of("/a.foo.c.pop", null),
Arguments.of("/a.foop", null),
Arguments.of("/a%2Efoo", null)
);
}

@ParameterizedTest
@MethodSource("suffixMatches")
public void testServletMultipleSuffixMappings(String path, String matched)
{
PathMappings<String> p = new PathMappings<>();
p.put(new ServletPathSpec("*.foo"), "resourceFoo");
p.put(new ServletPathSpec("*.bar"), "resourceBar");
p.put(new ServletPathSpec("*.foo.bar"), "resourceFooBar");
p.put(new ServletPathSpec("*.zed"), "resourceZed");

MatchedResource<String> matched;

matched = p.getMatched("/a.b.c.foo");
assertThat(matched.getResource(), is("resourceFoo"));

matched = p.getMatched("/a.b.c.bar");
assertThat(matched.getResource(), is("resourceBar"));

matched = p.getMatched("/a.b.c.pop");
assertNull(matched);

matched = p.getMatched("/a.foo.c.pop");
assertNull(matched);

matched = p.getMatched("/a%2Efoo");
assertNull(matched);
MatchedResource<String> match = p.getMatched(path);
if (matched == null)
assertThat(match, nullValue());
else
{
assertThat(match, notNullValue());
assertThat(p.getMatched(path).getResource(), equalTo(matched));
}
}

@Test
Expand Down
7 changes: 4 additions & 3 deletions jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExcludeSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,10 @@ public void exclude(T... element)
@Override
public boolean test(P t)
{
if (!_includes.isEmpty() && !_includePredicate.test(t))
return false;
return !_excludePredicate.test(t);
// return true IFF the passed object is not in the excluded set AND
// either the included set is empty OR the object is in the included set
return (_excludes.isEmpty() || !_excludePredicate.test(t)) &&
(_includes.isEmpty() || _includePredicate.test(t));
}

/**
Expand Down