Author: gagan
Date: Tue Feb 8 09:26:10 2011
New Revision: 1068301
URL: http://svn.apache.org/viewvc?rev=1068301&view=rev
Log:
Patch by nikhilmadan23 | Issue 4047043: Storing Strict No-Cache responses in
AbstractHttpCache | http://codereview.appspot.com/4047043/
Modified:
shindig/trunk/java/common/conf/shindig.properties
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
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/HttpResponseBuilder.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java
Modified: shindig/trunk/java/common/conf/shindig.properties
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/common/conf/shindig.properties?rev=1068301&r1=1068300&r2=1068301&view=diff
==============================================================================
--- shindig/trunk/java/common/conf/shindig.properties (original)
+++ shindig/trunk/java/common/conf/shindig.properties Tue Feb 8 09:26:10 2011
@@ -88,6 +88,10 @@ shindig.template-rewrite.extension-tag-n
shindig.cache.http.defaultTtl=3600000
shindig.cache.http.negativeCacheTtl=60000
+# The max-age with which strict no-cache resources should be stored in cache.
The default value is
+# -1, which indicates that strict no-cache resources should never be stored in
cache.
+shindig.cache.http.strict-no-cache-resource.max-age=-1
+
# A default refresh interval for XML files, since there is no natural way for
developers to
# specify this value, and most HTTP responses don't include good cache control
headers.
shindig.cache.xml.refreshInterval=300000
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java?rev=1068301&r1=1068300&r2=1068301&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
Tue Feb 8 09:26:10 2011
@@ -17,8 +17,12 @@
*/
package org.apache.shindig.gadgets.http;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Objects;
import com.google.common.base.Preconditions;
+import com.google.inject.Inject;
+import com.google.inject.name.Named;
+
import org.apache.shindig.auth.SecurityToken;
import org.apache.shindig.gadgets.AuthType;
import org.apache.shindig.gadgets.uri.UriCommon;
@@ -27,6 +31,10 @@ import org.apache.shindig.gadgets.uri.Ur
* Base class for content caches. Defines cache expiration rules and
* and restrictions on allowed content.
*
+ * Note that in the case where strictNoCacheResourceTtlInSeconds is
non-negative, strict no-cache
+ * resources are stored in the cache. In this case, only the
Cache-Control/Pragma headers are stored
+ * and not the original content or other headers.
+ *
* Implementations that override this are discouraged from using custom cache
keys unless there is
* actually customization in the request object itself. It is highly
recommended that you still
* use {@link #createKey} in the base class and append any custom data to the
end of the key instead
@@ -37,6 +45,13 @@ public abstract class AbstractHttpCache
private static final String RESIZE_WIDTH =
UriCommon.Param.RESIZE_WIDTH.getKey();
private static final String RESIZE_QUALITY =
UriCommon.Param.RESIZE_QUALITY.getKey();
+ // TTL to use for strict no-cache response. A value of -1 indicates that a
strict no-cache
+ // resource is never cached.
+ static final long DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL_SEC = -1;
+
+ @Inject(optional = true)
@Named("shindig.cache.http.strict-no-cache-resource.max-age")
+ private long strictNoCacheResourceTtlInSeconds =
DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL_SEC;
+
// Implement these methods to create a concrete HttpCache class.
protected abstract HttpResponse getResponseImpl(String key);
protected abstract void addResponseImpl(String key, HttpResponse response);
@@ -46,7 +61,8 @@ public abstract class AbstractHttpCache
if (isCacheable(request)) {
String keyString = createKey(request);
HttpResponse cached = getResponseImpl(keyString);
- if (responseStillUsable(cached)) {
+ if (responseStillUsable(cached) &&
+ (!cached.isStrictNoCache() || strictNoCacheResourceTtlInSeconds >
0)) {
return cached;
}
}
@@ -54,21 +70,34 @@ public abstract class AbstractHttpCache
}
public boolean addResponse(HttpRequest request, HttpResponse response) {
- if (isCacheable(request, response)) {
- // Both are cacheable. Check for forced cache TTL overrides.
- HttpResponseBuilder responseBuilder = new HttpResponseBuilder(response);
- int forcedTtl = request.getCacheTtl();
- if (forcedTtl != -1) {
- responseBuilder.setCacheTtl(forcedTtl);
+ HttpResponseBuilder responseBuilder;
+ boolean storeStrictNoCacheResources = (strictNoCacheResourceTtlInSeconds
>= 0);
+ if (isCacheable(request, response, storeStrictNoCacheResources)) {
+ if (storeStrictNoCacheResources && response.isStrictNoCache()) {
+ responseBuilder = buildStrictNoCacheHttpResponse(request, response);
+ } else {
+ responseBuilder = new HttpResponseBuilder(response);
}
-
- response = responseBuilder.create();
- String keyString = createKey(request);
- addResponseImpl(keyString, response);
- return true;
+ } else {
+ return false;
+ }
+ int forcedTtl = request.getCacheTtl();
+ if (forcedTtl != -1) {
+ responseBuilder.setCacheTtl(forcedTtl);
}
+ response = responseBuilder.create();
+ String keyString = createKey(request);
+ addResponseImpl(keyString, response);
+ return true;
+ }
- return false;
+ @VisibleForTesting
+ HttpResponseBuilder buildStrictNoCacheHttpResponse(HttpRequest request,
HttpResponse response) {
+ HttpResponseBuilder responseBuilder = new HttpResponseBuilder();
+ responseBuilder.setHeader("Cache-Control",
response.getHeader("Cache-Control"));
+ responseBuilder.setHeader("Pragma", response.getHeader("Pragma"));
+ responseBuilder.setCacheControlMaxAge(strictNoCacheResourceTtlInSeconds);
+ return responseBuilder;
}
public HttpResponse removeResponse(HttpRequest request) {
@@ -89,7 +118,8 @@ public abstract class AbstractHttpCache
"GET".equals(request.getHeader("X-Method-Override")));
}
- protected boolean isCacheable(HttpRequest request, HttpResponse response) {
+ protected boolean isCacheable(HttpRequest request, HttpResponse response,
+ boolean allowStrictNoCacheResponses) {
if (!isCacheable(request)) {
return false;
}
@@ -104,8 +134,8 @@ public abstract class AbstractHttpCache
return false;
}
- // If the HTTP response allows for it, we can cache.
- return !response.isStrictNoCache();
+ // If we allow strict no-cache responses or the HTTP response allows for
it, we can cache.
+ return allowStrictNoCacheResponses || !response.isStrictNoCache();
}
/**
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=1068301&r1=1068300&r2=1068301&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
Tue Feb 8 09:26:10 2011
@@ -80,8 +80,9 @@ public class DefaultRequestPipeline impl
if (!request.getIgnoreCache()) {
HttpResponse cachedResponse = httpCache.getResponse(request);
// Note that we don't remove invalidated entries from the cache as we
want them to be
- // available in the event of a backend fetch failure
- if (cachedResponse != null) {
+ // available in the event of a backend fetch failure.
+ // Note that strict no-cache entries are dummy responses and should not
be used.
+ if (cachedResponse != null && !cachedResponse.isStrictNoCache()) {
if (!cachedResponse.isStale()) {
if(invalidationService.isValid(request, cachedResponse)) {
return cachedResponse;
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=1068301&r1=1068300&r2=1068301&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
Tue Feb 8 09:26:10 2011
@@ -34,7 +34,9 @@ import org.w3c.dom.Document;
import org.w3c.dom.DocumentFragment;
import java.nio.charset.Charset;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.ListIterator;
import java.util.List;
import java.util.Map;
@@ -231,6 +233,22 @@ public class HttpResponseBuilder extends
return this;
}
+ public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) {
+ String cacheControl = getHeader("Cache-Control");
+ List<String> directives = Lists.newArrayList();
+ if (cacheControl != null) {
+ for (String directive : StringUtils.split(cacheControl, ',')) {
+ directive = directive.trim();
+ if (!directive.startsWith("max-age") || StringUtils.split(directive,
'=').length != 2) {
+ directives.add(directive);
+ }
+ }
+ }
+ directives.add("max-age=" + expirationTime);
+ setHeader("Cache-Control", StringUtils.join(directives, ','));
+ return this;
+ }
+
/**
* Sets cache-control headers indicating the response is not cacheable.
*/
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java?rev=1068301&r1=1068300&r2=1068301&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java
Tue Feb 8 09:26:10 2011
@@ -26,15 +26,24 @@ import static org.junit.Assert.assertTru
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.google.inject.Module;
+import com.google.inject.name.Names;
+import com.google.inject.util.Modules;
import org.apache.shindig.auth.BasicSecurityToken;
import org.apache.shindig.auth.SecurityToken;
import org.apache.shindig.common.uri.Uri;
+import org.apache.shindig.common.PropertiesModule;
import org.apache.shindig.gadgets.AuthType;
+import org.apache.shindig.gadgets.parse.ParseModule;
import org.apache.shindig.gadgets.oauth.OAuthArguments;
import org.apache.shindig.gadgets.spec.RequestAuthenticationInfo;
import org.apache.shindig.gadgets.uri.UriCommon.Param;
import org.easymock.EasyMock;
+import org.junit.Before;
import org.junit.Test;
import java.util.Map;
@@ -49,6 +58,22 @@ public class AbstractHttpCacheTest {
private static final String CONTAINER_NAME = "container";
private final TestHttpCache cache = new TestHttpCache();
+ // Cache designed to return 86400 for strictNoCacheResourceTtl.
+ private TestHttpCache extendedStrictNoCacheTtlCache;
+
+ @Before
+ public void setUp() {
+ Module module = new AbstractModule() {
+ @Override
+ public void configure() {
+ binder().bindConstant()
+
.annotatedWith(Names.named("shindig.cache.http.strict-no-cache-resource.max-age"))
+ .to(86400L);
+ }
+ };
+ Injector injector = Guice.createInjector(module);
+ extendedStrictNoCacheTtlCache = injector.getInstance(TestHttpCache.class);
+ }
@Test
public void createKeySimple() {
@@ -228,6 +253,9 @@ public class AbstractHttpCacheTest {
cache.map.put(key, response);
assertEquals(response, cache.getResponse(request));
+
+ extendedStrictNoCacheTtlCache.map.put(key, response);
+ assertEquals(response, extendedStrictNoCacheTtlCache.getResponse(request));
}
@Test
@@ -239,6 +267,10 @@ public class AbstractHttpCacheTest {
cache.map.put(key, response);
assertNull("Did not return null when method was POST",
cache.getResponse(request));
+
+ extendedStrictNoCacheTtlCache.map.put(key, response);
+ assertNull("Did not return null when method was POST",
+ extendedStrictNoCacheTtlCache.getResponse(request));
}
@Test
@@ -251,6 +283,9 @@ public class AbstractHttpCacheTest {
cache.map.put(key, response);
assertEquals(response, cache.getResponse(request));
+
+ extendedStrictNoCacheTtlCache.map.put(key, response);
+ assertEquals(response, extendedStrictNoCacheTtlCache.getResponse(request));
}
@Test
@@ -263,15 +298,23 @@ public class AbstractHttpCacheTest {
request.setIgnoreCache(true);
assertNull("Did not return null when ignoreCache was true",
cache.getResponse(request));
+
+ extendedStrictNoCacheTtlCache.map.put(key, response);
+ assertNull("Did not return null when ignoreCache was true",
+ extendedStrictNoCacheTtlCache.getResponse(request));
}
@Test
public void getResponseNotCacheable() {
HttpRequest request = new HttpRequest(DEFAULT_URI);
+ String key = cache.createKey(request);
HttpResponse response = new
HttpResponseBuilder().setStrictNoCache().create();
- cache.addResponse(request, response);
+ cache.map.put(key, response);
assertNull("Did not return null when response was uncacheable",
cache.getResponse(request));
+
+ extendedStrictNoCacheTtlCache.map.put(key, response);
+ assertEquals(response, extendedStrictNoCacheTtlCache.getResponse(request));
}
@Test
@@ -281,8 +324,11 @@ public class AbstractHttpCacheTest {
String key = cache.createKey(request);
assertTrue("response should have been cached", cache.addResponse(request,
response));
-
assertEquals(response, cache.map.get(key));
+
+ assertTrue("response should have been cached",
+ extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals(response, extendedStrictNoCacheTtlCache.map.get(key));
}
@Test
@@ -292,26 +338,39 @@ public class AbstractHttpCacheTest {
HttpResponse response = new HttpResponse("does not matter");
assertFalse("response should not have been cached",
cache.addResponse(request, response));
-
assertEquals(0, cache.map.size());
+
+ assertFalse("response should not have been cached",
+ extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals(0, extendedStrictNoCacheTtlCache.map.size());
}
@Test
public void addResponseNotCacheable() {
HttpRequest request = new HttpRequest(DEFAULT_URI);
HttpResponse response = new
HttpResponseBuilder().setStrictNoCache().create();
- assertFalse(cache.addResponse(request, response));
+ String key = cache.createKey(request);
+ assertFalse(cache.addResponse(request, response));
assertEquals(0, cache.map.size());
+
+ assertTrue("response should have been cached",
+ extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals(
+ extendedStrictNoCacheTtlCache.buildStrictNoCacheHttpResponse(request,
response).create(),
+ extendedStrictNoCacheTtlCache.map.get(key));
}
@Test
public void addResponseIfModifiedSince() {
HttpRequest request = new HttpRequest(DEFAULT_URI);
HttpResponse response = new
HttpResponseBuilder().setHttpStatusCode(HttpResponse.SC_NOT_MODIFIED).create();
- assertFalse(cache.addResponse(request, response));
+ assertFalse(cache.addResponse(request, response));
assertEquals(0, cache.map.size());
+
+ assertFalse(extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals(0, extendedStrictNoCacheTtlCache.map.size());
}
@Test
@@ -319,9 +378,12 @@ public class AbstractHttpCacheTest {
HttpRequest request = new HttpRequest(DEFAULT_URI)
.setMethod("POST");
HttpResponse response = new HttpResponse("does not matter");
- assertFalse(cache.addResponse(request, response));
+ assertFalse(cache.addResponse(request, response));
assertEquals(0, cache.map.size());
+
+ assertFalse(extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals(0, extendedStrictNoCacheTtlCache.map.size());
}
@Test
@@ -333,8 +395,10 @@ public class AbstractHttpCacheTest {
String key = cache.createKey(request);
assertTrue(cache.addResponse(request, response));
-
assertEquals(response, cache.map.get(key));
+
+ assertTrue(extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals(response, extendedStrictNoCacheTtlCache.map.get(key));
}
@Test
@@ -348,6 +412,10 @@ public class AbstractHttpCacheTest {
assertTrue(cache.addResponse(request, response));
assertEquals("public,max-age=10",
cache.map.get(key).getHeader("Cache-Control"));
+
+ assertTrue(extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals("public,max-age=10",
+
extendedStrictNoCacheTtlCache.map.get(key).getHeader("Cache-Control"));
}
@Test
@@ -364,6 +432,10 @@ public class AbstractHttpCacheTest {
assertTrue(cache.addResponse(request, response));
assertEquals("public,max-age=10",
cache.map.get(key).getHeader("Cache-Control"));
+
+ assertTrue(extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals("public,max-age=10",
+
extendedStrictNoCacheTtlCache.map.get(key).getHeader("Cache-Control"));
}
@Test
@@ -376,6 +448,42 @@ public class AbstractHttpCacheTest {
assertTrue(cache.addResponse(request, response));
assertEquals("no headers", cache.map.get(key).getResponseAsString());
+
+ assertTrue(extendedStrictNoCacheTtlCache.addResponse(request, response));
+ assertEquals("no headers",
extendedStrictNoCacheTtlCache.map.get(key).getResponseAsString());
+ }
+
+ @Test
+ public void buildStrictNoCacheHttpResponse() {
+ HttpResponse response = new HttpResponseBuilder()
+ .setResponseString("result")
+ .addHeader("Cache-Control", "private")
+ .addHeader("X-Method-Override", "GET")
+ .create();
+ assertTrue(response.isStrictNoCache());
+ HttpResponse builtResponse = extendedStrictNoCacheTtlCache
+ .buildStrictNoCacheHttpResponse(new HttpRequest(DEFAULT_URI),
response).create();
+
+ assertTrue(builtResponse.isStrictNoCache());
+ assertEquals("", builtResponse.getResponseAsString());
+ assertEquals("private,max-age=86400",
builtResponse.getHeader("Cache-Control"));
+ assertNull(builtResponse.getHeader("X-Method-Override"));
+ }
+
+ @Test
+ public void buildStrictNoCacheHttpResponseWithPragmaHeader() {
+ HttpResponse response = new HttpResponseBuilder()
+ .setResponseString("result")
+ .addHeader("Pragma", "no-cache")
+ .create();
+ assertTrue(response.isStrictNoCache());
+ HttpResponse builtResponse = cache
+ .buildStrictNoCacheHttpResponse(new HttpRequest(DEFAULT_URI),
response).create();
+
+ assertTrue(builtResponse.isStrictNoCache());
+ assertEquals("", builtResponse.getResponseAsString());
+ assertEquals("max-age=-1", builtResponse.getHeader("Cache-Control"));
+ assertEquals("no-cache", builtResponse.getHeader("Pragma"));
}
@Test
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java?rev=1068301&r1=1068300&r2=1068301&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java
Tue Feb 8 09:26:10 2011
@@ -106,6 +106,57 @@ public class HttpResponseBuilderTest {
}
@Test
+ public void setCacheControlMaxAge() {
+ HttpResponseBuilder builder = new HttpResponseBuilder()
+ .addHeader("Cache-Control", "public,max-age=100")
+ .setCacheControlMaxAge(12345);
+
+ Multimap<String, String> headers = builder.getHeaders();
+ assertEquals("public,max-age=12345",
headers.get("Cache-Control").iterator().next());
+ }
+
+ @Test
+ public void setCacheControlMaxAgeWithSpacesInCacheControlHeader() {
+ HttpResponseBuilder builder = new HttpResponseBuilder()
+ .addHeader("Cache-Control", "public, max-age=123, no-transform ")
+ .setCacheControlMaxAge(12345);
+
+ Multimap<String, String> headers = builder.getHeaders();
+ assertEquals("public,no-transform,max-age=12345",
+ headers.get("Cache-Control").iterator().next());
+ }
+
+ @Test
+ public void setCacheControlMaxAgeWithBadMaxAgeFormat() {
+ HttpResponseBuilder builder = new HttpResponseBuilder()
+ .addHeader("Cache-Control", "public, max-age=12=ab")
+ .setCacheControlMaxAge(12345);
+
+ Multimap<String, String> headers = builder.getHeaders();
+ assertEquals("public,max-age=12=ab,max-age=12345",
+ headers.get("Cache-Control").iterator().next());
+ }
+
+ @Test
+ public void setCacheControlMaxAgeWithNoInitialMaxAge() {
+ HttpResponseBuilder builder = new HttpResponseBuilder()
+ .addHeader("Cache-Control", "private")
+ .setCacheControlMaxAge(10000);
+
+ Multimap<String, String> headers = builder.getHeaders();
+ assertEquals("private,max-age=10000",
headers.get("Cache-Control").iterator().next());
+ }
+
+ @Test
+ public void setCacheControlMaxAgeWithNoCacheControlHeader() {
+ HttpResponseBuilder builder = new HttpResponseBuilder()
+ .setCacheControlMaxAge(86400);
+
+ Multimap<String, String> headers = builder.getHeaders();
+ assertEquals("max-age=86400",
headers.get("Cache-Control").iterator().next());
+ }
+
+ @Test
public void setCacheTtl() {
HttpResponseBuilder builder = new HttpResponseBuilder()
.addHeader("Pragma", "no-cache")
