Skip to content

Commit

Permalink
More optional etag gzip fixes for #5979
Browse files Browse the repository at this point in the history 
IF no separator defined, do not add a suffix to an etag.
Some cleanup of the implementation.
  • Loading branch information
@gregw
gregw committed Feb 18, 2021
1 parent addfbe8 commit 7d28b42
Show file tree
Hide file tree
Showing 10 changed files with 147 additions and 57 deletions.
102 changes: 89 additions & 13 deletions jetty-http/src/main/java/org/eclipse/jetty/http/CompressedContentFormat.java
View file
Expand Up @@ -20,6 +20,7 @@

import java.util.Objects;

import org.eclipse.jetty.util.QuotedStringTokenizer;
import org.eclipse.jetty.util.StringUtil;

public class CompressedContentFormat
Expand All @@ -37,18 +38,18 @@ public class CompressedContentFormat
public static final CompressedContentFormat BR = new CompressedContentFormat("br", ".br");
public static final CompressedContentFormat[] NONE = new CompressedContentFormat[0];

public final String _encoding;
public final String _extension;
public final String _etag;
public final String _etagQuote;
public final PreEncodedHttpField _contentEncoding;
private final String _encoding;
private final String _extension;
private final String _etagSuffix;
private final String _etagSuffixQuote;
private final PreEncodedHttpField _contentEncoding;

public CompressedContentFormat(String encoding, String extension)
{
_encoding = StringUtil.asciiToLowerCase(encoding);
_extension = StringUtil.asciiToLowerCase(extension);
_etag = ETAG_SEPARATOR + _encoding;
_etagQuote = _etag + "\"";
_etagSuffix = StringUtil.isEmpty(ETAG_SEPARATOR) ? "" : (ETAG_SEPARATOR + _encoding);
_etagSuffixQuote = _etagSuffix + "\"";
_contentEncoding = new PreEncodedHttpField(HttpHeader.CONTENT_ENCODING, _encoding);
}

Expand All @@ -61,20 +62,95 @@ public boolean equals(Object o)
return Objects.equals(_encoding, ccf._encoding) && Objects.equals(_extension, ccf._extension);
}

public String getEncoding()
{
return _encoding;
}

public String getExtension()
{
return _extension;
}

public String getEtagSuffix()
{
return _etagSuffix;
}

public HttpField getContentEncoding()
{
return _contentEncoding;
}

public String etag(String etag)
{
if (StringUtil.isEmpty(ETAG_SEPARATOR))
return etag;
int end = etag.length() - 1;
if (etag.charAt(end) == '"')
return etag.substring(0, end) + _etagSuffixQuote;
return etag + _etagSuffix;
}

@Override
public int hashCode()
{
return Objects.hash(_encoding, _extension);
}

public static boolean tagEquals(String etag, String tag)
public static boolean tagEquals(String etag, String etagWithSuffix)
{
if (etag.equals(tag))
// Handle simple equality
if (etag.equals(etagWithSuffix))
return true;

int separator = tag.lastIndexOf(ETAG_SEPARATOR);
if (separator > 0 && separator == etag.length() - 1)
return etag.regionMatches(0, tag, 0, separator);
return false;
// If no separator defined, then simple equality is only possible positive
if (StringUtil.isEmpty(ETAG_SEPARATOR))
return false;

// Are both tags quoted?
boolean etagQuoted = etag.endsWith("\"");
boolean etagSuffixQuoted = etagWithSuffix.endsWith("\"");

// Look for a separator
int separator = etagWithSuffix.lastIndexOf(ETAG_SEPARATOR);

// If both tags are quoted the same (the norm) then any difference must be the suffix
if (etagQuoted == etagSuffixQuoted)
return separator > 0 && etag.regionMatches(0, etagWithSuffix, 0, separator);

// If either tag is weak then we can't match because weak tags must be quoted
if (etagWithSuffix.startsWith("W/") || etag.startsWith("W/"))
return false;

// compare unquoted strong etags
etag = etagQuoted ? QuotedStringTokenizer.unquote(etag) : etag;
etagWithSuffix = etagSuffixQuoted ? QuotedStringTokenizer.unquote(etagWithSuffix) : etagWithSuffix;
separator = etagWithSuffix.lastIndexOf(ETAG_SEPARATOR);
if (separator > 0)
return etag.regionMatches(0, etagWithSuffix, 0, separator);

return Objects.equals(etag, etagWithSuffix);
}

