Repository: square/okhttp
Issue: Should okhttp be caching HTTP 307 (or 302) responses according to the w3 document?

I am using retrofit and okhttp in my Android app to download documents, images and music files. The files are hosted on Amazon through a CDN so the URLs change often. My backend server will try to use redirects to decrease the need to have to constantly update my content on my mobile app every time the CDN url changes. The devices should also cache responses in the case that the device is offline. For this reason, I am using 301 redirects, which I don't know is the best idea.

I was reading the description for 307 redirect at http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

---

10.3.8 307 Temporary Redirect

The requested resource resides temporarily under a different URI. 
Since the redirection MAY be altered on occasion, the client SHOULD 
continue to use the Request-URI for future requests. 
This response is only cacheable if indicated by a Cache-Control or Expires header field.

---

At http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html,

---

13.4 - Response Cacheability

A response received with a status code of 200, 203, 206, 300, 301 or 410 
MAY be stored by a cache and used in reply to a subsequent request, 
subject to the expiration mechanism, unless a cache-control directive 
prohibits caching. However, a cache that does not support the Range and 
Content-Range headers MUST NOT cache 206 (Partial Content) responses.

A response received with any other status code 
(e.g. status codes 302 and 307) MUST NOT be returned in a reply to 
a subsequent request unless there are cache-control directives or another 
header(s) that explicitly allow it. For example, these include the 
following: an Expires header (section 14.21); a "max-age", "s-maxage", 
"must- revalidate", "proxy-revalidate", "public" or "private" 
cache-control directive (section 14.9).

---

It seems that the description states that caching is possible. I haven't tried this yet, but I did notice that in okhttp's CacheStrategy.java class, it has the following code:

---

