Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #7748 - Backport of allow override of path mapping behavior in ServletContextHandler #7834

Merged
merged 1 commit into from Apr 6, 2022

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Apr 4, 2022

backport and fix to 9.4 for #7748

@gregw gregw requested a review from joakime April 4, 2022 19:09
@joakime joakime changed the title Jetty 9.4.x 7748 non servlet path specs Issue #7748 - Backport of allow override of path mapping behavior in ServletContextHandler Apr 4, 2022
@joakime joakime added Enhancement Sponsored This issue affects a user with a commercial support agreement labels Apr 4, 2022
@joakime joakime added this to In progress in Jetty 9.4.47 - 🧊 FROZEN 🥶 via automation Apr 4, 2022
@joakime
Copy link
Contributor

joakime commented Apr 4, 2022

[INFO] --- license-maven-plugin:4.1:check (check-java-headers) @ jetty-project ---
[INFO] Checking licenses...
[WARNING] Missing header in: /home/jenkins/agent/workspace/jetty.project_PR-7834/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RegressionServletTest.java
[WARNING] Missing header in: /home/jenkins/agent/workspace/jetty.project_PR-7834/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RegexServletTest.java

Keep in mind that Jetty 9.4.x has the old EPL 1.0 license headers.

@@ -140,7 +140,7 @@ public String getPathMatch(String path)
Matcher matcher = getMatcher(path);
if (matcher.matches())
{
if (matcher.groupCount() >= 1)
if (_group == PathSpecGroup.PREFIX_GLOB && matcher.groupCount() >= 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change?
This change is not present in 10 & 11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lachlan-roberts my intention is to move this change forward into 10 & 11.
I believe it is needed to make the methods getPathMatch and getPathInfo symmetric.

Specifically I would expect either pathMatch==/test && pathInfo==/info or pathMatch==/test/info && pathInfo==null, but never pathMatch==/test && pathInfo==null, since that loses information. Currently we get the later behavior!

So I have added the following test.

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

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

Note that this issue was not apparent in 10 & 11, because they do not use these methods to set the servletPath and pathInfo, but rather work it out differently (I'll check exactly why when I port this change forward).

Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from In progress to Reviewer approved Apr 5, 2022
lachlan-roberts
lachlan-roberts previously approved these changes Apr 5, 2022
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UriTemplatePathSpec has the same issue with the difference in behavior.

Example path spec ... /{first}/static/{other}

Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Review in progress Apr 5, 2022
+ Backport of #7748
+ Fix RegexPathSpec pathInfo
+ Fix UriTemplatePathSpec pathInfo
+ Test regression option to 93 behaviour

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw force-pushed the jetty-9.4.x-7748-non-servlet-pathSpecs branch from 2a56929 to 63baa1a Compare April 5, 2022 15:24
@gregw
Copy link
Contributor Author

gregw commented Apr 5, 2022

I just did a force push to squash this to 1 commit so I can start work on porting forward to 10, 11 etc.
yeah I know I always say don't do force pushes.... do as I say and not as I do :)

@gregw
Copy link
Contributor Author

gregw commented Apr 6, 2022

@joakime @lachlan-roberts review please

Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Apr 6, 2022
@gregw gregw merged commit 41ba531 into jetty-9.4.x Apr 6, 2022
Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Done Apr 6, 2022
@gregw gregw deleted the jetty-9.4.x-7748-non-servlet-pathSpecs branch April 6, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Allow overriding of url-pattern mapping in ServletContextHandler to allow for regex or uri-template matching
3 participants