diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index dd00f7f34..d5c703dc7 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,6 +1,10 @@ Changes in trunk ------------------- +* [HTTPCLIENT-1290] 304 cached response never reused with If-modified-since conditional + requests. + Contributed by Francois-Xavier Bonnet + * [HTTPCLIENT-1291] Absolute request URIs without an explicitly specified path are rewritten to have "/" path). Contributed by Oleg Kalnichevski @@ -20,8 +24,8 @@ Changes in trunk route differs from the host name specified in the request URI. Contributed by Oleg Kalnichevski -* Kerberos and SPNego auth schemes use incorrect authorization header name when authenticating - with a proxy. +* [HTTPCLIENT-1293] Kerberos and SPNego auth schemes use incorrect authorization header name + when authenticating with a proxy. Contributed by Oleg Kalnichevski * [HTTPCLIENT-1283] NTLM needs to use Locale-independent form of 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 bdbf385d0..ca1814f96 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 @@ -800,6 +800,7 @@ public class CachingExec implements ClientExecChain { responseCache.flushInvalidatedCacheEntriesFor(target, request, backendResponse); if (cacheable && !alreadyHaveNewerCacheEntry(target, request, backendResponse)) { try { + storeRequestIfModifiedSinceFor304Response(request, backendResponse); return Proxies.enhanceResponse(responseCache.cacheAndReturnResponse( target, request, backendResponse, requestDate, responseDate)); } finally { @@ -816,6 +817,24 @@ public class CachingExec implements ClientExecChain { return backendResponse; } + /** + * For 304 Not modified responses, adds a "Last-Modified" header with the + * value of the "If-Modified-Since" header passed in the request. This + * header is required to be able to reuse match the cache entry for + * subsequent requests but as defined in http specifications it is not + * included in 304 responses by backend servers. This header will not be + * included in the resulting response. + */ + private void storeRequestIfModifiedSinceFor304Response( + HttpRequest request, HttpResponse backendResponse) { + if (backendResponse.getStatusLine().getStatusCode() == HttpStatus.SC_NOT_MODIFIED) { + Header h = request.getFirstHeader("If-Modified-Since"); + if (h != null) { + backendResponse.addHeader("Last-Modified", h.getValue()); + } + } + } + private boolean alreadyHaveNewerCacheEntry(HttpHost target, HttpRequestWrapper request, HttpResponse backendResponse) { HttpCacheEntry existing = null; 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 b8b9ebe36..bd8cf2dd6 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 @@ -928,6 +928,7 @@ public class CachingHttpClient implements HttpClient { if (cacheable && !alreadyHaveNewerCacheEntry(target, request, backendResponse)) { try { + storeRequestIfModifiedSinceFor304Response(request, backendResponse); return responseCache.cacheAndReturnResponse(target, request, backendResponse, requestDate, responseDate); } catch (IOException ioe) { @@ -944,7 +945,25 @@ public class CachingHttpClient implements HttpClient { return backendResponse; } - private boolean alreadyHaveNewerCacheEntry(HttpHost target, HttpRequestWrapper request, + /** + * For 304 Not modified responses, adds a "Last-Modified" header with the + * value of the "If-Modified-Since" header passed in the request. This + * header is required to be able to reuse match the cache entry for + * subsequent requests but as defined in http specifications it is not + * included in 304 responses by backend servers. This header will not be + * included in the resulting response. + */ + private void storeRequestIfModifiedSinceFor304Response( + HttpRequest request, HttpResponse backendResponse) { + if (backendResponse.getStatusLine().getStatusCode() == HttpStatus.SC_NOT_MODIFIED) { + Header h = request.getFirstHeader("If-Modified-Since"); + if (h != null) { + backendResponse.addHeader("Last-Modified", h.getValue()); + } + } + } + + private boolean alreadyHaveNewerCacheEntry(HttpHost target, HttpRequest request, HttpResponse backendResponse) { HttpCacheEntry existing = null; try { 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 d03a6d25c..e848f6c00 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 @@ -52,6 +52,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import junit.framework.AssertionFailedError; + import org.apache.http.Header; import org.apache.http.HttpHost; import org.apache.http.HttpRequest; @@ -739,6 +741,43 @@ public class TestCachingExec { } + @Test + public void testReturns304ForIfModifiedSinceHeaderIf304ResponseInCache() throws Exception { + Date now = new Date(); + Date oneHourAgo = new Date(now.getTime() - 3600 * 1000L); + Date inTenMinutes = new Date(now.getTime() + 600 * 1000L); + impl = new CachingExec(mockBackend, new BasicHttpCache(), CacheConfig.DEFAULT); + HttpRequestWrapper req1 = HttpRequestWrapper.wrap(new HttpGet("http://foo.example.com/")); + req1.addHeader("If-Modified-Since", DateUtils.formatDate(oneHourAgo)); + HttpRequestWrapper req2 = HttpRequestWrapper.wrap(new HttpGet("http://foo.example.com/")); + req2.addHeader("If-Modified-Since", DateUtils.formatDate(oneHourAgo)); + + HttpResponse resp1 = new BasicHttpResponse(HttpVersion.HTTP_1_1, HttpStatus.SC_NOT_MODIFIED, "Not modified"); + resp1.setHeader("Date", DateUtils.formatDate(now)); + resp1.setHeader("Cache-control", "max-age=600"); + resp1.setHeader("Expires", DateUtils.formatDate(inTenMinutes)); + + expect(mockBackend.execute( + same(route), + isA(HttpRequestWrapper.class), + isA(HttpClientContext.class), + (HttpExecutionAware) isNull())).andReturn(Proxies.enhanceResponse(resp1)).once(); + + expect(mockBackend.execute( + same(route), + isA(HttpRequestWrapper.class), + isA(HttpClientContext.class), + (HttpExecutionAware) isNull())).andThrow( + new AssertionFailedError("Should have reused cached 304 response")).anyTimes(); + + replayMocks(); + impl.execute(route, req1); + HttpResponse result = impl.execute(route, req2); + verifyMocks(); + Assert.assertEquals(HttpStatus.SC_NOT_MODIFIED, result.getStatusLine().getStatusCode()); + Assert.assertFalse(result.containsHeader("Last-Modified")); + } + @Test public void testReturns200ForIfModifiedSinceDateIsLess() throws Exception { Date now = new Date();