Skip to content

Commit b67ed04

Browse files
gregwgregw
authored
Implement etag support in CompressionHandler (#13633)
* Implement etag support in CompressionHandler The best variable name is no variable name
1 parent 46b2ca0 commit b67ed04

File tree

10 files changed

+194
-114
lines changed

10 files changed

+194
-114
lines changed

‎jetty-core/jetty-compression/jetty-compression-brotli/src/test/java/org/eclipse/jetty/compression/brotli/BrotliCompressionTest.java‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,8 @@
1919
import com.aayushatharva.brotli4j.encoder.BrotliOutputStream;
2020
import org.junit.jupiter.api.Test;
2121

22-
import static org.hamcrest.MatcherAssert.assertThat;
23-
import static org.hamcrest.Matchers.is;
24-
2522
public class BrotliCompressionTest extends AbstractBrotliTest
2623
{
27-
@Test
28-
public void testStripSuffixes() throws Exception
29-
{
30-
startBrotli();
31-
assertThat(brotli.stripSuffixes("12345"), is("12345"));
32-
assertThat(brotli.stripSuffixes("12345, 666" + brotli.getEtagSuffix()), is("12345, 666"));
33-
assertThat(brotli.stripSuffixes("12345, 666" + brotli.getEtagSuffix() + ",W/\"9999" + brotli.getEtagSuffix() + "\""),
34-
is("12345, 666,W/\"9999\""));
35-
}
36-
3724
@Test
3825
public void testEncodeBehavior() throws Exception
3926
{

‎jetty-core/jetty-compression/jetty-compression-common/src/main/java/org/eclipse/jetty/compression/Compression.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import java.io.InputStream;
1818
import java.io.OutputStream;
1919
import java.util.List;
20+
import java.util.regex.Pattern;
2021

2122
import org.eclipse.jetty.http.EtagUtils;
2223
import org.eclipse.jetty.http.HttpField;
@@ -29,9 +30,11 @@
2930

3031
public abstract class Compression extends ContainerLifeCycle
3132
{
33+
3234
private final String encodingName;
3335
private final String etagSuffix;
3436
private final String etagSuffixQuote;
37+
private final Pattern etagSuffixPattern;
3538
private ByteBufferPool byteBufferPool;
3639
private Container container;
3740
private int bufferSize = 2048;
@@ -42,6 +45,11 @@ public Compression(String encoding)
4245
encodingName = encoding;
4346
etagSuffix = StringUtil.isEmpty(EtagUtils.ETAG_SEPARATOR) ? "" : (EtagUtils.ETAG_SEPARATOR + encodingName);
4447
etagSuffixQuote = etagSuffix + "\"";
48+
etagSuffixPattern = Pattern.compile(
49+
"((?:W/)?\\\"[^\\\"]*?)" + // Match a possibly weak etag starting with a quote (captured as $1)
50+
"--" + encodingName + // match the known etag suffix
51+
"(?=(?:--[a-z0-9]+)*" + // match zero or more other suffixes (lookahead, so not part of the match)
52+
"\\\")"); // match the closing quote (lookahead, so not part of the match)
4553
}
4654

4755
/**
@@ -279,7 +287,7 @@ public EncoderSink newEncoderSink(Content.Sink sink)
279287
public abstract EncoderSink newEncoderSink(Content.Sink sink, EncoderConfig config);
280288

281289
/**
282-
* Strip compression suffixes off etags
290+
* Strip this compression suffix off etags
283291
*
284292
* @param etagsList the list of etags to strip
285293
* @return the tags stripped of compression suffixes.
@@ -288,15 +296,7 @@ public String stripSuffixes(String etagsList)
288296
{
289297
if (StringUtil.isEmpty(EtagUtils.ETAG_SEPARATOR))
290298
return etagsList;
291-
292-
// This is a poor implementation that ignores list and tag structure
293-
while (true)
294-
{
295-
int i = etagsList.lastIndexOf(etagSuffix);
296-
if (i < 0)
297-
return etagsList;
298-
etagsList = etagsList.substring(0, i) + etagsList.substring(i + etagSuffix.length());
299-
}
299+
return etagSuffixPattern.matcher(etagsList).replaceAll("$1");
300300
}
301301

302302
@Override

‎jetty-core/jetty-compression/jetty-compression-gzip/src/test/java/org/eclipse/jetty/compression/gzip/GzipCompressionTest.java‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@
2323
import org.junit.jupiter.api.Test;
2424

2525
import static java.nio.charset.StandardCharsets.UTF_8;
26-
import static org.hamcrest.MatcherAssert.assertThat;
27-
import static org.hamcrest.Matchers.is;
2826
import static org.junit.jupiter.api.Assertions.assertEquals;
2927

3028
public class GzipCompressionTest extends AbstractGzipTest
@@ -59,16 +57,6 @@ public void testBigBlockWithExtraBytesViaGzipInputStream() throws Exception
5957
}
6058
}
6159

62-
@Test
63-
public void testStripSuffixes() throws Exception
64-
{
65-
startGzip();
66-
assertThat(gzip.stripSuffixes("12345"), is("12345"));
67-
assertThat(gzip.stripSuffixes("12345, 666" + gzip.getEtagSuffix()), is("12345, 666"));
68-
assertThat(gzip.stripSuffixes("12345, 666" + gzip.getEtagSuffix() + ",W/\"9999" + gzip.getEtagSuffix() + "\""),
69-
is("12345, 666,W/\"9999\""));
70-
}
71-
7260
/**
7361
* Proof that the {@link GZIPInputStream} can read multiple entire blocks of GZIP compressed content (headers + data + trailers)
7462
* as a single set of decoded data, and does not terminate at the first {@link Inflater#finished()} or when reaching the

‎jetty-core/jetty-compression/jetty-compression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionHandler.java‎

Lines changed: 59 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import org.eclipse.jetty.compression.server.internal.DecompressionRequest;
2424
import org.eclipse.jetty.http.BadMessageException;
2525
import org.eclipse.jetty.http.ComplianceViolation;
26-
import org.eclipse.jetty.http.EtagUtils;
2726
import org.eclipse.jetty.http.HttpException;
2827
import org.eclipse.jetty.http.HttpField;
2928
import org.eclipse.jetty.http.HttpFields;
@@ -41,6 +40,7 @@
4140
import org.eclipse.jetty.util.Callback;
4241
import org.eclipse.jetty.util.StringUtil;
4342
import org.eclipse.jetty.util.TypeUtil;
43+
import org.eclipse.jetty.util.component.LifeCycle;
4444
import org.slf4j.Logger;
4545
import org.slf4j.LoggerFactory;
4646

@@ -56,8 +56,6 @@
5656
*/
5757
public class CompressionHandler extends Handler.Wrapper
5858
{
59-
public static final String HANDLER_ETAGS = CompressionHandler.class.getPackageName() + ".ETag";
60-
6159
private static final Logger LOG = LoggerFactory.getLogger(CompressionHandler.class);
6260
private final HttpField varyAcceptEncoding = new PreEncodedHttpField(HttpHeader.VARY, HttpHeader.ACCEPT_ENCODING.asString());
6361
private final Map<String, Compression> supportedEncodings = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
@@ -205,6 +203,15 @@ protected void doStart() throws Exception
205203
}
206204

207205
super.doStart();
206+
207+
pathConfigs.values().forEach(c -> LifeCycle.start(c));
208+
}
209+
210+
@Override
211+
protected void doStop() throws Exception
212+
{
213+
pathConfigs.values().forEach(c -> LifeCycle.stop(c));
214+
super.doStop();
208215
}
209216

210217
@Override
@@ -240,7 +247,8 @@ public boolean handle(Request request, Response response, Callback callback) thr
240247
// The `Accept-Encoding` request header indicating the supported list of compression encoding techniques.
241248
List<QuotedQualityCSV.QualityValue> requestAcceptEncoding = List.of();
242249
// Tracks the `If-Match` or `If-None-Match` request headers contains an etag separator.
243-
boolean etagMatches = false;
250+
String ifNoneMatch = null;
251+
String ifMatch = null;
244252

245253
QuotedQualityCSV qualityCSV = null;
246254
HttpFields fields = request.getHeaders();
@@ -280,7 +288,8 @@ protected void onComplianceViolation(ComplianceViolation violation)
280288
}
281289
qualityCSV.addValue(field.getValue());
282290
}
283-
case IF_MATCH, IF_NONE_MATCH -> etagMatches |= field.getValue().contains(EtagUtils.ETAG_SEPARATOR);
291+
case IF_MATCH -> ifMatch = HttpField.asList(ifMatch, field.getValue());
292+
case IF_NONE_MATCH -> ifNoneMatch = HttpField.asList(ifNoneMatch, field.getValue());
284293
}
285294
}
286295

