From 38e6a997fcfe438701dbd0fe3e582e96ab00659f Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Thu, 20 Dec 2012 16:56:41 +0000 Subject: [PATCH] HTTPCLIENT-1250: Allow query string to be ignored when determining cacheability for HTTP 1.0 responses Contributed by Don Brown git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1424589 13f79535-47bb-0310-9956-ffa450edef68 --- RELEASE_NOTES.txt | 4 ++ .../http/impl/client/cache/CacheConfig.java | 30 +++++++- .../http/impl/client/cache/CachingExec.java | 3 +- .../impl/client/cache/CachingHttpClient.java | 3 +- .../client/cache/ResponseCachingPolicy.java | 21 ++++-- .../cache/TestProtocolRecommendations.java | 29 +------- .../cache/TestResponseCachingPolicy.java | 68 +++++++++++++++++-- 7 files changed, 115 insertions(+), 43 deletions(-) diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index f35e80849..2f32f00d1 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,6 +1,10 @@ Changes in trunk ------------------- +* [HTTPCLIENT-1250] Allow query string to be ignored when determining cacheability for HTTP 1.0 + responses. + Contributed by Don Brown + * [HTTPCLIENT-1284] HttpClient incorrectly generates Host header when physical connection route differs from the host name specified in the request URI. Contributed by Oleg Kalnichevski 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 ab445b49a..257806758 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 @@ -145,6 +145,7 @@ public class CacheConfig implements Cloneable { private int asynchronousWorkersCore; private int asynchronousWorkerIdleLifetimeSecs; private int revalidationQueueSize; + private boolean neverCacheHTTP10ResponsesWithQuery; /** * @deprecated (4.3) use {@link Builder}. @@ -176,7 +177,8 @@ public class CacheConfig implements Cloneable { int asynchronousWorkersMax, int asynchronousWorkersCore, int asynchronousWorkerIdleLifetimeSecs, - int revalidationQueueSize) { + int revalidationQueueSize, + boolean neverCacheHTTP10ResponsesWithQuery) { super(); this.maxObjectSize = maxObjectSize; this.maxCacheEntries = maxCacheEntries; @@ -240,6 +242,15 @@ public class CacheConfig implements Cloneable { this.maxObjectSize = maxObjectSize; } + /** + * 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 + */ + public boolean isNeverCacheHTTP10ResponsesWithQuery() { + return neverCacheHTTP10ResponsesWithQuery; + } + /** * Returns the maximum number of cache entries the cache will retain. */ @@ -470,6 +481,7 @@ public class CacheConfig implements Cloneable { private int asynchronousWorkersCore; private int asynchronousWorkerIdleLifetimeSecs; private int revalidationQueueSize; + private boolean neverCacheHTTP10ResponsesWithQuery; Builder() { this.maxObjectSize = DEFAULT_MAX_OBJECT_SIZE_BYTES; @@ -602,6 +614,18 @@ public class CacheConfig implements Cloneable { return this; } + /** + * Sets whether the cache should never cache HTTP 1.0 responses with a query string or not. + * @param neverCache1_0ResponsesWithQueryString true to never cache responses with a query + * string, false to cache if explicit cache headers are found. Set this to {@code true} + * to better emulate IE, which also never caches responses, regardless of what caching + * headers may be present. + */ + public Builder setNeverCache1_0ResponsesWithQueryString(boolean b) { + this.neverCacheHTTP10ResponsesWithQuery = b; + return this; + } + public CacheConfig build() { return new CacheConfig( maxObjectSize, @@ -614,7 +638,8 @@ public class CacheConfig implements Cloneable { asynchronousWorkersMax, asynchronousWorkersCore, asynchronousWorkerIdleLifetimeSecs, - revalidationQueueSize); + revalidationQueueSize, + neverCacheHTTP10ResponsesWithQuery); } } @@ -632,6 +657,7 @@ public class CacheConfig implements Cloneable { .append(", asynchronousWorkersCore=").append(this.asynchronousWorkersCore) .append(", asynchronousWorkerIdleLifetimeSecs=").append(this.asynchronousWorkerIdleLifetimeSecs) .append(", revalidationQueueSize=").append(this.revalidationQueueSize) + .append(", neverCacheHTTP10ResponsesWithQuery=").append(this.neverCacheHTTP10ResponsesWithQuery) .append("]"); return builder.toString(); } 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 468814430..f2997323b 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 @@ -133,7 +133,8 @@ public class CachingExec implements ClientExecChain { this.responseCompliance = new ResponseProtocolCompliance(); this.requestCompliance = new RequestProtocolCompliance(); this.responseCachingPolicy = new ResponseCachingPolicy( - this.cacheConfig.getMaxObjectSize(), this.cacheConfig.isSharedCache()); + this.cacheConfig.getMaxObjectSize(), this.cacheConfig.isSharedCache(), + this.cacheConfig.isNeverCacheHTTP10ResponsesWithQuery()); this.asynchRevalidator = makeAsynchronousValidator(config); } 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 953f25d1c..dff088a44 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 @@ -174,7 +174,8 @@ public class CachingHttpClient implements HttpClient { this.backend = client; this.responseCache = cache; this.validityPolicy = new CacheValidityPolicy(); - this.responseCachingPolicy = new ResponseCachingPolicy(maxObjectSizeBytes, sharedCache); + this.responseCachingPolicy = new ResponseCachingPolicy(maxObjectSizeBytes, sharedCache, + config.isNeverCacheHTTP10ResponsesWithQuery()); 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 825873718..067eef0f7 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 @@ -56,6 +56,7 @@ class ResponseCachingPolicy { private final long maxObjectSizeBytes; private final boolean sharedCache; + private final boolean neverCache1_0ResponsesWithQueryString; private final Log log = LogFactory.getLog(getClass()); private static final Set cacheableStatuses = new HashSet(Arrays.asList(HttpStatus.SC_OK, @@ -66,7 +67,6 @@ class ResponseCachingPolicy { private static final Set uncacheableStatuses = new HashSet(Arrays.asList(HttpStatus.SC_PARTIAL_CONTENT, HttpStatus.SC_SEE_OTHER)); - /** * 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. @@ -74,10 +74,15 @@ class ResponseCachingPolicy { * @param maxObjectSizeBytes the size to limit items into the cache * @param sharedCache whether to behave as a shared cache (true) or a * 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. */ - public ResponseCachingPolicy(long maxObjectSizeBytes, boolean sharedCache) { + public ResponseCachingPolicy(long maxObjectSizeBytes, boolean sharedCache, + boolean neverCache1_0ResponsesWithQueryString + ) { this.maxObjectSizeBytes = maxObjectSizeBytes; this.sharedCache = sharedCache; + this.neverCache1_0ResponsesWithQueryString = neverCache1_0ResponsesWithQueryString; } /** @@ -216,10 +221,14 @@ class ResponseCachingPolicy { return false; } - if (request.getRequestLine().getUri().contains("?") && - (!isExplicitlyCacheable(response) || from1_0Origin(response))) { - log.debug("Response was not cacheable."); - return false; + if (request.getRequestLine().getUri().contains("?")) { + if (neverCache1_0ResponsesWithQueryString && from1_0Origin(response)) { + log.debug("Response was not cacheable as it had a query string."); + return false; + } else if (!isExplicitlyCacheable(response)) { + log.debug("Response was not cacheable as it is missing explicit caching headers."); + return false; + } } if (expiresHeaderLessOrEqualToDateHeaderAndNoCacheControl(response)) { diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestProtocolRecommendations.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestProtocolRecommendations.java index 5045ce90f..332e216ea 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestProtocolRecommendations.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestProtocolRecommendations.java @@ -65,7 +65,6 @@ import org.junit.Test; */ public class TestProtocolRecommendations extends AbstractProtocolTest { - private Date tenSecondsFromNow; private Date now; private Date tenSecondsAgo; private Date twoMinutesAgo; @@ -77,7 +76,6 @@ public class TestProtocolRecommendations extends AbstractProtocolTest { now = new Date(); tenSecondsAgo = new Date(now.getTime() - 10 * 1000L); twoMinutesAgo = new Date(now.getTime() - 2 * 60 * 1000L); - tenSecondsFromNow = new Date(now.getTime() + 10 * 1000L); } /* "identity: The default (identity) encoding; the use of no @@ -1445,18 +1443,8 @@ public class TestProtocolRecommendations extends AbstractProtocolTest { * http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.9 */ @Test - public void responseToGetWithQueryFrom1_0OriginIsNotCached() + public void responseToGetWithQueryFrom1_0OriginAndNoExpiresIsNotCached() throws Exception { - HttpRequestWrapper req1 = HttpRequestWrapper.wrap( - new HttpGet("http://foo.example.com/bar?baz=quux")); - HttpResponse resp1 = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); - resp1.setEntity(HttpTestUtils.makeBody(200)); - resp1.setHeader("Content-Length","200"); - resp1.setHeader("Date", formatDate(now)); - resp1.setHeader("Expires", formatDate(tenSecondsFromNow)); - - backendExpectsAnyRequestAndReturn(resp1); - HttpRequestWrapper req2 = HttpRequestWrapper.wrap( new HttpGet("http://foo.example.com/bar?baz=quux")); HttpResponse resp2 = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); @@ -1467,25 +1455,13 @@ public class TestProtocolRecommendations extends AbstractProtocolTest { backendExpectsAnyRequestAndReturn(resp2); replayMocks(); - impl.execute(route, req1); impl.execute(route, req2); verifyMocks(); } @Test - public void responseToGetWithQueryFrom1_0OriginVia1_1ProxyIsNotCached() + public void responseToGetWithQueryFrom1_0OriginVia1_1ProxyAndNoExpiresIsNotCached() throws Exception { - HttpRequestWrapper req1 = HttpRequestWrapper.wrap( - new HttpGet("http://foo.example.com/bar?baz=quux")); - HttpResponse resp1 = new BasicHttpResponse(HttpVersion.HTTP_1_1, HttpStatus.SC_OK, "OK"); - resp1.setEntity(HttpTestUtils.makeBody(200)); - resp1.setHeader("Content-Length","200"); - resp1.setHeader("Date", formatDate(now)); - resp1.setHeader("Expires", formatDate(tenSecondsFromNow)); - resp1.setHeader("Via","1.0 someproxy"); - - backendExpectsAnyRequestAndReturn(resp1); - HttpRequestWrapper req2 = HttpRequestWrapper.wrap( new HttpGet("http://foo.example.com/bar?baz=quux")); HttpResponse resp2 = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); @@ -1497,7 +1473,6 @@ public class TestProtocolRecommendations extends AbstractProtocolTest { backendExpectsAnyRequestAndReturn(resp2); replayMocks(); - impl.execute(route, req1); impl.execute(route, req2); verifyMocks(); } 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 e1f28da0d..122b84956 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); + policy = new ResponseCachingPolicy(0, true, 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); + policy = new ResponseCachingPolicy(0, false, false); request = new BasicHttpRequest("GET","/",HTTP_1_1); request.setHeader("Authorization","Basic dXNlcjpwYXNzd2Q="); Assert.assertTrue(policy.isResponseCacheable(request,response)); @@ -225,7 +225,7 @@ public class TestResponseCachingPolicy { @Test public void test200ResponseWithPrivateCacheControlIsCacheableByNonSharedCache() { - policy = new ResponseCachingPolicy(0, false); + policy = new ResponseCachingPolicy(0, false, false); response.setStatusCode(HttpStatus.SC_OK); response.setHeader("Cache-Control", "private"); Assert.assertTrue(policy.isResponseCacheable("GET", response)); @@ -383,6 +383,13 @@ public class TestResponseCachingPolicy { Assert.assertFalse(policy.isResponseCacheable(request, response)); } + @Test + public void testResponsesToGETWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() { + policy = new ResponseCachingPolicy(0, true, true); + request = new BasicHttpRequest("GET", "/foo?s=bar"); + Assert.assertFalse(policy.isResponseCacheable(request, response)); + } + @Test public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheable() { request = new BasicHttpRequest("GET", "/foo?s=bar"); @@ -391,6 +398,15 @@ public class TestResponseCachingPolicy { Assert.assertTrue(policy.isResponseCacheable(request, response)); } + @Test + public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheableEvenWhen1_0QueryCachingDisabled() { + policy = new ResponseCachingPolicy(0, true, true); + request = new BasicHttpRequest("GET", "/foo?s=bar"); + response.setHeader("Date", formatDate(now)); + response.setHeader("Expires", formatDate(tenSecondsFromNow)); + Assert.assertTrue(policy.isResponseCacheable(request, response)); + } + @Test public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheable() { request = new BasicHttpRequest("GET", "/foo?s=bar"); @@ -399,7 +415,25 @@ public class TestResponseCachingPolicy { } @Test - public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWithExpires() { + public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWithSetting() { + policy = new ResponseCachingPolicy(0, true, true); + request = new BasicHttpRequest("GET", "/foo?s=bar"); + response = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); + Assert.assertFalse(policy.isResponseCacheable(request, response)); + } + + @Test + public void getsWithQueryParametersDirectlyFrom1_0OriginsAreCacheableWithExpires() { + request = new BasicHttpRequest("GET", "/foo?s=bar"); + response = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); + response.setHeader("Date", formatDate(now)); + response.setHeader("Expires", formatDate(tenSecondsFromNow)); + Assert.assertTrue(policy.isResponseCacheable(request, response)); + } + + @Test + public void getsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWithExpires() { + policy = new ResponseCachingPolicy(0, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response = new BasicHttpResponse(HttpVersion.HTTP_1_0, HttpStatus.SC_OK, "OK"); response.setHeader("Date", formatDate(now)); @@ -415,7 +449,19 @@ public class TestResponseCachingPolicy { } @Test - public void getsWithQueryParametersFrom1_0OriginsViaProxiesAreNotCacheableEvenWithExpires() { + public void getsWithQueryParametersFrom1_0OriginsViaProxiesAreCacheableWithExpires() { + request = new BasicHttpRequest("GET", "/foo?s=bar"); + Date now = new Date(); + Date tenSecondsFromNow = new Date(now.getTime() + 10 * 1000L); + response.setHeader("Date", formatDate(now)); + response.setHeader("Expires", formatDate(tenSecondsFromNow)); + response.setHeader("Via", "1.0 someproxy"); + Assert.assertTrue(policy.isResponseCacheable(request, response)); + } + + @Test + public void getsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEvenWithExpires() { + policy = new ResponseCachingPolicy(0, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); Date now = new Date(); Date tenSecondsFromNow = new Date(now.getTime() + 10 * 1000L); @@ -426,7 +472,17 @@ public class TestResponseCachingPolicy { } @Test - public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreNotCacheableEvenWithExpires() { + public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreCacheableWithExpires() { + request = new BasicHttpRequest("GET", "/foo?s=bar"); + response.setHeader("Date", formatDate(now)); + response.setHeader("Expires", formatDate(tenSecondsFromNow)); + response.setHeader("Via", "HTTP/1.0 someproxy"); + Assert.assertTrue(policy.isResponseCacheable(request, response)); + } + + @Test + public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCacheableEvenWithExpires() { + policy = new ResponseCachingPolicy(0, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", formatDate(now)); response.setHeader("Expires", formatDate(tenSecondsFromNow));