HTTPCLIENT-1290: 304 cached response never reused with If-modified-since conditional requests

Contributed by Francois-Xavier Bonnet <francois-xavier.bonnet at centraliens.net>

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1430245 13f79535-47bb-0310-9956-ffa450edef68

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 <francois-xavier.bonnet at centraliens.net>
+
 * [HTTPCLIENT-1291] Absolute request URIs without an explicitly specified path are rewritten 
   to have "/" path).  
   Contributed by Oleg Kalnichevski <olegk at apache.org>
@@ -20,8 +24,8 @@ Changes in trunk
   route differs from the host name specified in the request URI. 
   Contributed by Oleg Kalnichevski <olegk at apache.org>
 
-* Kerberos and SPNego auth schemes use incorrect authorization header name when authenticating
+* [HTTPCLIENT-1293] Kerberos and SPNego auth schemes use incorrect authorization header name 
-  with a proxy.
+  when authenticating with a proxy.
   Contributed by Oleg Kalnichevski <olegk at apache.org>
 
 * [HTTPCLIENT-1283] NTLM needs to use Locale-independent form of

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;

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 {

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();