public static boolean isCacheable(Response response, Request request) {
    // Always go to network for uncacheable response codes (RFC 2616, 13.4),
    // This implementation doesn't support caching partial content.
    int responseCode = response.code();
    if (responseCode != HttpURLConnection.HTTP_OK // 200
        && responseCode != HttpURLConnection.HTTP_NOT_AUTHORITATIVE // 203
        && responseCode != HttpURLConnection.HTTP_MULT_CHOICE // 300
        && responseCode != HttpURLConnection.HTTP_MOVED_PERM / 301
        && responseCode != HttpURLConnection.HTTP_GONE) { // 410
    return false;
}

---

And to verify that caching isn't enabled for from 302-308, the unit tests from CacheTest.java states the following

---

for (int i = 302; i <= 308; ++i) {
  assertCached(false, i);
}

---

So the okhttp code explicitly ignores caching for 307, and also 302. I just wanted clarification for this. Is the spec for 307 wrong? Or is something wrong with caching 307 that it was deliberately excluded? If I were to set my own cache headers for 307, it would seem to still be skipped.

Just another background information. I know Amazon will support caching with proper cache headers. But since I have to redirect through my own backend, if I don't have caching ability, my offline access to my redirect URL will not respond that I have a cached version from CDN locally. Of course, if I didn't redirect from my backend and go straight to Amazon, then caching wouldn't be a problem through Amazon. Could you provide some advice in this scenario?


Changed files:
okhttp/src/main/java/com/squareup/okhttp/internal/http/CacheStrategy.java
okhttp-urlconnection/src/test/java/com/squareup/okhttp/UrlConnectionCacheTest.java
okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java

diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java
index 9fba4601c..0fd174648 100644
--- a/okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java
+++ b/okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java
@@ -121,9 +121,12 @@ public final class CacheTest {
     assertCached(false, 207);
     assertCached(true, 300);
     assertCached(true, 301);
-    for (int i = 302; i <= 307; ++i) {
-      assertCached(false, i);
-    }
+    assertCached(true, 302);
+    assertCached(false, 303);
+    assertCached(false, 304);
+    assertCached(false, 305);
+    assertCached(false, 306);
+    assertCached(true, 307);
     assertCached(true, 308);
     for (int i = 400; i <= 406; ++i) {
       assertCached(false, i);
@@ -394,6 +397,63 @@ public final class CacheTest {
     assertEquals(2, cache.getHitCount());
   }
 
+  @Test public void foundCachedWithExpiresHeader() throws Exception {
+    temporaryRedirectCachedWithCachingHeader(302, "Expires", formatDate(1, TimeUnit.HOURS));
+  }
+
+  @Test public void foundCachedWithCacheControlHeader() throws Exception {
+    temporaryRedirectCachedWithCachingHeader(302, "Cache-Control", "max-age=60");
+  }
+
+  @Test public void temporaryRedirectCachedWithExpiresHeader() throws Exception {
+    temporaryRedirectCachedWithCachingHeader(307, "Expires", formatDate(1, TimeUnit.HOURS));
+  }
+
+  @Test public void temporaryRedirectCachedWithCacheControlHeader() throws Exception {
+    temporaryRedirectCachedWithCachingHeader(307, "Cache-Control", "max-age=60");
+  }
+
+  @Test public void foundNotCachedWithoutCacheHeader() throws Exception {
+    temporaryRedirectNotCachedWithoutCachingHeader(302);
+  }
+
+  @Test public void temporaryRedirectNotCachedWithoutCacheHeader() throws Exception {
+    temporaryRedirectNotCachedWithoutCachingHeader(307);
+  }
+
+  private void temporaryRedirectCachedWithCachingHeader(
+      int responseCode, String headerName, String headerValue) throws Exception {
+    server.enqueue(new MockResponse()
+        .setResponseCode(responseCode)
+        .addHeader(headerName, headerValue)
+        .addHeader("Location", "/a"));
+    server.enqueue(new MockResponse()
+        .addHeader(headerName, headerValue)
+        .setBody("a"));
+    server.enqueue(new MockResponse()
+        .setBody("b"));
+    server.enqueue(new MockResponse()
+        .setBody("c"));
+
+    URL url = server.getUrl("/");
+    assertEquals("a", get(url).body().string());
+    assertEquals("a", get(url).body().string());
+  }
+
+  private void temporaryRedirectNotCachedWithoutCachingHeader(int responseCode) throws Exception {
+    server.enqueue(new MockResponse()
+        .setResponseCode(responseCode)
+        .addHeader("Location", "/a"));
+    server.enqueue(new MockResponse()
+        .setBody("a"));
+    server.enqueue(new MockResponse()
+        .setBody("b"));
+
+    URL url = server.getUrl("/");
+    assertEquals("a", get(url).body().string());
+    assertEquals("b", get(url).body().string());
+  }
+
   @Test public void serverDisconnectsPrematurelyWithContentLengthHeader() throws IOException {
     testServerPrematureDisconnect(TransferKind.FIXED_LENGTH);
   }
diff --git a/okhttp-urlconnection/src/test/java/com/squareup/okhttp/UrlConnectionCacheTest.java b/okhttp-urlconnection/src/test/java/com/squareup/okhttp/UrlConnectionCacheTest.java
index 800124bdf..79d73f4ee 100644
--- a/okhttp-urlconnection/src/test/java/com/squareup/okhttp/UrlConnectionCacheTest.java
+++ b/okhttp-urlconnection/src/test/java/com/squareup/okhttp/UrlConnectionCacheTest.java
@@ -132,9 +132,12 @@ public final class UrlConnectionCacheTest {
     assertCached(false, 207);
     assertCached(true, 300);
     assertCached(true, 301);
-    for (int i = 302; i <= 307; ++i) {
-      assertCached(false, i);
-    }
+    assertCached(true, 302);
+    assertCached(false, 303);
+    assertCached(false, 304);
+    assertCached(false, 305);
+    assertCached(false, 306);
+    assertCached(true, 307);
     assertCached(true, 308);
     for (int i = 400; i <= 406; ++i) {
       assertCached(false, i);
@@ -158,12 +161,12 @@ public final class UrlConnectionCacheTest {
 
   private void assertCached(boolean shouldPut, int responseCode) throws Exception {
     server = new MockWebServer();
-    MockResponse response =
-        new MockResponse().addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS))
-            .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS))
-            .setResponseCode(responseCode)
-            .setBody("ABCDE")
-            .addHeader("WWW-Authenticate: challenge");
+    MockResponse response = new MockResponse()
+        .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS))
+        .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS))
+        .setResponseCode(responseCode)
+        .setBody("ABCDE")
+        .addHeader("WWW-Authenticate: challenge");
     if (responseCode == HttpURLConnection.HTTP_PROXY_AUTH) {
       response.addHeader("Proxy-Authenticate: Basic realm=\\"protected area\\"");
     } else if (responseCode == HttpURLConnection.HTTP_UNAUTHORIZED) {
diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/CacheStrategy.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/CacheStrategy.java
index 93c6d7e6d..69db3947f 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/CacheStrategy.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/CacheStrategy.java
@@ -7,8 +7,10 @@ import com.squareup.okhttp.Response;
 import java.util.Date;
 
 import static com.squareup.okhttp.internal.http.StatusLine.HTTP_PERM_REDIRECT;
+import static com.squareup.okhttp.internal.http.StatusLine.HTTP_TEMP_REDIRECT;
 import static java.net.HttpURLConnection.HTTP_GONE;
 import static java.net.HttpURLConnection.HTTP_MOVED_PERM;
+import static java.net.HttpURLConnection.HTTP_MOVED_TEMP;
 import static java.net.HttpURLConnection.HTTP_MULT_CHOICE;
 import static java.net.HttpURLConnection.HTTP_NOT_AUTHORITATIVE;
 import static java.net.HttpURLConnection.HTTP_OK;
@@ -39,26 +41,36 @@ public final class CacheStrategy {
    * request.
    */
   public static boolean isCacheable(Response response, Request request) {
-    // Always go to network for uncacheable response codes (RFC 2616, 13.4),
+    // Always go to network for uncacheable response codes (RFC 7231 section 6.1),
     // This implementation doesn't support caching partial content.
-    int responseCode = response.code();
-    if (responseCode != HTTP_OK
-        && responseCode != HTTP_NOT_AUTHORITATIVE
-        && responseCode != HTTP_MULT_CHOICE
-        && responseCode != HTTP_MOVED_PERM
-        && responseCode != HTTP_GONE
-        && responseCode != HTTP_PERM_REDIRECT) {
-      return false;
-    }
+    switch (response.code()) {
+      case HTTP_OK:
+      case HTTP_NOT_AUTHORITATIVE:
+      case HTTP_MULT_CHOICE:
+      case HTTP_MOVED_PERM:
+      case HTTP_GONE:
+      case HTTP_PERM_REDIRECT:
+        // These codes can be cached unless headers forbid it.
+        break;
+
+      case HTTP_MOVED_TEMP:
+      case HTTP_TEMP_REDIRECT:
+        // These codes can only be cached with the right response headers.
+        if (response.header("Expires") != null
+            || response.cacheControl().maxAgeSeconds() != -1
+            || response.cacheControl().sMaxAgeSeconds() != -1
+            || response.cacheControl().isPublic()) {
+          break;
+        }
+        // Fall-through.
 
-    // A 'no-store' directive on request or response prevents the response from being cached.
-    CacheControl responseCaching = response.cacheControl();
-    CacheControl requestCaching = request.cacheControl();
-    if (responseCaching.noStore() || requestCaching.noStore()) {
-      return false;
+      default:
+        // All other codes cannot be cached.
+        return false;
     }
 
-    return true;
+    // A 'no-store' directive on request or response prevents the response from being cached.
+    return !response.cacheControl().noStore() && !request.cacheControl().noStore();
   }
 
   public static class Factory {