Added test for Regex mapping for Servlet

[gregw]

• needed to open up ServletHandler to allow regex mappings

Signed-off-by: Greg Wilkins [email protected]

[joakime]

@gregw are you thinking of this as an alternate proposal over PR #5854 ?

[gregw]

@gregw are you thinking of this as an alternate proposal over PR #5854 ?

@joakime not specifically. I had observed recent client conversation about regex matching for servlets and knew that we had regex capability in our matcher. So I just wanted to test what would be involved to use that capability and it turns out that with a simple change to ServletHandler this is at least possible for some trivial cases. I had forgotten about #5854 and will have to look again to see what it is doing that this PR does not achieve (as it touches a lot more files).

So this PR really is just a thought bubble for consideration.... but it does indicate that a very trivial change with zero risk may enable some extra behaviors.

[joakime]

@gregw I updated this PR and cleaned up the testcases.
Since you cannot approve your own PR in github, I'm just going to ask.
Are you good with merging this? (I already approved it)

[gregw]

@joakime good to merge!

[gregw]

@joakime I'm not sure tagging PRs with merge-12-needed is the right thing to do, as the PRs get closed. We need to tag the associated issue, which can be left open.

[joakime]

The PR contains the commits and eventual merge commit, those are what we want to ensure make it forward to jetty 12.

[gregw]

@joakime I'm back porting this to 9.4, which doesn't have the org.eclipse.jetty.http.pathmap.PathMappings#getMatch method. Instead it use the getPathMatch and getPathInfo methods.

Unfortunately these methods are not symmetric, and I'm wondering if you know why?
For example I would expect the following test to pass:

    @Test
    public void testMiddleSpecPaths()
    {
        RegexPathSpec spec = new RegexPathSpec("^/[Tt]est(/.*)?$");
        assertTrue(spec.matches("/test/info"));
        assertThat(spec.getPathMatch("/test/info"), equalTo("/test"));
        assertThat(spec.getPathInfo("/test/info"), equalTo("/info"));
    }

But it fails because the pathinfo returned is null??? I would expect either pathMatch==/test && pathInfo==/info or pathMatch==/test/info && pathInfo==null, but not pathMatch==/test && pathInfo==null, since that loses information.

If you look at the implementations, they are not symmetric: getPathMatch is conditional on matcher.groupCount() >= 1, whilst getPathInfo is conditional on PathSpecGroup.PREFIX_GLOB. Surely these should be the same test so both methods will split the path the same way?

Do you have any idea why it is like this? What might I break if I fix this?

[joakime]

The pattern ^/[Tt]est(/.*)?$ is interpreted as /*test/* which isn't a prefix pattern.
That's more of a middle pattern. The internal signature is detected as glllg which becomes PathSpecGroup.MIDDLE_GLOB, which cannot have a pathInfo.

If you want case insensitive, then we should perhaps open up regex flags to the new RegexPathSpec(String regex, int flags), so we can use new RegexPathSpec("^/test(/.*)?$", Pattern.CASE_INSENSITIVE) style of declaration?

[gregw]

@joakime I agree that it is a middle glob pattern. My issue is that the pathMatch and pathInfo are not implemented the same way. PathInfo tests for PREFIX, but pathMatch only cares that there are match groups.
In my PR I have made them both conditional on PREFIX_GLOB. I just want to know if there was a reason to implement them in a way that broke the invariant of path == contextPath + servletPath + pathInfo