Jetty 12 - request.getContext().getContextPath() should return full context, not just last one

[joakime]

Jetty version(s)
12

Description
The request.getContext().getContextPath() only returns the last context seen, not the full context path up to this point.

    @Test
    public void testNestedThreeDeepContextHandler() throws Exception
    {
        ContextHandler contextHandlerA = new ContextHandler();
        contextHandlerA.setContextPath("/a");
        ContextHandler contextHandlerB = new ContextHandler();
        contextHandlerB.setContextPath("/b");
        ContextHandler contextHandlerC = new ContextHandler();
        contextHandlerC.setContextPath("/c");

        contextHandlerA.setHandler(contextHandlerB);
        contextHandlerB.setHandler(contextHandlerC);
        contextHandlerC.setHandler(new Handler.Processor()
        {
            @Override
            public void process(Request request, Response response, Callback callback)
            {
                response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8");
                String msg = "contextPath=" + request.getContext().getContextPath();
                response.write(true, BufferUtil.toBuffer(msg), callback);
            }
        });

        startServer(contextHandlerA);

        String rawRequest = """
            GET /a/b/c/d HTTP/1.1\r
            Host: local\r
            Connection: close\r
                        
            """;
        HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(rawRequest));
        assertEquals(HttpStatus.OK_200, response.getStatus());
        assertEquals("contextPath=/a/b/c", response.getContent());
    }

In this scenario, where we have 3 ContextHandler nested inside of each other, the final Handler only sees the last contextPath.
(The assertion fails, as it expected /a/b/c but only got /c)

This makes it hard to rewrite / redirect / change the URI without the knowledge of the full context.
(for example, putting a RewriteHandler after a ContextHandler)

[sbordet]

Uhm. From the point of view of the last Handler, the contextPath is just /c.

The question is what does request.getPathInContext() return? I'd expect just /d.

And I expect request.getHttpURI().getPath() to be /a/b/c/d.

I would expect to receive a contextPath=/a/b/c if: contextHandler = new ContextHandler("/a/b/c") (not sure if that is even possible; must contextPath be a single segment only?).

[joakime]

The question is what does request.getPathInContext() return? I'd expect just /d.

And I expect request.getHttpURI().getPath() to be /a/b/c/d.

Using the original setup of /a -> /b -> /c -> handler and requesting /a/b/c/d
The results are ...

contextPath=/c
pathInContext=/d
httpURI.getPath=/a/b/c/d

[gregw]

This is kind of a very strange setup... but let's go with it.

The request.getContext().getContextPath() should definitely be only "/c", as that is the context and it is conceivable that there are multiple paths a request can take to get there (although we don't really support setting them up, but then this is a strange edge case).

Perhaps a new method request.getFullContextPath() is needed that would return "/a/b/c", as that is the full context of the request. I'm just not sure there is enough of a use-case to support the expense of maintaining this.... although I guess we could use the request wrappers to peal back the contexts and construct this path only if requested. But then it will be hard for any handler to know if it should use the cheap getContext().getContextPath() or the more expensive request.getFullContextPath()? Hmm let me ponder....

[joakime]

Lets not go down this path just yet.
We need to better understand the implications of things like RewriteHandler under a ContextHandler first.
The differences in modifying and overriding/wrapping the request.getHttpURI() (like RewriteHandler does) vs the whole request.getPathInContext() behaviors inside of (and outside of) a ContextHandler)

[joakime]

A related question, If we had a setup of /a -> /c -> /c -> MyHandler (yes, that's intentionally two nested /c contexts)

how could MyHandler know the proper Context Path to issue a redirect with/to?

[gregw]

So I think we can support this without too much expense with the following method on Request:

    default String getContextPath()
    {
        Context context = getContext();
        return context == null ? null : context.getContextPath();
    }

which we would implement in Request.Wrapper as:

        @Override
        public String getContextPath()
        {
            Context context = getContext();
            if (context == null)
                return null;

            Request.Wrapper wrapper = this;

            String contextPath = context.getContextPath();

            while (wrapper != null)
            {
                Request wrapped = wrapper.getWrapped();

                Context outer = wrapped.getContext();
                if (context != outer)
                {
                    if (outer == null || outer instanceof Server.ServerContext)
                        return contextPath;
                    contextPath = URIUtil.addPaths(outer.getContextPath(), contextPath);
                    context = outer;
                }

                wrapper = wrapped instanceof Request.Wrapper w ? w : null;
            }
            return contextPath;
        }

Which for the simple case of a single Context over the root Context would devolve to the moderately cheap:

        @Override
        public String getContextPath()
        {
            Context context = getContext();
            Request.Wrapper wrapper = this;
            String contextPath = context.getContextPath();
            Context outer = wrapper.getWrapped().getContext();
             return (outer instanceof Server.ServerContext) ? contextPath : null;
        }

So we could do it.

@joakime can you test this with your example?

[gregw]

Note that we'd need to be explicit in the javadoc about the difference between request.getContextPath() and request.getContext().getContextPath(), although for 99.99% of apps, they will produce the same result.

[joakime]

@joakime can you test this with your example?

Where do you see that last snippet getContextPath() (with the Request.Wrapper wrapper = this) going?
The ContextHandler.Context doesn't fit.

[gregw]

@joakime that last snippet is not really code.... that is what the full impl devolves down to in the common case of a single context. I was trying to demonstrate how it would be rather cheap unless we did have nested contexts.

[joakime]

@gregw the code in PR #8751 (currently draft, and doesn't compile) has your changes and the updated test case from the initial comment on this issue.

[joakime]

@joakime that last snippet is not really code....

Reverted that change...

So request.getContextPath() is now /a/b/c, and request.getContext().getContextPath() is now /c with these changes.

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[joakime]

Closing as PR #8758 addressed this