You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by be...@apache.org on 2008/07/21 08:01:04 UTC
svn commit: r678354 - in /incubator/shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/http/
main/java/org/apache/shindig/gadgets/oauth/
test/java/org/apache/shindig/gadgets/http/
test/java/org/apache/shindig/gadgets/oauth/
Author: beaton
Date: Sun Jul 20 23:01:03 2008
New Revision: 678354
URL: http://svn.apache.org/viewvc?rev=678354&view=rev
Log:
Add caching of OAuth responses.
Modified:
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcher.java
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherFactory.java
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthFetcherTest.java
Modified: incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java?rev=678354&r1=678353&r2=678354&view=diff
==============================================================================
--- incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (original)
+++ incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java Sun Jul 20 23:01:03 2008
@@ -331,17 +331,25 @@
this.headers.put("Cache-Control", Lists.newArrayList("public,max-age=" + forcedCacheTtl));
}
}
+
+ /**
+ * Sets cache-control headers indicating the response is not cacheable.
+ */
+ public void setNoCache() {
+ this.headers.put("Cache-Control", Lists.newArrayList("no-cache"));
+ this.headers.put("Pragma", Lists.newArrayList("no-cache"));
+ }
/**
* @return consolidated cache expiration time or -1
*/
public long getCacheExpiration() {
- if (httpStatusCode != SC_OK) {
- return getDate() + negativeCacheTtl;
- }
if (isStrictNoCache()) {
return -1;
}
+ if (httpStatusCode != SC_OK) {
+ return getDate() + negativeCacheTtl;
+ }
long maxAge = getCacheControlMaxAge();
if (maxAge != -1) {
return getDate() + maxAge;
Modified: incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcher.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcher.java?rev=678354&r1=678353&r2=678354&view=diff
==============================================================================
--- incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcher.java (original)
+++ incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcher.java Sun Jul 20 23:01:03 2008
@@ -21,6 +21,8 @@
import org.apache.shindig.common.crypto.BlobCrypterException;
import org.apache.shindig.gadgets.ChainedContentFetcher;
import org.apache.shindig.gadgets.GadgetException;
+import org.apache.shindig.gadgets.http.HttpCache;
+import org.apache.shindig.gadgets.http.HttpCacheKey;
import org.apache.shindig.gadgets.http.HttpFetcher;
import org.apache.shindig.gadgets.http.HttpResponse;
import org.apache.shindig.gadgets.http.HttpRequest;
@@ -142,6 +144,11 @@
* to the gadget spec for information (e.g. OAuth URLs).
*/
private final boolean bypassSpecCache;
+
+ /**
+ * HTTP cache.
+ */
+ private HttpCache cache;
/**
*
@@ -151,13 +158,15 @@
* @param authToken user's gadget security token
* @param params OAuth fetch parameters sent from makeRequest
* @param tokenStore storage for long lived tokens.
+ * @param cache cache to use for HTTP responses.
*/
public OAuthFetcher(
GadgetOAuthTokenStore tokenStore,
BlobCrypter oauthCrypter,
HttpFetcher nextFetcher,
SecurityToken authToken,
- OAuthRequestParams params) {
+ OAuthRequestParams params,
+ HttpCache cache) {
super(nextFetcher);
this.oauthCrypter = oauthCrypter;
this.authToken = authToken;
@@ -180,6 +189,7 @@
this.origClientState = new HashMap<String, String>();
}
this.tokenStore = tokenStore;
+ this.cache = cache;
}
/**
@@ -226,6 +236,12 @@
}
public HttpResponse fetch(HttpRequest request) throws GadgetException {
+ HttpCacheKey cacheKey = makeCacheKey(request);
+ HttpResponse response = cache.getResponse(cacheKey, request);
+ if (response != null) {
+ return response;
+ }
+
try {
lookupOAuthMetadata();
} catch (GadgetException e) {
@@ -234,11 +250,7 @@
}
this.realRequest = request;
- // Work around for busted HttpCache interface that can't
- // properly handle authenticated content.
- this.realRequest.getOptions().ignoreCache = true;
- HttpResponse response = null;
int attempts = 0;
boolean retry;
do {
@@ -259,9 +271,25 @@
GadgetException.Code.INTERNAL_SERVER_ERROR,
"No response for OAuth fetch to " + realRequest.getUri());
}
- return response;
+ return cache.addResponse(cacheKey, request, response);
}
+ // Builds up a cache key based on the same key that we use into the OAuth
+ // token storage, which should identify precisely which data to return for the
+ // response. Using the OAuth access token as the cache key is another
+ // possibility.
+ private HttpCacheKey makeCacheKey(HttpRequest request) {
+ HttpCacheKey key = new HttpCacheKey(request);
+ key.set("authentication", "oauth");
+ OAuthStore.TokenKey tokenKey = buildTokenKey();
+ key.set("user", tokenKey.getUserId());
+ key.set("gadget", tokenKey.getGadgetUri());
+ key.set("instance", Long.toString(tokenKey.getModuleId()));
+ key.set("service", tokenKey.getServiceName());
+ key.set("token", tokenKey.getTokenName());
+ return key;
+ }
+
private HttpResponse buildErrorResponse(GadgetException e) {
if (error == null) {
error = OAuthError.UNKNOWN_PROBLEM;
@@ -566,6 +594,7 @@
private HttpResponse buildNonDataResponse() {
HttpResponse response = new HttpResponse(0, null, null);
addResponseMetadata(response);
+ response.setNoCache();
return response;
}
@@ -678,6 +707,9 @@
realRequest.getContentType(),
realRequest.getPostBodyAsString(),
realRequest.getOptions());
+
+ // Not externally cacheable.
+ oauthHttpRequest.getOptions().ignoreCache = true;
HttpResponse response = nextFetcher.fetch(oauthHttpRequest);
Modified: incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherFactory.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherFactory.java?rev=678354&r1=678353&r2=678354&view=diff
==============================================================================
--- incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherFactory.java (original)
+++ incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherFactory.java Sun Jul 20 23:01:03 2008
@@ -24,6 +24,7 @@
import org.apache.shindig.common.crypto.Crypto;
import org.apache.shindig.gadgets.GadgetException;
import org.apache.shindig.gadgets.GadgetSpecFactory;
+import org.apache.shindig.gadgets.http.HttpCache;
import org.apache.shindig.gadgets.http.HttpFetcher;
import com.google.inject.Inject;
@@ -42,6 +43,9 @@
/** persistent storage for OAuth tokens */
protected GadgetOAuthTokenStore tokenStore;
+ /** shared HTTP cache */
+ private HttpCache cache;
+
private static final Logger logger
= Logger.getLogger(OAuthFetcherFactory.class.getName());
@@ -50,7 +54,7 @@
* BlobCrypter and consumer keys/secrets read from oauth.js
*/
@Inject
- public OAuthFetcherFactory(GadgetSpecFactory specFactory) {
+ public OAuthFetcherFactory(GadgetSpecFactory specFactory, HttpCache cache) {
try {
this.oauthCrypter = new BasicBlobCrypter(
Crypto.getRandomBytes(BasicBlobCrypter.MASTER_KEY_MIN_LEN));
@@ -59,6 +63,7 @@
new BasicGadgetOAuthTokenStore(new BasicOAuthStore(), specFactory);
store.initFromConfigFile();
this.tokenStore = store;
+ this.cache = cache;
} catch (Throwable t) {
// Since this happens at startup, we don't want to kill the server just
// because we can't initialize the OAuth config.
@@ -71,12 +76,15 @@
*
* @param oauthCrypter used to wrap client side state
* @param tokenStore used as interface to persistent token store.
+ * @param cache shared HTTP cache
*/
protected OAuthFetcherFactory(
BlobCrypter oauthCrypter,
- GadgetOAuthTokenStore tokenStore) {
+ GadgetOAuthTokenStore tokenStore,
+ HttpCache cache) {
this.oauthCrypter = oauthCrypter;
this.tokenStore = tokenStore;
+ this.cache = cache;
}
/**
@@ -95,7 +103,7 @@
SecurityToken token,
OAuthRequestParams params) throws GadgetException {
OAuthFetcher fetcher = new OAuthFetcher(
- tokenStore, oauthCrypter, nextFetcher, token, params);
+ tokenStore, oauthCrypter, nextFetcher, token, params, cache);
return fetcher;
}
}
Modified: incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java?rev=678354&r1=678353&r2=678354&view=diff
==============================================================================
--- incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java (original)
+++ incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java Sun Jul 20 23:01:03 2008
@@ -249,6 +249,12 @@
long ttl = HttpResponse.error().getCacheTtl();
assertTrue(ttl <= HttpResponse.DEFAULT_TTL && ttl > 0);
}
+
+ public void testStrictNoCacheAndNegativeCaching() {
+ HttpResponse response = new HttpResponse(401, UTF8_DATA, null);
+ response.setNoCache();
+ assertEquals(-1, response.getCacheTtl());
+ }
public void testNullHeaderNamesStripped() {
addHeader(null, "dummy");
Modified: incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthFetcherTest.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthFetcherTest.java?rev=678354&r1=678353&r2=678354&view=diff
==============================================================================
--- incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthFetcherTest.java (original)
+++ incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthFetcherTest.java Sun Jul 20 23:01:03 2008
@@ -30,6 +30,8 @@
import org.apache.shindig.common.crypto.BlobCrypter;
import org.apache.shindig.gadgets.FakeGadgetSpecFactory;
import org.apache.shindig.gadgets.GadgetException;
+import org.apache.shindig.gadgets.http.BasicHttpCache;
+import org.apache.shindig.gadgets.http.HttpCache;
import org.apache.shindig.gadgets.http.HttpFetcher;
import org.apache.shindig.gadgets.http.HttpRequest;
import org.apache.shindig.gadgets.http.HttpResponse;
@@ -49,6 +51,7 @@
private GadgetOAuthTokenStore tokenStore;
private BlobCrypter blobCrypter;
private FakeOAuthServiceProvider serviceProvider;
+ private HttpCache cache;
public static final String GADGET_URL = "http://www.example.com/gadget.xml";
public static final String GADGET_URL_NO_KEY =
@@ -59,6 +62,7 @@
serviceProvider = new FakeOAuthServiceProvider();
tokenStore = getOAuthStore();
blobCrypter = new BasicBlobCrypter("abcdefghijklmnop".getBytes());
+ cache = new BasicHttpCache(10);
}
/**
@@ -133,7 +137,7 @@
public HttpFetcher getFetcher(SecurityToken authToken,
OAuthRequestParams params) throws GadgetException {
OAuthFetcher fetcher = new OAuthFetcher(
- tokenStore, blobCrypter, serviceProvider, authToken, params);
+ tokenStore, blobCrypter, serviceProvider, authToken, params, cache);
return fetcher;
}
@@ -236,6 +240,7 @@
clientState, false));
request = new HttpRequest(
new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
clientState = response.getMetadata().get("oauthState");
@@ -250,6 +255,7 @@
clientState, false));
request = new HttpRequest(
new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
assertEquals("User data is reapproved", response.getResponseAsString());
}
@@ -294,6 +300,7 @@
clientState, false));
request = new HttpRequest(
new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
clientState = response.getMetadata().get("oauthState");
@@ -308,6 +315,7 @@
clientState, false));
request = new HttpRequest(
new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
assertEquals("User data is reapproved", response.getResponseAsString());
}
@@ -371,6 +379,7 @@
false));
request = new HttpRequest(
new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
clientState = response.getMetadata().get("oauthState");
assertNotNull(clientState);
@@ -387,8 +396,8 @@
getSecurityToken("owner", "owner"),
new OAuthRequestParams(FakeGadgetSpecFactory.SERVICE_NAME, null,
clientState, false));
- request = new HttpRequest(
- new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
assertEquals("User data is hello-oauth", response.getResponseAsString());
clientState = response.getMetadata().get("oauthState");
@@ -401,8 +410,8 @@
getSecurityToken("owner", "owner"),
new OAuthRequestParams(FakeGadgetSpecFactory.SERVICE_NAME, null,
clientState, false));
- request = new HttpRequest(
- new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
assertEquals("User data is hello-oauth", response.getResponseAsString());
clientState = response.getMetadata().get("oauthState");
@@ -417,8 +426,8 @@
getSecurityToken("owner", "owner"),
new OAuthRequestParams(FakeGadgetSpecFactory.SERVICE_NAME, null, null,
false));
- request = new HttpRequest(
- new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
Map<String, String> metadata = response.getMetadata();
assertNotNull(metadata);
@@ -438,8 +447,8 @@
getSecurityToken("owner", "owner"),
new OAuthRequestParams(FakeGadgetSpecFactory.SERVICE_NAME, null,
clientState, false));
- request = new HttpRequest(
- new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
clientState = response.getMetadata().get("oauthState");
@@ -490,8 +499,7 @@
reqToken.secret);
fetcher = getFetcher(getSecurityToken("owner", "owner"), params);
- request = new HttpRequest(
- new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
response = fetcher.fetch(request);
String clientState = response.getMetadata().get("oauthState");
@@ -504,8 +512,8 @@
assertEquals(1, serviceProvider.getResourceAccessCount());
fetcher = getFetcher(getSecurityToken("owner", "owner"), params);
- request = new HttpRequest(
- new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
assertEquals("User data is preapproved", response.getResponseAsString());
@@ -514,8 +522,8 @@
assertEquals(2, serviceProvider.getResourceAccessCount());
fetcher = getFetcher(getSecurityToken("owner", "owner"), params);
- request = new HttpRequest(
- new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ request.getOptions().ignoreCache = true;
response = fetcher.fetch(request);
assertEquals("User data is preapproved", response.getResponseAsString());
@@ -610,5 +618,52 @@
assertEquals("User data is hello-oauth", response.getResponseAsString());
}
+ @Test
+ public void testCachedResponse() throws Exception {
+ HttpFetcher fetcher;
+ HttpRequest request;
+ HttpResponse response;
+
+ assertEquals(0, serviceProvider.getRequestTokenCount());
+ assertEquals(0, serviceProvider.getAccessTokenCount());
+ assertEquals(0, serviceProvider.getResourceAccessCount());
+
+ fetcher = getFetcher(
+ getSecurityToken("owner", "owner"),
+ new OAuthRequestParams(FakeGadgetSpecFactory.SERVICE_NAME, null, null,
+ false));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ response = fetcher.fetch(request);
+ String clientState = response.getMetadata().get("oauthState");
+ assertNotNull(clientState);
+ String approvalUrl = response.getMetadata().get("oauthApprovalUrl");
+ assertNotNull(approvalUrl);
+
+ serviceProvider.browserVisit(approvalUrl + "&user_data=hello-oauth");
+ fetcher = getFetcher(
+ getSecurityToken("owner", "owner"),
+ new OAuthRequestParams(FakeGadgetSpecFactory.SERVICE_NAME, null,
+ clientState, false));
+ request = new HttpRequest(new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ response = fetcher.fetch(request);
+ assertEquals("User data is hello-oauth", response.getResponseAsString());
+
+ assertEquals(1, serviceProvider.getRequestTokenCount());
+ assertEquals(1, serviceProvider.getAccessTokenCount());
+ assertEquals(1, serviceProvider.getResourceAccessCount());
+
+ fetcher = getFetcher(
+ getSecurityToken("owner", "somebody else"),
+ new OAuthRequestParams(FakeGadgetSpecFactory.SERVICE_NAME, null, null,
+ false));
+ request = new HttpRequest(
+ new URI(FakeOAuthServiceProvider.RESOURCE_URL));
+ response = fetcher.fetch(request);
+ assertEquals("User data is hello-oauth", response.getResponseAsString());
+
+ assertEquals(1, serviceProvider.getRequestTokenCount());
+ assertEquals(1, serviceProvider.getAccessTokenCount());
+ assertEquals(1, serviceProvider.getResourceAccessCount());
+ }
}
Re: svn commit: r678354 - in /incubator/shindig/trunk/java/gadgets/src: main/java/org/apache/shindig/gadgets/http/ main/java/org/apache/shindig/gadgets/oauth/ test/java/org/apache/shindig/gadgets/http/ test/java/org/apache/shindig/gadgets/oauth/
Posted by Kevin Brown <et...@google.com>.
On Sun, Jul 20, 2008 at 11:01 PM, <be...@apache.org> wrote:
> +
> + /**
> + * Sets cache-control headers indicating the response is not cacheable.
> + */
> + public void setNoCache() {
> + this.headers.put("Cache-Control", Lists.newArrayList("no-cache"));
> + this.headers.put("Pragma", Lists.newArrayList("no-cache"));
> + }
This also needs to remove Expires headers.
>
>
> /**
> * @return consolidated cache expiration time or -1
> */
> public long getCacheExpiration() {
> - if (httpStatusCode != SC_OK) {
> - return getDate() + negativeCacheTtl;
> - }
> if (isStrictNoCache()) {
> return -1;
> }
> + if (httpStatusCode != SC_OK) {
> + return getDate() + negativeCacheTtl;
> + }
This breaks negative caching, since most servers send no cache directives on
error responses. The original reason for adding negative caching in the
first place is that a single popular gadget host having problems results in
a huge number of bad http requests, wasting resources and making an already
troubled site just get hit even harder, since now it has to endure 100% of
traffic. This is no different for OAuth than it is for any other request
type.
> +
> + /**
> + * HTTP cache.
> + */
This comment seems rather redundant to me.
> + private HttpCache cache;
>
> /**
> *
> @@ -151,13 +158,15 @@
> * @param authToken user's gadget security token
> * @param params OAuth fetch parameters sent from makeRequest
> * @param tokenStore storage for long lived tokens.
> + * @param cache cache to use for HTTP responses.
> */
> public OAuthFetcher(
> GadgetOAuthTokenStore tokenStore,
> BlobCrypter oauthCrypter,
> HttpFetcher nextFetcher,
> SecurityToken authToken,
> - OAuthRequestParams params) {
> + OAuthRequestParams params,
> + HttpCache cache) {
Now that we're up to 7 parameters on this constructor, can we accept that
making everything an HttpFetcher is not the best design choice we've ever
made? :) This is especially bad since only two of these parameters vary per
invocation. This is extremely confusing code to follow, and could be much
better written as a singleton with an inner class for request handling.
>
> private HttpResponse buildErrorResponse(GadgetException e) {
> if (error == null) {
> error = OAuthError.UNKNOWN_PROBLEM;
> @@ -566,6 +594,7 @@
> private HttpResponse buildNonDataResponse() {
> HttpResponse response = new HttpResponse(0, null, null);
> addResponseMetadata(response);
> + response.setNoCache();
Why add a new method for this instead of passing the headers in the ctor?
HttpResponse is already very problematic, as it should be immutable, but we
keep adding mutators to it. It's very hard to implement a cache of objects
when the data in the cache can be modified, and as a result most caches have
to be written more defensively than necessary, with copies being produced on
every read.