Converted PathMappings to be an AbstractMap (#9213)

This was primarily done to avoid surprise of the previous behaviour of puts for a duplicate key being ignored.
The use of the AbstractMap class now provides javadoc and known/expected behaviour for key methods.
The only significant behaviour change is in the return type of some key methods, with the old/previous value replacing a boolean.
Some stream usage has also been replaced by the more efficient iterator.
This commit is contained in:
Greg Wilkins 2023-01-31 09:19:01 +11:00 committed by GitHub
parent 4d7c38f3eb
commit c871fa4402
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 241 additions and 169 deletions

View File

@ -13,11 +13,13 @@
package org.eclipse.jetty.http.pathmap;
import java.util.Map;
import org.eclipse.jetty.util.annotation.ManagedAttribute;
import org.eclipse.jetty.util.annotation.ManagedObject;
@ManagedObject("Mapped Resource")
public class MappedResource<E> implements Comparable<MappedResource<E>>
public class MappedResource<E> implements Comparable<MappedResource<E>>, Map.Entry<PathSpec, E>
{
private final PathSpec pathSpec;
private final E resource;
@ -90,6 +92,24 @@ public class MappedResource<E> implements Comparable<MappedResource<E>>
return true;
}
@Override
public PathSpec getKey()
{
return getPathSpec();
}
@Override
public E getValue()
{
return getResource();
}
@Override
public E setValue(E value)
{
throw new UnsupportedOperationException();
}
@ManagedAttribute(value = "path spec", readonly = true)
public PathSpec getPathSpec()
{

View File

@ -14,9 +14,9 @@
package org.eclipse.jetty.http.pathmap;
import java.io.IOException;
import java.util.AbstractMap;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
@ -41,10 +41,11 @@ import org.slf4j.LoggerFactory;
* @param <E> the type of mapping endpoint
*/
@ManagedObject("Path Mappings")
public class PathMappings<E> implements Iterable<MappedResource<E>>, Dumpable
public class PathMappings<E> extends AbstractMap<PathSpec, 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 final Set<MappedResource<E>> _mappings = new TreeSet<>(Map.Entry.comparingByKey());
/**
* When _orderIsSignificant is true, the order of the MappedResources is significant and a match needs to be iteratively
@ -67,6 +68,14 @@ public class PathMappings<E> implements Iterable<MappedResource<E>>, Dumpable
private MappedResource<E> _servletRoot;
private MappedResource<E> _servletDefault;
@Override
public Set<Entry<PathSpec, E>> entrySet()
{
@SuppressWarnings("unchecked")
Set<Map.Entry<PathSpec, E>> entries = (Set<Map.Entry<PathSpec, E>>)(Set<? extends Map.Entry<PathSpec, E>>)_mappings;
return entries;
}
@Override
public String dump()
{
@ -108,9 +117,9 @@ public class PathMappings<E> implements Iterable<MappedResource<E>>, Dumpable
return _mappings.stream();
}
public void removeIf(Predicate<MappedResource<E>> predicate)
public boolean removeIf(Predicate<MappedResource<E>> predicate)
{
_mappings.removeIf(predicate);
return _mappings.removeIf(predicate);
}
/**
@ -352,130 +361,145 @@ public class PathMappings<E> implements Iterable<MappedResource<E>>, Dumpable
return _mappings.iterator();
}
public E get(PathSpec spec)
@Override
public E get(Object key)
{
return _mappings.stream()
.filter(mappedResource -> mappedResource.getPathSpec().equals(spec))
.map(MappedResource::getResource)
.findFirst()
.orElse(null);
return key instanceof PathSpec pathSpec ? get(pathSpec) : null;
}
public boolean put(String pathSpecString, E resource)
public E get(PathSpec pathSpec)
{
if (pathSpec == null)
return null;
for (MappedResource<E> mr : _mappings)
{
if (pathSpec.equals(mr.getKey()))
return mr.getValue();
}
return null;
}
public E put(String pathSpecString, E resource)
{
return put(PathSpec.from(pathSpecString), resource);
}
public boolean put(PathSpec pathSpec, E resource)
@Override
public E put(PathSpec pathSpec, E resource)
{
E old = remove(pathSpec);
MappedResource<E> entry = new MappedResource<>(pathSpec, resource);
boolean added = _mappings.add(entry);
_mappings.add(entry);
if (LOG.isDebugEnabled())
LOG.debug("{} {} to {}", added ? "Added" : "Ignored", entry, this);
LOG.debug("Added {} replacing {} to {}", entry, old, this);
if (added)
switch (pathSpec.getGroup())
{
switch (pathSpec.getGroup())
{
case EXACT:
if (pathSpec instanceof ServletPathSpec)
{
String exact = pathSpec.getDeclaration();
if (exact != null)
_exactMap.put(exact, entry);
}
else
{
// This is not a Servlet mapping, turn off optimization on Exact
// TODO: see if we can optimize all Regex / UriTemplate versions here too.
// 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;
_orderIsSignificant = true;
}
break;
case PREFIX_GLOB:
if (pathSpec instanceof ServletPathSpec)
{
String prefix = pathSpec.getPrefix();
if (prefix != null)
_prefixMap.put(prefix, entry);
}
else
{
// This is not a Servlet mapping, turn off optimization on Prefix
// TODO: see if we can optimize all Regex / UriTemplate versions here too.
// 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;
_orderIsSignificant = true;
}
break;
case SUFFIX_GLOB:
if (pathSpec instanceof ServletPathSpec)
{
String suffix = pathSpec.getSuffix();
if (suffix != null)
_suffixMap.put(suffix, entry);
}
else
{
// This is not a Servlet mapping, turn off optimization on Suffix
// TODO: see if we can optimize all Regex / UriTemplate versions here too.
// 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;
_orderIsSignificant = true;
}
break;
case ROOT:
if (pathSpec instanceof ServletPathSpec)
{
if (_servletRoot == null)
_servletRoot = entry;
}
else
{
_orderIsSignificant = true;
}
break;
case DEFAULT:
if (pathSpec instanceof ServletPathSpec)
{
if (_servletDefault == null)
_servletDefault = entry;
}
else
{
_orderIsSignificant = true;
}
break;
default:
}
case EXACT:
if (pathSpec instanceof ServletPathSpec)
{
String exact = pathSpec.getDeclaration();
if (exact != null)
_exactMap.put(exact, entry);
}
else
{
// This is not a Servlet mapping, turn off optimization on Exact
// TODO: see if we can optimize all Regex / UriTemplate versions here too.
// 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;
_orderIsSignificant = true;
}
break;
case PREFIX_GLOB:
if (pathSpec instanceof ServletPathSpec)
{
String prefix = pathSpec.getPrefix();
if (prefix != null)
_prefixMap.put(prefix, entry);
}
else
{
// This is not a Servlet mapping, turn off optimization on Prefix
// TODO: see if we can optimize all Regex / UriTemplate versions here too.
// 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;
_orderIsSignificant = true;
}
break;
case SUFFIX_GLOB:
if (pathSpec instanceof ServletPathSpec)
{
String suffix = pathSpec.getSuffix();
if (suffix != null)
_suffixMap.put(suffix, entry);
}
else
{
// This is not a Servlet mapping, turn off optimization on Suffix
// TODO: see if we can optimize all Regex / UriTemplate versions here too.
// 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;
_orderIsSignificant = true;
}
break;
case ROOT:
if (pathSpec instanceof ServletPathSpec)
{
if (_servletRoot == null)
_servletRoot = entry;
}
else
{
_orderIsSignificant = true;
}
break;
case DEFAULT:
if (pathSpec instanceof ServletPathSpec)
{
if (_servletDefault == null)
_servletDefault = entry;
}
else
{
_orderIsSignificant = true;
}
break;
default:
}
return added;
return old;
}
@SuppressWarnings("incomplete-switch")
public boolean remove(PathSpec pathSpec)
@Override
public E remove(Object key)
{
return key instanceof PathSpec pathSpec ? remove(pathSpec) : null;
}
public E remove(PathSpec pathSpec)
{
Iterator<MappedResource<E>> iter = _mappings.iterator();
boolean removed = false;
E removed = null;
while (iter.hasNext())
{
if (iter.next().getPathSpec().equals(pathSpec))
MappedResource<E> entry = iter.next();
if (entry.getPathSpec().equals(pathSpec))
{
removed = true;
removed = entry.getResource();
iter.remove();
break;
}
}
if (LOG.isDebugEnabled())
LOG.debug("{} {} to {}", removed ? "Removed" : "Ignored", pathSpec, this);
LOG.debug("Removed {} at {} from {}", removed, pathSpec, this);
if (removed)
if (removed != null)
{
switch (pathSpec.getGroup())
{

View File

@ -56,13 +56,13 @@ public class PathSpecSet extends AbstractSet<String> implements Predicate<String
@Override
public boolean add(String s)
{
return specs.put(PathSpec.from(s), Boolean.TRUE);
return specs.put(PathSpec.from(s), Boolean.TRUE) == null;
}
@Override
public boolean remove(Object o)
{
return specs.remove(asPathSpec(o));
return specs.remove(asPathSpec(o)) != null;
}
@Override

View File

@ -13,6 +13,7 @@
package org.eclipse.jetty.http.pathmap;
import java.util.Map;
import java.util.stream.Stream;
import org.junit.jupiter.api.Test;
@ -23,6 +24,7 @@ import org.junit.jupiter.params.provider.MethodSource;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
@ -270,15 +272,15 @@ public class PathMappingsTest
public void testPutRejectsDuplicates()
{
PathMappings<String> p = new PathMappings<>();
assertThat(p.put(new UriTemplatePathSpec("/a/{var1}/c"), "resourceA"), is(true));
assertThat(p.put(new UriTemplatePathSpec("/a/{var2}/c"), "resourceAA"), is(false));
assertThat(p.put(new UriTemplatePathSpec("/a/b/c"), "resourceB"), is(true));
assertThat(p.put(new UriTemplatePathSpec("/a/b/c"), "resourceBB"), is(false));
assertThat(p.put(new ServletPathSpec("/a/b/c"), "resourceBB"), is(true));
assertThat(p.put(new RegexPathSpec("/a/b/c"), "resourceBB"), is(true));
assertThat(p.put(new UriTemplatePathSpec("/a/{var2}/c"), "resourceA"), nullValue());
assertThat(p.put(new UriTemplatePathSpec("/a/{var1}/c"), "resourceAA"), is("resourceA"));
assertThat(p.put(new UriTemplatePathSpec("/a/b/c"), "resourceB"), nullValue());
assertThat(p.put(new UriTemplatePathSpec("/a/b/c"), "resourceBB"), is("resourceB"));
assertThat(p.put(new ServletPathSpec("/a/b/c"), "resourceBB"), nullValue());
assertThat(p.put(new RegexPathSpec("/a/b/c"), "resourceBB"), nullValue());
assertThat(p.put(new ServletPathSpec("/*"), "resourceC"), is(true));
assertThat(p.put(new RegexPathSpec("/(.*)"), "resourceCC"), is(true));
assertThat(p.put(new ServletPathSpec("/*"), "resourceC"), nullValue());
assertThat(p.put(new RegexPathSpec("/(.*)"), "resourceCC"), nullValue());
}
@Test
@ -373,27 +375,27 @@ public class PathMappingsTest
PathMappings<String> p = new PathMappings<>();
p.put(new UriTemplatePathSpec("/a/{var1}/c"), "resourceA");
assertThat(p.remove(new UriTemplatePathSpec("/a/{var1}/c")), is(true));
assertThat(p.remove(new UriTemplatePathSpec("/a/{var1}/c")), is("resourceA"));
p.put(new UriTemplatePathSpec("/a/{var1}/c"), "resourceA");
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/a/{b}/c")), is(true));
assertThat(p.remove(new UriTemplatePathSpec("/a/{b}/c")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new UriTemplatePathSpec("/a/{b}/c")), is("resourceA"));
assertThat(p.remove(new UriTemplatePathSpec("/a/{b}/c")), nullValue());
p.put(new UriTemplatePathSpec("/{var1}/b/c"), "resourceA");
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/{a}/b/c")), is(true));
assertThat(p.remove(new UriTemplatePathSpec("/{a}/b/c")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new UriTemplatePathSpec("/{a}/b/c")), is("resourceA"));
assertThat(p.remove(new UriTemplatePathSpec("/{a}/b/c")), nullValue());
p.put(new UriTemplatePathSpec("/a/b/{var1}"), "resourceA");
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/a/b/{c}")), is(true));
assertThat(p.remove(new UriTemplatePathSpec("/a/b/{c}")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new UriTemplatePathSpec("/a/b/{c}")), is("resourceA"));
assertThat(p.remove(new UriTemplatePathSpec("/a/b/{c}")), nullValue());
p.put(new UriTemplatePathSpec("/{var1}/{var2}/{var3}"), "resourceA");
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/{a}/{b}/{c}")), is(true));
assertThat(p.remove(new UriTemplatePathSpec("/{a}/{b}/{c}")), is(false));
assertThat(p.remove(new UriTemplatePathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new UriTemplatePathSpec("/{a}/{b}/{c}")), is("resourceA"));
assertThat(p.remove(new UriTemplatePathSpec("/{a}/{b}/{c}")), nullValue());
}
@Test
@ -402,24 +404,24 @@ public class PathMappingsTest
PathMappings<String> p = new PathMappings<>();
p.put(new RegexPathSpec("/a/(.*)/c"), "resourceA");
assertThat(p.remove(new RegexPathSpec("/a/b/c")), is(false));
assertThat(p.remove(new RegexPathSpec("/a/(.*)/c")), is(true));
assertThat(p.remove(new RegexPathSpec("/a/(.*)/c")), is(false));
assertThat(p.remove(new RegexPathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new RegexPathSpec("/a/(.*)/c")), is("resourceA"));
assertThat(p.remove(new RegexPathSpec("/a/(.*)/c")), nullValue());
p.put(new RegexPathSpec("/(.*)/b/c"), "resourceA");
assertThat(p.remove(new RegexPathSpec("/a/b/c")), is(false));
assertThat(p.remove(new RegexPathSpec("/(.*)/b/c")), is(true));
assertThat(p.remove(new RegexPathSpec("/(.*)/b/c")), is(false));
assertThat(p.remove(new RegexPathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new RegexPathSpec("/(.*)/b/c")), is("resourceA"));
assertThat(p.remove(new RegexPathSpec("/(.*)/b/c")), nullValue());
p.put(new RegexPathSpec("/a/b/(.*)"), "resourceA");
assertThat(p.remove(new RegexPathSpec("/a/b/c")), is(false));
assertThat(p.remove(new RegexPathSpec("/a/b/(.*)")), is(true));
assertThat(p.remove(new RegexPathSpec("/a/b/(.*)")), is(false));
assertThat(p.remove(new RegexPathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new RegexPathSpec("/a/b/(.*)")), is("resourceA"));
assertThat(p.remove(new RegexPathSpec("/a/b/(.*)")), nullValue());
p.put(new RegexPathSpec("/a/b/c"), "resourceA");
assertThat(p.remove(new RegexPathSpec("/a/b/d")), is(false));
assertThat(p.remove(new RegexPathSpec("/a/b/c")), is(true));
assertThat(p.remove(new RegexPathSpec("/a/b/c")), is(false));
assertThat(p.remove(new RegexPathSpec("/a/b/d")), nullValue());
assertThat(p.remove(new RegexPathSpec("/a/b/c")), is("resourceA"));
assertThat(p.remove(new RegexPathSpec("/a/b/c")), nullValue());
}
@Test
@ -428,34 +430,34 @@ public class PathMappingsTest
PathMappings<String> p = new PathMappings<>();
p.put(new ServletPathSpec("/a/*"), "resourceA");
assertThat(p.remove(new ServletPathSpec("/a/b")), is(false));
assertThat(p.remove(new ServletPathSpec("/a/*")), is(true));
assertThat(p.remove(new ServletPathSpec("/a/*")), is(false));
assertThat(p.remove(new ServletPathSpec("/a/b")), nullValue());
assertThat(p.remove(new ServletPathSpec("/a/*")), is("resourceA"));
assertThat(p.remove(new ServletPathSpec("/a/*")), nullValue());
p.put(new ServletPathSpec("/a/b/*"), "resourceA");
assertThat(p.remove(new ServletPathSpec("/a/b/c")), is(false));
assertThat(p.remove(new ServletPathSpec("/a/b/*")), is(true));
assertThat(p.remove(new ServletPathSpec("/a/b/*")), is(false));
assertThat(p.remove(new ServletPathSpec("/a/b/c")), nullValue());
assertThat(p.remove(new ServletPathSpec("/a/b/*")), is("resourceA"));
assertThat(p.remove(new ServletPathSpec("/a/b/*")), nullValue());
p.put(new ServletPathSpec("*.do"), "resourceA");
assertThat(p.remove(new ServletPathSpec("*.gz")), is(false));
assertThat(p.remove(new ServletPathSpec("*.do")), is(true));
assertThat(p.remove(new ServletPathSpec("*.do")), is(false));
assertThat(p.remove(new ServletPathSpec("*.gz")), nullValue());
assertThat(p.remove(new ServletPathSpec("*.do")), is("resourceA"));
assertThat(p.remove(new ServletPathSpec("*.do")), nullValue());
p.put(new ServletPathSpec("/"), "resourceA");
assertThat(p.remove(new ServletPathSpec("/a")), is(false));
assertThat(p.remove(new ServletPathSpec("/")), is(true));
assertThat(p.remove(new ServletPathSpec("/")), is(false));
assertThat(p.remove(new ServletPathSpec("/a")), nullValue());
assertThat(p.remove(new ServletPathSpec("/")), is("resourceA"));
assertThat(p.remove(new ServletPathSpec("/")), nullValue());
p.put(new ServletPathSpec(""), "resourceA");
assertThat(p.remove(new ServletPathSpec("/")), is(false));
assertThat(p.remove(new ServletPathSpec("")), is(true));
assertThat(p.remove(new ServletPathSpec("")), is(false));
assertThat(p.remove(new ServletPathSpec("/")), nullValue());
assertThat(p.remove(new ServletPathSpec("")), is("resourceA"));
assertThat(p.remove(new ServletPathSpec("")), nullValue());
p.put(new ServletPathSpec("/a/b/c"), "resourceA");
assertThat(p.remove(new ServletPathSpec("/a/b/d")), is(false));
assertThat(p.remove(new ServletPathSpec("/a/b/c")), is(true));
assertThat(p.remove(new ServletPathSpec("/a/b/c")), is(false));
assertThat(p.remove(new ServletPathSpec("/a/b/d")), nullValue());
assertThat(p.remove(new ServletPathSpec("/a/b/c")), is("resourceA"));
assertThat(p.remove(new ServletPathSpec("/a/b/c")), nullValue());
}
@Test
@ -510,6 +512,37 @@ public class PathMappingsTest
assertThat(p.getMatched("/foo.do").getResource(), equalTo("resourceS"));
assertThat(p.getMatched("/a/foo.do").getResource(), equalTo("resourceP"));
assertThat(p.getMatched("/b/foo.do").getResource(), equalTo("resourceS"));
}
@Test
public void testAsMap()
{
PathMappings<String> p = new PathMappings<>();
p.put(new ServletPathSpec(""), "resourceR");
p.put(new ServletPathSpec("/"), "resourceD");
p.put(new ServletPathSpec("/exact"), "REPLACED");
p.put(new ServletPathSpec("/exact"), "resourceE");
p.put(new ServletPathSpec("/a/*"), "resourceP");
p.put(new ServletPathSpec("*.do"), "resourceS");
@SuppressWarnings("redundant")
Map<PathSpec, String> map = p;
assertThat(map.keySet(), containsInAnyOrder(
new ServletPathSpec(""),
new ServletPathSpec("/"),
new ServletPathSpec("/exact"),
new ServletPathSpec("/a/*"),
new ServletPathSpec("*.do")
));
assertThat(map.values(), containsInAnyOrder(
"resourceR",
"resourceD",
"resourceE",
"resourceP",
"resourceS"
));
}
}

View File

@ -73,22 +73,17 @@ public class PathMappingsHandler extends Handler.AbstractContainer
throw new IllegalStateException("Cannot add mapping: " + this);
// check that self isn't present
if (handler == this || handler instanceof Handler.Container container && container.getDescendants().contains(this))
if (handler == this)
throw new IllegalStateException("Unable to addHandler of self: " + handler);
// check existing mappings
for (MappedResource<Handler> entry : mappings)
{
Handler entryHandler = entry.getResource();
if (entryHandler == this ||
entryHandler == handler ||
(entryHandler instanceof Handler.Container container && container.getDescendants().contains(this)))
throw new IllegalStateException("addMapping loop detected: " + handler);
}
// check for loops
if (handler instanceof Handler.Container container && container.getDescendants().contains(this))
throw new IllegalStateException("loop detected: " + handler);
// add new mapping and remove any old
Handler old = mappings.get(pathSpec);
mappings.put(pathSpec, handler);
addBean(handler);
updateBean(old, handler);
}
@Override

View File

@ -205,7 +205,7 @@ public class WebSocketMappings implements Dumpable, LifeCycle.Listener
public boolean removeMapping(PathSpec pathSpec)
{
return mappings.remove(pathSpec);
return mappings.remove(pathSpec) != null;
}
/**