Better ETag handling

apache · Jan 12, 2024 · c8f958d · c8f958d
1 parent 2e7e29c
commit c8f958d
Show file tree

Hide file tree

Showing 12 changed files with 132 additions and 90 deletions.
diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntry.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntry.java
@@ -40,6 +40,7 @@
 import java.util.stream.Collectors;
 
 import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.Internal;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
@@ -79,6 +80,8 @@ public class HttpCacheEntry implements MessageHeaders, Serializable {
     private final AtomicReference<Instant> expiresRef;
     private final AtomicReference<Instant> lastModifiedRef;
 
+    private final AtomicReference<ETag> eTagRef;
+
     /**
      * Internal constructor that makes no validation of the input parameters and makes
      * no copies of the original client request and the origin response.
@@ -107,6 +110,7 @@ public HttpCacheEntry(
         this.dateRef = new AtomicReference<>();
         this.expiresRef = new AtomicReference<>();
         this.lastModifiedRef = new AtomicReference<>();
+        this.eTagRef = new AtomicReference<>();
     }
 
     /**
@@ -179,6 +183,7 @@ public HttpCacheEntry(
         this.dateRef = new AtomicReference<>();
         this.expiresRef = new AtomicReference<>();
         this.lastModifiedRef = new AtomicReference<>();
+        this.eTagRef = new AtomicReference<>();
     }
 
     /**
@@ -396,6 +401,23 @@ public Instant getLastModified() {
         return getInstant(lastModifiedRef, HttpHeaders.LAST_MODIFIED);
     }
 
+    /**
+     * @since 5.4
+     */
+    public ETag getETag() {
+        ETag eTag = eTagRef.get();
+        if (eTag == null) {
+            eTag = ETag.get(this);
+            if (eTag == null) {
+                return null;
+            }
+            if (!eTagRef.compareAndSet(null, eTag)) {
+                eTag = eTagRef.get();
+            }
+        }
+        return eTag;
+    }
+
     /**
      * Returns the {@link Resource} containing the origin response body.
      */

diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java
@@ -30,7 +30,6 @@
 import java.io.InterruptedIOException;
 import java.nio.ByteBuffer;
 import java.time.Instant;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
@@ -55,6 +54,7 @@
 import org.apache.hc.client5.http.impl.ExecSupport;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.apache.hc.client5.http.schedule.SchedulingStrategy;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
 import org.apache.hc.core5.concurrent.CancellableDependency;
@@ -1206,16 +1206,17 @@ void negotiateResponseFromVariants(
             final Collection<CacheHit> variants) {
         final String exchangeId = scope.exchangeId;
         final CancellableDependency operation = scope.cancellableDependency;
-        final Map<String, CacheHit> variantMap = new HashMap<>();
+        final Map<ETag, CacheHit> variantMap = new HashMap<>();
         for (final CacheHit variant : variants) {
-            final Header header = variant.entry.getFirstHeader(HttpHeaders.ETAG);
-            if (header != null) {
-                variantMap.put(header.getValue(), variant);
+            final ETag eTag = variant.entry.getETag();
+            if (eTag != null) {
+                variantMap.put(eTag, variant);
             }
         }
+
         final HttpRequest conditionalRequest = conditionalRequestBuilder.buildConditionalRequestFromVariants(
-                BasicRequestBuilder.copy(request).build(),
-                new ArrayList<>(variantMap.keySet()));
+                request,
+                variantMap.keySet());
 
         final Instant requestDate = getCurrentDate();
         chainProceed(conditionalRequest, entityProducer, scope, chain, new AsyncExecCallback() {
@@ -1270,14 +1271,13 @@ public AsyncDataConsumer handleResponse(
                 if (backendResponse.getCode() != HttpStatus.SC_NOT_MODIFIED) {
                     callback = new BackendResponseHandler(target, request, requestDate, responseDate, scope, asyncExecCallback);
                 } else {
-                    final Header resultEtagHeader = backendResponse.getFirstHeader(HttpHeaders.ETAG);
-                    if (resultEtagHeader == null) {
+                    final ETag resultEtag = ETag.get(backendResponse);
+                    if (resultEtag == null) {
                         if (LOG.isDebugEnabled()) {
                             LOG.debug("{} 304 response did not contain ETag", exchangeId);
                         }
                         callback = new AsyncExecCallbackWrapper(() -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed);
                     } else {
-                        final String resultEtag = resultEtagHeader.getValue();
                         final CacheHit match = variantMap.get(resultEtag);
                         if (match == null) {
                             if (LOG.isDebugEnabled()) {

diff --git a/...lient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java b/...lient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java
@@ -33,7 +33,6 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -46,11 +45,11 @@
 import org.apache.hc.client5.http.cache.ResourceFactory;
 import org.apache.hc.client5.http.cache.ResourceIOException;
 import org.apache.hc.client5.http.impl.Operations;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.concurrent.CallbackContribution;
 import org.apache.hc.core5.concurrent.Cancellable;
 import org.apache.hc.core5.concurrent.ComplexCancellable;
 import org.apache.hc.core5.concurrent.FutureCallback;
-import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.HttpRequest;
@@ -520,10 +519,10 @@ public void completed(final HttpCacheEntry root) {
                     if (LOG.isDebugEnabled()) {
                         LOG.debug("Evicting root cache entry {}", rootKey);
                     }
-                    final Header existingETag = root.getFirstHeader(HttpHeaders.ETAG);
-                    final Header newETag = response.getFirstHeader(HttpHeaders.ETAG);
+                    final ETag existingETag = root.getETag();
+                    final ETag newETag = ETag.get(response);
                     if (existingETag != null && newETag != null &&
-                            !Objects.equals(existingETag.getValue(), newETag.getValue()) &&
+                            !ETag.strongCompare(existingETag, newETag) &&
                             !HttpCacheEntry.isNewer(root, response)) {
                         evictAll(root, rootKey);
                     }

diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java
@@ -32,7 +32,6 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Objects;
 import java.util.Set;
 
 import org.apache.hc.client5.http.cache.HttpCacheCASOperation;
@@ -43,7 +42,7 @@
 import org.apache.hc.client5.http.cache.Resource;
 import org.apache.hc.client5.http.cache.ResourceFactory;
 import org.apache.hc.client5.http.cache.ResourceIOException;
-import org.apache.hc.core5.http.Header;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.HttpRequest;
@@ -329,10 +328,10 @@ private void evict(final String rootKey, final HttpResponse response) {
         if (root == null) {
             return;
         }
-        final Header existingETag = root.getFirstHeader(HttpHeaders.ETAG);
-        final Header newETag = response.getFirstHeader(HttpHeaders.ETAG);
+        final ETag existingETag = root.getETag();
+        final ETag newETag = ETag.get(response);
         if (existingETag != null && newETag != null &&
-                !Objects.equals(existingETag.getValue(), newETag.getValue()) &&
+                !ETag.strongCompare(existingETag, newETag) &&
                 !HttpCacheEntry.isNewer(root, response)) {
             evictAll(root, rootKey);
         }

diff --git a/...ache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java b/...ache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java
@@ -33,6 +33,7 @@
 import org.apache.hc.client5.http.cache.Resource;
 import org.apache.hc.client5.http.cache.ResourceIOException;
 import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpHeaders;
@@ -108,9 +109,9 @@ SimpleHttpResponse generateNotModifiedResponse(final HttpCacheEntry entry) {
 
         // - ETag and/or Content-Location, if the header would have been sent
         //   in a 200 response to the same request
-        final Header etagHeader = entry.getFirstHeader(HttpHeaders.ETAG);
-        if (etagHeader != null) {
-            response.addHeader(etagHeader);
+        final ETag eTag = entry.getETag();
+        if (eTag != null) {
+            response.addHeader(new BasicHeader(HttpHeaders.ETAG, eTag.toString()));
         }
 
         final Header contentLocationHeader = entry.getFirstHeader(HttpHeaders.CONTENT_LOCATION);
@@ -142,7 +143,7 @@ SimpleHttpResponse generateNotModifiedResponse(final HttpCacheEntry entry) {
         //above listed fields unless said metadata exists for the purpose of
         //guiding cache updates (e.g., Last-Modified might be useful if the
         //response does not have an ETag field).
-        if (etagHeader == null) {
+        if (eTag == null) {
             final Header lastModifiedHeader = entry.getFirstHeader(HttpHeaders.LAST_MODIFIED);
             if (lastModifiedHeader != null) {
                 response.addHeader(lastModifiedHeader);

diff --git a/...src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java b/...src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java
@@ -41,8 +41,8 @@
 import org.apache.hc.client5.http.cache.RequestCacheControl;
 import org.apache.hc.client5.http.cache.ResponseCacheControl;
 import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.http.Header;
-import org.apache.hc.core5.http.HeaderElement;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.HttpStatus;
@@ -332,13 +332,14 @@ boolean hasSupportedLastModifiedValidator(final HttpRequest request) {
      * @return boolean does the etag validator match
      */
     boolean etagValidatorMatches(final HttpRequest request, final HttpCacheEntry entry) {
-        final Header etagHeader = entry.getFirstHeader(HttpHeaders.ETAG);
-        final String etag = (etagHeader != null) ? etagHeader.getValue() : null;
-        final Iterator<HeaderElement> it = MessageSupport.iterate(request, HttpHeaders.IF_NONE_MATCH);
+        final ETag etag = entry.getETag();
+        if (etag == null) {
+            return false;
+        }
+        final Iterator<String> it = MessageSupport.iterateTokens(request, HttpHeaders.IF_NONE_MATCH);
         while (it.hasNext()) {
-            final HeaderElement elt = it.next();
-            final String reqEtag = elt.toString();
-            if (("*".equals(reqEtag) && etag != null) || reqEtag.equals(etag)) {
+            final String token = it.next();
+            if ("*".equals(token) || ETag.weakCompare(etag, ETag.parse(token))) {
                 return true;
             }
         }

diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java
@@ -29,7 +29,6 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.time.Instant;
-import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -49,6 +48,7 @@
 import org.apache.hc.client5.http.classic.ExecChainHandler;
 import org.apache.hc.client5.http.impl.ExecSupport;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.ContentType;
@@ -605,17 +605,17 @@ ClassicHttpResponse negotiateResponseFromVariants(
             final List<CacheHit> variants) throws IOException, HttpException {
         final String exchangeId = scope.exchangeId;
 
-        final Map<String, CacheHit> variantMap = new HashMap<>();
+        final Map<ETag, CacheHit> variantMap = new HashMap<>();
         for (final CacheHit variant : variants) {
-            final Header header = variant.entry.getFirstHeader(HttpHeaders.ETAG);
-            if (header != null) {
-                variantMap.put(header.getValue(), variant);
+            final ETag eTag = variant.entry.getETag();
+            if (eTag != null) {
+                variantMap.put(eTag, variant);
             }
         }
 
         final ClassicHttpRequest conditionalRequest = conditionalRequestBuilder.buildConditionalRequestFromVariants(
                 request,
-                new ArrayList<>(variantMap.keySet()));
+                variantMap.keySet());
 
         final Instant requestDate = getCurrentDate();
         final ClassicHttpResponse backendResponse = chain.proceed(conditionalRequest, scope);
@@ -629,15 +629,14 @@ ClassicHttpResponse negotiateResponseFromVariants(
                 backendResponse.close();
             }
 
-            final Header resultEtagHeader = backendResponse.getFirstHeader(HttpHeaders.ETAG);
-            if (resultEtagHeader == null) {
+            final ETag resultEtag = ETag.get(backendResponse);
+            if (resultEtag == null) {
                 if (LOG.isDebugEnabled()) {
                     LOG.debug("{} 304 response did not contain ETag", exchangeId);
                 }
                 return callBackend(target, request, scope, chain);
             }
 
-            final String resultEtag = resultEtagHeader.getValue();
             final CacheHit match = variantMap.get(resultEtag);
             if (match == null) {
                 if (LOG.isDebugEnabled()) {

diff --git a/...-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ConditionalRequestBuilder.java b/...-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ConditionalRequestBuilder.java
@@ -26,16 +26,18 @@
  */
 package org.apache.hc.client5.http.impl.cache;
 
-import java.util.List;
+import java.util.Collection;
 
 import org.apache.hc.client5.http.cache.HeaderConstants;
 import org.apache.hc.client5.http.cache.HttpCacheEntry;
 import org.apache.hc.client5.http.cache.ResponseCacheControl;
+import org.apache.hc.client5.http.validator.ETag;
 import org.apache.hc.core5.function.Factory;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpRequest;
-import org.apache.hc.core5.http.message.MessageSupport;
+import org.apache.hc.core5.http.message.BufferedHeader;
+import org.apache.hc.core5.util.CharArrayBuffer;
 
 class ConditionalRequestBuilder<T extends HttpRequest> {
 
@@ -59,9 +61,9 @@ class ConditionalRequestBuilder<T extends HttpRequest> {
     public T buildConditionalRequest(final ResponseCacheControl cacheControl, final T request, final HttpCacheEntry cacheEntry) {
         final T newRequest = messageCopier.create(request);
 
-        final Header eTag = cacheEntry.getFirstHeader(HttpHeaders.ETAG);
+        final ETag eTag = cacheEntry.getETag();
         if (eTag != null) {
-            newRequest.setHeader(HttpHeaders.IF_NONE_MATCH, eTag.getValue());
+            newRequest.setHeader(HttpHeaders.IF_NONE_MATCH, eTag.toString());
         }
         final Header lastModified = cacheEntry.getFirstHeader(HttpHeaders.LAST_MODIFIED);
         if (lastModified != null) {
@@ -85,9 +87,20 @@ public T buildConditionalRequest(final ResponseCacheControl cacheControl, final
      * @param variants
      * @return the wrapped request
      */
-    public T buildConditionalRequestFromVariants(final T request, final List<String> variants) {
+    public T buildConditionalRequestFromVariants(final T request, final Collection<ETag> variants) {
         final T newRequest = messageCopier.create(request);
-        newRequest.setHeader(MessageSupport.headerOfTokens(HttpHeaders.IF_NONE_MATCH, variants));
+        final CharArrayBuffer buffer = new CharArrayBuffer(256);
+        buffer.append(HttpHeaders.IF_NONE_MATCH);
+        buffer.append(": ");
+        int i = 0;
+        for (final ETag variant : variants) {
+            if (i > 0) {
+                buffer.append(", ");
+            }
+            variant.format(buffer);
+            i++;
+        }
+        newRequest.setHeader(BufferedHeader.create(buffer));
         return newRequest;
     }