From c3e000485971a323f7439be661dcb8f028d13cbe Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 20 Oct 2022 10:26:37 -0500 Subject: [PATCH 1/9] Introduce PathMappingsHandler --- .../server/handler/PathMappingsHandler.java | 60 +++++ .../handler/PathMappingsHandlerTest.java | 242 ++++++++++++++++++ 2 files changed, 302 insertions(+) create mode 100644 jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java create mode 100644 jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java new file mode 100644 index 000000000000..3b91e7ec204b --- /dev/null +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -0,0 +1,60 @@ +// +// ======================================================================== +// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.server.handler; + +import org.eclipse.jetty.http.pathmap.MatchedResource; +import org.eclipse.jetty.http.pathmap.PathMappings; +import org.eclipse.jetty.http.pathmap.PathSpec; +import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.Request; + +/** + * A Handler that delegates to other handlers through a configured {@link PathMappings}. + */ +public class PathMappingsHandler extends Handler.Wrapper +{ + private final PathMappings mappings = new PathMappings<>(); + + public void addMapping(PathSpec pathSpec, Handler handler) + { + if (isStarted()) + throw new IllegalStateException("Cannot add mapping: " + this); + + mappings.put(pathSpec, handler); + } + + @Override + protected void doStart() throws Exception + { + mappings.getMappings().forEach((mapped) -> addBean(mapped.getResource())); + super.doStart(); + } + + @Override + protected void doStop() throws Exception + { + super.doStop(); + mappings.getMappings().forEach((mapped) -> removeBean(mapped.getResource())); + } + + @Override + public Request.Processor handle(Request request) throws Exception + { + String pathInContext = request.getPathInContext(); + MatchedResource matchedResource = mappings.getMatched(pathInContext); + if (matchedResource == null) + return super.handle(request); + return matchedResource.getResource().handle(request); + } +} diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java new file mode 100644 index 000000000000..55307cc6e483 --- /dev/null +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java @@ -0,0 +1,242 @@ +// +// ======================================================================== +// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.server.handler; + +import java.nio.charset.StandardCharsets; +import java.util.stream.Stream; + +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.http.pathmap.ServletPathSpec; +import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.LocalConnector; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Response; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class PathMappingsHandlerTest +{ + private Server server; + private LocalConnector connector; + + public void startServer(Handler handler) throws Exception + { + server = new Server(); + connector = new LocalConnector(server); + server.addConnector(connector); + + server.addHandler(handler); + server.start(); + } + + @AfterEach + public void stopServer() + { + LifeCycle.stop(server); + } + + public HttpTester.Response executeRequest(String rawRequest) throws Exception + { + String rawResponse = connector.getResponse(rawRequest); + return HttpTester.parseResponse(rawResponse); + } + + /** + * Test where there are no mappings, and no wrapper. + */ + @Test + public void testEmpty() throws Exception + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + contextHandler.setHandler(pathMappingsHandler); + + startServer(contextHandler); + + HttpTester.Response response = executeRequest(""" + GET / HTTP/1.1\r + Host: local\r + Connection: close\r + + """); + assertEquals(HttpStatus.NOT_FOUND_404, response.getStatus()); + } + + /** + * Test where there are no mappings, and only a wrapper. + */ + @Test + public void testOnlyWrapper() throws Exception + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + pathMappingsHandler.setHandler(new SimpleHandler("WrapperFoo Hit")); + contextHandler.setHandler(pathMappingsHandler); + + startServer(contextHandler); + + HttpTester.Response response = executeRequest(""" + GET / HTTP/1.1\r + Host: local\r + Connection: close\r + + """); + assertEquals(HttpStatus.OK_200, response.getStatus()); + assertEquals("WrapperFoo Hit", response.getContent()); + } + + /** + * Test where there is only a single mapping, and no wrapper. + */ + @Test + public void testOnlyMappingSuffix() throws Exception + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("PhpExample Hit")); + contextHandler.setHandler(pathMappingsHandler); + + startServer(contextHandler); + + HttpTester.Response response = executeRequest(""" + GET /hello HTTP/1.1\r + Host: local\r + Connection: close\r + + """); + assertEquals(HttpStatus.NOT_FOUND_404, response.getStatus()); + + response = executeRequest(""" + GET /hello.php HTTP/1.1\r + Host: local\r + Connection: close\r + + """); + assertEquals(HttpStatus.OK_200, response.getStatus()); + assertEquals("PhpExample Hit", response.getContent()); + } + + /** + * Test where there is only a single mapping, and a wrapper for fallback. + */ + @Test + public void testOneMappingAndWrapper() throws Exception + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + pathMappingsHandler.setHandler(new SimpleHandler("WrapperFoo Hit")); + pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("PhpExample Hit")); + contextHandler.setHandler(pathMappingsHandler); + + startServer(contextHandler); + + HttpTester.Response response = executeRequest(""" + GET /hello HTTP/1.1\r + Host: local\r + Connection: close\r + + """); + assertEquals(HttpStatus.OK_200, response.getStatus()); + assertEquals("WrapperFoo Hit", response.getContent()); + + response = executeRequest(""" + GET /hello.php HTTP/1.1\r + Host: local\r + Connection: close\r + + """); + assertEquals(HttpStatus.OK_200, response.getStatus()); + assertEquals("PhpExample Hit", response.getContent()); + } + + public static Stream severalMappingsInput() + { + return Stream.of( + Arguments.of("/hello", HttpStatus.OK_200, "FakeResourceHandler Hit"), + Arguments.of("/index.html", HttpStatus.OK_200, "FakeSpecificStaticHandler Hit"), + Arguments.of("/index.php", HttpStatus.OK_200, "PhpHandler Hit"), + Arguments.of("/config.php", HttpStatus.OK_200, "PhpHandler Hit"), + Arguments.of("/css/main.css", HttpStatus.OK_200, "FakeResourceHandler Hit") + ); + } + + /** + * Test where there are a few mappings, with a root mapping, and no wrapper. + * This means the wrapper would not ever be hit, as all inputs would match at + * least 1 mapping. + */ + @ParameterizedTest + @MethodSource("severalMappingsInput") + public void testSeveralMappingAndNoWrapper(String requestPath, int expectedStatus, String expectedResponseBody) throws Exception + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + pathMappingsHandler.addMapping(new ServletPathSpec("/"), new SimpleHandler("FakeResourceHandler Hit")); + pathMappingsHandler.addMapping(new ServletPathSpec("/index.html"), new SimpleHandler("FakeSpecificStaticHandler Hit")); + pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("PhpHandler Hit")); + contextHandler.setHandler(pathMappingsHandler); + + startServer(contextHandler); + + HttpTester.Response response = executeRequest(""" + GET %s HTTP/1.1\r + Host: local\r + Connection: close\r + + """.formatted(requestPath)); + assertEquals(expectedStatus, response.getStatus()); + assertEquals(expectedResponseBody, response.getContent()); + } + + private static class SimpleHandler extends Handler.Processor + { + private final String message; + + public SimpleHandler(String message) + { + this.message = message; + } + + @Override + public void process(Request request, Response response, Callback callback) + { + assertTrue(isStarted()); + response.setStatus(HttpStatus.OK_200); + response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8"); + response.write(true, BufferUtil.toBuffer(message, StandardCharsets.UTF_8), callback); + } + } +} From 333634e41614e62afe14a5be1de9c077d60b83d6 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 20 Oct 2022 12:06:33 -0500 Subject: [PATCH 2/9] Adding DEBUG --- .../jetty/server/handler/PathMappingsHandler.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java index 3b91e7ec204b..ba2dede39184 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -18,12 +18,15 @@ import org.eclipse.jetty.http.pathmap.PathSpec; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A Handler that delegates to other handlers through a configured {@link PathMappings}. */ public class PathMappingsHandler extends Handler.Wrapper { + private final static Logger LOG = LoggerFactory.getLogger(PathMappingsHandler.class); private final PathMappings mappings = new PathMappings<>(); public void addMapping(PathSpec pathSpec, Handler handler) @@ -54,7 +57,13 @@ public Request.Processor handle(Request request) throws Exception String pathInContext = request.getPathInContext(); MatchedResource matchedResource = mappings.getMatched(pathInContext); if (matchedResource == null) + { + if (LOG.isDebugEnabled()) + LOG.debug("No match on pathInContext of {}", pathInContext); return super.handle(request); + } + if (LOG.isDebugEnabled()) + LOG.debug("Matched pathInContext of {} to {} -> {}", pathInContext, matchedResource.getPathSpec(), matchedResource.getResource()); return matchedResource.getResource().handle(request); } } From edf1bf3b9f15ed5c2fceb70a659d0e05b1c7105f Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 20 Oct 2022 12:13:34 -0500 Subject: [PATCH 3/9] Checkstyle fix --- .../org/eclipse/jetty/server/handler/PathMappingsHandler.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java index ba2dede39184..000ff0258f5f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -26,7 +26,8 @@ */ public class PathMappingsHandler extends Handler.Wrapper { - private final static Logger LOG = LoggerFactory.getLogger(PathMappingsHandler.class); + private static final Logger LOG = LoggerFactory.getLogger(PathMappingsHandler.class); + private final PathMappings mappings = new PathMappings<>(); public void addMapping(PathSpec pathSpec, Handler handler) From 2f10a67eeec8012ac2179331b11882ac07844376 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 24 Oct 2022 10:17:59 -0500 Subject: [PATCH 4/9] Fixing dump of pathMappings & start/stop logic --- .../server/handler/PathMappingsHandler.java | 26 +++++++++++++++-- .../handler/PathMappingsHandlerTest.java | 28 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java index 000ff0258f5f..23cd3e7f3f2b 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -13,17 +13,23 @@ package org.eclipse.jetty.server.handler; +import java.io.IOException; + +import org.eclipse.jetty.http.pathmap.MappedResource; import org.eclipse.jetty.http.pathmap.MatchedResource; import org.eclipse.jetty.http.pathmap.PathMappings; import org.eclipse.jetty.http.pathmap.PathSpec; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.util.component.Dumpable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** * A Handler that delegates to other handlers through a configured {@link PathMappings}. */ + public class PathMappingsHandler extends Handler.Wrapper { private static final Logger LOG = LoggerFactory.getLogger(PathMappingsHandler.class); @@ -38,10 +44,22 @@ public void addMapping(PathSpec pathSpec, Handler handler) mappings.put(pathSpec, handler); } + @Override + public void dump(Appendable out, String indent) throws IOException + { + Dumpable.dumpObjects(out, indent, this, mappings); + } + @Override protected void doStart() throws Exception { - mappings.getMappings().forEach((mapped) -> addBean(mapped.getResource())); + Server server = getServer(); + for (MappedResource matchedResource : mappings.getMappings()) + { + Handler handler = matchedResource.getResource(); + handler.setServer(server); + handler.start(); + } super.doStart(); } @@ -49,7 +67,11 @@ protected void doStart() throws Exception protected void doStop() throws Exception { super.doStop(); - mappings.getMappings().forEach((mapped) -> removeBean(mapped.getResource())); + for (MappedResource matchedResource : mappings.getMappings()) + { + Handler handler = matchedResource.getResource(); + handler.stop(); + } } @Override diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java index 55307cc6e483..3ef6fdb86b4c 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java @@ -34,6 +34,8 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -221,6 +223,26 @@ public void testSeveralMappingAndNoWrapper(String requestPath, int expectedStatu assertEquals(expectedResponseBody, response.getContent()); } + @Test + public void testDump() throws Exception + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + pathMappingsHandler.addMapping(new ServletPathSpec("/"), new SimpleHandler("FakeResourceHandler Hit")); + pathMappingsHandler.addMapping(new ServletPathSpec("/index.html"), new SimpleHandler("FakeSpecificStaticHandler Hit")); + pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("PhpHandler Hit")); + contextHandler.setHandler(pathMappingsHandler); + + startServer(contextHandler); + + String dump = contextHandler.dump(); + assertThat(dump, containsString("FakeResourceHandler")); + assertThat(dump, containsString("FakeSpecificStaticHandler")); + assertThat(dump, containsString("PhpHandler")); + } + private static class SimpleHandler extends Handler.Processor { private final String message; @@ -238,5 +260,11 @@ public void process(Request request, Response response, Callback callback) response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8"); response.write(true, BufferUtil.toBuffer(message, StandardCharsets.UTF_8), callback); } + + @Override + public String toString() + { + return String.format("%s[msg=\"%s\"]", SimpleHandler.class.getSimpleName(), message); + } } } From a52e47bcc12f2e3a3fde51f442a1125d9a9c165d Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 24 Oct 2022 12:04:06 -0500 Subject: [PATCH 5/9] Changed from Handler.Wrapper to Handler.Abstract --- .../server/handler/PathMappingsHandler.java | 4 +- .../handler/PathMappingsHandlerTest.java | 60 ------------------- 2 files changed, 2 insertions(+), 62 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java index 23cd3e7f3f2b..46a6e3d0422f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -30,7 +30,7 @@ * A Handler that delegates to other handlers through a configured {@link PathMappings}. */ -public class PathMappingsHandler extends Handler.Wrapper +public class PathMappingsHandler extends Handler.Abstract { private static final Logger LOG = LoggerFactory.getLogger(PathMappingsHandler.class); @@ -83,7 +83,7 @@ public Request.Processor handle(Request request) throws Exception { if (LOG.isDebugEnabled()) LOG.debug("No match on pathInContext of {}", pathInContext); - return super.handle(request); + return null; } if (LOG.isDebugEnabled()) LOG.debug("Matched pathInContext of {} to {} -> {}", pathInContext, matchedResource.getPathSpec(), matchedResource.getResource()); diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java index 3ef6fdb86b4c..43123ecfc6ba 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java @@ -89,31 +89,6 @@ public void testEmpty() throws Exception assertEquals(HttpStatus.NOT_FOUND_404, response.getStatus()); } - /** - * Test where there are no mappings, and only a wrapper. - */ - @Test - public void testOnlyWrapper() throws Exception - { - ContextHandler contextHandler = new ContextHandler(); - contextHandler.setContextPath("/"); - - PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); - pathMappingsHandler.setHandler(new SimpleHandler("WrapperFoo Hit")); - contextHandler.setHandler(pathMappingsHandler); - - startServer(contextHandler); - - HttpTester.Response response = executeRequest(""" - GET / HTTP/1.1\r - Host: local\r - Connection: close\r - - """); - assertEquals(HttpStatus.OK_200, response.getStatus()); - assertEquals("WrapperFoo Hit", response.getContent()); - } - /** * Test where there is only a single mapping, and no wrapper. */ @@ -147,41 +122,6 @@ public void testOnlyMappingSuffix() throws Exception assertEquals("PhpExample Hit", response.getContent()); } - /** - * Test where there is only a single mapping, and a wrapper for fallback. - */ - @Test - public void testOneMappingAndWrapper() throws Exception - { - ContextHandler contextHandler = new ContextHandler(); - contextHandler.setContextPath("/"); - - PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); - pathMappingsHandler.setHandler(new SimpleHandler("WrapperFoo Hit")); - pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("PhpExample Hit")); - contextHandler.setHandler(pathMappingsHandler); - - startServer(contextHandler); - - HttpTester.Response response = executeRequest(""" - GET /hello HTTP/1.1\r - Host: local\r - Connection: close\r - - """); - assertEquals(HttpStatus.OK_200, response.getStatus()); - assertEquals("WrapperFoo Hit", response.getContent()); - - response = executeRequest(""" - GET /hello.php HTTP/1.1\r - Host: local\r - Connection: close\r - - """); - assertEquals(HttpStatus.OK_200, response.getStatus()); - assertEquals("PhpExample Hit", response.getContent()); - } - public static Stream severalMappingsInput() { return Stream.of( From 67e7f0eea2d360f784f1d6ba19e49a1c60c9d613 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 24 Oct 2022 16:27:37 -0500 Subject: [PATCH 6/9] Adding missing tests for setHandler/addHandler loop detection --- .../server/handler/ContextHandlerTest.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java index 733a6bb0433b..e742c4ba9d79 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java @@ -60,6 +60,7 @@ import static org.hamcrest.Matchers.sameInstance; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; public class ContextHandlerTest @@ -586,6 +587,31 @@ public void process(Request request, Response response, Callback callback) assertThat(result.get(), equalTo("OK")); } + @Test + public void testSetHandlerLoopSelf() + { + ContextHandler contextHandlerA = new ContextHandler(); + assertThrows(IllegalStateException.class, () -> contextHandlerA.setHandler(contextHandlerA)); + } + + @Test + public void testSetHandlerLoopDeepWrapper() + { + ContextHandler contextHandlerA = new ContextHandler(); + Handler.Wrapper handlerWrapper = new Handler.Wrapper(); + contextHandlerA.setHandler(handlerWrapper); + assertThrows(IllegalStateException.class, () -> handlerWrapper.setHandler(contextHandlerA)); + } + + @Test + public void testAddHandlerLoopDeep() + { + ContextHandler contextHandlerA = new ContextHandler(); + Handler.Collection handlerCollection = new Handler.Collection(); + contextHandlerA.setHandler(handlerCollection); + assertThrows(IllegalStateException.class, () -> handlerCollection.addHandler(contextHandlerA)); + } + private static class ScopeListener implements ContextHandler.ContextScopeListener { private static final Request NULL = new Request.Wrapper(null); From 788a02a45bf82ccd991262c0502e06d48d8a19a5 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 24 Oct 2022 16:27:58 -0500 Subject: [PATCH 7/9] Add PathMappingsHandler.addMapping() loop detection & tests --- .../jetty/http/pathmap/PathMappings.java | 6 ++ .../server/handler/PathMappingsHandler.java | 50 ++++++++++++- .../handler/PathMappingsHandlerTest.java | 71 +++++++++++++++++++ 3 files changed, 125 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java index 8664fbf3896c..bd339006fc36 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java @@ -21,6 +21,7 @@ import java.util.Set; import java.util.TreeSet; import java.util.function.Predicate; +import java.util.stream.Stream; import org.eclipse.jetty.util.Index; import org.eclipse.jetty.util.annotation.ManagedAttribute; @@ -88,6 +89,11 @@ public void reset() _suffixMap.clear(); } + public Stream> streamResources() + { + return _mappings.stream(); + } + public void removeIf(Predicate> predicate) { _mappings.removeIf(predicate); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java index 46a6e3d0422f..13d84dbb43e7 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -14,6 +14,8 @@ package org.eclipse.jetty.server.handler; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import org.eclipse.jetty.http.pathmap.MappedResource; import org.eclipse.jetty.http.pathmap.MatchedResource; @@ -30,17 +32,61 @@ * A Handler that delegates to other handlers through a configured {@link PathMappings}. */ -public class PathMappingsHandler extends Handler.Abstract +public class PathMappingsHandler extends Handler.AbstractContainer { private static final Logger LOG = LoggerFactory.getLogger(PathMappingsHandler.class); private final PathMappings mappings = new PathMappings<>(); + @Override + public void addHandler(Handler handler) + { + throw new IllegalArgumentException("Arbitrary addHandler() not supported, use addMapping() instead"); + } + + @Override + public List getHandlers() + { + return mappings.streamResources().map(MappedResource::getResource).toList(); + } + + @Override + public List getDescendants() + { + List descendants = new ArrayList<>(); + for (MappedResource entry : mappings) + { + Handler entryHandler = entry.getResource(); + descendants.add(entryHandler); + + if (entryHandler instanceof Handler.Container container) + { + descendants.addAll(container.getDescendants()); + } + } + return descendants; + } + public void addMapping(PathSpec pathSpec, Handler handler) { if (isStarted()) throw new IllegalStateException("Cannot add mapping: " + this); + // check that self isn't present + if (handler == this || handler instanceof Handler.Container container && container.getDescendants().contains(this)) + throw new IllegalStateException("Unable to addHandler of self: " + handler); + + // check existing mappings + for (MappedResource entry : mappings) + { + Handler entryHandler = entry.getResource(); + + if (entryHandler == this || + entryHandler == handler || + (entryHandler instanceof Handler.Container container && container.getDescendants().contains(this))) + throw new IllegalStateException("addMapping loop detected: " + handler); + } + mappings.put(pathSpec, handler); } @@ -81,7 +127,7 @@ public Request.Processor handle(Request request) throws Exception MatchedResource matchedResource = mappings.getMatched(pathInContext); if (matchedResource == null) { - if (LOG.isDebugEnabled()) + if (LOG.isDebugEnabled()) LOG.debug("No match on pathInContext of {}", pathInContext); return null; } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java index 43123ecfc6ba..e98e38c167ba 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java @@ -14,6 +14,8 @@ package org.eclipse.jetty.server.handler; import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Objects; import java.util.stream.Stream; import org.eclipse.jetty.http.HttpHeader; @@ -35,8 +37,10 @@ import org.junit.jupiter.params.provider.MethodSource; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; public class PathMappingsHandlerTest @@ -183,6 +187,73 @@ public void testDump() throws Exception assertThat(dump, containsString("PhpHandler")); } + @Test + public void testGetDescendantsSimple() + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + pathMappingsHandler.addMapping(new ServletPathSpec("/"), new SimpleHandler("default")); + pathMappingsHandler.addMapping(new ServletPathSpec("/index.html"), new SimpleHandler("specific")); + pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("php")); + + List actualHandlers = pathMappingsHandler.getDescendants().stream().map(Objects::toString).toList(); + + String[] expectedHandlers = { + "SimpleHandler[msg=\"default\"]", + "SimpleHandler[msg=\"specific\"]", + "SimpleHandler[msg=\"php\"]" + }; + assertThat(actualHandlers, containsInAnyOrder(expectedHandlers)); + } + + @Test + public void testGetDescendantsDeep() + { + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/"); + + Handler.Collection handlerCollection = new Handler.Collection(); + handlerCollection.addHandler(new SimpleHandler("phpIndex")); + Handler.Wrapper handlerWrapper = new Handler.Wrapper(new SimpleHandler("other")); + handlerCollection.addHandler(handlerWrapper); + + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + pathMappingsHandler.addMapping(new ServletPathSpec("/"), new SimpleHandler("default")); + pathMappingsHandler.addMapping(new ServletPathSpec("/index.html"), new SimpleHandler("specific")); + pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), handlerCollection); + + List actualHandlers = pathMappingsHandler.getDescendants().stream().map(Objects::toString).toList(); + + String[] expectedHandlers = { + "SimpleHandler[msg=\"default\"]", + "SimpleHandler[msg=\"specific\"]", + handlerCollection.toString(), + handlerWrapper.toString(), + "SimpleHandler[msg=\"phpIndex\"]", + "SimpleHandler[msg=\"other\"]" + }; + assertThat(actualHandlers, containsInAnyOrder(expectedHandlers)); + } + + @Test + public void testAddLoopSelf() + { + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + assertThrows(IllegalStateException.class, () -> pathMappingsHandler.addMapping(new ServletPathSpec("/self"), pathMappingsHandler)); + } + + @Test + public void testAddLoopContext() + { + ContextHandler contextHandler = new ContextHandler(); + PathMappingsHandler pathMappingsHandler = new PathMappingsHandler(); + contextHandler.setHandler(pathMappingsHandler); + + assertThrows(IllegalStateException.class, () -> pathMappingsHandler.addMapping(new ServletPathSpec("/loop"), contextHandler)); + } + private static class SimpleHandler extends Handler.Processor { private final String message; From de10fe9af1657fa3ab7e1d090ff4437eaa3002db Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 24 Oct 2022 16:47:55 -0500 Subject: [PATCH 8/9] Updates from review --- .../server/handler/PathMappingsHandler.java | 47 +++---------------- .../handler/PathMappingsHandlerTest.java | 2 + 2 files changed, 8 insertions(+), 41 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java index 13d84dbb43e7..83018d4ae547 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -14,8 +14,8 @@ package org.eclipse.jetty.server.handler; import java.io.IOException; -import java.util.ArrayList; import java.util.List; +import java.util.function.Supplier; import org.eclipse.jetty.http.pathmap.MappedResource; import org.eclipse.jetty.http.pathmap.MatchedResource; @@ -23,7 +23,6 @@ import org.eclipse.jetty.http.pathmap.PathSpec; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; -import org.eclipse.jetty.server.Server; import org.eclipse.jetty.util.component.Dumpable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -45,26 +44,15 @@ public void addHandler(Handler handler) } @Override - public List getHandlers() + public void addHandler(Supplier supplier) { - return mappings.streamResources().map(MappedResource::getResource).toList(); + throw new IllegalArgumentException("Arbitrary addHandler() not supported, use addMapping() instead"); } @Override - public List getDescendants() + public List getHandlers() { - List descendants = new ArrayList<>(); - for (MappedResource entry : mappings) - { - Handler entryHandler = entry.getResource(); - descendants.add(entryHandler); - - if (entryHandler instanceof Handler.Container container) - { - descendants.addAll(container.getDescendants()); - } - } - return descendants; + return mappings.streamResources().map(MappedResource::getResource).toList(); } public void addMapping(PathSpec pathSpec, Handler handler) @@ -88,6 +76,7 @@ public void addMapping(PathSpec pathSpec, Handler handler) } mappings.put(pathSpec, handler); + addBean(handler); } @Override @@ -96,30 +85,6 @@ public void dump(Appendable out, String indent) throws IOException Dumpable.dumpObjects(out, indent, this, mappings); } - @Override - protected void doStart() throws Exception - { - Server server = getServer(); - for (MappedResource matchedResource : mappings.getMappings()) - { - Handler handler = matchedResource.getResource(); - handler.setServer(server); - handler.start(); - } - super.doStart(); - } - - @Override - protected void doStop() throws Exception - { - super.doStop(); - for (MappedResource matchedResource : mappings.getMappings()) - { - Handler handler = matchedResource.getResource(); - handler.stop(); - } - } - @Override public Request.Processor handle(Request request) throws Exception { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java index e98e38c167ba..95af61ccc32d 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/PathMappingsHandlerTest.java @@ -185,6 +185,8 @@ public void testDump() throws Exception assertThat(dump, containsString("FakeResourceHandler")); assertThat(dump, containsString("FakeSpecificStaticHandler")); assertThat(dump, containsString("PhpHandler")); + assertThat(dump, containsString("PathMappings[size=3]")); + } @Test From ffb7ef6cd5ae1475cf6c6d818aba9baeab291739 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 24 Oct 2022 17:03:57 -0500 Subject: [PATCH 9/9] use UnsupportedOperationException instead --- .../org/eclipse/jetty/server/handler/PathMappingsHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java index 83018d4ae547..3dffc3e3acd3 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/PathMappingsHandler.java @@ -40,13 +40,13 @@ public class PathMappingsHandler extends Handler.AbstractContainer @Override public void addHandler(Handler handler) { - throw new IllegalArgumentException("Arbitrary addHandler() not supported, use addMapping() instead"); + throw new UnsupportedOperationException("Arbitrary addHandler() not supported, use addMapping() instead"); } @Override public void addHandler(Supplier supplier) { - throw new IllegalArgumentException("Arbitrary addHandler() not supported, use addMapping() instead"); + throw new UnsupportedOperationException("Arbitrary addHandler() not supported, use addMapping() instead"); } @Override