@@ -317,44 +326,37 @@ protected void onComplianceViolation(ComplianceViolation violation)
317326
request, requestContentEncoding, requestAcceptEncoding, decompressEncoding, compressEncoding);
318327
}
319328

320-
if (decompressEncoding == null && compressEncoding == null)
321-
{
322-
if (LOG.isDebugEnabled())
323-
LOG.debug("skipping compression and decompression: no request encoding matches");
324-
// No need for a Vary header, as we will never deflate
325-
return next.handle(request, response, callback);
326-
}
329+
// wrap request if etags need to be adjusted
330+
if (ifMatch != null || ifNoneMatch != null)
331+
request = new StripEtagRequest(request, ifMatch, ifNoneMatch);
327332

328-
Request decompressionRequest = request;
329-
Response compressionResponse = response;
333+
// wrap the request if we can decompress.
334+
if (decompressEncoding != null)
335+
request = newDecompressionRequest(request, decompressEncoding);
330336

331-
// We need to wrap the request IFF we can inflate or have seen etags with compression separators.
332-
if (decompressEncoding != null || etagMatches)
333-
decompressionRequest = newDecompressionRequest(request, decompressEncoding);
334-
335-
// Wrap the response IFF we can deflate.
337+
// wrap the response if we can deflate.
336338
if (compressEncoding != null)
337339
{
338340
// The response may vary based on the presence or lack of Accept-Encoding.
339341
response.getHeaders().ensureField(varyAcceptEncoding);
340-
compressionResponse = newCompressionResponse(request, response, compressEncoding, config);
342+
response = newCompressionResponse(request, response, compressEncoding, config);
341343
}
342344

