From 49b344242b7245e1aaf78ab2748137f0ec70ae13 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 13 Nov 2023 23:13:51 +1100 Subject: [PATCH] Various cleanups of Handler.insertHandler (#10792) * Various cleanups of Handler.insertHandler * Added missing call to relinkHandlers() in setHandler() after calling super. * Moved call to relinkHandlers() in insertHandler(), as the various setSession|Security|ServletHandler() already call relinkHandlers(). Signed-off-by: Simone Bordet --------- Signed-off-by: Simone Bordet Co-authored-by: Simone Bordet --- .../org/eclipse/jetty/server/Handler.java | 10 +-- .../jetty/ee10/security/jaspi/JaspiTest.java | 4 +- .../ee10/servlet/ServletContextHandler.java | 40 ++++++------ .../jetty/ee10/servlet/ServletHandler.java | 19 +++--- .../jetty/ee10/servlet/SessionHandler.java | 62 ++++++++++++++++-- .../security/ConstraintSecurityHandler.java | 4 +- .../servlet/ServletContextHandlerTest.java | 63 ++++++++++++++++--- .../jetty/ee9/nested/ContextHandler.java | 11 ++++ .../jetty/ee9/nested/HandlerWrapper.java | 21 ++++--- .../jetty/ee9/nested/ContextHandlerTest.java | 53 +++++++++++++++- .../ee9/servlet/ServletContextHandler.java | 32 +++++----- .../servlet/ServletContextHandlerTest.java | 30 ++++++--- 12 files changed, 258 insertions(+), 91 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Handler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Handler.java index 27e50ed7ec12..691a741f67ca 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Handler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Handler.java @@ -325,8 +325,8 @@ default List getHandlers() } /** - *

Inserts the given {@code Handler} (and its chain of {@code Handler}s) - * in front of the child of this {@code Handler}.

+ *

Inserts the given {@code Handler} (and possible chain of {@code Handler}s) + * between this {@code Handler} and its current {@link #getHandler() child}. *

For example, if this {@code Handler} {@code A} has a child {@code B}, * inserting {@code Handler} {@code X} built as a chain {@code Handler}s * {@code X-Y-Z} results in the structure {@code A-X-Y-Z-B}.

@@ -335,11 +335,7 @@ default List getHandlers() */ default void insertHandler(Singleton handler) { - Singleton tail = handler; - while (tail.getHandler() instanceof Wrapper) - { - tail = (Wrapper)tail.getHandler(); - } + Singleton tail = handler.getTail(); if (tail.getHandler() != null) throw new IllegalArgumentException("bad tail of inserted wrapper chain"); diff --git a/jetty-ee10/jetty-ee10-jaspi/src/test/java/org/eclipse/jetty/ee10/security/jaspi/JaspiTest.java b/jetty-ee10/jetty-ee10-jaspi/src/test/java/org/eclipse/jetty/ee10/security/jaspi/JaspiTest.java index 6fff50aefa3f..5803a3712cfa 100644 --- a/jetty-ee10/jetty-ee10-jaspi/src/test/java/org/eclipse/jetty/ee10/security/jaspi/JaspiTest.java +++ b/jetty-ee10/jetty-ee10-jaspi/src/test/java/org/eclipse/jetty/ee10/security/jaspi/JaspiTest.java @@ -135,7 +135,7 @@ public void before() throws Exception JaspiAuthenticatorFactory jaspiAuthFactory = new JaspiAuthenticatorFactory(); ConstraintSecurityHandler security = new ConstraintSecurityHandler(); - context.setHandler(security); + context.setSecurityHandler(security); security.setAuthenticatorFactory(jaspiAuthFactory); Constraint constraint = new Constraint.Builder() @@ -152,7 +152,7 @@ public void before() throws Exception other.setContextPath("/other"); other.addServlet(new TestServlet(), "/"); ConstraintSecurityHandler securityOther = new ConstraintSecurityHandler(); - other.setHandler(securityOther); + other.setSecurityHandler(securityOther); securityOther.setAuthenticatorFactory(jaspiAuthFactory); securityOther.addConstraintMapping(mapping); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index 2021f47d47f7..1a9371ff687d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -953,6 +953,7 @@ else if (handler instanceof ServletHandler) if (handler != null) LOG.warn("ServletContextHandler.setHandler should not be called directly. Use insertHandler or setSessionHandler etc."); super.setHandler(handler); + relinkHandlers(); } } @@ -1128,7 +1129,7 @@ protected ServletContextRequest newServletContextRequest(ServletChannel servletC } @Override - protected ServletContextRequest wrapRequest(Request request, Response response) + protected ContextRequest wrapRequest(Request request, Response response) { // Need to ask directly to the Context for the pathInContext, rather than using // Request.getPathInContext(), as the request is not yet wrapped in this Context. @@ -1136,10 +1137,10 @@ protected ServletContextRequest wrapRequest(Request request, Response response) MatchedResource matchedResource = _servletHandler.getMatchedServlet(decodedPathInContext); if (matchedResource == null) - return null; + return wrapNoServlet(request, response); ServletHandler.MappedServlet mappedServlet = matchedResource.getResource(); if (mappedServlet == null) - return null; + return wrapNoServlet(request, response); // Get a servlet request, possibly from a cached version in the channel attributes. Attributes cache = request.getComponents().getCache(); @@ -1161,12 +1162,20 @@ protected ServletContextRequest wrapRequest(Request request, Response response) return servletContextRequest; } + private ContextRequest wrapNoServlet(Request request, Response response) + { + Handler next = getServletHandler().getHandler(); + if (next == null) + return null; + return super.wrapRequest(request, response); + } + @Override protected ContextResponse wrapResponse(ContextRequest request, Response response) { if (request instanceof ServletContextRequest servletContextRequest) return servletContextRequest.getServletContextResponse(); - throw new IllegalArgumentException(); + return super.wrapResponse(request, response); } @Override @@ -1662,26 +1671,17 @@ else if (handler instanceof ServletHandler) setServletHandler((ServletHandler)handler); else { + // We cannot call super.insertHandler here, because it uses this.setHandler + // which sets the servletHandlers next handler. + // This is the same insert code, but uses super.setHandler, which sets this + // handler's next handler. Singleton tail = handler.getTail(); if (tail.getHandler() != null) throw new IllegalArgumentException("bad tail of inserted wrapper chain"); - - // Skip any injected handlers - Singleton h = this; - while (h.getHandler() instanceof Singleton wrapper) - { - if (wrapper instanceof SessionHandler || - wrapper instanceof SecurityHandler || - wrapper instanceof ServletHandler) - break; - h = wrapper; - } - - Handler next = h.getHandler(); - doSetHandler(h, handler); - doSetHandler(tail, next); + tail.setHandler(getHandler()); + super.setHandler(handler); + relinkHandlers(); } - relinkHandlers(); } /** diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java index 8f6e8005f7e9..f92103e7225a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java @@ -452,16 +452,21 @@ protected IdentityService getIdentityService() @Override public boolean handle(Request request, Response response, Callback callback) throws Exception { - // We will always have a ServletContextRequest as we must be within a ServletContextHandler + // We have a ServletContextRequest only when an enclosing ServletContextHandler matched a Servlet ServletChannel servletChannel = Request.get(request, ServletContextRequest.class, ServletContextRequest::getServletChannel); + if (servletChannel != null) + { + if (LOG.isDebugEnabled()) + LOG.debug("handle {} {} {} {} {}", this, servletChannel, request, response, callback); - if (LOG.isDebugEnabled()) - LOG.debug("handle {} {} {} {}", this, request, response, callback); + // But request, response and/or callback may have been wrapped after the ServletContextHandler, so update the channel. + servletChannel.associate(request, response, callback); + servletChannel.handle(); + return true; + } - // But request, response and/or callback may have been wrapped after the ServletContextHandler, so update the channel. - servletChannel.associate(request, response, callback); - servletChannel.handle(); - return true; + // Otherwise, there is no matching servlet so we pass to our next handler (if any) + return super.handle(request, response, callback); } /** diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java index 189f8b21d2d3..c3196f2c788d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java @@ -386,7 +386,17 @@ public SessionHandler() @Override public ManagedSession getManagedSession(Request request) { - return Request.as(request, ServletContextRequest.class).getManagedSession(); + ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class); + if (servletContextRequest != null) + return servletContextRequest.getManagedSession(); + + NonServletSessionRequest nonServletSessionRequest = Request.as(request, NonServletSessionRequest.class); + if (nonServletSessionRequest != null) + return nonServletSessionRequest.getManagedSession(); + + if (request.getSession(false) instanceof ManagedSession managedSession) + return managedSession; + return null; } /** @@ -674,21 +684,61 @@ public boolean handle(Request request, Response response, Callback callback) thr if (next == null) return false; - ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class); addSessionStreamWrapper(request); // find and set the session if one exists RequestedSession requestedSession = resolveRequestedSessionId(request); - servletContextRequest.setRequestedSession(requestedSession); + + ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class); + if (servletContextRequest == null) + request = new NonServletSessionRequest(request, response, requestedSession); + else + servletContextRequest.setRequestedSession(requestedSession); // Handle changed ID or max-age refresh, but only if this is not a redispatched request HttpCookie cookie = access(requestedSession.session(), request.getConnectionMetaData().isSecure()); if (cookie != null) + Response.putCookie(response, cookie); + + return next.handle(request, response, callback); + } + + private class NonServletSessionRequest extends Request.Wrapper + { + private final Response _response; + private RequestedSession _session; + + public NonServletSessionRequest(Request request, Response response, RequestedSession requestedSession) { - ServletContextResponse servletContextResponse = servletContextRequest.getServletContextResponse(); - Response.putCookie(servletContextResponse, cookie); + super(request); + _response = response; + _session = requestedSession; } - return next.handle(request, response, callback); + @Override + public Session getSession(boolean create) + { + ManagedSession session = _session.session(); + + if (session != null || !create) + return session; + + newSession(getWrapped(), _session.sessionId(), ms -> + _session = new RequestedSession(ms, _session.sessionId(), true)); + + session = _session.session(); + if (session == null) + throw new IllegalStateException("Create session failed"); + + HttpCookie cookie = getSessionCookie(session, isSecure()); + if (cookie != null) + Response.replaceCookie(_response, cookie); + return session; + } + + ManagedSession getManagedSession() + { + return _session.session(); + } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/security/ConstraintSecurityHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/security/ConstraintSecurityHandler.java index 1fcd65bf5fd3..035486a1d496 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/security/ConstraintSecurityHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/security/ConstraintSecurityHandler.java @@ -33,8 +33,6 @@ import jakarta.servlet.annotation.ServletSecurity.EmptyRoleSemantic; import jakarta.servlet.annotation.ServletSecurity.TransportGuarantee; import org.eclipse.jetty.ee10.servlet.ServletContextHandler; -import org.eclipse.jetty.ee10.servlet.security.ConstraintAware; -import org.eclipse.jetty.ee10.servlet.security.ConstraintMapping; import org.eclipse.jetty.http.pathmap.MappedResource; import org.eclipse.jetty.http.pathmap.MatchedResource; import org.eclipse.jetty.http.pathmap.PathMappings; @@ -186,7 +184,7 @@ public static List removeConstraintMappingsForPath(String pat } /** - * Generate Constraints and ContraintMappings for the given url pattern and ServletSecurityElement + * Generate Constraints and ConstraintMappings for the given url pattern and ServletSecurityElement * * @param name the name * @param pathSpec the path spec diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletContextHandlerTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletContextHandlerTest.java index 97c8172c9038..b29f7bb0b2eb 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletContextHandlerTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletContextHandlerTest.java @@ -71,6 +71,7 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.pathmap.MatchedResource; +import org.eclipse.jetty.io.Content; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.security.Constraint; import org.eclipse.jetty.security.SecurityHandler; @@ -104,6 +105,7 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -1965,9 +1967,9 @@ public void testReplaceServletHandlerWithoutServlet() throws Exception } @Test - public void testReplaceHandler() throws Exception + public void testInsertHandler() throws Exception { - ServletContextHandler servletContextHandler = new ServletContextHandler(); + ServletContextHandler servletContextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS | ServletContextHandler.SECURITY); ServletHolder sh = new ServletHolder(new TestServlet()); servletContextHandler.addServlet(sh, "/foo"); final AtomicBoolean contextInit = new AtomicBoolean(false); @@ -1975,7 +1977,6 @@ public void testReplaceHandler() throws Exception servletContextHandler.addEventListener(new ServletContextListener() { - @Override public void contextInitialized(ServletContextEvent sce) { @@ -1990,15 +1991,26 @@ public void contextDestroyed(ServletContextEvent sce) contextDestroy.set(true); } }); - ServletHandler shandler = servletContextHandler.getServletHandler(); - ResourceHandler rh = new ResourceHandler(); rh.setBaseResource(ResourceFactory.of(rh).newResource(Paths.get("."))); - servletContextHandler.insertHandler(rh); - assertEquals(shandler, servletContextHandler.getServletHandler()); - assertEquals(rh, servletContextHandler.getHandler()); - assertEquals(rh.getHandler(), shandler); + + Handler last = new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) + { + return false; + } + }; + servletContextHandler.getServletHandler().setHandler(last); + + assertThat(servletContextHandler.getHandler(), sameInstance(rh)); + assertThat(rh.getHandler(), instanceOf(SessionHandler.class)); + assertThat(servletContextHandler.getSessionHandler().getHandler(), instanceOf(SecurityHandler.class)); + assertThat(servletContextHandler.getSecurityHandler().getHandler(), instanceOf(ServletHandler.class)); + assertThat(servletContextHandler.getServletHandler().getHandler(), sameInstance(last)); + _server.setHandler(servletContextHandler); _server.start(); assertTrue(contextInit.get()); @@ -2007,7 +2019,7 @@ public void contextDestroyed(ServletContextEvent sce) } @Test - public void testFallThrough() throws Exception + public void testFallThroughContextHandler() throws Exception { Handler.Sequence list = new Handler.Sequence(); _server.setHandler(list); @@ -2038,6 +2050,37 @@ public boolean handle(Request request, Response response, Callback callback) thr assertThat(response, Matchers.containsString("404 Fell Through")); } + @Test + public void testFallThroughServletHandler() throws Exception + { + ServletContextHandler root = new ServletContextHandler("/", ServletContextHandler.SESSIONS | ServletContextHandler.SECURITY); + _server.setHandler(root); + + ServletHandler servlet = root.getServletHandler(); + servlet.setEnsureDefaultServlet(false); + servlet.addServletWithMapping(HelloServlet.class, "/hello/*"); + + servlet.setHandler(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + Content.Sink.write(response, true, "Fell Through Handler: " + request.getSession(true).getId(), callback); + return true; + } + }); + + _server.start(); + + String response = _connector.getResponse("GET /hello HTTP/1.0\r\n\r\n"); + assertThat(response, Matchers.containsString("200 OK")); + + response = _connector.getResponse("GET /other HTTP/1.0\r\n\r\n"); + assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, Matchers.containsString("Set-Cookie: JSESSIONID=")); + assertThat(response, Matchers.containsString("Fell Through Handler")); + } + /** * Test behavior of new {@link org.eclipse.jetty.util.Decorator}, with * new DecoratedObjectFactory class diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java index c69c82fb6a22..ec4297a09b61 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java @@ -285,6 +285,15 @@ public CoreContextHandler getCoreContextHandler() return _coreContextHandler; } + /** + * Insert a handler between the {@link #getCoreContextHandler()} and this handler. + * @param coreHandler A core handler to insert + */ + public void insertHandler(org.eclipse.jetty.server.Handler.Singleton coreHandler) + { + getCoreContextHandler().insertHandler(coreHandler); + } + @Override public void dump(Appendable out, String indent) throws IOException { @@ -2607,6 +2616,8 @@ protected void doStop() throws Exception @Override public void insertHandler(Singleton handler) { + // We cannot call super.insertHandler here, because it uses this.setHandler + // which gives a warning. This is the same code, but uses super.setHandler Singleton tail = handler.getTail(); if (tail.getHandler() != null) throw new IllegalArgumentException("bad tail of inserted wrapper chain"); diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HandlerWrapper.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HandlerWrapper.java index 17759204f3d9..f6cdacc53e85 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HandlerWrapper.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HandlerWrapper.java @@ -81,6 +81,18 @@ public void setHandler(Handler handler) updateBean(old, _handler, true); } + /** + * Get the tail of a chain of {@link HandlerWrapper}s. + * @return The last {@link HandlerWrapper} in a chain of {@link HandlerWrapper}s + */ + public HandlerWrapper getTail() + { + HandlerWrapper tail = this; + while (tail.getHandler() instanceof HandlerWrapper handlerWrapper) + tail = handlerWrapper; + return tail; + } + /** * Replace the current handler with another HandlerWrapper * linked to the current handler. @@ -98,14 +110,7 @@ public void insertHandler(HandlerWrapper wrapper) if (wrapper == null) throw new IllegalArgumentException(); - HandlerWrapper tail = wrapper; - while (tail.getHandler() instanceof HandlerWrapper) - { - tail = (HandlerWrapper)tail.getHandler(); - } - if (tail.getHandler() != null) - throw new IllegalArgumentException("bad tail of inserted wrapper chain"); - + HandlerWrapper tail = wrapper.getTail(); Handler next = getHandler(); setHandler(wrapper); tail.setHandler(next); diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ContextHandlerTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ContextHandlerTest.java index d07babf27fa5..e9286c5eda65 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ContextHandlerTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ContextHandlerTest.java @@ -32,9 +32,12 @@ import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Context; +import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.LocalConnector; +import org.eclipse.jetty.server.Response; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.ContextHandlerCollection; +import org.eclipse.jetty.util.Callback; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -48,6 +51,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.sameInstance; public class ContextHandlerTest { @@ -62,7 +66,6 @@ public void before() throws Exception _connector = new LocalConnector(_server); _server.addConnector(_connector); - _contextHandler = new ContextHandler(); _server.setHandler(_contextHandler); } @@ -812,6 +815,54 @@ public void exitScope(ContextHandler.APIContext context, Request request) "Core exit http://0.0.0.0/")); } + @Test + public void testInsertHandler() throws Exception + { + HelloHandler helloHandler = new HelloHandler(); + _contextHandler.setHandler(helloHandler); + Handler.Wrapper coreHandler = new Handler.Wrapper() + { + @Override + public boolean handle(org.eclipse.jetty.server.Request request, Response response, Callback callback) throws Exception + { + response.getHeaders().put("Core", "Inserted"); + return super.handle(request, response, callback); + } + }; + _contextHandler.insertHandler(coreHandler); + + HandlerWrapper nestedHandler = new HandlerWrapper() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + response.setHeader("Nested", "Inserted"); + super.handle(target, baseRequest, request, response); + } + }; + _contextHandler.insertHandler(nestedHandler); + + assertThat(_contextHandler.getCoreContextHandler().getHandler(), sameInstance(coreHandler)); + assertThat(coreHandler.getHandler().toString(), containsString("CoreToNestedHandler")); + assertThat(_contextHandler.getHandler(), sameInstance(nestedHandler)); + assertThat(nestedHandler.getHandler(), sameInstance(helloHandler)); + + _server.start(); + + String rawResponse = _connector.getResponse(""" + GET / HTTP/1.0 + + """); + + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + + assertThat(response.getStatus(), is(200)); + assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat(response.getContent(), containsString("Hello")); + assertThat(response.get("Core"), is("Inserted")); + assertThat(response.get("Nested"), is("Inserted")); + } + private static class TestErrorHandler extends ErrorHandler implements ErrorHandler.ErrorPageMapper { @Override diff --git a/jetty-ee9/jetty-ee9-servlet/src/main/java/org/eclipse/jetty/ee9/servlet/ServletContextHandler.java b/jetty-ee9/jetty-ee9-servlet/src/main/java/org/eclipse/jetty/ee9/servlet/ServletContextHandler.java index 416ecb0b240a..d819163e7c27 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/main/java/org/eclipse/jetty/ee9/servlet/ServletContextHandler.java +++ b/jetty-ee9/jetty-ee9-servlet/src/main/java/org/eclipse/jetty/ee9/servlet/ServletContextHandler.java @@ -213,23 +213,6 @@ public boolean addEventListener(EventListener listener) return false; } - @Override - public void setHandler(Handler handler) - { - if (handler instanceof SessionHandler) - setSessionHandler((SessionHandler)handler); - else if (handler instanceof SecurityHandler) - setSecurityHandler((SecurityHandler)handler); - else if (handler instanceof ServletHandler) - setServletHandler((ServletHandler)handler); - else - { - if (handler != null) - LOG.warn("ServletContextHandler.setHandler should not be called directly. Use insertHandler or setSessionHandler etc."); - super.setHandler(handler); - } - } - private void doSetHandler(HandlerWrapper wrapper, Handler handler) { if (wrapper == this) @@ -683,6 +666,19 @@ public void setServletHandler(ServletHandler servletHandler) relinkHandlers(); } + @Override + public void setHandler(Handler handler) + { + if (handler instanceof SessionHandler) + setSessionHandler((SessionHandler)handler); + else if (handler instanceof SecurityHandler) + setSecurityHandler((SecurityHandler)handler); + else if (handler instanceof ServletHandler) + setServletHandler((ServletHandler)handler); + else + getServletHandler().setHandler(handler); + } + /** * Insert a HandlerWrapper before the first Session, Security or ServletHandler * but after any other HandlerWrappers. @@ -707,6 +703,8 @@ else if (handler instanceof ServletHandler) throw new IllegalArgumentException("bad tail of inserted wrapper chain"); // Skip any injected handlers + // This is not the best behavior, but is retained here for jetty-10/11 compatibility + // but is changed in ee10 and beyond HandlerWrapper h = this; while (h.getHandler() instanceof HandlerWrapper) { diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletContextHandlerTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletContextHandlerTest.java index ee118bdee332..78301bc026df 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletContextHandlerTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletContextHandlerTest.java @@ -60,9 +60,9 @@ import jakarta.servlet.http.HttpSessionIdListener; import jakarta.servlet.http.HttpSessionListener; import org.eclipse.jetty.ee9.nested.ContextHandler; +import org.eclipse.jetty.ee9.nested.Handler; import org.eclipse.jetty.ee9.nested.HandlerWrapper; import org.eclipse.jetty.ee9.nested.Request; -import org.eclipse.jetty.ee9.nested.ResourceHandler; import org.eclipse.jetty.ee9.nested.Response; import org.eclipse.jetty.ee9.nested.SessionHandler; import org.eclipse.jetty.ee9.security.RoleInfo; @@ -90,6 +90,7 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -1827,9 +1828,10 @@ public void testReplaceServletHandlerWithoutServlet() throws Exception } @Test - public void testReplaceHandler() throws Exception + public void testInsertHandler() throws Exception { - ServletContextHandler servletContextHandler = new ServletContextHandler(); + ServletContextHandler servletContextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS | ServletContextHandler.SECURITY); + servletContextHandler.setBaseResourceAsString("."); ServletHolder sh = new ServletHolder(new TestServlet()); servletContextHandler.addServlet(sh, "/foo"); final AtomicBoolean contextInit = new AtomicBoolean(false); @@ -1837,7 +1839,6 @@ public void testReplaceHandler() throws Exception servletContextHandler.addEventListener(new ServletContextListener() { - @Override public void contextInitialized(ServletContextEvent sce) { @@ -1852,14 +1853,23 @@ public void contextDestroyed(ServletContextEvent sce) contextDestroy.set(true); } }); - ServletHandler shandler = servletContextHandler.getServletHandler(); - ResourceHandler rh = new ResourceHandler(); + org.eclipse.jetty.server.handler.ResourceHandler coreResourceHandler = new org.eclipse.jetty.server.handler.ResourceHandler(); + HandlerWrapper nestedHandler = new HandlerWrapper(); + Handler last = new HandlerWrapper(); + + servletContextHandler.insertHandler(coreResourceHandler); + servletContextHandler.insertHandler(nestedHandler); + servletContextHandler.setHandler(last); + + assertThat(servletContextHandler.getCoreContextHandler().getHandler(), sameInstance(coreResourceHandler)); + assertThat(coreResourceHandler.getHandler().toString(), containsString("CoreToNestedHandler@")); + assertThat(servletContextHandler.getHandler(), sameInstance(nestedHandler)); + assertThat(nestedHandler.getHandler(), instanceOf(SessionHandler.class)); + assertThat(servletContextHandler.getSessionHandler().getHandler(), instanceOf(SecurityHandler.class)); + assertThat(servletContextHandler.getSecurityHandler().getHandler(), instanceOf(ServletHandler.class)); + assertThat(servletContextHandler.getServletHandler().getHandler(), sameInstance(last)); - servletContextHandler.insertHandler(rh); - assertEquals(shandler, servletContextHandler.getServletHandler()); - assertEquals(rh, servletContextHandler.getHandler()); - assertEquals(rh.getHandler(), shandler); _server.setHandler(servletContextHandler); _server.start(); assertTrue(contextInit.get());