Use different cache keys for different methods. (google#191)

RFC 7234 states that cache keys should contain both the request method and target URI. Volley's was only including the URI. However, simply changing the key as specified would result in complete invalidation of existing caches, which, while technically being safe, could have significant latency impact for many apps. Given that the majority of cached requests should be using GET, we omit the method for GET requests so that these cache entries have the same key, while adding the method for other requests so they don't collide unexpectedly. This will result in a cache clear for apps which are reliant on caching non-GET requests, which should be significantly rarer. If caches were previously poisoned due to this bug, it's possible they will remain poisoned until the cache entry expires. But this is no worse than the previous behavior. Finally - for DEPRECATED_GET_OR_POST, we can't tell which method will be used, so we assume it is GET (identical to previous behavior). In a future release, we will disable POST caching by default, as Volley's implementation is rarely going to be correct (and it is far from trivial to implement correctly, which is why most caches don't bother). Fixes google#154
adesanyaaa · May 21, 2018 · cb1df92 · cb1df92
1 parent a0f4d39
commit cb1df92
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 3 deletions.
diff --git a/src/main/java/com/android/volley/Request.java b/src/main/java/com/android/volley/Request.java
@@ -93,6 +93,7 @@ public interface Method {
     private RequestQueue mRequestQueue;
 
     /** Whether or not responses to this request should be cached. */
+    // TODO(#190): Turn this off by default for anything other than GET requests.
     private boolean mShouldCache = true;
 
     /** Whether or not this request has been canceled. */
@@ -286,7 +287,18 @@ public String getUrl() {
 
     /** Returns the cache key for this request. By default, this is the URL. */
     public String getCacheKey() {
-        return getUrl();
+        String url = getUrl();
+        // If this is a GET request, just use the URL as the key.
+        // For callers using DEPRECATED_GET_OR_POST, we assume the method is GET, which matches
+        // legacy behavior where all methods had the same cache key. We can't determine which method
+        // will be used because doing so requires calling getPostBody() which is expensive and may
+        // throw AuthFailureError.
+        // TODO(#190): Remove support for non-GET methods.
+        int method = getMethod();
+        if (method == Method.GET || method == Method.DEPRECATED_GET_OR_POST) {
+            return url;
+        }
+        return Integer.toString(method) + '-' + url;
     }
 
     /**

diff --git a/src/test/java/com/android/volley/RequestTest.java b/src/test/java/com/android/volley/RequestTest.java
@@ -20,6 +20,7 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
+import com.android.volley.Request.Method;
 import com.android.volley.Request.Priority;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -84,9 +85,30 @@ public void urlParsing() {
         assertFalse(0 == goodProtocol.getTrafficStatsTag());
     }
 
+    @Test
+    public void getCacheKey() {
+        assertEquals(
+                "http://example.com",
+                new UrlParseRequest(Method.GET, "http://example.com").getCacheKey());
+        assertEquals(
+                "http://example.com",
+                new UrlParseRequest(Method.DEPRECATED_GET_OR_POST, "http://example.com")
+                        .getCacheKey());
+        assertEquals(
+                "1-http://example.com",
+                new UrlParseRequest(Method.POST, "http://example.com").getCacheKey());
+        assertEquals(
+                "2-http://example.com",
+                new UrlParseRequest(Method.PUT, "http://example.com").getCacheKey());
+    }
+
     private static class UrlParseRequest extends Request<Object> {
-        public UrlParseRequest(String url) {
-            super(Request.Method.GET, url, null);
+        UrlParseRequest(String url) {
+            this(Method.GET, url);
+        }
+
+        UrlParseRequest(int method, String url) {
+            super(method, url, null);
         }
 
         @Override