343345
if (LOG.isDebugEnabled())
344-
LOG.debug("handle {} {} {}", decompressionRequest, compressionResponse, this);
346+
LOG.debug("handle {} {} {}", request, response, this);
345347

346-
if (next.handle(decompressionRequest, compressionResponse, callback))
348+
if (next.handle(request, response, callback))
347349
return true;
348350

349-
if (request instanceof DecompressionRequest decompressRequest)
350-
decompressRequest.destroy();
351+
if (decompressEncoding != null)
352+
Request.as(request, DecompressionRequest.class).destroy();
351353

352354
return false;
353355
}
354356

355357
private Compression getCompression(String encoding)
356358
{
357-
Compression compression = supportedEncodings.get(encoding);
359+
Compression compression = encoding == null ? null : supportedEncodings.get(encoding);
358360
if (compression == null)
359361
{
360362
if (LOG.isDebugEnabled())
@@ -388,4 +390,36 @@ public String toString()
388390
{
389391
return String.format("%s@%x{%s,supported=%s}", TypeUtil.toShortName(getClass()), hashCode(), getState(), String.join(",", supportedEncodings.keySet()));
390392
}
393+
394+
private class StripEtagRequest extends Request.Wrapper
395+
{
396+
private final HttpFields _strippedHttpFields;
397+
398+
StripEtagRequest(Request request, String ifMatch, String ifNoneMatch)
399+
{
400+
super(request);
401+
{
402+
HttpFields.Mutable fields = HttpFields.build(request.getHeaders());
403+
if (ifMatch != null)
404+
{
405+
for (Compression known : supportedEncodings.values())
406+
ifMatch = known.stripSuffixes(ifMatch);
407+
fields.put(HttpHeader.IF_MATCH, ifMatch);
408+
}
409+
if (ifNoneMatch != null)
410+
{
411+
for (Compression known : supportedEncodings.values())
412+
ifNoneMatch = known.stripSuffixes(ifNoneMatch);
413+
fields.put(HttpHeader.IF_NONE_MATCH, ifNoneMatch);
414+
}
415+
_strippedHttpFields = fields.asImmutable();
416+
}
417+
}
418+
419+
@Override
420+
public HttpFields getHeaders()
421+
{
422+
return _strippedHttpFields;
423+
}
424+
}
391425
}

‎jetty-core/jetty-compression/jetty-compression-server/src/main/java/org/eclipse/jetty/compression/server/internal/CompressionResponse.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.eclipse.jetty.compression.EncoderSink;
2121
import org.eclipse.jetty.compression.server.CompressionConfig;
2222
import org.eclipse.jetty.http.HttpField;
23+
import org.eclipse.jetty.http.HttpFields;
2324
import org.eclipse.jetty.http.HttpHeader;
2425
import org.eclipse.jetty.http.HttpMethod;
2526
import org.eclipse.jetty.http.HttpStatus;
@@ -50,6 +51,8 @@ public CompressionResponse(Request request, Response wrapped, Compression compre
5051
@Override
5152
public void write(boolean last, ByteBuffer content, Callback callback)
5253
{
54+
HttpFields.Mutable headers = getHeaders();
55+
5356
switch (state.get())
5457
{
5558
case MIGHT_COMPRESS ->
@@ -68,9 +71,16 @@ public void write(boolean last, ByteBuffer content, Callback callback)
6871
return;
6972
}
7073

71-
// TODO: handle 304's etag.
74+
if (status == HttpStatus.NOT_MODIFIED_304)
75+
{
76+
if (LOG.isDebugEnabled())
77+
LOG.debug("no compression for status {} {}", status, this);
78+
state.compareAndSet(State.MIGHT_COMPRESS, State.NOT_COMPRESSING);
79+
headers.computeField(HttpHeader.ETAG, (name, value) -> (value == null || value.isEmpty()) ? null : new HttpField(HttpHeader.ETAG, compression.etag(value.get(0).getValue())));
80+
super.write(last, content, callback);
81+
}
7282

73-
HttpField contentTypeField = getHeaders().getField(HttpHeader.CONTENT_TYPE);
83+
HttpField contentTypeField = headers.getField(HttpHeader.CONTENT_TYPE);
7484
if (contentTypeField != null)
7585
{
7686
String mimeType = MimeTypes.getContentTypeWithoutCharset(contentTypeField.getValue());
@@ -85,7 +95,7 @@ public void write(boolean last, ByteBuffer content, Callback callback)
8595
}
8696

8797
// Did the application explicitly set the Content-Encoding?
88-
String contentEncoding = getHeaders().get(HttpHeader.CONTENT_ENCODING);
98+
String contentEncoding = headers.get(HttpHeader.CONTENT_ENCODING);
8999
if (contentEncoding != null)
90100
{
91101
if (LOG.isDebugEnabled())
@@ -105,7 +115,7 @@ public void write(boolean last, ByteBuffer content, Callback callback)
105115
return;
106116
}
107117

108-
long contentLength = getHeaders().getLongField(HttpHeader.CONTENT_LENGTH);
118+
long contentLength = headers.getLongField(HttpHeader.CONTENT_LENGTH);
109119
if (contentLength < 0 && last)
110120
contentLength = BufferUtil.length(content);
111121
if (contentLength >= 0 && contentLength < compression.getMinCompressSize())
@@ -124,9 +134,9 @@ public void write(boolean last, ByteBuffer content, Callback callback)
124134
this.encoderSink = compression.newEncoderSink(getWrapped());
125135

126136
// Adjust the headers.
127-
getHeaders().put(compression.getContentEncodingField());
128-
getHeaders().remove(HttpHeader.CONTENT_LENGTH);
129-
// TODO: etag.
137+
headers.put(compression.getContentEncodingField());
138+
headers.remove(HttpHeader.CONTENT_LENGTH);
139+
headers.computeField(HttpHeader.ETAG, (name, value) -> (value == null || value.isEmpty()) ? null : new HttpField(HttpHeader.ETAG, compression.etag(value.get(0).getValue())));
130140

131141
this.write(last, content, callback);
132142
}

‎jetty-core/jetty-compression/jetty-compression-server/src/main/java/org/eclipse/jetty/compression/server/internal/DecompressionRequest.java‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717

1818
import org.eclipse.jetty.compression.Compression;
1919
import org.eclipse.jetty.compression.DecoderSource;
20-
import org.eclipse.jetty.compression.server.CompressionHandler;
2120
import org.eclipse.jetty.http.HttpField;
2221
import org.eclipse.jetty.http.HttpFields;
2322
import org.eclipse.jetty.http.HttpHeader;
@@ -101,20 +100,7 @@ else if (field.containsLast(compression.getEncodingName()))
101100
}
102101
}
103102
}
104-
case IF_MATCH, IF_NONE_MATCH ->
105-
{
106-
String etags = field.getValue();
107-
String etagsNoSuffix = compression.stripSuffixes(etags);
108-
if (!etagsNoSuffix.equals(etags))
109-
{
110-
i.set(new HttpField(field.getHeader(), etagsNoSuffix));
111-
request.setAttribute(CompressionHandler.HANDLER_ETAGS, etags);
112-
}
113-
}
114-
case CONTENT_LENGTH ->
115-
{
116-
i.set(new HttpField("X-Content-Length", field.getValue()));
117-
}
103+
case CONTENT_LENGTH -> i.set(new HttpField("X-Content-Length", field.getValue()));
118104
}
119105
}
120106
return newFields.asImmutable();

0 commit comments

Comments
 (0)