From 1492f57a84fcc36b320e736dadb9cae931ca99e7 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Mon, 30 Oct 2023 17:29:20 +0100 Subject: [PATCH] HTTPCLIENT-2277: Cache update bug fix --- .../http/cache/HttpCacheEntryFactory.java | 13 ++++++++--- .../http/impl/cache/BasicHttpAsyncCache.java | 4 ++++ .../http/impl/cache/BasicHttpCache.java | 6 ++++- .../http/cache/TestHttpCacheEntryFactory.java | 22 ++++++++++++------- 4 files changed, 33 insertions(+), 12 deletions(-) diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java index f8dc90d81..91c6ae26a 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java @@ -232,12 +232,16 @@ public class HttpCacheEntryFactory { * * @param requestInstant Date/time when the request was made (Used for age calculations) * @param responseInstant Date/time that the response came back (Used for age calculations) + * @param host Target host + * @param request Original client request (a deep copy of this object is made) * @param response Origin response (a deep copy of this object is made) * @param entry Existing cache entry. */ public HttpCacheEntry createUpdated( final Instant requestInstant, final Instant responseInstant, + final HttpHost host, + final HttpRequest request, final HttpResponse response, final HttpCacheEntry entry) { Args.notNull(requestInstant, "Request instant"); @@ -249,13 +253,16 @@ public class HttpCacheEntryFactory { if (HttpCacheEntry.isNewer(entry, response)) { return entry; } + final String s = CacheKeyGenerator.getRequestUri(host, request); + final URI uri = CacheKeyGenerator.normalize(s); + final HeaderGroup requestHeaders = filterHopByHopHeaders(request); final HeaderGroup mergedHeaders = mergeHeaders(entry, response); return new HttpCacheEntry( requestInstant, responseInstant, - entry.getRequestMethod(), - entry.getRequestURI(), - headers(entry.requestHeaderIterator()), + request.getMethod(), + uri.toASCIIString(), + requestHeaders, entry.getStatus(), mergedHeaders, entry.getResource(), diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java index 2e2568283..e0da6e04f 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java @@ -435,6 +435,8 @@ class BasicHttpAsyncCache implements HttpAsyncCache { final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, stale.entry); final String variantKey = cacheKeyGenerator.generateVariantKey(request, updatedEntry); @@ -456,6 +458,8 @@ class BasicHttpAsyncCache implements HttpAsyncCache { final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, negotiated.entry); 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 index fe61c7611..b63561068 100644 --- 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 @@ -266,6 +266,8 @@ class BasicHttpCache implements HttpCache { final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, stale.entry); final String variantKey = cacheKeyGenerator.generateVariantKey(request, updatedEntry); @@ -286,8 +288,10 @@ class BasicHttpCache implements HttpCache { final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, - negotiated.entry); + negotiated.entry); storeInternal(negotiated.getEntryKey(), updatedEntry); final String rootKey = cacheKeyGenerator.generateKey(host, request); diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java index 24adff6ea..96e3fdb0b 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java @@ -211,7 +211,7 @@ public class TestHttpCacheEntryFactory { @Test public void testUpdateCacheEntryReturnsDifferentEntryInstance() { entry = HttpTestUtils.makeCacheEntry(); - final HttpCacheEntry newEntry = impl.createUpdated(requestDate, responseDate, response, entry); + final HttpCacheEntry newEntry = impl.createUpdated(requestDate, responseDate, host, request, response, entry); Assertions.assertNotSame(newEntry, entry); } @@ -339,17 +339,23 @@ public class TestHttpCacheEntryFactory { new BasicHeader("Cache-Control", "public") ); - final HttpCacheEntry updatedEntry = impl.createUpdated(tenSecondsAgo, oneSecondAgo, response, entry); + request.setHeaders( + new BasicHeader("X-custom", "my other stuff"), + new BasicHeader(HttpHeaders.ACCEPT, "stuff, other-stuff"), + new BasicHeader(HttpHeaders.ACCEPT_LANGUAGE, "en, de") + ); + + final HttpCacheEntry updatedEntry = impl.createUpdated(tenSecondsAgo, oneSecondAgo, host, request, response, entry); MatcherAssert.assertThat(updatedEntry, HttpCacheEntryMatcher.equivalent( HttpTestUtils.makeCacheEntry( tenSecondsAgo, oneSecondAgo, Method.GET, - "/stuff", + "http://foo.example.com:80/stuff", new Header[]{ - new BasicHeader("X-custom", "my stuff"), - new BasicHeader(HttpHeaders.ACCEPT, "stuff"), + new BasicHeader("X-custom", "my other stuff"), + new BasicHeader(HttpHeaders.ACCEPT, "stuff, other-stuff"), new BasicHeader(HttpHeaders.ACCEPT_LANGUAGE, "en, de") }, HttpStatus.SC_NOT_MODIFIED, @@ -374,7 +380,7 @@ public class TestHttpCacheEntryFactory { response.setHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)); response.setHeader("ETag", "\"old-etag\""); - final HttpCacheEntry newEntry = impl.createUpdated(Instant.now(), Instant.now(), response, entry); + final HttpCacheEntry newEntry = impl.createUpdated(Instant.now(), Instant.now(), host, request, response, entry); Assertions.assertSame(newEntry, entry); } @@ -382,7 +388,7 @@ public class TestHttpCacheEntryFactory { @Test public void testUpdateHasLatestRequestAndResponseDates() { entry = HttpTestUtils.makeCacheEntry(tenSecondsAgo, eightSecondsAgo); - final HttpCacheEntry updated = impl.createUpdated(twoSecondsAgo, oneSecondAgo, response, entry); + final HttpCacheEntry updated = impl.createUpdated(twoSecondsAgo, oneSecondAgo, host, request, response, entry); Assertions.assertEquals(twoSecondsAgo, updated.getRequestInstant()); Assertions.assertEquals(oneSecondAgo, updated.getResponseInstant()); @@ -393,7 +399,7 @@ public class TestHttpCacheEntryFactory { entry = HttpTestUtils.makeCacheEntry(); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); Assertions.assertThrows(IllegalArgumentException.class, () -> - impl.createUpdated(Instant.now(), Instant.now(), response, entry)); + impl.createUpdated(Instant.now(), Instant.now(), host, request, response, entry)); } }