HTTPCLIENT-995: cache returns cached responses even if validators not consistent with all conditional headers

Contributed by Jonathan Moore <jonathan_moore at comcast.com>


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

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheValidityPolicy.java

@@ -30,7 +30,6 @@ import java.util.Date;
 
 import org.apache.http.Header;
 import org.apache.http.HeaderElement;
-import org.apache.http.HttpRequest;
 import org.apache.http.annotation.Immutable;
 import org.apache.http.client.cache.HeaderConstants;
 import org.apache.http.client.cache.HttpCacheEntry;
@@ -79,28 +78,6 @@ class CacheValidityPolicy {
                 || entry.getFirstHeader(HeaderConstants.LAST_MODIFIED) != null;
     }
 
-    public boolean modifiedSince(final HttpCacheEntry entry, final HttpRequest request) {
-        Header unmodHeader = request.getFirstHeader(HeaderConstants.IF_UNMODIFIED_SINCE);
-
-        if (unmodHeader == null) {
-            return false;
-        }
-
-        try {
-            Date unmodifiedSinceDate = DateUtils.parseDate(unmodHeader.getValue());
-            Date lastModifiedDate = DateUtils.parseDate(entry.getFirstHeader(
-                    HeaderConstants.LAST_MODIFIED).getValue());
-
-            if (unmodifiedSinceDate.before(lastModifiedDate)) {
-                return true;
-            }
-        } catch (DateParseException e) {
-            return false;
-        }
-
-        return false;
-    }
-
     public boolean mustRevalidate(final HttpCacheEntry entry) {
         return hasCacheControlDirective(entry, "must-revalidate");
     }

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachedResponseSuitabilityChecker.java

@@ -26,6 +26,8 @@
  */
 package org.apache.http.impl.client.cache;
 
+import java.util.Date;
+
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.http.Header;
@@ -35,6 +37,8 @@ import org.apache.http.HttpRequest;
 import org.apache.http.annotation.Immutable;
 import org.apache.http.client.cache.HeaderConstants;
 import org.apache.http.client.cache.HttpCacheEntry;
