From 908b81dd5adabedeb706674ab2ecc203e954e01a Mon Sep 17 00:00:00 2001 From: Jonathan Moore Date: Thu, 6 Jun 2013 21:25:04 +0000 Subject: [PATCH] HTTPCLIENT-1353: 303 redirects should be cacheable Commit of patch #1 to synchronous client from James Leigh git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1490448 13f79535-47bb-0310-9956-ffa450edef68 --- .../http/impl/client/cache/CacheConfig.java | 42 ++++++++++- .../http/impl/client/cache/CachingExec.java | 2 +- .../impl/client/cache/CachingHttpClient.java | 2 +- .../client/cache/ResponseCachingPolicy.java | 19 +++-- .../cache/TestResponseCachingPolicy.java | 72 ++++++++++++++++--- 5 files changed, 119 insertions(+), 18 deletions(-) diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheConfig.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheConfig.java index 877a27314..b2ad348a0 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheConfig.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheConfig.java @@ -56,6 +56,15 @@ import org.apache.http.util.Args; * browser cache), then you will want to {@link * CacheConfig#setSharedCache(boolean) turn off the shared cache setting}.

* + *

303 caching. RFC2616 explicitly disallows caching 303 responses; + * however, the HTTPbis working group says they can be cached + * if explicitly indicated in the response headers and permitted by the request method. + * (They also indicate that disallowing 303 caching is actually an unintended + * spec error in RFC2616). + * This behavior is off by default, to err on the side of a conservative + * adherence to the existing standard, but you may want to + * {@link Builder#setAllow303Caching(boolean) enable it}. + * *

Heuristic caching. Per RFC2616, a cache may cache certain cache * entries even if no explicit cache control headers are set by the origin. * This behavior is off by default, but you may want to turn this on if you @@ -100,6 +109,10 @@ public class CacheConfig implements Cloneable { */ public final static int DEFAULT_MAX_UPDATE_RETRIES = 1; + /** Default setting for 303 caching + */ + public final static boolean DEFAULT_303_CACHING_ENABLED = false; + /** Default setting for heuristic caching */ public final static boolean DEFAULT_HEURISTIC_CACHING_ENABLED = false; @@ -139,6 +152,7 @@ public class CacheConfig implements Cloneable { private long maxObjectSize; private int maxCacheEntries; private int maxUpdateRetries; + private boolean allow303Caching; private boolean heuristicCachingEnabled; private float heuristicCoefficient; private long heuristicDefaultLifetime; @@ -158,6 +172,7 @@ public class CacheConfig implements Cloneable { this.maxObjectSize = DEFAULT_MAX_OBJECT_SIZE_BYTES; this.maxCacheEntries = DEFAULT_MAX_CACHE_ENTRIES; this.maxUpdateRetries = DEFAULT_MAX_UPDATE_RETRIES; + this.allow303Caching = false; this.heuristicCachingEnabled = false; this.heuristicCoefficient = DEFAULT_HEURISTIC_COEFFICIENT; this.heuristicDefaultLifetime = DEFAULT_HEURISTIC_LIFETIME; @@ -172,6 +187,7 @@ public class CacheConfig implements Cloneable { final long maxObjectSize, final int maxCacheEntries, final int maxUpdateRetries, + final boolean allow303Caching, final boolean heuristicCachingEnabled, final float heuristicCoefficient, final long heuristicDefaultLifetime, @@ -185,6 +201,7 @@ public class CacheConfig implements Cloneable { this.maxObjectSize = maxObjectSize; this.maxCacheEntries = maxCacheEntries; this.maxUpdateRetries = maxUpdateRetries; + this.allow303Caching = allow303Caching; this.heuristicCachingEnabled = heuristicCachingEnabled; this.heuristicCoefficient = heuristicCoefficient; this.heuristicDefaultLifetime = heuristicDefaultLifetime; @@ -287,6 +304,14 @@ public class CacheConfig implements Cloneable { this.maxUpdateRetries = maxUpdateRetries; } + /** + * Returns whether 303 caching is enabled. + * @return {@code true} if it is enabled. + */ + public boolean is303CachingEnabled() { + return allow303Caching; + } + /** * Returns whether heuristic caching is enabled. * @return {@code true} if it is enabled. @@ -493,6 +518,7 @@ public class CacheConfig implements Cloneable { private long maxObjectSize; private int maxCacheEntries; private int maxUpdateRetries; + private boolean allow303Caching; private boolean heuristicCachingEnabled; private float heuristicCoefficient; private long heuristicDefaultLifetime; @@ -507,6 +533,7 @@ public class CacheConfig implements Cloneable { this.maxObjectSize = DEFAULT_MAX_OBJECT_SIZE_BYTES; this.maxCacheEntries = DEFAULT_MAX_CACHE_ENTRIES; this.maxUpdateRetries = DEFAULT_MAX_UPDATE_RETRIES; + this.allow303Caching = false; this.heuristicCachingEnabled = false; this.heuristicCoefficient = DEFAULT_HEURISTIC_COEFFICIENT; this.heuristicDefaultLifetime = DEFAULT_HEURISTIC_LIFETIME; @@ -542,6 +569,16 @@ public class CacheConfig implements Cloneable { return this; } + /** + * Enables or disables 303 caching. + * @param allow303Caching should be {@code true} to + * permit 303 caching, {@code false} to disable it. + */ + public Builder setAllow303Caching(final boolean allow303Caching) { + this.allow303Caching = allow303Caching; + return this; + } + /** * Enables or disables heuristic caching. * @param heuristicCachingEnabled should be {@code true} to @@ -652,6 +689,7 @@ public class CacheConfig implements Cloneable { maxObjectSize, maxCacheEntries, maxUpdateRetries, + allow303Caching, heuristicCachingEnabled, heuristicCoefficient, heuristicDefaultLifetime, @@ -670,7 +708,9 @@ public class CacheConfig implements Cloneable { final StringBuilder builder = new StringBuilder(); builder.append("[maxObjectSize=").append(this.maxObjectSize) .append(", maxCacheEntries=").append(this.maxCacheEntries) - .append(", maxUpdateRetries=").append(this.heuristicCachingEnabled) + .append(", maxUpdateRetries=").append(this.maxUpdateRetries) + .append(", 303CachingEnabled=").append(this.allow303Caching) + .append(", heuristicCachingEnabled=").append(this.heuristicCachingEnabled) .append(", heuristicCoefficient=").append(this.heuristicCoefficient) .append(", heuristicDefaultLifetime=").append(this.heuristicDefaultLifetime) .append(", isSharedCache=").append(this.isSharedCache) diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java index a41dd2715..c55487338 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java @@ -139,7 +139,7 @@ public class CachingExec implements ClientExecChain { this.requestCompliance = new RequestProtocolCompliance(); this.responseCachingPolicy = new ResponseCachingPolicy( this.cacheConfig.getMaxObjectSize(), this.cacheConfig.isSharedCache(), - this.cacheConfig.isNeverCacheHTTP10ResponsesWithQuery()); + this.cacheConfig.isNeverCacheHTTP10ResponsesWithQuery(), config.is303CachingEnabled()); this.asynchRevalidator = asynchRevalidator; } diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java index 34f1a3578..9302bf97e 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java @@ -175,7 +175,7 @@ public class CachingHttpClient implements HttpClient { this.responseCache = cache; this.validityPolicy = new CacheValidityPolicy(); this.responseCachingPolicy = new ResponseCachingPolicy(maxObjectSizeBytes, sharedCache, - config.isNeverCacheHTTP10ResponsesWithQuery()); + config.isNeverCacheHTTP10ResponsesWithQuery(), config.is303CachingEnabled()); this.responseGenerator = new CachedHttpResponseGenerator(this.validityPolicy); this.cacheableRequestPolicy = new CacheableRequestPolicy(); this.suitabilityChecker = new CachedResponseSuitabilityChecker(this.validityPolicy, config); diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/ResponseCachingPolicy.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/ResponseCachingPolicy.java index a47de019b..544420395 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/ResponseCachingPolicy.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/ResponseCachingPolicy.java @@ -63,9 +63,7 @@ class ResponseCachingPolicy { HttpStatus.SC_MULTIPLE_CHOICES, HttpStatus.SC_MOVED_PERMANENTLY, HttpStatus.SC_GONE)); - private static final Set uncacheableStatuses = - new HashSet(Arrays.asList(HttpStatus.SC_PARTIAL_CONTENT, - HttpStatus.SC_SEE_OTHER)); + private final Set uncacheableStatuses; /** * Define a cache policy that limits the size of things that should be stored * in the cache to a maximum of {@link HttpResponse} bytes in size. @@ -75,13 +73,22 @@ class ResponseCachingPolicy { * non-shared/private cache (false) * @param neverCache1_0ResponsesWithQueryString true to never cache HTTP 1.0 responses with a query string, false * to cache if explicit cache headers are found. + * @param allow303Caching if this policy is permitted to cache 303 response */ - public ResponseCachingPolicy(final long maxObjectSizeBytes, final boolean sharedCache, - final boolean neverCache1_0ResponsesWithQueryString - ) { + public ResponseCachingPolicy(final long maxObjectSizeBytes, + final boolean sharedCache, + final boolean neverCache1_0ResponsesWithQueryString, + boolean allow303Caching) { this.maxObjectSizeBytes = maxObjectSizeBytes; this.sharedCache = sharedCache; this.neverCache1_0ResponsesWithQueryString = neverCache1_0ResponsesWithQueryString; + if (allow303Caching) { + uncacheableStatuses = new HashSet( + Arrays.asList(HttpStatus.SC_PARTIAL_CONTENT)); + } else { + uncacheableStatuses = new HashSet(Arrays.asList( + HttpStatus.SC_PARTIAL_CONTENT, HttpStatus.SC_SEE_OTHER)); + } } /** diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestResponseCachingPolicy.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestResponseCachingPolicy.java index 3ba0aad06..d595dfbdb 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestResponseCachingPolicy.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestResponseCachingPolicy.java @@ -61,7 +61,7 @@ public class TestResponseCachingPolicy { sixSecondsAgo = new Date(now.getTime() - 6 * 1000L); tenSecondsFromNow = new Date(now.getTime() + 10 * 1000L); - policy = new ResponseCachingPolicy(0, true, false); + policy = new ResponseCachingPolicy(0, true, false, false); request = new BasicHttpRequest("GET","/",HTTP_1_1); response = new BasicHttpResponse( new BasicStatusLine(HTTP_1_1, HttpStatus.SC_OK, "")); @@ -83,7 +83,7 @@ public class TestResponseCachingPolicy { @Test public void testResponsesToRequestsWithAuthorizationHeadersAreCacheableByNonSharedCache() { - policy = new ResponseCachingPolicy(0, false, false); + policy = new ResponseCachingPolicy(0, false, false, false); request = new BasicHttpRequest("GET","/",HTTP_1_1); request.setHeader("Authorization","Basic dXNlcjpwYXNzd2Q="); Assert.assertTrue(policy.isResponseCacheable(request,response)); @@ -159,6 +159,24 @@ public class TestResponseCachingPolicy { Assert.assertFalse(policy.isResponseCacheable("GET", response)); } + @Test + public void testPlain303ResponseCodeIsNotCacheableUnderDefaultBehavior() { + response.setStatusCode(HttpStatus.SC_SEE_OTHER); + response.removeHeaders("Expires"); + response.removeHeaders("Cache-Control"); + Assert.assertFalse(policy.isResponseCacheable("GET", response)); + } + + @Test + public void testPlain303ResponseCodeIsNotCacheableEvenIf303CachingEnabled() { + policy = new ResponseCachingPolicy(0, true, false, true); + response.setStatusCode(HttpStatus.SC_SEE_OTHER); + response.removeHeaders("Expires"); + response.removeHeaders("Cache-Control"); + Assert.assertFalse(policy.isResponseCacheable("GET", response)); + } + + @Test public void testPlain307ResponseCodeIsNotCacheable() { response.setStatusCode(HttpStatus.SC_TEMPORARY_REDIRECT); @@ -225,7 +243,7 @@ public class TestResponseCachingPolicy { @Test public void test200ResponseWithPrivateCacheControlIsCacheableByNonSharedCache() { - policy = new ResponseCachingPolicy(0, false, false); + policy = new ResponseCachingPolicy(0, false, false, false); response.setStatusCode(HttpStatus.SC_OK); response.setHeader("Cache-Control", "private"); Assert.assertTrue(policy.isResponseCacheable("GET", response)); @@ -385,7 +403,7 @@ public class TestResponseCachingPolicy { @Test public void testResponsesToGETWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(0, true, true); + policy = new ResponseCachingPolicy(0, true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); Assert.assertFalse(policy.isResponseCacheable(request, response)); } @@ -400,7 +418,7 @@ public class TestResponseCachingPolicy { @Test public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(0, true, true); + policy = new ResponseCachingPolicy(0, true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatDate(now)); response.setHeader("Expires", DateUtils.formatDate(tenSecondsFromNow)); @@ -416,7 +434,7 @@ public class TestResponseCachingPolicy { @Test public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWithSetting() { - policy = new ResponseCachingPolicy(0, true, true); + policy = new ResponseCachingPolicy(0, true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); response = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); Assert.assertFalse(policy.isResponseCacheable(request, response)); @@ -433,7 +451,7 @@ public class TestResponseCachingPolicy { @Test public void getsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true); + policy = new ResponseCachingPolicy(0, true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); response = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); response.setHeader("Date", DateUtils.formatDate(now)); @@ -461,7 +479,7 @@ public class TestResponseCachingPolicy { @Test public void getsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true); + policy = new ResponseCachingPolicy(0, true, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); final Date now = new Date(); final Date tenSecondsFromNow = new Date(now.getTime() + 10 * 1000L); @@ -482,7 +500,7 @@ public class TestResponseCachingPolicy { @Test public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true); + policy = new ResponseCachingPolicy(0, true, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatDate(now)); response.setHeader("Expires", DateUtils.formatDate(tenSecondsFromNow)); @@ -516,6 +534,42 @@ public class TestResponseCachingPolicy { Assert.assertFalse(policy.isResponseCacheable(request, response)); } + @Test + public void test302WithExplicitCachingHeaders() { + response.setStatusCode(HttpStatus.SC_MOVED_TEMPORARILY); + response.setHeader("Date", DateUtils.formatDate(now)); + response.setHeader("Cache-Control","max-age=300"); + Assert.assertTrue(policy.isResponseCacheable(request, response)); + } + + @Test + public void test303WithExplicitCachingHeadersUnderDefaultBehavior() { + // RFC 2616 says: 303 should not be cached + response.setStatusCode(HttpStatus.SC_SEE_OTHER); + response.setHeader("Date", DateUtils.formatDate(now)); + response.setHeader("Cache-Control","max-age=300"); + Assert.assertFalse(policy.isResponseCacheable(request, response)); + } + + @Test + public void test303WithExplicitCachingHeadersWhenPermittedByConfig() { + // HTTPbis working group says ok if explicitly indicated by + // response headers + policy = new ResponseCachingPolicy(0, true, false, true); + response.setStatusCode(HttpStatus.SC_SEE_OTHER); + response.setHeader("Date", DateUtils.formatDate(now)); + response.setHeader("Cache-Control","max-age=300"); + Assert.assertTrue(policy.isResponseCacheable(request, response)); + } + + @Test + public void test307WithExplicitCachingHeaders() { + response.setStatusCode(HttpStatus.SC_TEMPORARY_REDIRECT); + response.setHeader("Date", DateUtils.formatDate(now)); + response.setHeader("Cache-Control","max-age=300"); + Assert.assertTrue(policy.isResponseCacheable(request, response)); + } + @Test public void otherStatusCodesAreCacheableWithExplicitCachingHeaders() { response.setStatusCode(HttpStatus.SC_NOT_FOUND);