* Better conditional logic in GzipHandler * Correct test expectations * Use super.handle() where appropriate Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
This commit is contained in:
parent
122a20043b
commit
546c382255
|
@ -664,6 +664,12 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
@Override
|
@Override
|
||||||
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
|
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
|
||||||
{
|
{
|
||||||
|
if (baseRequest.isHandled())
|
||||||
|
{
|
||||||
|
super.handle(target, baseRequest, request, response);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
final ServletContext context = baseRequest.getServletContext();
|
final ServletContext context = baseRequest.getServletContext();
|
||||||
final String path = baseRequest.getPathInContext();
|
final String path = baseRequest.getPathInContext();
|
||||||
LOG.debug("{} handle {} in {}", this, baseRequest, context);
|
LOG.debug("{} handle {} in {}", this, baseRequest, context);
|
||||||
|
@ -671,7 +677,7 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
if (!_dispatchers.contains(baseRequest.getDispatcherType()))
|
if (!_dispatchers.contains(baseRequest.getDispatcherType()))
|
||||||
{
|
{
|
||||||
LOG.debug("{} excluded by dispatcherType {}", this, baseRequest.getDispatcherType());
|
LOG.debug("{} excluded by dispatcherType {}", this, baseRequest.getDispatcherType());
|
||||||
_handler.handle(target, baseRequest, request, response);
|
super.handle(target, baseRequest, request, response);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -688,6 +694,15 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
baseRequest.getHttpInput().addInterceptor(gzipHttpInputInterceptor);
|
baseRequest.getHttpInput().addInterceptor(gzipHttpInputInterceptor);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// From here on out, the response output gzip determination is made
|
||||||
|
|
||||||
|
// Don't attempt to modify the response output if it's already committed.
|
||||||
|
if (response.isCommitted())
|
||||||
|
{
|
||||||
|
super.handle(target, baseRequest, request, response);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
// Are we already being gzipped?
|
// Are we already being gzipped?
|
||||||
HttpOutput out = baseRequest.getResponse().getHttpOutput();
|
HttpOutput out = baseRequest.getResponse().getHttpOutput();
|
||||||
HttpOutput.Interceptor interceptor = out.getInterceptor();
|
HttpOutput.Interceptor interceptor = out.getInterceptor();
|
||||||
|
@ -764,7 +779,7 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
if (alreadyGzipped)
|
if (alreadyGzipped)
|
||||||
{
|
{
|
||||||
LOG.debug("{} already intercepting {}", this, request);
|
LOG.debug("{} already intercepting {}", this, request);
|
||||||
_handler.handle(target, baseRequest, request, response);
|
super.handle(target, baseRequest, request, response);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -772,7 +787,7 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
if (!_methods.test(baseRequest.getMethod()))
|
if (!_methods.test(baseRequest.getMethod()))
|
||||||
{
|
{
|
||||||
LOG.debug("{} excluded by method {}", this, request);
|
LOG.debug("{} excluded by method {}", this, request);
|
||||||
_handler.handle(target, baseRequest, request, response);
|
super.handle(target, baseRequest, request, response);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -781,7 +796,7 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
if (!isPathGzipable(path))
|
if (!isPathGzipable(path))
|
||||||
{
|
{
|
||||||
LOG.debug("{} excluded by path {}", this, request);
|
LOG.debug("{} excluded by path {}", this, request);
|
||||||
_handler.handle(target, baseRequest, request, response);
|
super.handle(target, baseRequest, request, response);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -794,7 +809,7 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
{
|
{
|
||||||
LOG.debug("{} excluded by path suffix mime type {}", this, request);
|
LOG.debug("{} excluded by path suffix mime type {}", this, request);
|
||||||
// handle normally without setting vary header
|
// handle normally without setting vary header
|
||||||
_handler.handle(target, baseRequest, request, response);
|
super.handle(target, baseRequest, request, response);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
@ -804,9 +819,7 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
|
||||||
{
|
{
|
||||||
// install interceptor and handle
|
// install interceptor and handle
|
||||||
out.setInterceptor(new GzipHttpOutputInterceptor(this, getVaryField(), baseRequest.getHttpChannel(), origInterceptor, isSyncFlush()));
|
out.setInterceptor(new GzipHttpOutputInterceptor(this, getVaryField(), baseRequest.getHttpChannel(), origInterceptor, isSyncFlush()));
|
||||||
|
super.handle(target, baseRequest, request, response);
|
||||||
if (_handler != null)
|
|
||||||
_handler.handle(target, baseRequest, request, response);
|
|
||||||
}
|
}
|
||||||
finally
|
finally
|
||||||
{
|
{
|
||||||
|
|
|
@ -0,0 +1,124 @@
|
||||||
|
//
|
||||||
|
// ========================================================================
|
||||||
|
// 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.servlet;
|
||||||
|
|
||||||
|
import java.io.IOException;
|
||||||
|
import java.util.concurrent.LinkedBlockingQueue;
|
||||||
|
import javax.servlet.ServletException;
|
||||||
|
import javax.servlet.http.HttpServletRequest;
|
||||||
|
import javax.servlet.http.HttpServletResponse;
|
||||||
|
|
||||||
|
import org.eclipse.jetty.client.HttpClient;
|
||||||
|
import org.eclipse.jetty.client.api.ContentResponse;
|
||||||
|
import org.eclipse.jetty.server.Handler;
|
||||||
|
import org.eclipse.jetty.server.Request;
|
||||||
|
import org.eclipse.jetty.server.Server;
|
||||||
|
import org.eclipse.jetty.server.ServerConnector;
|
||||||
|
import org.eclipse.jetty.server.handler.AbstractHandler;
|
||||||
|
import org.eclipse.jetty.server.handler.DefaultHandler;
|
||||||
|
import org.eclipse.jetty.server.handler.HandlerCollection;
|
||||||
|
import org.eclipse.jetty.server.handler.ResourceHandler;
|
||||||
|
import org.eclipse.jetty.server.handler.gzip.GzipHandler;
|
||||||
|
import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
|
||||||
|
import org.eclipse.jetty.util.component.LifeCycle;
|
||||||
|
import org.eclipse.jetty.util.resource.PathResource;
|
||||||
|
import org.junit.jupiter.api.AfterEach;
|
||||||
|
import org.junit.jupiter.api.Test;
|
||||||
|
|
||||||
|
import static org.hamcrest.MatcherAssert.assertThat;
|
||||||
|
import static org.hamcrest.Matchers.containsString;
|
||||||
|
import static org.hamcrest.Matchers.is;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Tests of behavior of GzipHandler when Request.isHandled() or Response.isCommitted() is true
|
||||||
|
*/
|
||||||
|
public class GzipHandlerIsHandledTest
|
||||||
|
{
|
||||||
|
public WorkDir workDir;
|
||||||
|
|
||||||
|
private Server server;
|
||||||
|
private HttpClient client;
|
||||||
|
public LinkedBlockingQueue<String> events = new LinkedBlockingQueue<>();
|
||||||
|
|
||||||
|
public void startServer(Handler rootHandler) throws Exception
|
||||||
|
{
|
||||||
|
server = new Server();
|
||||||
|
ServerConnector connector = new ServerConnector(server);
|
||||||
|
connector.setPort(0);
|
||||||
|
server.addConnector(connector);
|
||||||
|
|
||||||
|
server.setHandler(rootHandler);
|
||||||
|
server.start();
|
||||||
|
|
||||||
|
client = new HttpClient();
|
||||||
|
client.start();
|
||||||
|
}
|
||||||
|
|
||||||
|
@AfterEach
|
||||||
|
public void tearDown()
|
||||||
|
{
|
||||||
|
LifeCycle.stop(client);
|
||||||
|
LifeCycle.stop(server);
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
public void testRequest() throws Exception
|
||||||
|
{
|
||||||
|
HandlerCollection handlers = new HandlerCollection();
|
||||||
|
|
||||||
|
ResourceHandler resourceHandler = new ResourceHandler();
|
||||||
|
resourceHandler.setBaseResource(new PathResource(workDir.getPath()));
|
||||||
|
resourceHandler.setDirectoriesListed(true);
|
||||||
|
resourceHandler.setHandler(new EventHandler(events, "ResourceHandler"));
|
||||||
|
|
||||||
|
GzipHandler gzipHandler = new GzipHandler();
|
||||||
|
gzipHandler.setMinGzipSize(32);
|
||||||
|
gzipHandler.setHandler(new EventHandler(events, "GzipHandler-wrapped-handler"));
|
||||||
|
|
||||||
|
handlers.setHandlers(new Handler[]{resourceHandler, gzipHandler, new DefaultHandler()});
|
||||||
|
|
||||||
|
startServer(handlers);
|
||||||
|
|
||||||
|
ContentResponse response = client.GET(server.getURI().resolve("/"));
|
||||||
|
assertThat("response.status", response.getStatus(), is(200));
|
||||||
|
// we should have received a directory listing from the ResourceHandler
|
||||||
|
assertThat("response.content", response.getContentAsString(), containsString("Directory: /"));
|
||||||
|
// resource handler should have handled the request
|
||||||
|
// the gzip handler and default handlers should have been executed, seeing as this is a HandlerCollection
|
||||||
|
// but the gzip handler should not have acted on the request, as the response is committed
|
||||||
|
assertThat("One event should have been recorded", events.size(), is(1));
|
||||||
|
// the event handler should see the request.isHandled = true
|
||||||
|
// and response.isCommitted = true as the gzip handler didn't really do anything due to these
|
||||||
|
// states and let the wrapped handler (the EventHandler in this case) make the call on what it should do.
|
||||||
|
assertThat("Event indicating that GzipHandler-wrapped-handler ran", events.remove(), is("GzipHandler-wrapped-handler [request.isHandled=true, response.isCommitted=true]"));
|
||||||
|
}
|
||||||
|
|
||||||
|
private static class EventHandler extends AbstractHandler
|
||||||
|
{
|
||||||
|
private final LinkedBlockingQueue<String> events;
|
||||||
|
private final String action;
|
||||||
|
|
||||||
|
public EventHandler(LinkedBlockingQueue<String> events, String action)
|
||||||
|
{
|
||||||
|
this.events = events;
|
||||||
|
this.action = action;
|
||||||
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
|
||||||
|
{
|
||||||
|
events.offer(String.format("%s [request.isHandled=%b, response.isCommitted=%b]", action, baseRequest.isHandled(), response.isCommitted()));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
Loading…
Reference in New Issue