From a92a196de24a66e0218268f1c7e367bbab22eacc Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Fri, 14 Dec 2012 17:02:46 +0000 Subject: [PATCH] HTTPCLIENT-1277: Caching client sends a 304 to an unconditional request. Contributed by Francois-Xavier Bonnet git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1421977 13f79535-47bb-0310-9956-ffa450edef68 --- RELEASE_NOTES.txt | 3 ++ .../CachedResponseSuitabilityChecker.java | 5 +++ .../http/impl/client/cache/CachingExec.java | 4 +- .../impl/client/cache/TestCachingExec.java | 43 ++++++++++++++++++- 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index 8f4103096..a24c90256 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,6 +1,9 @@ Changes in trunk ------------------- +* [HTTPCLIENT-1277] Caching client sends a 304 to an unconditional request. + Contributed by Francois-Xavier Bonnet + * [HTTPCLIENT-1278] Update NTLM documentation. Contributed by Karl Wright diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachedResponseSuitabilityChecker.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachedResponseSuitabilityChecker.java index 439f3df34..4ae114756 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachedResponseSuitabilityChecker.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachedResponseSuitabilityChecker.java @@ -34,6 +34,7 @@ import org.apache.http.Header; import org.apache.http.HeaderElement; import org.apache.http.HttpHost; import org.apache.http.HttpRequest; +import org.apache.http.HttpStatus; import org.apache.http.annotation.Immutable; import org.apache.http.client.cache.HeaderConstants; import org.apache.http.client.cache.HttpCacheEntry; @@ -146,6 +147,10 @@ class CachedResponseSuitabilityChecker { return false; } + if (!isConditional(request) && entry.getStatusCode() == HttpStatus.SC_NOT_MODIFIED) { + return false; + } + if (isConditional(request) && !allConditionalsMatch(request, entry, now)) { return false; } 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 71bfffa05..6228794bb 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 @@ -283,7 +283,9 @@ public class CachingExec implements ClientExecChain { } else if (!mayCallBackend(request)) { log.debug("Cache entry not suitable but only-if-cached requested"); out = Proxies.enhanceResponse(generateGatewayTimeout(context)); - } else if (validityPolicy.isRevalidatable(entry)) { + } else if (validityPolicy.isRevalidatable(entry) + && !(entry.getStatusCode() == HttpStatus.SC_NOT_MODIFIED + && !suitabilityChecker.isConditional(request))) { log.debug("Revalidating cache entry"); return revalidateCacheEntry(route, request, context, execAware, entry, now); } else { diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java index 2e2ba9351..508746734 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java @@ -1631,7 +1631,48 @@ public class TestCachingExec { assertEquals("etag", result2.getFirstHeader("Etag").getValue()); } - private IExpectationSetters backendExpectsAnyRequestAndReturn( + @Test + public void testDoesNotSend304ForNonConditionalRequest() throws Exception { + + Date now = new Date(); + Date inOneMinute = new Date(System.currentTimeMillis()+60000); + + impl = new CachingExec(mockBackend, new BasicHttpCache(),CacheConfig.DEFAULT); + + HttpRequestWrapper req1 = HttpRequestWrapper.wrap(new HttpGet("http://foo.example.com/")); + req1.addHeader("If-None-Match", "etag"); + + HttpRequestWrapper req2 = HttpRequestWrapper.wrap(new HttpGet("http://foo.example.com/")); + + HttpResponse resp1 = new BasicHttpResponse(HttpVersion.HTTP_1_1, HttpStatus.SC_NOT_MODIFIED, "Not modified"); + resp1.setHeader("Date", DateUtils.formatDate(now)); + resp1.setHeader("Cache-Control","public, max-age=60"); + resp1.setHeader("Expires",DateUtils.formatDate(inOneMinute)); + resp1.setHeader("Etag", "etag"); + resp1.setHeader("Vary", "Accept-Encoding"); + + HttpResponse resp2 = new BasicHttpResponse(HttpVersion.HTTP_1_1, HttpStatus.SC_OK, "Ok"); + resp2.setHeader("Date", DateUtils.formatDate(now)); + resp2.setHeader("Cache-Control","public, max-age=60"); + resp2.setHeader("Expires",DateUtils.formatDate(inOneMinute)); + resp2.setHeader("Etag", "etag"); + resp2.setHeader("Vary", "Accept-Encoding"); + resp2.setEntity(HttpTestUtils.makeBody(128)); + + backendExpectsAnyRequestAndReturn(resp1); + backendExpectsAnyRequestAndReturn(resp2).anyTimes(); + replayMocks(); + HttpResponse result1 = impl.execute(route, req1); + HttpResponse result2 = impl.execute(route, req2); + verifyMocks(); + + assertEquals(HttpStatus.SC_NOT_MODIFIED, result1.getStatusLine().getStatusCode()); + assertNull(result1.getEntity()); + assertEquals(HttpStatus.SC_OK, result2.getStatusLine().getStatusCode()); + Assert.assertNotNull(result2.getEntity()); + } + + private IExpectationSetters backendExpectsAnyRequestAndReturn( HttpResponse response) throws Exception { CloseableHttpResponse resp = mockBackend.execute( EasyMock.isA(HttpRoute.class),