Deprecate the never-cache-responses-with-query options and make them opt-in

Assigning heuristic freshness to a response with a query component is no longer prohibited by the HTTP caching specification. ResponseCachingPolicy now consults the never-cache-query options only when they are explicitly enabled, so in the default configuration a response with a query component is cached like any other. isNeverCacheHTTP10ResponsesWithQuery, isNeverCacheHTTP11ResponsesWithQuery and their builder setters are deprecated, as is HTTP/1.0 response caching. An origin that does not want a response cached should send an explicit directive such as Cache-Control: no-cache.

Author: arturobernalg

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

@@ -41,6 +41,12 @@
  * {@link CacheConfig#isNeverCacheHTTP11ResponsesWithQuery()},
  * {@link CacheConfig#isStaleIfErrorEnabled()}</p>
  *
+ * <p><b>Query string and HTTP/1.0 caching.</b> The options that suppress caching of responses with a query
+ * component are deprecated, since assigning heuristic freshness to such a response is no longer prohibited; an
+ * origin that does not want a response cached should send an explicit directive such as
+ * {@code Cache-Control: no-cache}. HTTP/1.0 response caching is deprecated as a whole and will be removed in a
+ * future release.</p>
+ *
  * <p><b>Cache size.</b> If the backend storage supports these limits, one
  * can specify the {@link CacheConfig#getMaxCacheEntries maximum number of
  * cache entries} as well as the {@link CacheConfig#getMaxObjectSize()}
@@ -189,7 +195,13 @@ public long getMaxObjectSize() {
      * Returns whether the cache will never cache HTTP 1.0 responses with a query string or not.
      * @return {@code true} to not cache query string responses, {@code false} to cache if explicit cache headers are
      * found
+     *
+     * @deprecated Assigning heuristic freshness to a response with a query component is no longer prohibited, so this
+     * option no longer serves a purpose. An origin that does not want such a response cached should send an explicit
+     * directive such as {@code Cache-Control: no-cache}. HTTP/1.0 response caching is deprecated and will be removed
+     * in a future release.
      */
+    @Deprecated
     public boolean isNeverCacheHTTP10ResponsesWithQuery() {
         return neverCacheHTTP10ResponsesWithQuery;
     }
@@ -205,7 +217,12 @@ public boolean isNeverCacheHTTP10ResponsesWithQuery() {
      * @return {@code true} if HTTP/1.1 responses with query strings should never be cached;
      * {@code false} otherwise.
      * @since 5.4
+     *
+     * @deprecated Assigning heuristic freshness to a response with a query component is no longer prohibited, so this
+     * option no longer serves a purpose. An origin that does not want such a response cached should send an explicit
+     * directive such as {@code Cache-Control: no-cache}.
      */
+    @Deprecated
     public boolean isNeverCacheHTTP11ResponsesWithQuery() {
         return neverCacheHTTP11ResponsesWithQuery;
     }
@@ -499,7 +516,13 @@ public Builder setAsynchronousWorkers(final int asynchronousWorkers) {
          * to better emulate IE, which also never caches responses, regardless of what caching
          * headers may be present.
          * @return this instance.
+         *
+         * @deprecated Assigning heuristic freshness to a response with a query component is no longer prohibited, so
+         * this option no longer serves a purpose. An origin that does not want such a response cached should send an
+         * explicit directive such as {@code Cache-Control: no-cache}. HTTP/1.0 response caching is deprecated and will
+         * be removed in a future release.
          */
+        @Deprecated
         public Builder setNeverCacheHTTP10ResponsesWithQueryString(
                 final boolean neverCacheHTTP10ResponsesWithQuery) {
             this.neverCacheHTTP10ResponsesWithQuery = neverCacheHTTP10ResponsesWithQuery;
@@ -535,7 +558,12 @@ public Builder setFreshnessCheckEnabled(final boolean freshnessCheckEnabled) {
          *
          * @param neverCacheHTTP11ResponsesWithQuery whether to never cache HTTP/1.1 responses with a query string
          * @return this instance.
+         *
+         * @deprecated Assigning heuristic freshness to a response with a query component is no longer prohibited, so
+         * this option no longer serves a purpose. An origin that does not want such a response cached should send an
+         * explicit directive such as {@code Cache-Control: no-cache}.
          */
+        @Deprecated
         public Builder setNeverCacheHTTP11ResponsesWithQueryString(
                 final boolean neverCacheHTTP11ResponsesWithQuery) {
             this.neverCacheHTTP11ResponsesWithQuery = neverCacheHTTP11ResponsesWithQuery;

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

@@ -75,6 +75,8 @@ public class CachingExecBase {
         this.cacheConfig = config != null ? config : CacheConfig.DEFAULT;
     }
 
+    // The query-string caching options are deprecated but still honoured while they remain on the API.
+    @SuppressWarnings("deprecation")
     CachingExecBase(final CacheConfig config) {
         super();
         this.cacheConfig = config != null ? config : CacheConfig.DEFAULT;

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

@@ -122,14 +122,17 @@ public boolean isResponseCacheable(final RequestCacheControl requestCacheControl
             return false;
         }
 
-        if (request.getPath().contains("?")) {
-            if (neverCache1_0ResponsesWithQueryString && from1_0Origin(response)) {
-                LOG.debug("Response is not cacheable as it had a query string");
-                return false;
-            } else if (!neverCache1_1ResponsesWithQueryString && !isExplicitlyCacheable(cacheControl, response)) {
-                LOG.debug("Response is not cacheable as it is missing explicit caching headers");
-                return false;
-            }
+        if (neverCache1_0ResponsesWithQueryString
+                && request.getPath().contains("?")
+                && from1_0Origin(response)) {
+            LOG.debug("Response is not cacheable as it had a query string");
+            return false;
+        }
+        if (neverCache1_1ResponsesWithQueryString
+                && request.getPath().contains("?")
+                && !isExplicitlyCacheable(cacheControl, response)) {
+            LOG.debug("Response is not cacheable as it is missing explicit caching headers");
+            return false;
         }
 
         if (cacheControl.isMustUnderstand()) {

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

@@ -521,27 +521,21 @@ void testResponsesThatAreSmallEnoughAreCacheable() {
     }
 
     @Test
-    void testResponsesToGETWithQueryParamsButNoExplicitCachingAreNotCacheable() {
+    void testResponsesToGETWithQueryParamsButNoExplicitCachingAreCacheable() {
         request = new BasicHttpRequest("GET", "/foo?s=bar");
-        Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
-    }
-
-    @Test
-    void testResponsesToHEADWithQueryParamsButNoExplicitCachingAreNotCacheable() {
-        request = new BasicHttpRequest("HEAD", "/foo?s=bar");
-        Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
+        Assertions.assertTrue(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
     }
 
     @Test
-    void testResponsesToGETWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() {
-        policy = new ResponseCachingPolicy(true, true, false);
+    void testResponsesToGETWithQueryParamsAreNotCacheableWhenHTTP11QueryCachingDisabled() {
+        policy = new ResponseCachingPolicy(true, false, true);
         request = new BasicHttpRequest("GET", "/foo?s=bar");
         Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
     }
 
     @Test
-    void testResponsesToHEADWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() {
-        policy = new ResponseCachingPolicy(true, true, false);
+    void testResponsesToHEADWithQueryParamsButNoExplicitCachingAreNotCacheable() {
+        policy = new ResponseCachingPolicy(true, false, true);
         request = new BasicHttpRequest("HEAD", "/foo?s=bar");
         Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
     }
@@ -583,6 +577,7 @@ void testResponsesToHEADWithQueryParamsAndExplicitCachingAreCacheableEvenWhen1_0
 
     @Test
     void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheable() {
+        policy = new ResponseCachingPolicy(true, true, false);
         request = new BasicHttpRequest("GET", "/foo?s=bar");
         response = new BasicHttpResponse(HttpStatus.SC_OK, "OK");
         response.setVersion(HttpVersion.HTTP_1_0);
@@ -591,6 +586,7 @@ void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheable() {
 
     @Test
     void headsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheable() {
+        policy = new ResponseCachingPolicy(true, true, false);
         request = new BasicHttpRequest("HEAD", "/foo?s=bar");
         response = new BasicHttpResponse(HttpStatus.SC_OK, "OK");
         response.setVersion(HttpVersion.HTTP_1_0);
@@ -660,13 +656,15 @@ void headsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWithExpi
 
     @Test
     void getsWithQueryParametersFrom1_0OriginsViaProxiesAreNotCacheable() {
+        policy = new ResponseCachingPolicy(true, true, false);
         request = new BasicHttpRequest("GET", "/foo?s=bar");
         response.setHeader(HttpHeaders.VIA, "1.0 someproxy");
         Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
     }
 
     @Test
     void headsWithQueryParametersFrom1_0OriginsViaProxiesAreNotCacheable() {
+        policy = new ResponseCachingPolicy(true, true, false);
         request = new BasicHttpRequest("HEAD", "/foo?s=bar");
         response.setHeader(HttpHeaders.VIA, "1.0 someproxy");
         Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
@@ -864,6 +862,7 @@ void testIsResponseCacheable() {
         policy = new ResponseCachingPolicy(true, false, true);
         response.setCode(HttpStatus.SC_OK);
         response.setHeader("Date", DateUtils.formatStandardDate(now));
+        responseCacheControl = ResponseCacheControl.builder().setMaxAge(3600).build();
         assertTrue(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response));
     }