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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Apr 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

{
int idx = matcher.start(1);
if (idx > 0)
Expand Down
2 changes: 1 addition & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,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)
{
int idx = matcher.start(1);
if (idx > 0)
Expand Down
16 changes: 16 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class RegexPathSpecTest
{
Expand Down Expand Up @@ -76,6 +78,20 @@ public void testMiddleSpec()
assertNotMatches(spec, "/rest/list");
}

@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());
}

@Test
public void testMiddleSpecNoGrouping()
{
Expand Down
16 changes: 16 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

/**
* Tests for URI Template Path Specs
Expand Down Expand Up @@ -147,6 +149,20 @@ public void testMiddleVarPathSpec()
assertEquals("b", mapped.get("var"), "Spec.pathParams[var]");
}

@Test
public void testPathInfo()
{
UriTemplatePathSpec spec = new UriTemplatePathSpec("/test/{var}");
assertTrue(spec.matches("/test/info"));
assertThat(spec.getPathMatch("/test/info"), equalTo("/test"));
assertThat(spec.getPathInfo("/test/info"), equalTo("info"));

spec = new UriTemplatePathSpec("/{x}/test/{y}");
assertTrue(spec.matches("/try/test/info"));
assertThat(spec.getPathMatch("/try/test/info"), equalTo("/try/test/info"));
assertThat(spec.getPathInfo("/try/test/info"), nullValue());
}

@Test
public void testOneVarPathSpec()
{
Expand Down
18 changes: 12 additions & 6 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ protected synchronized void doStop()
}
}
else
servletHolders.add(_servlets[i]); //only retain embedded
servletHolders.add(_servlets[i]); //only retain embedded
}
}

Expand All @@ -314,7 +314,7 @@ protected synchronized void doStop()
ServletMapping[] sms = servletMappings.toArray(new ServletMapping[0]);
updateBeans(_servletMappings, sms);
_servletMappings = sms;

if (_contextHandler != null)
_contextHandler.contextDestroyed();

Expand Down Expand Up @@ -1151,7 +1151,7 @@ public void addFilterMapping(FilterMapping mapping)
else
{
//there are existing entries. If this is a programmatic filtermapping, it is added at the end of the list.
//If this is a normal filtermapping, it is inserted after all the other filtermappings (matchBefores and normals),
//If this is a normal filtermapping, it is inserted after all the other filtermappings (matchBefores and normals),
//but before the first matchAfter filtermapping.
if (source == Source.JAVAX_API)
{
Expand Down Expand Up @@ -1299,7 +1299,13 @@ protected synchronized void updateNameMappings()
}
}

protected synchronized void updateMappings()
protected PathSpec asPathSpec(String pathSpec)
{
// By default only allow servlet path specs
return new ServletPathSpec(pathSpec);
}

protected void updateMappings()
{
// update filter mappings
if (_filterMappings == null)
Expand Down Expand Up @@ -1388,7 +1394,7 @@ protected synchronized void updateMappings()
finalMapping = mapping;
else
{
//already have a candidate - only accept another one
//already have a candidate - only accept another one
//if the candidate is a default, or we're allowing duplicate mappings
if (finalMapping.isDefault())
finalMapping = mapping;
Expand Down Expand Up @@ -1421,7 +1427,7 @@ else if (isAllowDuplicateMappings())
finalMapping.getServletName(),
_servletNameMap.get(finalMapping.getServletName()).getSource());

pm.put(new ServletPathSpec(pathSpec), _servletNameMap.get(finalMapping.getServletName()));
pm.put(asPathSpec(pathSpec), _servletNameMap.get(finalMapping.getServletName()));
}

_servletPathMap = pm;
Expand Down