+import org.apache.http.impl.cookie.DateParseException;
+import org.apache.http.impl.cookie.DateUtils;
 
 /**
  * Determines whether a given {@link HttpCacheEntry} is suitable to be
@@ -81,8 +85,13 @@ class CachedResponseSuitabilityChecker {
             return false;
         }
 
-        if (validityStrategy.modifiedSince(entry, request)) {
+        if (hasUnsupportedConditionalHeaders(request)) {
-            log.debug("Cache entry modified times didn't line up. Cache Entry should not be used");
+            log.debug("Request contained conditional headers we don't handle");
+            return false;
+        }
+
+        if (containsEtagAndLastModifiedValidators(request)
+            && !allConditionalsMatch(request, entry)) {
             return false;
         }
 
@@ -146,4 +155,93 @@ class CachedResponseSuitabilityChecker {
         log.debug("Response from cache was suitable");
         return true;
     }
+
+    private boolean hasUnsupportedConditionalHeaders(HttpRequest request) {
+        return (request.getFirstHeader("If-Range") != null
+                || request.getFirstHeader("If-Match") != null
+                || hasValidDateField(request, "If-Unmodified-Since"));
+    }
+
+    /**
+     * Should return false if some conditionals would allow a
+     * normal request but some would not.
+     * @param request
+     * @param entry
+     * @return
+     */
+    private boolean allConditionalsMatch(HttpRequest request,
+            HttpCacheEntry entry) {
+        Header etagHeader = entry.getFirstHeader("ETag");
+        String etag = (etagHeader != null) ? etagHeader.getValue() : null;
+        Header[] ifNoneMatch = request.getHeaders("If-None-Match");
+        if (ifNoneMatch != null && ifNoneMatch.length > 0) {
+            boolean matched = false;
+            for(Header h : ifNoneMatch) {
+                for(HeaderElement elt : h.getElements()) {
+                    String reqEtag = elt.toString();
+                    if (("*".equals(reqEtag) && etag != null)
+                        || reqEtag.equals(etag)) {
+                        matched = true;
+                        break;
+                    }
+                }
+            }
+            if (!matched) return false;
+        }
+        Header lmHeader = entry.getFirstHeader("Last-Modified");
+        Date lastModified = null;
+        try {
+            if (lmHeader != null) {
+                lastModified = DateUtils.parseDate(lmHeader.getValue());
+            }
+        } catch (DateParseException dpe) {
+            // nop
+        }
+        for(Header h : request.getHeaders("If-Modified-Since")) {
+            try {
+                Date cond = DateUtils.parseDate(h.getValue());
+                if (lastModified == null
+                    || lastModified.after(cond)) {
+                    return false;
+                }
+            } catch (DateParseException dpe) {
+            }
+        }
+        return true;
+    }
+
+    private boolean containsEtagAndLastModifiedValidators(HttpRequest request) {
+        boolean hasEtagValidators = (hasEtagIfRangeHeader(request)
+                || request.getFirstHeader("If-Match") != null
+                || request.getFirstHeader("If-None-Match") != null);
+        if (!hasEtagValidators) return false;
+        final boolean hasLastModifiedValidators =
+            hasValidDateField(request, "If-Modified-Since")
+            || hasValidDateField(request, "If-Unmodified-Since")
+            || hasValidDateField(request, "If-Range");
+        return hasLastModifiedValidators;
+    }
+
+    private boolean hasEtagIfRangeHeader(HttpRequest request) {
+        for(Header h : request.getHeaders("If-Range")) {
+            try {
+                DateUtils.parseDate(h.getValue());
+            } catch (DateParseException dpe) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private boolean hasValidDateField(HttpRequest request, String headerName) {
+        for(Header h : request.getHeaders(headerName)) {
+            try {
+                DateUtils.parseDate(h.getValue());
+                return true;
+            } catch (DateParseException dpe) {
+                // ignore malformed dates
+            }
+        }
+        return false;
+    }
 }

httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachedResponseSuitabilityChecker.java

@@ -78,7 +78,6 @@ public class TestCachedResponseSuitabilityChecker {
     public void testSuitableIfContentLengthHeaderIsRight() {
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
 
         replayMocks();
         boolean result = impl.canCachedResponseBeUsed(host, request, entry);
@@ -92,7 +91,6 @@ public class TestCachedResponseSuitabilityChecker {
     public void testSuitableIfCacheEntryIsFresh() {
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
 
         replayMocks();
 
@@ -120,7 +118,6 @@ public class TestCachedResponseSuitabilityChecker {
         request.addHeader("Cache-Control", "no-cache");
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
 
         replayMocks();
 
@@ -134,7 +131,6 @@ public class TestCachedResponseSuitabilityChecker {
         request.addHeader("Cache-Control", "max-age=10");
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
         currentAge(20L);
 
         replayMocks();
@@ -149,7 +145,6 @@ public class TestCachedResponseSuitabilityChecker {
         request.addHeader("Cache-Control", "max-age=10");
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
         currentAge(5L);
 
         replayMocks();
@@ -164,7 +159,6 @@ public class TestCachedResponseSuitabilityChecker {
         request.addHeader("Cache-Control", "min-fresh=10");
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
         freshnessLifetime(15L);
 
         replayMocks();
@@ -179,7 +173,6 @@ public class TestCachedResponseSuitabilityChecker {
         request.addHeader("Cache-Control", "min-fresh=10");
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
         freshnessLifetime(5L);
 
         replayMocks();
@@ -208,7 +201,6 @@ public class TestCachedResponseSuitabilityChecker {
         request.addHeader(new BasicHeader("Cache-Control", "max-age=foo"));
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
 
         replayMocks();
 
@@ -225,7 +217,6 @@ public class TestCachedResponseSuitabilityChecker {
 
         responseIsFresh(true);
         contentLengthMatchesActualLength(true);
-        modifiedSince(false, request);
 
         replayMocks();
 
@@ -251,11 +242,6 @@ public class TestCachedResponseSuitabilityChecker {
                 mockValidityPolicy.isResponseFresh(entry)).andReturn(fresh);
     }
 
-    private void modifiedSince(boolean modified, HttpRequest request) {
-        EasyMock.expect(
-                mockValidityPolicy.modifiedSince(entry, request)).andReturn(modified);
-    }
-
     private void contentLengthMatchesActualLength(boolean b) {
         EasyMock.expect(
                 mockValidityPolicy.contentLengthHeaderMatchesActualLength(entry)).andReturn(b);

httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestProtocolRequirements.java

@@ -2878,7 +2878,7 @@ public class TestProtocolRequirements extends AbstractProtocolTest {
 
         HttpRequest req2 = new BasicHttpRequest("GET", "/", HttpVersion.HTTP_1_1);
         req2.setHeader("If-None-Match", "W/\"etag\"");
-        req2.setHeader("If-Unmodified-Since", DateUtils.formatDate(twentySecondsAgo));
+        req2.setHeader("If-Modified-Since", DateUtils.formatDate(twentySecondsAgo));
 
         // must hit the origin again for the second request
         EasyMock.expect(
@@ -2893,6 +2893,36 @@ public class TestProtocolRequirements extends AbstractProtocolTest {
         Assert.assertFalse(HttpStatus.SC_NOT_MODIFIED == result.getStatusLine().getStatusCode());
     }
 
+    @Test
+    public void testConditionalRequestWhereAllValidatorsMatchMayBeServedFromCache()
+            throws Exception {
+        Date now = new Date();
+        Date tenSecondsAgo = new Date(now.getTime() - 10 * 1000L);
+
+        HttpRequest req1 = new BasicHttpRequest("GET", "/", HttpVersion.HTTP_1_1);
+        HttpResponse resp1 = make200Response();
+        resp1.setHeader("Date", DateUtils.formatDate(now));
+        resp1.setHeader("Cache-Control", "max-age=3600");
+        resp1.setHeader("Last-Modified", DateUtils.formatDate(tenSecondsAgo));
+        resp1.setHeader("ETag", "W/\"etag\"");
+
+        HttpRequest req2 = new BasicHttpRequest("GET", "/", HttpVersion.HTTP_1_1);
+        req2.setHeader("If-None-Match", "W/\"etag\"");
+        req2.setHeader("If-Modified-Since", DateUtils.formatDate(tenSecondsAgo));
+
+        // may hit the origin again for the second request
+        EasyMock.expect(
+                mockBackend.execute(EasyMock.isA(HttpHost.class),
+                        EasyMock.isA(HttpRequest.class),
+                        (HttpContext) EasyMock.isNull())).andReturn(resp1).times(1,2);
+
+        replayMocks();
+        impl.execute(host, req1);
+        impl.execute(host, req2);
+        verifyMocks();
+    }
+
+
     /*
      * "However, a cache that does not support the Range and Content-Range
      * headers MUST NOT cache 206 (Partial Content) responses."