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.

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

Delegate compression to servlet container #557

Merged
merged 6 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
* [Getting Started](docs/getting-started.adoc)
* Developer Reference
* [URL binding reference](docs/reference.adoc)
* [HTTP compression](docs/compression.adoc)
* [Internationalization support](docs/i18n.adoc)
* [Javadoc](https://javadoc.jenkins.io/component/stapler/)
* [Stapler jelly tag library reference and XML Schema](docs/jelly-taglib-ref.adoc)
46 changes: 3 additions & 43 deletions core/src/main/java/org/kohsuke/stapler/ResponseImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,13 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.zip.GZIPOutputStream;
import javax.servlet.ServletException;
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpServletResponseWrapper;
import net.sf.json.JsonConfig;
import org.apache.commons.io.IOUtils;
import org.kohsuke.stapler.compression.CompressionFilter;
import org.kohsuke.stapler.compression.FilterServletOutputStream;
import org.kohsuke.stapler.export.DataWriter;
import org.kohsuke.stapler.export.ExportConfig;
import org.kohsuke.stapler.export.Flavor;
Expand Down Expand Up @@ -293,7 +290,7 @@ public void serveExposedBean(StaplerRequest req, Object exposedBean, ExportConfi
String pad = null;
Flavor flavor = config.getFlavor();
setContentType(flavor.contentType);
Writer w = getCompressedWriter(req);
Writer w = getWriter();

if (flavor == Flavor.JSON
|| flavor == Flavor.JSONP) { // for compatibility reasons, accept JSON for JSONP as well.
Expand Down Expand Up @@ -347,51 +344,14 @@ private void writeOne(TreePruner pruner, DataWriter dw, Object item) throws IOEx
p.writeTo(item, pruner, dw);
}

private boolean acceptsGzip(HttpServletRequest req) {
String acceptEncoding = req.getHeader("Accept-Encoding");
return acceptEncoding != null && acceptEncoding.contains("gzip");
}

@Override
public OutputStream getCompressedOutputStream(HttpServletRequest req) throws IOException {
if (mode != null) { // we already made the call and created OutputStream/Writer
return getOutputStream();
}

if (!acceptsGzip(req)) {
return getOutputStream(); // compression not applicable here
}

if (CompressionFilter.activate(req)) {
return getOutputStream(); // CompressionFilter will set up compression. no need to do anything
}

// CompressionFilter not available, so do it on our own.
// see CompressionFilter for why this is not desirable
setHeader("Content-Encoding", "gzip");
return recordOutput(
new FilterServletOutputStream(new GZIPOutputStream(super.getOutputStream()), super.getOutputStream()));
return getOutputStream();
}

@Override
public Writer getCompressedWriter(HttpServletRequest req) throws IOException {
if (mode != null) {
return getWriter();
}

if (!acceptsGzip(req)) {
return getWriter(); // compression not available
}

if (CompressionFilter.activate(req)) {
return getWriter(); // CompressionFilter will set up compression. no need to do anything
}

// CompressionFilter not available, so do it on our own.
// see CompressionFilter for why this is not desirable
setHeader("Content-Encoding", "gzip");
return recordOutput(new PrintWriter(
new OutputStreamWriter(new GZIPOutputStream(super.getOutputStream()), getCharacterEncoding())));
return getWriter();
}

@Override
Expand Down
37 changes: 4 additions & 33 deletions core/src/main/java/org/kohsuke/stapler/Stapler.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -610,21 +609,6 @@ boolean serveStaticResource(
// a comprehensive discussion on this topic
rsp.setHeader("X-Content-Type-Options", "nosniff");

int idx = fileName.lastIndexOf('.');
String ext = fileName.substring(idx + 1);

OutputStream out = null;
if (mimeType.startsWith("text/") || TEXT_FILES.contains(ext)) {
// Need to duplicate this logic from ResponseImpl.getCompressedOutputStream,
// since we want to set content length if we are not using encoding.
String acceptEncoding = req.getHeader("Accept-Encoding");
if (acceptEncoding != null && acceptEncoding.contains("gzip")) {
// with gzip compression, Content-Length header needs to indicate the # of bytes after compression,
// so we can't compute it upfront.
out = rsp.getCompressedOutputStream(req);
}
}

// somewhat limited implementation of the partial GET
String range = req.getHeader("Range");
if (range != null
Expand Down Expand Up @@ -663,18 +647,11 @@ boolean serveStaticResource(
}
}

if (out == null) {
if (contentLength != -1) {
rsp.setHeader("Content-Length", Long.toString(contentLength));
}
out = rsp.getOutputStream();
}

byte[] buf = new byte[1024];
int len;
while ((len = in.read(buf)) > 0) {
out.write(buf, 0, len);
if (contentLength != -1) {
rsp.setHeader("Content-Length", Long.toString(contentLength));
}
OutputStream out = rsp.getOutputStream();
in.transferTo(out);
out.close();
return true;
} finally {
Expand Down Expand Up @@ -1109,12 +1086,6 @@ public static Stapler getCurrent() {

private static final Logger LOGGER = Logger.getLogger(Stapler.class.getName());

/**
* Extensions that look like text files.
*/
private static final Set<String> TEXT_FILES = new HashSet<>(
Arrays.asList("css", "js", "html", "txt", "java", "htm", "c", "cpp", "h", "rb", "pl", "py", "xml", "json"));

/**
* Get raw servlet path (decoded in TokenList).
*/
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/org/kohsuke/stapler/StaplerResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -229,13 +229,18 @@ default void serveExposedBean(StaplerRequest req, Object exposedBean, ExportConf
*
* @param req
* Used to determine whether the client supports compression
* @deprecated use {@link #getOutputStream}
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
*/
@Deprecated
OutputStream getCompressedOutputStream(HttpServletRequest req) throws IOException;

/**
* Works like {@link #getCompressedOutputStream(HttpServletRequest)} but this
* method is for {@link #getWriter()}.
*
* @deprecated use {@link #getWriter}
*/
@Deprecated
Writer getCompressedWriter(HttpServletRequest req) throws IOException;

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package org.kohsuke.stapler;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.IOException;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class UncaughtExceptionFilter implements Filter {
private ServletContext context;

@Override
public void init(FilterConfig filterConfig) throws ServletException {
context = filterConfig.getServletContext();
}

@Override
public void doFilter(ServletRequest req, ServletResponse rsp, FilterChain filterChain)
throws IOException, ServletException {
try {
filterChain.doFilter(req, rsp);
} catch (IOException | Error | RuntimeException | ServletException e) {
if (DISABLED) {
throw e;
}
reportException(e, (HttpServletRequest) req, (HttpServletResponse) rsp);
}
}

private void reportException(Throwable e, HttpServletRequest req, HttpServletResponse rsp)
throws IOException, ServletException {
getUncaughtExceptionHandler(context).reportException(e, context, req, rsp);
}

@Override
public void destroy() {}

public static void setUncaughtExceptionHandler(ServletContext context, UncaughtExceptionHandler handler) {
context.setAttribute(UncaughtExceptionHandler.class.getName(), handler);
}

public static UncaughtExceptionHandler getUncaughtExceptionHandler(ServletContext context) {
UncaughtExceptionHandler h =
(UncaughtExceptionHandler) context.getAttribute(UncaughtExceptionHandler.class.getName());
if (h == null) {
h = UncaughtExceptionHandler.DEFAULT;
}
return h;
}

/**
* Disables the effect of {@link #setUncaughtExceptionHandler}, letting all errors be rethrown.
*/
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy switch.")
public static boolean DISABLED = Boolean.getBoolean(UncaughtExceptionFilter.class.getName() + ".disabled");
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GitHub diff page makes this unreviewable, as it is a rename of CompressionFilter with some simplifications. Here is the actual diff. Please only comment on the changed portions, not the original code which I am not changing.

--- core/src/main/java/org/kohsuke/stapler/compression/CompressionFilter.java	2024-06-12 12:42:12.076182060 -0700
+++ /tmp/UncaughtExceptionFilter.java	2024-06-12 12:42:11.216183675 -0700
@@ -1,4 +1,4 @@
-package org.kohsuke.stapler.compression;
+package org.kohsuke.stapler;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.IOException;
@@ -12,28 +12,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-/**
- * Pimps up {@link HttpServletResponse} so that it understands "Content-Encoding: gzip" and compress the response.
- *
- * <p>
- * When exceptions are processed within web applications, different unrelated parts of the webapp can end up calling
- * {@link HttpServletResponse#getOutputStream()}. This fundamentally doesn't work with the notion that the application
- * needs to process "Content-Encoding:gzip" on its own. Such app has to maintain a GZIP output stream on its own,
- * since {@link HttpServletResponse} doesn't know that its output is written through a compressed stream.
- *
- * <p>
- * Another place this break-down can be seen is when a servlet throws an exception that the container handles.
- * It tries to render an error page, but of course it wouldn't know that "Content-Encoding:gzip" is set, so it
- * will fail to write in the correct format.
- *
- * <p>
- * The only way to properly fix this is to make {@link HttpServletResponse} smart enough that it returns
- * the compression-transparent stream from {@link HttpServletResponse#getOutputStream()} (and it would also
- * have to process the exception thrown from the app.) This filter does exactly that.
- *
- * @author Kohsuke Kawaguchi
- */
-public class CompressionFilter implements Filter {
+public class UncaughtExceptionFilter implements Filter {
     private ServletContext context;
 
     @Override
@@ -42,34 +21,18 @@
     }
 
     @Override
-    public void doFilter(ServletRequest _req, ServletResponse _rsp, FilterChain filterChain)
+    public void doFilter(ServletRequest req, ServletResponse rsp, FilterChain filterChain)
             throws IOException, ServletException {
-        Object old1 = swapAttribute(_req, CompressionFilter.class, true);
-
-        CompressionServletResponse rsp = new CompressionServletResponse((HttpServletResponse) _rsp);
-        Object old2 = swapAttribute(_req, CompressionServletResponse.class, rsp);
-
         try {
-            filterChain.doFilter(_req, rsp);
+            filterChain.doFilter(req, rsp);
         } catch (IOException | Error | RuntimeException | ServletException e) {
             if (DISABLED) {
                 throw e;
             }
-            reportException(e, (HttpServletRequest) _req, rsp);
-        } finally {
-            rsp.close();
-
-            _req.setAttribute(CompressionFilter.class.getName(), old1);
-            _req.setAttribute(CompressionServletResponse.class.getName(), old2);
+            reportException(e, (HttpServletRequest) req, (HttpServletResponse) rsp);
         }
     }
 
-    private Object swapAttribute(ServletRequest req, Class<?> key, Object value) {
-        Object old = req.getAttribute(key.getName());
-        req.setAttribute(key.getName(), value);
-        return old;
-    }
-
     private void reportException(Throwable e, HttpServletRequest req, HttpServletResponse rsp)
             throws IOException, ServletException {
         getUncaughtExceptionHandler(context).reportException(e, context, req, rsp);
@@ -92,33 +55,8 @@
     }
 
     /**
-     * Is this request already wrapped into {@link CompressionFilter}?
-     */
-    public static boolean has(ServletRequest req) {
-        return req.getAttribute(CompressionServletResponse.class.getName()) != null;
-    }
-
-    /**
-     * Is this request already wrapped into {@link CompressionFilter},
-     * activate that, so that {@link ServletResponse#getOutputStream()} will return
-     * a stream that automatically handles compression.
-     */
-    public static boolean activate(ServletRequest req) throws IOException {
-        CompressionServletResponse rsp =
-                (CompressionServletResponse) req.getAttribute(CompressionServletResponse.class.getName());
-        if (rsp != null) {
-            rsp.activate();
-            return true;
-        } else {
-            return false;
-        }
-    }
-
-    /**
      * Disables the effect of {@link #setUncaughtExceptionHandler}, letting all errors be rethrown.
-     * Despite its name, this flag does <strong>not</strong> disable {@link CompressionFilter} itself!
-     * Rather use {@code DefaultScriptInvoker.COMPRESS_BY_DEFAULT}.
      */
     @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy switch.")
-    public static boolean DISABLED = Boolean.getBoolean(CompressionFilter.class.getName() + ".disabled");
+    public static boolean DISABLED = Boolean.getBoolean(UncaughtExceptionFilter.class.getName() + ".disabled");
 }

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.kohsuke.stapler.compression;
package org.kohsuke.stapler;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.IOException;
Expand All @@ -10,13 +10,9 @@
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.kohsuke.stapler.Stapler;

/**
* Handles an exception caught by {@link CompressionFilter}.
*
* See {@link CompressionFilter} javadoc for why this exception needs to be handled
* by us and can't just be handled by the servlet container like it does all others.
* Handles an exception caught by {@link UncaughtExceptionFilter}.
*
* @author Kohsuke Kawaguchi
*/
Expand Down

This file was deleted.

Loading