jetty.project icon indicating copy to clipboard operation
jetty.project copied to clipboard

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

Open joakime opened this issue 1 year ago • 5 comments

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

Fixes: #11494

joakime avatar Mar 07 '24 15:03 joakime

@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 avatar Mar 09 '24 09:03 gregw

@joakime thoughts?

gregw avatar Mar 11 '24 17:03 gregw

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.

joakime avatar Mar 11 '24 17:03 joakime

@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 avatar Mar 11 '24 17:03 gregw

I've tagged this for 12.1.x. Please rebase.

gregw avatar Aug 22 '24 23:08 gregw