Skip to content

Commit

Permalink
Issue #7858 - GzipHandler request.isHandled support (#8013)
Browse files Browse the repository at this point in the history
* Better conditional logic in GzipHandler
* Correct test expectations
* Use super.handle() where appropriate

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
  • Loading branch information
joakime committed May 18, 2022
1 parent 122a200 commit 546c382
Show file tree
Hide file tree
Showing 2 changed files with 145 additions and 8 deletions.
Expand Up @@ -664,14 +664,20 @@ public void setInflateBufferSize(int size)
@Override
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 String path = baseRequest.getPathInContext();
LOG.debug("{} handle {} in {}", this, baseRequest, context);

if (!_dispatchers.contains(baseRequest.getDispatcherType()))
{
LOG.debug("{} excluded by dispatcherType {}", this, baseRequest.getDispatcherType());
_handler.handle(target, baseRequest, request, response);
super.handle(target, baseRequest, request, response);
return;
}

Expand All @@ -688,6 +694,15 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
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?
HttpOutput out = baseRequest.getResponse().getHttpOutput();
HttpOutput.Interceptor interceptor = out.getInterceptor();
Expand Down Expand Up @@ -764,15 +779,15 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches())
if (alreadyGzipped)
{
LOG.debug("{} already intercepting {}", this, request);
_handler.handle(target, baseRequest, request, response);
super.handle(target, baseRequest, request, response);
return;
}

// If not a supported method - no Vary because no matter what client, this URI is always excluded
if (!_methods.test(baseRequest.getMethod()))
{
LOG.debug("{} excluded by method {}", this, request);
_handler.handle(target, baseRequest, request, response);
super.handle(target, baseRequest, request, response);
return;
}

Expand All @@ -781,7 +796,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches())
if (!isPathGzipable(path))
{
LOG.debug("{} excluded by path {}", this, request);
_handler.handle(target, baseRequest, request, response);
super.handle(target, baseRequest, request, response);
return;
}

Expand All @@ -794,7 +809,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches())
{
LOG.debug("{} excluded by path suffix mime type {}", this, request);
// handle normally without setting vary header
_handler.handle(target, baseRequest, request, response);
super.handle(target, baseRequest, request, response);
return;
}
}
Expand All @@ -804,9 +819,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches())
{
// install interceptor and handle
out.setInterceptor(new GzipHttpOutputInterceptor(this, getVaryField(), baseRequest.getHttpChannel(), origInterceptor, isSyncFlush()));

if (_handler != null)
_handler.handle(target, baseRequest, request, response);
super.handle(target, baseRequest, request, response);
}
finally
{
Expand Down
@@ -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()));
}
}
}

0 comments on commit 546c382

Please sign in to comment.