Issue #11494 - PathMappingsHandler exposes PathSpec and Context based on PathSpec.

[joakime]

• Introduces Context.Wrapper
• Introduces PathMappingsHandler.PathSpecRequest (wrapper, with custom Context)

Fixes: #11494

[gregw]

I like the direction this is going.... but a few niggles/questions....

I'd also like to consider the option of setting the contexts base resource with the mapping... as then all the mappings could be to the same ResourceHandler and it would just use the context to find the mapping!

[gregw]

I don't think this is correct. I think you can just use the default method.

[gregw]

@joakime maybe implement it like:

public class PathMappingsHandler
{
    // ...

    public static class Contextual extends PathMappingsHandler
    {
        @Override
        public void addMapping(PathSpec pathSpec, Handler handler)
        {
            addMapping(pathSpec, null, handler);
        }

        public void addMapping(PathSpec pathSpec, Resource baseResource, Handler handler)
        {
            switch (pathSpec.getGroup())
            {
                case PREFIX_GLOB, ROOT, EXACT, DEFAULT ->
                {
                }
                default -> throw new IllegalArgumentException("Only prefix patterns");
            }
            ContextHandler context = new ContextHandler(pathSpec.getPrefix());
            if (baseResource != null)
                context.setBaseResource(baseResource);
            context.setHandler(handler);
            super.addMapping(pathSpec, context);
        }
    }
}

So it ends up being a convenience class that wraps passed handlers with our existing ContextHandler? This will dump better and work if code looks for ContextHandlers

[gregw]

@joakime thoughts?

[joakime]

The truth of the context-path (and path info) is from the PathSpec, and while we have our own, and can make some good guesses on how it should behave, we are also making those assumptions based on what we experience.

We shouldn't assume that the pathInfo is always what's left over from the context-path.
We shouldn't even assume that the context-path is the first portion of the path.
The truth comes from the combination of the PathSpec and the resulting MatchedPath produced from the canonical path.

What if someone has the following RegexPathSpec ?

pathMappingsHandler.addMapping(new RegexPathSpec("^/rest/(?<name>.*)/ver-[0-9]+/(?<info>.*)$"), new ContextDumpHandler());

That will return on the input path /rest/api/users/ver-1/groupfoo/baruser :

• contextPath = api/users
• pathInContext = groupfoo/baruser

I added this example to the testcases in this PR.

[gregw]

@joakime

That will return on the input path /rest/api/users/ver-1/groupfoo/baruser :

• contextPath = api/users
• pathInContext = groupfoo/baruser

I added this example to the testcases in this PR

I think that breaks a fundamental invariant that we have (and probably should better document): HttpURI.path == contextPath + pathInContext.
This is a subset of the servlet spec invariant that HttpURI = contextPath + servletPath + pathInfo.

If we do not respect these invariants, then things will break. So we cannot use RegEx matching to determine a contextPath unless it is in the PREFIX group and meets the invariant.

[gregw]

I now definitely think we should make this optional. It may be that a PathMappingsHandler is being used to map to "contexts"... in which case this request/context wrapping is good. But it might also be used within a context, so that makes no sense.

This should either be optional behaviour on this handler or moved to a subclass PathMappingsHandler.ContextMapping