Improve RFC 9111 cache control compliance
diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java index 7a4d03d..91b36b6 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java
@@ -79,12 +79,10 @@ SimpleHttpResponse generateResponse(final HttpRequest request, final HttpCacheEn } final TimeValue age = this.validityStrategy.getCurrentAge(entry, now); - if (TimeValue.isPositive(age)) { - if (age.compareTo(CacheSupport.MAX_AGE) >= 0) { - response.setHeader(HttpHeaders.AGE, Long.toString(CacheSupport.MAX_AGE.toSeconds())); - } else { - response.setHeader(HttpHeaders.AGE, Long.toString(age.toSeconds())); - } + if (age.compareTo(CacheSupport.MAX_AGE) >= 0) { + response.setHeader(HttpHeaders.AGE, Long.toString(CacheSupport.MAX_AGE.toSeconds())); + } else { + response.setHeader(HttpHeaders.AGE, Long.toString(age.toSeconds())); } return response;
diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java index 069bf40..d930473 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java
@@ -102,11 +102,6 @@ public CacheSuitability assessSuitability(final RequestCacheControl requestCache return CacheSuitability.MISMATCH; } - if (!requestHeadersMatch(request, entry)) { - LOG.debug("Request headers nominated by the cached response do not match those of the request associated with the cache entry"); - return CacheSuitability.MISMATCH; - } - if (requestCacheControl.isNoCache()) { LOG.debug("Request contained no-cache directive; the cache entry must be re-validated"); return CacheSuitability.REVALIDATION_REQUIRED; @@ -148,7 +143,8 @@ public CacheSuitability assessSuitability(final RequestCacheControl requestCache return CacheSuitability.REVALIDATION_REQUIRED; } - if (!fresh && sharedCache && responseCacheControl.isProxyRevalidate()) { + if (!fresh && sharedCache + && (responseCacheControl.isProxyRevalidate() || responseCacheControl.getSharedMaxAge() > -1)) { LOG.debug("Response from cache is not suitable due to the response proxy-revalidate requirement"); return CacheSuitability.REVALIDATION_REQUIRED; }
diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java index 9672fe2..f5d25c3 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java
@@ -131,13 +131,18 @@ public boolean isResponseCacheable(final ResponseCacheControl cacheControl, fina } } - if (cacheControl.isMustUnderstand() && !understoodStatusCode(code)) { - // must-understand cache directive overrides no-store - LOG.debug("Response contains a status code that the cache does not understand, so it's not cacheable"); - return false; - } - - if (isExplicitlyNonCacheable(cacheControl)) { + if (cacheControl.isMustUnderstand()) { + if (!understoodStatusCode(code)) { + // must-understand: cache does not understand the status code, do not store + LOG.debug("Response contains a status code that the cache does not understand, so it's not cacheable"); + return false; + } + // Status code is in a recognized range; treat no-store as overridden. + if (sharedCache && cacheControl.isCachePrivate()) { + LOG.debug("Response is private and cannot be cached by a shared cache"); + return false; + } + } else if (isExplicitlyNonCacheable(cacheControl)) { LOG.debug("Response is explicitly non-cacheable per cache control directive"); return false; } @@ -185,12 +190,19 @@ public boolean isResponseCacheable(final ResponseCacheControl cacheControl, fina return isExplicitlyCacheable(cacheControl, response) || isHeuristicallyCacheable(cacheControl, code, responseDate, responseExpires); } + // Heuristically cacheable status codes private static boolean isKnownCacheableStatusCode(final int status) { return status == HttpStatus.SC_OK || status == HttpStatus.SC_NON_AUTHORITATIVE_INFORMATION || + status == HttpStatus.SC_NO_CONTENT || status == HttpStatus.SC_MULTIPLE_CHOICES || status == HttpStatus.SC_MOVED_PERMANENTLY || - status == HttpStatus.SC_GONE; + status == 308 /* Permanent Redirect */ || + status == HttpStatus.SC_NOT_FOUND || + status == HttpStatus.SC_METHOD_NOT_ALLOWED || + status == HttpStatus.SC_GONE || + status == HttpStatus.SC_REQUEST_URI_TOO_LONG || + status == HttpStatus.SC_NOT_IMPLEMENTED; } private static boolean isKnownNonCacheableStatusCode(final int status) { @@ -345,8 +357,8 @@ private Duration calculateFreshnessLifetime(final ResponseCacheControl cacheCont return DEFAULT_FRESHNESS_DURATION; // 5 minutes } - // Check if s-maxage is present and use its value if it is - if (cacheControl.getSharedMaxAge() != -1) { + // s-maxage applies only to shared caches + if (sharedCache && cacheControl.getSharedMaxAge() != -1) { return Duration.ofSeconds(cacheControl.getSharedMaxAge()); } else if (cacheControl.getMaxAge() != -1) { return Duration.ofSeconds(cacheControl.getMaxAge());
diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedHttpResponseGenerator.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedHttpResponseGenerator.java index 59eca4b..a909630 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedHttpResponseGenerator.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedHttpResponseGenerator.java
@@ -56,6 +56,8 @@ void setUp() { entry = HttpTestUtils.makeCacheEntry(); request = HttpTestUtils.makeDefaultRequest(); mockValidityPolicy = mock(CacheValidityPolicy.class); + when(mockValidityPolicy.getCurrentAge(isA(HttpCacheEntry.class), isA(Instant.class))) + .thenReturn(TimeValue.ofSeconds(0L)); impl = new CachedHttpResponseGenerator(mockValidityPolicy); } @@ -93,15 +95,17 @@ void testAgeHeaderIsPopulatedWithCurrentAgeOfCacheEntryIfNonZero() throws Except } @Test - void testAgeHeaderIsNotPopulatedIfCurrentAgeOfCacheEntryIsZero() throws Exception { + void testAgeHeaderIsPopulatedIfCurrentAgeOfCacheEntryIsZero() throws Exception { currentAge(TimeValue.ofSeconds(0L)); final SimpleHttpResponse response = impl.generateResponse(request, entry); verify(mockValidityPolicy).getCurrentAge(same(entry), isA(Instant.class)); + // A cache MUST generate an Age header field final Header ageHdr = response.getFirstHeader("Age"); - Assertions.assertNull(ageHdr); + Assertions.assertNotNull(ageHdr); + Assertions.assertEquals(0L, Long.parseLong(ageHdr.getValue())); } @Test
diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java index 416fe15..61df079 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java
@@ -458,6 +458,36 @@ void testNotSuitableIfGetRequestWithHeadCacheEntry() { } @Test + void testNotSuitableIfStaleWithSharedMaxAgeInSharedCache() { + final CacheConfig sharedConfig = CacheConfig.custom() + .setSharedCache(true) + .build(); + impl = new CachedResponseSuitabilityChecker(sharedConfig); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + responseCacheControl = ResponseCacheControl.builder() + .setSharedMaxAge(5) + .build(); + Assertions.assertEquals(CacheSuitability.REVALIDATION_REQUIRED, + impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); + } + + @Test + void testSuitableIfFreshWithSharedMaxAgeInSharedCache() { + final CacheConfig sharedConfig = CacheConfig.custom() + .setSharedCache(true) + .build(); + impl = new CachedResponseSuitabilityChecker(sharedConfig); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + responseCacheControl = ResponseCacheControl.builder() + .setSharedMaxAge(3600) + .build(); + Assertions.assertEquals(CacheSuitability.FRESH, + impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); + } + + @Test void testSuitableIfErrorRequestCacheControl() { // Prepare a cache entry with HEAD method entry = makeEntry(Method.GET, "/foo",
diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java index 4a11bec..4169653 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java
@@ -1638,7 +1638,7 @@ void testUnknownMethodRequestsAreWrittenThroughToOrigin() throws Exception { } @Test - void testTransmitsAgeHeaderIfIncomingAgeHeaderTooBig() throws Exception { + void testReplacesAgeHeaderIfIncomingAgeHeaderTooBig() throws Exception { final String reallyOldAge = "1" + Long.MAX_VALUE; originResponse.setHeader("Age",reallyOldAge); @@ -1646,8 +1646,12 @@ void testTransmitsAgeHeaderIfIncomingAgeHeaderTooBig() throws Exception { final ClassicHttpResponse result = execute(request); - Assertions.assertEquals(reallyOldAge, - result.getFirstHeader("Age").getValue()); + // A cache MUST generate an Age header field, replacing any present in the response + // with a value equal to the stored response's current_age + final Header ageHdr = result.getFirstHeader("Age"); + Assertions.assertNotNull(ageHdr); + final long ageValue = Long.parseLong(ageHdr.getValue()); + Assertions.assertTrue(ageValue >= 0); } @Test
diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java index bdddf7c..fe263e9 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java
@@ -29,7 +29,6 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; -import java.time.Duration; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.Random; @@ -370,14 +369,15 @@ void testControlAnyCacheControlCacheable() { } @Test - void testControlWithout200Cacheable() { - HttpResponse response404 = new BasicHttpResponse(HttpStatus.SC_NOT_FOUND, ""); + void testControlWithNonCacheableStatusCode() { + // 402 Payment Required is not a heuristically cacheable status code + HttpResponse response402 = new BasicHttpResponse(HttpStatus.SC_PAYMENT_REQUIRED, ""); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response404)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response402)); - response404 = new BasicHttpResponse(HttpStatus.SC_GATEWAY_TIMEOUT, ""); + response402 = new BasicHttpResponse(HttpStatus.SC_GATEWAY_TIMEOUT, ""); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response404)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response402)); } @Test @@ -814,80 +814,24 @@ void otherStatusCodesAreCacheableWithExplicitCachingHeaders() { } @Test - void testIsResponseCacheableNullCacheControl() { - - // Set up test data - final Duration tenSecondsFromNow = Duration.ofSeconds(10); - + void testCacheableWithExpiresAndMaxAge() { response = new BasicHttpResponse(HttpStatus.SC_OK, ""); response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); - response.setHeader(HttpHeaders.EXPIRES, DateUtils.formatStandardDate(Instant.now().plus(tenSecondsFromNow))); + response.setHeader(HttpHeaders.EXPIRES, DateUtils.formatStandardDate(Instant.now().plusSeconds(10))); - - // Create ResponseCachingPolicy instance and test the method policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET", "/foo"); + + // Cacheable with Expires header alone assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); - } - - @Test - void testIsResponseCacheableNotNullCacheControlSmaxAge60() { - - // Set up test data - final Duration tenSecondsFromNow = Duration.ofSeconds(10); - - response = new BasicHttpResponse(HttpStatus.SC_OK, ""); - response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); - response.setHeader(HttpHeaders.EXPIRES, DateUtils.formatStandardDate(Instant.now().plus(tenSecondsFromNow))); - - - // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false); - request = new BasicHttpRequest("GET", "/foo"); + // Cacheable with explicit max-age responseCacheControl = ResponseCacheControl.builder() .setMaxAge(60) .build(); assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } - @Test - void testIsResponseCacheableNotNullCacheControlMaxAge60() { - - // Set up test data - final Duration tenSecondsFromNow = Duration.ofSeconds(10); - - response = new BasicHttpResponse(HttpStatus.SC_OK, ""); - response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); - response.setHeader(HttpHeaders.EXPIRES, DateUtils.formatStandardDate(Instant.now().plus(tenSecondsFromNow))); - - - // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false); - request = new BasicHttpRequest("GET", "/foo"); - responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(60) - .build(); - assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); - } - - @Test - void testIsResponseCacheableNotExsiresAndDate() { - - // Set up test data - final Duration tenSecondsFromNow = Duration.ofSeconds(10); - - response = new BasicHttpResponse(HttpStatus.SC_OK, ""); - response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); - response.setHeader(HttpHeaders.EXPIRES, DateUtils.formatStandardDate(Instant.now().plus(tenSecondsFromNow))); - - - // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false); - request = new BasicHttpRequest("GET", "/foo"); - assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); - } - private int getRandomStatus() { final int rnd = new Random().nextInt(acceptableCodes.length); @@ -1000,4 +944,117 @@ void testMustRevalidateWithAuthorizationIsCacheable() { "Response with must-revalidate and Authorization header should be cacheable in shared cache."); } + @Test + void testMustUnderstandWithNoStoreAndUnderstoodStatusIsCacheable() { + response = new BasicHttpResponse(HttpStatus.SC_OK, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + responseCacheControl = ResponseCacheControl.builder() + .setMustUnderstand(true) + .setNoStore(true) + .setCachePublic(true) + .build(); + assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testMustUnderstandWithNoStoreAndUnknownStatusIsNotCacheable() { + response = new BasicHttpResponse(600, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + responseCacheControl = ResponseCacheControl.builder() + .setMustUnderstand(true) + .setNoStore(true) + .setCachePublic(true) + .build(); + assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testNoStoreWithoutMustUnderstandIsNotCacheable() { + response = new BasicHttpResponse(HttpStatus.SC_OK, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + responseCacheControl = ResponseCacheControl.builder() + .setNoStore(true) + .build(); + assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testMustUnderstandWithNoStoreAndPrivateNotCacheableBySharedCache() { + // must-understand overrides no-store but private must still be enforced + response = new BasicHttpResponse(HttpStatus.SC_OK, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + responseCacheControl = ResponseCacheControl.builder() + .setMustUnderstand(true) + .setNoStore(true) + .setCachePrivate(true) + .build(); + assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testMustUnderstandWithNoStoreAndPrivateCacheableByNonSharedCache() { + policy = new ResponseCachingPolicy(false, false, false); + response = new BasicHttpResponse(HttpStatus.SC_OK, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + responseCacheControl = ResponseCacheControl.builder() + .setMustUnderstand(true) + .setNoStore(true) + .setCachePrivate(true) + .build(); + assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testPrivateCacheIgnoresSharedMaxAgeForFreshness() { + policy = new ResponseCachingPolicy(false, false, false); + response = new BasicHttpResponse(HttpStatus.SC_OK, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + responseCacheControl = ResponseCacheControl.builder() + .setSharedMaxAge(0) + .setMaxAge(3600) + .build(); + assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testHeuristicallyCacheable204() { + response = new BasicHttpResponse(HttpStatus.SC_NO_CONTENT, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Last-Modified", DateUtils.formatStandardDate(sixSecondsAgo)); + assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testHeuristicallyCacheable404() { + response = new BasicHttpResponse(HttpStatus.SC_NOT_FOUND, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + response.setHeader("Last-Modified", DateUtils.formatStandardDate(sixSecondsAgo)); + assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testHeuristicallyCacheable405() { + response = new BasicHttpResponse(HttpStatus.SC_METHOD_NOT_ALLOWED, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + response.setHeader("Last-Modified", DateUtils.formatStandardDate(sixSecondsAgo)); + assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + void testHeuristicallyCacheable501() { + response = new BasicHttpResponse(HttpStatus.SC_NOT_IMPLEMENTED, ""); + response.setHeader("Date", DateUtils.formatStandardDate(now)); + response.setHeader("Content-Length", "0"); + response.setHeader("Last-Modified", DateUtils.formatStandardDate(sixSecondsAgo)); + assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + } \ No newline at end of file