Clean up of BasicHttpCache and related test classes

httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java

@@ -32,12 +32,11 @@ import java.util.Map;
 
 import org.apache.hc.client5.http.StandardMethods;
 import org.apache.hc.client5.http.cache.HeaderConstants;
+import org.apache.hc.client5.http.cache.HttpCacheCASOperation;
 import org.apache.hc.client5.http.cache.HttpCacheEntry;
 import org.apache.hc.client5.http.cache.HttpCacheInvalidator;
 import org.apache.hc.client5.http.cache.HttpCacheStorage;
-import org.apache.hc.client5.http.cache.HttpCacheCASOperation;
 import org.apache.hc.client5.http.cache.HttpCacheUpdateException;
-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;
@@ -50,9 +49,8 @@ import org.apache.logging.log4j.Logger;
 
 class BasicHttpCache implements HttpCache {
 
-    private final CacheKeyGenerator uriExtractor;
-    private final ResourceFactory resourceFactory;
     private final CacheEntryUpdater cacheEntryUpdater;
+    private final CacheKeyGenerator cacheKeyGenerator;
     private final HttpCacheInvalidator cacheInvalidator;
     private final HttpCacheStorage storage;
 
@@ -61,11 +59,10 @@ class BasicHttpCache implements HttpCache {
     public BasicHttpCache(
             final ResourceFactory resourceFactory,
             final HttpCacheStorage storage,
-            final CacheKeyGenerator uriExtractor,
+            final CacheKeyGenerator cacheKeyGenerator,
             final HttpCacheInvalidator cacheInvalidator) {
-        this.resourceFactory = resourceFactory;
-        this.uriExtractor = uriExtractor;
         this.cacheEntryUpdater = new CacheEntryUpdater(resourceFactory);
+        this.cacheKeyGenerator = cacheKeyGenerator;
         this.storage = storage;
         this.cacheInvalidator = cacheInvalidator;
     }
@@ -73,8 +70,8 @@ class BasicHttpCache implements HttpCache {
     public BasicHttpCache(
             final ResourceFactory resourceFactory,
             final HttpCacheStorage storage,
-            final CacheKeyGenerator uriExtractor) {
+            final CacheKeyGenerator cacheKeyGenerator) {
-        this(resourceFactory, storage, uriExtractor, new DefaultCacheInvalidator(uriExtractor, storage));
+        this(resourceFactory, storage, cacheKeyGenerator, new DefaultCacheInvalidator(cacheKeyGenerator, storage));
     }
 
     public BasicHttpCache(final ResourceFactory resourceFactory, final HttpCacheStorage storage) {
@@ -92,7 +89,7 @@ class BasicHttpCache implements HttpCache {
     @Override
     public void flushCacheEntriesFor(final HttpHost host, final HttpRequest request) throws ResourceIOException {
         if (!StandardMethods.isSafe(request.getMethod())) {
-            final String uri = uriExtractor.generateKey(host, request);
+            final String uri = cacheKeyGenerator.generateKey(host, request);
             storage.removeEntry(uri);
         }
     }
@@ -115,7 +112,7 @@ class BasicHttpCache implements HttpCache {
 
     void storeNonVariantEntry(
             final HttpHost target, final HttpRequest req, final HttpCacheEntry entry) throws ResourceIOException {
-        final String uri = uriExtractor.generateKey(target, req);
+        final String uri = cacheKeyGenerator.generateKey(target, req);
         storage.putEntry(uri, entry);
     }
 
@@ -123,9 +120,9 @@ class BasicHttpCache implements HttpCache {
             final HttpHost target,
             final HttpRequest req,
             final HttpCacheEntry entry) throws ResourceIOException {
-        final String parentCacheKey = uriExtractor.generateKey(target, req);
+        final String parentCacheKey = cacheKeyGenerator.generateKey(target, req);
-        final String variantKey = uriExtractor.generateVariantKey(req, entry);
+        final String variantKey = cacheKeyGenerator.generateVariantKey(req, entry);
-        final String variantURI = uriExtractor.generateVariantURI(target, req, entry);
+        final String variantURI = cacheKeyGenerator.generateVariantURI(target, req, entry);
         storage.putEntry(variantURI, entry);
 
         try {
@@ -133,7 +130,7 @@ class BasicHttpCache implements HttpCache {
 
                 @Override
                 public HttpCacheEntry execute(final HttpCacheEntry existing) throws ResourceIOException {
-                    return doGetUpdatedParentEntry(req.getRequestUri(), existing, entry, variantKey, variantURI);
+                    return cacheEntryUpdater.updateParentCacheEntry(req.getRequestUri(), existing, entry, variantKey, variantURI);
                 }
 
             });
@@ -145,9 +142,9 @@ class BasicHttpCache implements HttpCache {
     @Override
     public void reuseVariantEntryFor(
             final HttpHost target, final HttpRequest req, final Variant variant) throws ResourceIOException {
-        final String parentCacheKey = uriExtractor.generateKey(target, req);
+        final String parentCacheKey = cacheKeyGenerator.generateKey(target, req);
         final HttpCacheEntry entry = variant.getEntry();
-        final String variantKey = uriExtractor.generateVariantKey(req, entry);
+        final String variantKey = cacheKeyGenerator.generateVariantKey(req, entry);
         final String variantCacheKey = variant.getCacheKey();
 
         try {
@@ -155,7 +152,7 @@ class BasicHttpCache implements HttpCache {
 
                 @Override
                 public HttpCacheEntry execute(final HttpCacheEntry existing) throws ResourceIOException {
-                    return doGetUpdatedParentEntry(req.getRequestUri(), existing, entry, variantKey, variantCacheKey);
+                    return cacheEntryUpdater.updateParentCacheEntry(req.getRequestUri(), existing, entry, variantKey, variantCacheKey);
                 }
 
             });
@@ -164,32 +161,6 @@ class BasicHttpCache implements HttpCache {
         }
     }
 
-    HttpCacheEntry doGetUpdatedParentEntry(
-            final String requestId,
-            final HttpCacheEntry existing,
-            final HttpCacheEntry entry,
-            final String variantKey,
-            final String variantCacheKey) throws ResourceIOException {
-        HttpCacheEntry src = existing;
-        if (src == null) {
-            src = entry;
-        }
-
-        Resource resource = null;
-        if (src.getResource() != null) {
-            resource = resourceFactory.copy(requestId, src.getResource());
-        }
-        final Map<String,String> variantMap = new HashMap<>(src.getVariantMap());
-        variantMap.put(variantKey, variantCacheKey);
-        return new HttpCacheEntry(
-                src.getRequestDate(),
-                src.getResponseDate(),
-                src.getStatus(),
-                src.getAllHeaders(),
-                resource,
-                variantMap);
-    }
-
     @Override
     public HttpCacheEntry updateCacheEntry(
             final HttpHost target,
@@ -229,32 +200,21 @@ class BasicHttpCache implements HttpCache {
             final ByteArrayBuffer content,
             final Date requestSent,
             final Date responseReceived) throws ResourceIOException {
-        final Resource resource;
+        final HttpCacheEntry entry = cacheEntryUpdater.createtCacheEntry(request, originResponse, content, requestSent, responseReceived);
-        if (content != null) {
-            resource = resourceFactory.generate(request.getRequestUri(), content.array(), 0, content.length());
-        } else {
-            resource = null;
-        }
-        final HttpCacheEntry entry = new HttpCacheEntry(
-                requestSent,
-                responseReceived,
-                originResponse.getCode(),
-                originResponse.getAllHeaders(),
-                resource);
         storeInCache(host, request, entry);
         return entry;
     }
 
     @Override
     public HttpCacheEntry getCacheEntry(final HttpHost host, final HttpRequest request) throws ResourceIOException {
-        final HttpCacheEntry root = storage.getEntry(uriExtractor.generateKey(host, request));
+        final HttpCacheEntry root = storage.getEntry(cacheKeyGenerator.generateKey(host, request));
         if (root == null) {
             return null;
         }
         if (!root.hasVariants()) {
             return root;
         }
-        final String variantCacheKey = root.getVariantMap().get(uriExtractor.generateVariantKey(request, root));
+        final String variantCacheKey = root.getVariantMap().get(cacheKeyGenerator.generateVariantKey(request, root));
         if (variantCacheKey == null) {
             return null;
         }
@@ -271,7 +231,7 @@ class BasicHttpCache implements HttpCache {
     public Map<String, Variant> getVariantCacheEntriesWithEtags(final HttpHost host, final HttpRequest request)
             throws ResourceIOException {
         final Map<String,Variant> variants = new HashMap<>();
-        final HttpCacheEntry root = storage.getEntry(uriExtractor.generateKey(host, request));
+        final HttpCacheEntry root = storage.getEntry(cacheKeyGenerator.generateKey(host, request));
         if (root == null || !root.hasVariants()) {
             return variants;
         }

httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheEntryUpdater.java

@@ -29,8 +29,10 @@ package org.apache.hc.client5.http.impl.cache;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
 import java.util.ListIterator;
+import java.util.Map;
 
 import org.apache.hc.client5.http.cache.HeaderConstants;
 import org.apache.hc.client5.http.cache.HttpCacheEntry;
@@ -42,9 +44,11 @@ import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
 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.HttpResponse;
 import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.util.Args;
+import org.apache.hc.core5.util.ByteArrayBuffer;
 
 /**
  * Update a {@link HttpCacheEntry} with new or updated information based on the latest
@@ -67,17 +71,29 @@ class CacheEntryUpdater {
         this.resourceFactory = resourceFactory;
     }
 
+    public HttpCacheEntry createtCacheEntry(
+            final HttpRequest request,
+            final HttpResponse originResponse,
+            final ByteArrayBuffer content,
+            final Date requestSent,
+            final Date responseReceived) throws ResourceIOException {
+        final Resource resource;
+        if (content != null) {
+            resource = resourceFactory.generate(request.getRequestUri(), content.array(), 0, content.length());
+        } else {
+            resource = null;
+        }
+        return new HttpCacheEntry(
+                requestSent,
+                responseReceived,
+                originResponse.getCode(),
+                originResponse.getAllHeaders(),
+                resource);
+    }
+
     /**
      * Update the entry with the new information from the response.  Should only be used for
      * 304 responses.
-     *
-     * @param requestId
-     * @param entry The cache Entry to be updated
-     * @param requestDate When the request was performed
-     * @param responseDate When the response was gotten
-     * @param response The HttpResponse from the backend server call
-     * @return HttpCacheEntry an updated version of the cache entry
-     * @throws ResourceIOException if something bad happens while trying to read the body from the original entry
      */
     public HttpCacheEntry updateCacheEntry(
             final String requestId,
@@ -171,4 +187,30 @@ class CacheEntryUpdater {
         return false;
     }
 
+    public HttpCacheEntry updateParentCacheEntry(
+            final String requestId,
+            final HttpCacheEntry existing,
+            final HttpCacheEntry entry,
+            final String variantKey,
+            final String variantCacheKey) throws ResourceIOException {
+        HttpCacheEntry src = existing;
+        if (src == null) {
+            src = entry;
+        }
+
+        Resource resource = null;
+        if (src.getResource() != null) {
+            resource = resourceFactory.copy(requestId, src.getResource());
+        }
+        final Map<String,String> variantMap = new HashMap<>(src.getVariantMap());
+        variantMap.put(variantKey, variantCacheKey);
+        return new HttpCacheEntry(
+                src.getRequestDate(),
+                src.getResponseDate(),
+                src.getStatus(),
+                src.getAllHeaders(),
+                resource,
+                variantMap);
+    }
+
 }

httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/ContainsHeaderMatcher.java

@@ -0,0 +1,74 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.cache;
+
+import java.util.Iterator;
+
+import org.apache.hc.client5.http.cache.HttpCacheEntry;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.MessageHeaders;
+import org.apache.hc.core5.util.LangUtils;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Factory;
+import org.hamcrest.Matcher;
+
+public class ContainsHeaderMatcher extends BaseMatcher<HttpCacheEntry> {
+
+    private final String headerName;
+    private final Object headerValue;
+
+    public ContainsHeaderMatcher(final String headerName, final Object headerValue) {
+        this.headerName = headerName;
+        this.headerValue = headerValue;
+    }
+
+    @Override
+    public boolean matches(final Object item) {
+        if (item instanceof MessageHeaders) {
+            final MessageHeaders messageHeaders = (MessageHeaders) item;
+            for (final Iterator<Header> it = messageHeaders.headerIterator(); it.hasNext(); ) {
+                final Header header = it.next();
+                if (headerName.equalsIgnoreCase(header.getName()) && LangUtils.equals(headerValue, header.getValue())) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
+    @Override
+    public void describeTo(final Description description) {
+        description.appendText("contains header ").appendValue(headerValue).appendText(": ").appendValue(headerValue);
+    }
+
+    @Factory
+    public static Matcher<HttpCacheEntry> contains(final String headerName, final Object headerValue) {
+        return new ContainsHeaderMatcher(headerName, headerValue);
+    }
+
+}

httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestBasicHttpCache.java

@@ -33,7 +33,6 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 
 import java.util.Date;
-import java.util.HashMap;
 import java.util.Map;
 
 import org.apache.hc.client5.http.cache.HeaderConstants;
@@ -181,25 +180,6 @@ public class TestBasicHttpCache {
         assertNull(backing.map.get(key));
     }
 
-    @Test
-    public void testCacheUpdateAddsVariantURIToParentEntry() throws Exception {
-        final String parentCacheKey = "parentCacheKey";
-        final String variantCacheKey = "variantCacheKey";
-        final String existingVariantKey = "existingVariantKey";
-        final String newVariantCacheKey = "newVariantCacheKey";
-        final String newVariantKey = "newVariantKey";
-        final Map<String,String> existingVariants = new HashMap<>();
-        existingVariants.put(existingVariantKey, variantCacheKey);
-        final HttpCacheEntry parent = HttpTestUtils.makeCacheEntry(existingVariants);
-        final HttpCacheEntry variant = HttpTestUtils.makeCacheEntry();
-
-        final HttpCacheEntry result = impl.doGetUpdatedParentEntry(parentCacheKey, parent, variant, newVariantKey, newVariantCacheKey);
-        final Map<String,String> resultMap = result.getVariantMap();
-        assertEquals(2, resultMap.size());
-        assertEquals(variantCacheKey, resultMap.get(existingVariantKey));
-        assertEquals(newVariantCacheKey, resultMap.get(newVariantKey));
-    }
-
     @Test
     public void testStoreInCachePutsNonVariantEntryInPlace() throws Exception {
         final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry();

httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheEntryUpdater.java

@@ -28,10 +28,13 @@ package org.apache.hc.client5.http.impl.cache;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 
 import java.io.IOException;
 import java.util.Date;
+import java.util.HashMap;
+import java.util.Map;
 
 import org.apache.hc.client5.http.cache.HttpCacheEntry;
 import org.apache.hc.client5.http.utils.DateUtils;
@@ -93,11 +96,8 @@ public class TestCacheEntryUpdater {
         final HttpCacheEntry updatedEntry = impl.updateCacheEntry(null, entry,
                 new Date(), new Date(), response);
 
-
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("Date", DateUtils.formatDate(responseDate)));
-        final Header[] updatedHeaders = updatedEntry.getAllHeaders();
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("ETag", "\"etag\""));
-        assertEquals(2, updatedHeaders.length);
-        headersContain(updatedHeaders, "Date", DateUtils.formatDate(responseDate));
-        headersContain(updatedHeaders, "ETag", "\"etag\"");
     }
 
     @Test
@@ -116,13 +116,10 @@ public class TestCacheEntryUpdater {
         final HttpCacheEntry updatedEntry = impl.updateCacheEntry(null, entry,
                 new Date(), new Date(), response);
 
-        final Header[] updatedHeaders = updatedEntry.getAllHeaders();
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("Date", DateUtils.formatDate(requestDate)));
-
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("ETag", "\"etag\""));
-        assertEquals(4, updatedHeaders.length);
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("Last-Modified", DateUtils.formatDate(responseDate)));
-        headersContain(updatedHeaders, "Date", DateUtils.formatDate(requestDate));
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("Cache-Control", "public"));
-        headersContain(updatedHeaders, "ETag", "\"etag\"");
-        headersContain(updatedHeaders, "Last-Modified", DateUtils.formatDate(responseDate));
-        headersContain(updatedHeaders, "Cache-Control", "public");
     }
 
     @Test
@@ -139,13 +136,10 @@ public class TestCacheEntryUpdater {
         final HttpCacheEntry updatedEntry = impl.updateCacheEntry(null, entry,
                 new Date(), new Date(), response);
 
-        final Header[] updatedHeaders = updatedEntry.getAllHeaders();
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("Date", DateUtils.formatDate(requestDate)));
-        assertEquals(4, updatedHeaders.length);
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("ETag", "\"etag\""));
-
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("Last-Modified", DateUtils.formatDate(responseDate)));
-        headersContain(updatedHeaders, "Date", DateUtils.formatDate(requestDate));
+        assertThat(updatedEntry, ContainsHeaderMatcher.contains("Cache-Control", "public"));
-        headersContain(updatedHeaders, "ETag", "\"etag\"");
-        headersContain(updatedHeaders, "Last-Modified", DateUtils.formatDate(responseDate));
-        headersContain(updatedHeaders, "Cache-Control", "public");
     }
 
     @Test
@@ -160,9 +154,8 @@ public class TestCacheEntryUpdater {
         response.setHeader("ETag", "\"old-etag\"");
         final HttpCacheEntry result = impl.updateCacheEntry("A", entry, new Date(),
                 new Date(), response);
-        assertEquals(2, result.getAllHeaders().length);
+        assertThat(result, ContainsHeaderMatcher.contains("Date", DateUtils.formatDate(oneSecondAgo)));
-        headersContain(result.getAllHeaders(), "Date", DateUtils.formatDate(oneSecondAgo));
+        assertThat(result, ContainsHeaderMatcher.contains("ETag", "\"new-etag\""));
-        headersContain(result.getAllHeaders(), "ETag", "\"new-etag\"");
     }
 
     @Test
@@ -234,15 +227,23 @@ public class TestCacheEntryUpdater {
         }
     }
 
-    private void headersContain(final Header[] headers, final String name, final String value) {
+    @Test
-        for (final Header header : headers) {
+    public void testCacheUpdateAddsVariantURIToParentEntry() throws Exception {
-            if (header.getName().equals(name)) {
+        final String parentCacheKey = "parentCacheKey";
-                if (header.getValue().equals(value)) {
+        final String variantCacheKey = "variantCacheKey";
-                    return;
+        final String existingVariantKey = "existingVariantKey";
-                }
+        final String newVariantCacheKey = "newVariantCacheKey";
-            }
+        final String newVariantKey = "newVariantKey";
-        }
+        final Map<String,String> existingVariants = new HashMap<>();
-        fail("Header [" + name + ": " + value + "] not found in headers.");
+        existingVariants.put(existingVariantKey, variantCacheKey);
+        final HttpCacheEntry parent = HttpTestUtils.makeCacheEntry(existingVariants);
+        final HttpCacheEntry variant = HttpTestUtils.makeCacheEntry();
+
+        final HttpCacheEntry result = impl.updateParentCacheEntry(parentCacheKey, parent, variant, newVariantKey, newVariantCacheKey);
+        final Map<String,String> resultMap = result.getVariantMap();
+        assertEquals(2, resultMap.size());
+        assertEquals(variantCacheKey, resultMap.get(existingVariantKey));
+        assertEquals(newVariantCacheKey, resultMap.get(newVariantKey));
     }
 
 }