More optional etag gzip fixes for #5979 (#5986)

* More optional etag gzip fixes for #5979

IF no separator defined, do not add a suffix to an etag.
Some cleanup of the implementation.

* More optional etag gzip fixes for #5979

updates from review

jetty-http/src/main/java/org/eclipse/jetty/http/CompressedContentFormat.java

@@ -20,6 +20,7 @@
 
 import java.util.Objects;
 
+import org.eclipse.jetty.util.QuotedStringTokenizer;
 import org.eclipse.jetty.util.StringUtil;
 
 public class CompressedContentFormat
@@ -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);
     }
 
@@ -61,20 +62,104 @@ 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;
+    }
+
+    /** Get an etag with suffix that represents this compressed type.
+     * @param etag An etag
+     * @return An etag with compression suffix, or the etag itself if no suffix is configured.
+     */
+    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)
+    /** Check etags for equality, accounting for quoting and compression suffixes.
+     * @param etag An etag without a compression suffix
+     * @param etagWithSuffix An etag optionally with a compression suffix.
+     * @return True if the tags are equal.
+     */
+    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;
     }
 }

jetty-http/src/main/java/org/eclipse/jetty/http/PrecompressedHttpContent.java

@@ -71,7 +71,7 @@ public HttpField getETag()
     @Override
     public String getETagValue()
     {
-        return _content.getResource().getWeakETag(_format._etag);
+        return _content.getResource().getWeakETag(_format.getEtagSuffix());
     }
 
     @Override
@@ -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
@@ -167,7 +167,9 @@ 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("%s@%x{e=%s,r=%s|%s,lm=%s|%s,ct=%s}",
+            this.getClass().getSimpleName(), hashCode(),
+            _format,
             _content.getResource(), _precompressedContent.getResource(),
             _content.getResource().lastModified(), _precompressedContent.getResource().lastModified(),
             getContentType());

jetty-http/src/test/java/org/eclipse/jetty/http/GZIPContentDecoderTest.java

@@ -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;
@@ -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

jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java

@@ -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())
                     {
@@ -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);
@@ -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()

jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java

@@ -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())

jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java

@@ -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)
@@ -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());
                 }
             }
 
@@ -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;
             }
 
@@ -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;

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java

@@ -307,7 +307,7 @@ public boolean isGzip()
     {
         for (CompressedContentFormat formats : _resourceService.getPrecompressedFormats())
         {
-            if (CompressedContentFormat.GZIP._encoding.equals(formats._encoding))
+            if (CompressedContentFormat.GZIP.getEncoding().equals(formats.getEncoding()))
                 return true;
         }
         return false;