From ddac0083db1f4d8fccf68c23254751c4167565a1 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Sat, 1 Nov 2014 11:05:56 -0400 Subject: [PATCH] Fix request cache-control to match spec. Fixes https://github.com/square/okhttp/issues/1081 --- .../java/com/squareup/okhttp/CacheTest.java | 43 +++++++++++++++++++ .../com/squareup/okhttp/CacheControl.java | 8 ++-- .../okhttp/internal/http/CacheStrategy.java | 6 ++- .../okhttp/internal/http/HeaderParser.java | 6 +-- 4 files changed, 54 insertions(+), 9 deletions(-) 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 dcda4e99b8f5..b9fa9d036420 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java @@ -825,6 +825,28 @@ private void testMethodInvalidates(String requestMethod) throws Exception { assertEquals("A", get(url).body().string()); } + @Test public void clientSideNoStore() throws Exception { + server.enqueue(new MockResponse() + .addHeader("Cache-Control: max-age=60") + .setBody("A")); + server.enqueue(new MockResponse() + .addHeader("Cache-Control: max-age=60") + .setBody("B")); + + Request request1 = new Request.Builder() + .url(server.getUrl("/")) + .cacheControl(new CacheControl.Builder().noStore().build()) + .build(); + Response response1 = client.newCall(request1).execute(); + assertEquals("A", response1.body().string()); + + Request request2 = new Request.Builder() + .url(server.getUrl("/")) + .build(); + Response response2 = client.newCall(request2).execute(); + assertEquals("B", response2.body().string()); + } + @Test public void nonIdentityEncodingAndConditionalCache() throws Exception { assertNonIdentityEncodingCached( new MockResponse().addHeader("Last-Modified: " + formatDate(-2, TimeUnit.HOURS)) @@ -958,6 +980,27 @@ private void assertNonIdentityEncodingCached(MockResponse response) throws Excep assertEquals("110 HttpURLConnection \"Response is stale\"", response.header("Warning")); } + @Test public void requestMaxStaleDirectiveWithNoValue() throws IOException { + // Add a stale response to the cache. + server.enqueue(new MockResponse() + .setBody("A") + .addHeader("Cache-Control: max-age=120") + .addHeader("Date: " + formatDate(-4, TimeUnit.MINUTES))); + server.enqueue(new MockResponse() + .setBody("B")); + + assertEquals("A", get(server.getUrl("/")).body().string()); + + // With max-stale, we'll return that stale response. + Request request = new Request.Builder() + .url(server.getUrl("/")) + .header("Cache-Control", "max-stale") + .build(); + Response response = client.newCall(request).execute(); + assertEquals("A", response.body().string()); + assertEquals("110 HttpURLConnection \"Response is stale\"", response.header("Warning")); + } + @Test public void requestMaxStaleNotHonoredWithMustRevalidate() throws IOException { server.enqueue(new MockResponse() .setBody("A") diff --git a/okhttp/src/main/java/com/squareup/okhttp/CacheControl.java b/okhttp/src/main/java/com/squareup/okhttp/CacheControl.java index c19d77958b7e..774f785760f0 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/CacheControl.java +++ b/okhttp/src/main/java/com/squareup/okhttp/CacheControl.java @@ -192,17 +192,17 @@ public static CacheControl parse(Headers headers) { } else if ("no-store".equalsIgnoreCase(directive)) { noStore = true; } else if ("max-age".equalsIgnoreCase(directive)) { - maxAgeSeconds = HeaderParser.parseSeconds(parameter); + maxAgeSeconds = HeaderParser.parseSeconds(parameter, -1); } else if ("s-maxage".equalsIgnoreCase(directive)) { - sMaxAgeSeconds = HeaderParser.parseSeconds(parameter); + sMaxAgeSeconds = HeaderParser.parseSeconds(parameter, -1); } else if ("public".equalsIgnoreCase(directive)) { isPublic = true; } else if ("must-revalidate".equalsIgnoreCase(directive)) { mustRevalidate = true; } else if ("max-stale".equalsIgnoreCase(directive)) { - maxStaleSeconds = HeaderParser.parseSeconds(parameter); + maxStaleSeconds = HeaderParser.parseSeconds(parameter, Integer.MAX_VALUE); } else if ("min-fresh".equalsIgnoreCase(directive)) { - minFreshSeconds = HeaderParser.parseSeconds(parameter); + minFreshSeconds = HeaderParser.parseSeconds(parameter, -1); } else if ("only-if-cached".equalsIgnoreCase(directive)) { onlyIfCached = true; } else if ("no-transform".equalsIgnoreCase(directive)) { 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 2363d53b6332..44f745147034 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 @@ -60,7 +60,9 @@ public static boolean isCacheable(Response response, Request request) { return false; } - if (responseCaching.noStore()) { + // A 'no-store' directive on request or response prevents the response from being cached. + CacheControl requestCaching = request.cacheControl(); + if (responseCaching.noStore() || requestCaching.noStore()) { return false; } @@ -124,7 +126,7 @@ public Factory(long nowMillis, Request request, Response cacheResponse) { } else if ("ETag".equalsIgnoreCase(fieldName)) { etag = value; } else if ("Age".equalsIgnoreCase(fieldName)) { - ageSeconds = HeaderParser.parseSeconds(value); + ageSeconds = HeaderParser.parseSeconds(value, -1); } else if (OkHeaders.SENT_MILLIS.equalsIgnoreCase(fieldName)) { sentRequestMillis = Long.parseLong(value); } else if (OkHeaders.RECEIVED_MILLIS.equalsIgnoreCase(fieldName)) { diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HeaderParser.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HeaderParser.java index e9af13026ca0..55f82ada47b9 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HeaderParser.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HeaderParser.java @@ -47,9 +47,9 @@ public static int skipWhitespace(String input, int pos) { /** * Returns {@code value} as a positive integer, or 0 if it is negative, or - * -1 if it cannot be parsed. + * {@code defaultValue} if it cannot be parsed. */ - public static int parseSeconds(String value) { + public static int parseSeconds(String value, int defaultValue) { try { long seconds = Long.parseLong(value); if (seconds > Integer.MAX_VALUE) { @@ -60,7 +60,7 @@ public static int parseSeconds(String value) { return (int) seconds; } } catch (NumberFormatException e) { - return -1; + return defaultValue; } }