public String stripSuffixes(String etagsList)
{
if (StringUtil.isEmpty(ETAG_SEPARATOR))
return etagsList;

// This is a poor implementation that ignores list and tag structure
while (true)
{
int i = etagsList.lastIndexOf(_etagSuffix);
if (i < 0)
return etagsList;
etagsList = etagsList.substring(0, i) + etagsList.substring(i + _etagSuffix.length());
}
}

@Override
public String toString()
{
return _encoding;
}
}
8 changes: 4 additions & 4 deletions jetty-http/src/main/java/org/eclipse/jetty/http/PrecompressedHttpContent.java
View file
Expand Up @@ -71,7 +71,7 @@ public HttpField getETag()
@Override
public String getETagValue()
{
return _content.getResource().getWeakETag(_format._etag);
return _content.getResource().getWeakETag(_format.getEtagSuffix());
}

@Override
Expand Down Expand Up @@ -101,13 +101,13 @@ public String getContentTypeValue()
@Override
public HttpField getContentEncoding()
{
return _format._contentEncoding;
return _format.getContentEncoding();
}

@Override
public String getContentEncodingValue()
{
return _format._contentEncoding.getValue();
return _format.getContentEncoding().getValue();
}

@Override
Expand Down Expand Up @@ -167,7 +167,7 @@ public ReadableByteChannel getReadableByteChannel() throws IOException
@Override
public String toString()
{
return String.format("PrecompressedHttpContent@%x{e=%s,r=%s|%s,lm=%s|%s,ct=%s}", hashCode(), _format._encoding,
return String.format("PrecompressedHttpContent@%x{e=%s,r=%s|%s,lm=%s|%s,ct=%s}", hashCode(), _format,
_content.getResource(), _precompressedContent.getResource(),
_content.getResource().lastModified(), _precompressedContent.getResource().lastModified(),
getContentType());
Expand Down
22 changes: 20 additions & 2 deletions jetty-http/src/test/java/org/eclipse/jetty/http/GZIPContentDecoderTest.java
View file
Expand Up @@ -35,6 +35,8 @@
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import static org.eclipse.jetty.http.CompressedContentFormat.BR;
import static org.eclipse.jetty.http.CompressedContentFormat.GZIP;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -80,9 +82,25 @@ public void testCompressedContentFormat()
{
assertTrue(CompressedContentFormat.tagEquals("tag", "tag"));
assertTrue(CompressedContentFormat.tagEquals("\"tag\"", "\"tag\""));
assertTrue(CompressedContentFormat.tagEquals("\"tag\"", "\"tag--gzip\""));
assertFalse(CompressedContentFormat.tagEquals("Zag", "Xag--gzip"));
assertTrue(CompressedContentFormat.tagEquals("\"tag\"", "\"tag" + GZIP.getEtagSuffix() + "\""));
assertTrue(CompressedContentFormat.tagEquals("\"tag\"", "\"tag" + BR.getEtagSuffix() + "\""));
assertTrue(CompressedContentFormat.tagEquals("W/\"1234567\"", "W/\"1234567\""));
assertTrue(CompressedContentFormat.tagEquals("W/\"1234567\"", "W/\"1234567" + GZIP.getEtagSuffix() + "\""));

assertFalse(CompressedContentFormat.tagEquals("Zag", "Xag" + GZIP.getEtagSuffix()));
assertFalse(CompressedContentFormat.tagEquals("xtag", "tag"));
assertFalse(CompressedContentFormat.tagEquals("W/\"1234567\"", "W/\"1234111\""));
assertFalse(CompressedContentFormat.tagEquals("W/\"1234567\"", "W/\"1234111" + GZIP.getEtagSuffix() + "\""));

assertTrue(CompressedContentFormat.tagEquals("12345", "\"12345\""));
assertTrue(CompressedContentFormat.tagEquals("\"12345\"", "12345"));
assertTrue(CompressedContentFormat.tagEquals("12345", "\"12345" + GZIP.getEtagSuffix() + "\""));
assertTrue(CompressedContentFormat.tagEquals("\"12345\"", "12345" + GZIP.getEtagSuffix()));

assertThat(GZIP.stripSuffixes("12345"), is("12345"));
assertThat(GZIP.stripSuffixes("12345, 666" + GZIP.getEtagSuffix()), is("12345, 666"));
assertThat(GZIP.stripSuffixes("12345, 666" + GZIP.getEtagSuffix() + ",W/\"9999" + GZIP.getEtagSuffix() + "\""),
is("12345, 666,W/\"9999\""));
}

@Test
Expand Down
6 changes: 3 additions & 3 deletions jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java
View file
Expand Up @@ -235,7 +235,7 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS
Map<CompressedContentFormat, CachedHttpContent> precompresssedContents = new HashMap<>(_precompressedFormats.length);
for (CompressedContentFormat format : _precompressedFormats)
{
String compressedPathInContext = pathInContext + format._extension;
String compressedPathInContext = pathInContext + format.getExtension();
CachedHttpContent compressedContent = _cache.get(compressedPathInContext);
if (compressedContent == null || compressedContent.isValid())
{
Expand Down Expand Up @@ -280,7 +280,7 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS
Map<CompressedContentFormat, HttpContent> compressedContents = new HashMap<>();
for (CompressedContentFormat format : _precompressedFormats)
{
String compressedPathInContext = pathInContext + format._extension;
String compressedPathInContext = pathInContext + format.getExtension();
CachedHttpContent compressedContent = _cache.get(compressedPathInContext);
if (compressedContent != null && compressedContent.isValid() && compressedContent.getResource().lastModified() >= resource.lastModified())
compressedContents.put(format, compressedContent);
Expand Down Expand Up @@ -693,7 +693,7 @@ public class CachedPrecompressedHttpContent extends PrecompressedHttpContent
_content = content;
_precompressedContent = precompressedContent;

_etag = (CachedContentFactory.this._etags) ? new PreEncodedHttpField(HttpHeader.ETAG, _content.getResource().getWeakETag(format._etag)) : null;
_etag = (CachedContentFactory.this._etags) ? new PreEncodedHttpField(HttpHeader.ETAG, _content.getResource().getWeakETag(format.getEtagSuffix())) : null;
}

public boolean isValid()
Expand Down
2 changes: 1 addition & 1 deletion jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java
View file
Expand Up @@ -84,7 +84,7 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS
Map<CompressedContentFormat, HttpContent> compressedContents = new HashMap<>(_precompressedFormats.length);
for (CompressedContentFormat format : _precompressedFormats)
{
String compressedPathInContext = pathInContext + format._extension;
String compressedPathInContext = pathInContext + format.getExtension();
Resource compressedResource = _factory.getResource(compressedPathInContext);
if (compressedResource != null && compressedResource.exists() && compressedResource.lastModified() >= resource.lastModified() &&
compressedResource.length() < resource.length())
Expand Down
10 changes: 5 additions & 5 deletions jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java
View file
Expand Up @@ -141,7 +141,7 @@ public CompressedContentFormat[] getPrecompressedFormats()
public void setPrecompressedFormats(CompressedContentFormat[] precompressedFormats)
{
_precompressedFormats = precompressedFormats;
_preferredEncodingOrder = stream(_precompressedFormats).map(f -> f._encoding).toArray(String[]::new);
_preferredEncodingOrder = stream(_precompressedFormats).map(f -> f.getEncoding()).toArray(String[]::new);
}

public void setEncodingCacheSize(int encodingCacheSize)
Expand Down Expand Up @@ -282,7 +282,7 @@ public boolean doGet(HttpServletRequest request, HttpServletResponse response)
if (LOG.isDebugEnabled())
LOG.debug("precompressed={}", precompressedContent);
content = precompressedContent;
response.setHeader(HttpHeader.CONTENT_ENCODING.asString(), precompressedContentEncoding._encoding);
response.setHeader(HttpHeader.CONTENT_ENCODING.asString(), precompressedContentEncoding.getEncoding());
}
}

Expand Down Expand Up @@ -355,7 +355,7 @@ private CompressedContentFormat getBestPrecompressedContent(List<String> preferr
{
for (CompressedContentFormat format : availableFormats)
{
if (format._encoding.equals(encoding))
if (format.getEncoding().equals(encoding))
return format;
}

Expand Down Expand Up @@ -531,9 +531,9 @@ protected boolean passConditionalHeaders(HttpServletRequest request, HttpServlet
if (etag != null)
{
QuotedCSV quoted = new QuotedCSV(true, ifm);
for (String tag : quoted)
for (String etagWithSuffix : quoted)
{
if (CompressedContentFormat.tagEquals(etag, tag))
if (CompressedContentFormat.tagEquals(etag, etagWithSuffix))
{
match = true;
break;
Expand Down

0 comments on commit 7d28b42

Please sign in to comment.