Author: ssievers
Date: Sat Mar 9 22:44:54 2013
New Revision: 1454767
URL: http://svn.apache.org/r1454767
Log:
SHINDIG-1905 | DefaultRequestPipeline should attempt conditional gets for
stale, cached responses
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java?rev=1454767&r1=1454766&r2=1454767&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
Sat Mar 9 22:44:54 2013
@@ -18,6 +18,8 @@
*/
package org.apache.shindig.gadgets.http;
+import com.google.common.collect.Multimap;
+import com.google.common.net.HttpHeaders;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -61,7 +63,7 @@ public class DefaultRequestPipeline impl
@Inject(optional = true) @Named("shindig.http.date-drift-limit-ms")
private static long responseDateDriftLimit = DEFAULT_DRIFT_LIMIT_MS;
-
+
//class name for logging purpose
private static final String classname =
DefaultRequestPipeline.class.getName();
private static final Logger LOG =
Logger.getLogger(classname,MessageKeys.MESSAGES);
@@ -100,7 +102,7 @@ public class DefaultRequestPipeline impl
if (!cachedResponse.isStale()) {
if (invalidationService.isValid(request, cachedResponse)) {
if(LOG.isLoggable(Level.FINEST)) {
- LOG.logp(Level.FINEST, classname, method,
MessageKeys.CACHED_RESPONSE,
+ LOG.logp(Level.FINEST, classname, method,
MessageKeys.CACHED_RESPONSE,
new Object[]{request.getUri().toString()});
}
return cachedResponse;
@@ -114,6 +116,22 @@ public class DefaultRequestPipeline impl
}
}
}
+
+ // If we have a stale response, perform a conditional GET.
+ // Note: Fixing up the request with these headers will not affect http
response caching. See
+ // org.apache.shindig.gadgets.http.AbstractHttpCache.createKey(HttpRequest)
+ if (staleResponse != null) {
+ final String lastModified =
staleResponse.getHeader(HttpHeaders.LAST_MODIFIED);
+ if (lastModified != null) {
+ request.setHeader(HttpHeaders.IF_MODIFIED_SINCE, lastModified);
+ }
+
+ final String etag = staleResponse.getHeader(HttpHeaders.ETAG);
+ if (etag != null) {
+ request.setHeader(HttpHeaders.IF_NONE_MATCH, etag);
+ }
+ }
+
HttpResponse fetchedResponse = fetchResponse(request);
fetchedResponse = fixFetchedResponse(request, fetchedResponse,
invalidatedResponse,
staleResponse);
@@ -219,6 +237,38 @@ public class DefaultRequestPipeline impl
fetchedResponse = maybeFixDriftTime(fetchedResponse);
+ // 304 Not Modified
+ // Return the stale response and update what is in the cache with any new
headers per
+ // http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
+ //
+ // "If a cache uses a received 304 response to update a cache entry,
+ // the cache MUST update the entry to reflect any new field values
+ // given in the response."
+ if (fetchedResponse.getHttpStatusCode() == HttpResponse.SC_NOT_MODIFIED) {
+
+ // Update the stale response's headers with the new headers from the
fetched response.
+ // If the response from the remote server doesn't have an "Expires" or
"Cache-Control" header,
+ // we should service the current request and remove the stale response
from the cache. We rely
+ // on those headers to be present so that the response that is in the
cache will return the
+ // correct value when isStale() is called. Without removing the stale
response, we would get
+ // stuck in a loop of doing conditional GETs for the same stale resource
every time it is
+ // requested.
+ final Multimap<String, String> fetchedResponseHeaders =
fetchedResponse.getHeaders();
+ if (fetchedResponse.getCacheControlMaxAge() == -1
+ && fetchedResponse.getExpiresTime() == -1) {
+ // CONSIDER: We could try to recurse in this case and do a
non-conditional-get for the resource.
+ return httpCache.removeResponse(request);
+ }
+
+ HttpResponseBuilder httpResponseBuilder = new
HttpResponseBuilder(staleResponse);
+ for (String headerName : fetchedResponseHeaders.keySet()) {
+ httpResponseBuilder.removeHeader(headerName);
+ }
+
+ httpResponseBuilder.addAllHeaders(fetchedResponse.getHeaders());
+ return cacheFetchedResponse(request, httpResponseBuilder.create());
+ }
+
if (!fetchedResponse.isError() && !request.getIgnoreCache() &&
request.getCacheTtl() != 0) {
try {
fetchedResponse =
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java?rev=1454767&r1=1454766&r2=1454767&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
Sat Mar 9 22:44:54 2013
@@ -445,7 +445,7 @@ public final class HttpResponse implemen
/**
* @return the expiration time from the Expires header or -1 if not set
*/
- private long getExpiresTime() {
+ public long getExpiresTime() {
String expires = getHeader(HttpHeaders.EXPIRES);
if (expires != null) {
Date expiresDate = DateUtil.parseRfc1123Date(expires);
@@ -668,6 +668,8 @@ public final class HttpResponse implemen
return new LinkedList<String>(); //To change body of implemented
methods use File | Settings | File Templates.
}
}
+
+ // FIXME: Why isn't this a ListMultimap? Headers should be ordered and we
want to be able to do type checks on our Multimap.
public static Multimap<String,String> newHeaderMultimap() {
TreeMap<String,Collection<String>> map = new
TreeMap<String,Collection<String>>(String.CASE_INSENSITIVE_ORDER);
return Multimaps.newMultimap(map, HEADER_COLLECTION_SUPPLIER);
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java?rev=1454767&r1=1454766&r2=1454767&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
Sat Mar 9 22:44:54 2013
@@ -212,6 +212,20 @@ public class HttpResponseBuilder extends
return this;
}
+ /**
+ * Adds all the headers in the given multimap to the HttpResponse that is
under construction.
+ *
+ * @param headers
+ * A multimap of header keys and values. WARNING: This Multimap
should be one constructed
+ * by
org.apache.shindig.gadgets.http.HttpResponse.newHeaderMultimap()
+ * @return <code>this</code>
+ */
+ public HttpResponseBuilder addAllHeaders(Multimap<String, String> headers) {
+ this.headers.putAll(headers);
+ incrementNumChanges();
+ return this;
+ }
+
public Collection<String> removeHeader(String name) {
Collection<String> ret = headers.removeAll(name);
if (ret != null) {
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java?rev=1454767&r1=1454766&r2=1454767&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java
Sat Mar 9 22:44:54 2013
@@ -22,6 +22,7 @@ import static org.junit.Assert.assertEqu
import static org.junit.Assert.assertSame;
import com.google.common.collect.Maps;
+import com.google.common.net.HttpHeaders;
import com.google.inject.Provider;
import org.apache.shindig.common.uri.Uri;
@@ -39,6 +40,7 @@ import java.util.Map;
public class DefaultRequestPipelineTest {
private static final Uri DEFAULT_URI =
Uri.parse("http://example.org/gadget.xml";);
+ private static final String RFC1123_EPOCH = DateUtil.formatRfc1123Date(0);
private final FakeHttpFetcher fetcher = new FakeHttpFetcher();
private final FakeHttpCache cache = new FakeHttpCache();
@@ -217,6 +219,105 @@ public class DefaultRequestPipelineTest
}
@Test
+ public void authTypeNoneStaleConditionalGet() throws Exception {
+ // Cached response that is stale. Test that a conditional GET is used.
+ // Verify that the cached response is updated and returned.
+ // Verify that the 304 doesn't get cached.
+ HttpRequest request = new
HttpRequest(DEFAULT_URI).setAuthType(AuthType.NONE);
+
+ HttpResponse cached = new HttpResponseBuilder()
+ .setHeader(HttpHeaders.LAST_MODIFIED,
RFC1123_EPOCH)
+ .setHeader(HttpHeaders.ETAG, "ETAG")
+ .setCacheTtl(-1)
+ .create();
+ cache.data.put(DEFAULT_URI, cached);
+
+ String expiresDate = DateUtil.formatRfc1123Date(System.currentTimeMillis()
+ 3600 * 1000);
+ String maxAge = "max-age=3600";
+ HttpResponse notModified = new HttpResponseBuilder()
+
.setHttpStatusCode(HttpResponse.SC_NOT_MODIFIED)
+ .setHeader(HttpHeaders.EXPIRES,
expiresDate)
+ .setHeader(HttpHeaders.CACHE_CONTROL,
maxAge)
+ .create();
+ fetcher.response = notModified;
+ HttpResponse response = pipeline.execute(request);
+
+ HttpResponse expectedResponse = new HttpResponseBuilder(cached)
+ .setHeader(HttpHeaders.EXPIRES,
expiresDate)
+ .setHeader(HttpHeaders.CACHE_CONTROL,
maxAge)
+ .create();
+
+ assertEquals(RFC1123_EPOCH,
fetcher.request.getHeader(HttpHeaders.IF_MODIFIED_SINCE));
+ assertEquals("ETAG", fetcher.request.getHeader(HttpHeaders.IF_NONE_MATCH));
+ assertEquals(expectedResponse, response);
+ assertEquals(expectedResponse, cache.data.get(DEFAULT_URI));
+ assertEquals(1, cache.readCount);
+ assertEquals(1, cache.writeCount);
+ assertEquals(1, fetcher.fetchCount);
+ }
+
+ @Test
+ public void authTypeNoneStaleConditionalGetNoExpiresNoMaxAge() throws
Exception {
+ // Cached response that is stale and a conditional GET is used. Response
has no Expires
+ // header and no Cache-Control header with max-age. Remove the cached
entry from the cache and
+ // return it.
+ HttpRequest request = new
HttpRequest(DEFAULT_URI).setAuthType(AuthType.NONE);
+
+ HttpResponse cached = new HttpResponseBuilder()
+ .setHeader(HttpHeaders.LAST_MODIFIED,
RFC1123_EPOCH)
+ .setCacheTtl(-1)
+ .create();
+ cache.data.put(DEFAULT_URI, cached);
+
+ HttpResponse notModified = new HttpResponseBuilder()
+
.setHttpStatusCode(HttpResponse.SC_NOT_MODIFIED)
+ .create();
+ fetcher.response = notModified;
+ HttpResponse response = pipeline.execute(request);
+
+ assertEquals(RFC1123_EPOCH,
fetcher.request.getHeader(HttpHeaders.IF_MODIFIED_SINCE));
+ assertEquals(cached, response);
+ assertEquals(null, cache.data.get(DEFAULT_URI));
+ assertEquals(1, cache.readCount);
+ assertEquals(0, cache.writeCount);
+ assertEquals(1, fetcher.fetchCount);
+ }
+
+ @Test
+ public void authTypeNoneStaleConditionalGetNoLastModified() throws Exception
{
+ // Cached response is stale and has no Last-Modified header on it. Test
that an
+ // If-Modified-Since header is not issued.
+ HttpRequest request = new
HttpRequest(DEFAULT_URI).setAuthType(AuthType.NONE);
+
+ HttpResponse cached = new HttpResponseBuilder()
+ .setCacheTtl(-1)
+ .create();
+ cache.data.put(DEFAULT_URI, cached);
+
+ fetcher.response = HttpResponse.error(); // Really don't care what this is.
+ pipeline.execute(request);
+
+ assertEquals(null,
fetcher.request.getHeader(HttpHeaders.IF_MODIFIED_SINCE));
+ }
+
+ @Test
+ public void authTypeNoneStaleConditionalGetNoEtag() throws Exception {
+ // Cached response is stale and has no Etag header on it. Test that an
+ // If-None-Match header is not issued.
+ HttpRequest request = new
HttpRequest(DEFAULT_URI).setAuthType(AuthType.NONE);
+
+ HttpResponse cached = new HttpResponseBuilder()
+ .setCacheTtl(-1)
+ .create();
+ cache.data.put(DEFAULT_URI, cached);
+
+ fetcher.response = HttpResponse.error(); // Really don't care what this is.
+ pipeline.execute(request);
+
+ assertEquals(null, fetcher.request.getHeader(HttpHeaders.IF_NONE_MATCH));
+ }
+
+ @Test
public void authTypeOAuthNotCached() throws Exception {
HttpRequest request = new
HttpRequest(DEFAULT_URI).setAuthType(AuthType.OAUTH);
@@ -337,7 +438,7 @@ public class DefaultRequestPipelineTest
}
public HttpResponse removeResponse(HttpRequest key) {
- throw new UnsupportedOperationException();
+ return data.remove(key.getUri());
}
public String createKey(HttpRequest request) {
