Limited invalidation support

This is an initial implementation of the "Limited Invalidation" spec. Containers are expected to implement their own InvalidationService based on their caching behavior

This implementation differs from the spec in a couple of ways that I believe are worth proposing as clarifications.
- The service endpoint is "cache/invalidate" not "invalidate" for REST. For RPC the method is "cache.invalidate"
- Only 2-legged consumer OAuth calls can invalidate URLs
- Security token for rendering gadgets can only be used to invalidate the viewers data
-  Viewer data is invalidated by default and a simple GET /social/rest/cache/invalidate will trigger it

The default request pipeline is altered to allow invalidated content to be returned if fetching a replacement causes an error as is suggested in the spec.

[awiner, 2009-02-27 22:40:51]

Didn't look much at the tests.

http://codereview.appspot.com/23041/diff/1/11
File java/common/src/main/java/org/apache/shindig/auth/AuthenticationMode.java
(right):

http://codereview.appspot.com/23041/diff/1/11#newcode24
Line 24: 
some docs for each?  Esp. OAUTH vs. OAUTH_CONSUMER_REQUEST

http://codereview.appspot.com/23041/diff/1/11#newcode29
Line 29: COOKIE;
should have static method for String -> AuthenticationMode?

http://codereview.appspot.com/23041/diff/1/8
File java/common/src/main/java/org/apache/shindig/auth/BasicSecurityToken.java
(right):

http://codereview.appspot.com/23041/diff/1/8#newcode145
Line 145: return AuthenticationMode.SECURITY_TOKEN_URL_PARAMETER.name();
shouldn't this be passed into the constructor, not assumed?

http://codereview.appspot.com/23041/diff/1/7
File java/common/src/main/java/org/apache/shindig/auth/SecurityToken.java
(right):

http://codereview.appspot.com/23041/diff/1/7#newcode70
Line 70: * @see{AuthenticationMode}
@see AuthenticationMode - no {}

http://codereview.appspot.com/23041/diff/1/23
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java
(right):

http://codereview.appspot.com/23041/diff/1/23#newcode96
Line 96: 
fillInStackTrace() is automatically called by the Throwable constructor - you
don't need to call it manually.

http://codereview.appspot.com/23041/diff/1/14
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultInvalidationService.java
(right):

http://codereview.appspot.com/23041/diff/1/14#newcode40
Line 40: private HttpCache httpCache;
can all be final

http://codereview.appspot.com/23041/diff/1/14#newcode57
Line 57: httpCache.removeResponse(new HttpRequest(uri));
This will only remove the unsigned requests for those URIs.  Don't we need to
remove signed and unsigned?   Or is this a spec clarification - signed requests
can only be invalidated by invalidating the whole user, unsigned requests only
by invalidating the URL?

http://codereview.appspot.com/23041/diff/1/14#newcode62
Line 62: Long mark = marker.incrementAndGet();
this strategy could use some local explanation.  It's non-obvious.

http://codereview.appspot.com/23041/diff/1/14#newcode69
Line 69: if (request.getOAuthArguments() == null) {
should just be request.getAuthType() == AuthType.NONE

http://codereview.appspot.com/23041/diff/1/14#newcode81
Line 81: if (request.getSecurityToken() == null || request.getOAuthArguments()
== null) {
ditto

http://codereview.appspot.com/23041/diff/1/14#newcode101
Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" + userId;
what purpose does the long prefix serve?  It's going over the net a lot, so
would rather see it kept small (if it's needed at all)

http://codereview.appspot.com/23041/diff/1/14#newcode101
Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" + userId;
invalidationhandler code allows both appUrl and appId.  This requires appId; 
should be consistent.

http://codereview.appspot.com/23041/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/23041/diff/1/21#newcode58
Line 58: // Note that we dont remove invalidated entried from the cache as we
want them to be
entried -> entries

http://codereview.appspot.com/23041/diff/1/21#newcode61
Line 61: invalidationService.isValid(request, cachedResponse)) {
would rewrite this a bit:

if (cachedResponse != null) {
  if (cachedResponse.isStale()) {
    cachedResponse = null;
  } else if (invalidationService.isValid(request, cachedResponse)) {
    return cachedResponse;
  }
}

Then remove the check of isStale() below.  This clarifies that stale responses
just never get used.

http://codereview.appspot.com/23041/diff/1/21#newcode80
Line 80: // Use the invalidated cache response if it is not stale. We dont
update its
dont -> don't

http://codereview.appspot.com/23041/diff/1/9
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationHandler.java
(right):

http://codereview.appspot.com/23041/diff/1/9#newcode46
Line 46: private InvalidationService invalidation;
final

http://codereview.appspot.com/23041/diff/1/9#newcode54
Line 54: @Operation(httpMethods = {"POST","GET"}, path = "/invalidate")
why POST and GET?  Spec only mentions POST.

http://codereview.appspot.com/23041/diff/1/9#newcode60
Line 60: "Cannot invalidate content wihtout specifying application");
wihtout -> without

http://codereview.appspot.com/23041/diff/1/9#newcode63
Line 63: // Is the invalidation call from the application backend. If not we
dont allow
dont -> don't

http://codereview.appspot.com/23041/diff/1/9#newcode66
Line 66: AuthenticationMode.OAUTH_CONSUMER_REQUEST.name());
might as well flip the .equals() order to prevent NPEs

http://codereview.appspot.com/23041/diff/1/9#newcode69
Line 69: Set<String> userIds = Sets.newLinkedHashSet();
why linked hash sets?  More expensive, and don't see order as relevant here.

http://codereview.appspot.com/23041/diff/1/9#newcode101
Line 101: if (!resources.isEmpty()) {
check of isEmpty() not really needed

http://codereview.appspot.com/23041/diff/1/6
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationService.java
(right):

http://codereview.appspot.com/23041/diff/1/6#newcode50
Line 50: * @param
missing variable name

[louiscryan, 2009-02-28 00:15:57]

http://codereview.appspot.com/23041/diff/1/11
File java/common/src/main/java/org/apache/shindig/auth/AuthenticationMode.java
(right):

http://codereview.appspot.com/23041/diff/1/11#newcode24
Line 24: 
On 2009/02/27 22:40:51, awiner wrote:
> some docs for each?  Esp. OAUTH vs. OAUTH_CONSUMER_REQUEST

Done.

http://codereview.appspot.com/23041/diff/1/11#newcode29
Line 29: COOKIE;
On 2009/02/27 22:40:51, awiner wrote:
> should have static method for String -> AuthenticationMode?

valueOf works fine here

http://codereview.appspot.com/23041/diff/1/8
File java/common/src/main/java/org/apache/shindig/auth/BasicSecurityToken.java
(right):

http://codereview.appspot.com/23041/diff/1/8#newcode145
Line 145: return AuthenticationMode.SECURITY_TOKEN_URL_PARAMETER.name();
On 2009/02/27 22:40:51, awiner wrote:
> shouldn't this be passed into the constructor, not assumed?

This is correct, this class is only used for gadget render tokens. The name is
somewhat misleading though, we use different token implementations for other
cases.

http://codereview.appspot.com/23041/diff/1/7
File java/common/src/main/java/org/apache/shindig/auth/SecurityToken.java
(right):

http://codereview.appspot.com/23041/diff/1/7#newcode70
Line 70: * @see{AuthenticationMode}
On 2009/02/27 22:40:51, awiner wrote:
> @see AuthenticationMode - no {}
> 

Done.

http://codereview.appspot.com/23041/diff/1/14
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultInvalidationService.java
(right):

http://codereview.appspot.com/23041/diff/1/14#newcode40
Line 40: private HttpCache httpCache;
On 2009/02/27 22:40:51, awiner wrote:
> can all be final

Done.

http://codereview.appspot.com/23041/diff/1/14#newcode57
Line 57: httpCache.removeResponse(new HttpRequest(uri));
On 2009/02/27 22:40:51, awiner wrote:
> This will only remove the unsigned requests for those URIs.  Don't we need to
> remove signed and unsigned?   Or is this a spec clarification - signed
requests
> can only be invalidated by invalidating the whole user, unsigned requests only
> by invalidating the URL?

Yup, it needs to be clearer in the spec. Fetches of gadget specs, message
bundles etc are all unsigned.

http://codereview.appspot.com/23041/diff/1/14#newcode62
Line 62: Long mark = marker.incrementAndGet();
On 2009/02/27 22:40:51, awiner wrote:
> this strategy could use some local explanation.  It's non-obvious.

Added a long discussion to class

http://codereview.appspot.com/23041/diff/1/14#newcode69
Line 69: if (request.getOAuthArguments() == null) {
On 2009/02/27 22:40:51, awiner wrote:
> should just be request.getAuthType() == AuthType.NONE

Done.

http://codereview.appspot.com/23041/diff/1/14#newcode81
Line 81: if (request.getSecurityToken() == null || request.getOAuthArguments()
== null) {
On 2009/02/27 22:40:51, awiner wrote:
> ditto

Done.

http://codereview.appspot.com/23041/diff/1/14#newcode101
Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" + userId;
On 2009/02/27 22:40:51, awiner wrote:
> what purpose does the long prefix serve?  It's going over the net a lot, so
> would rather see it kept small (if it's needed at all)

It doesnt go over the net for this implementation. Other implementations should
implement appropriately. Aim here is to be as readable as possible. This is fine
for in-mem and is fairly consistent with what we do elsewhere (see Http keys)

http://codereview.appspot.com/23041/diff/1/14#newcode101
Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" + userId;
On 2009/02/27 22:40:51, awiner wrote:
> invalidationhandler code allows both appUrl and appId.  This requires appId; 
> should be consistent.

Done.

http://codereview.appspot.com/23041/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/23041/diff/1/21#newcode58
Line 58: // Note that we dont remove invalidated entried from the cache as we
want them to be
On 2009/02/27 22:40:51, awiner wrote:
> entried -> entries

Done.

http://codereview.appspot.com/23041/diff/1/21#newcode61
Line 61: invalidationService.isValid(request, cachedResponse)) {
On 2009/02/27 22:40:51, awiner wrote:
> would rewrite this a bit:
> 
> if (cachedResponse != null) {
>   if (cachedResponse.isStale()) {
>     cachedResponse = null;
>   } else if (invalidationService.isValid(request, cachedResponse)) {
>     return cachedResponse;
>   }
> }
> 
> Then remove the check of isStale() below.  This clarifies that stale responses
> just never get used.

Done.

http://codereview.appspot.com/23041/diff/1/21#newcode80
Line 80: // Use the invalidated cache response if it is not stale. We dont
update its
On 2009/02/27 22:40:51, awiner wrote:
> dont -> don't

Done.

http://codereview.appspot.com/23041/diff/1/9
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationHandler.java
(right):

http://codereview.appspot.com/23041/diff/1/9#newcode46
Line 46: private InvalidationService invalidation;
On 2009/02/27 22:40:51, awiner wrote:
> final

Done.

http://codereview.appspot.com/23041/diff/1/9#newcode54
Line 54: @Operation(httpMethods = {"POST","GET"}, path = "/invalidate")
On 2009/02/27 22:40:51, awiner wrote:
> why POST and GET?  Spec only mentions POST.

Allows for convenience flushing for the viewerid by hitting a URL in the browser
with the appropriate security token. I can remove later if it gets confusing but
seemed useful

http://codereview.appspot.com/23041/diff/1/9#newcode60
Line 60: "Cannot invalidate content wihtout specifying application");
On 2009/02/27 22:40:51, awiner wrote:
> wihtout -> without

Done.

http://codereview.appspot.com/23041/diff/1/9#newcode63
Line 63: // Is the invalidation call from the application backend. If not we
dont allow
On 2009/02/27 22:40:51, awiner wrote:
> dont -> don't

Done.

http://codereview.appspot.com/23041/diff/1/9#newcode66
Line 66: AuthenticationMode.OAUTH_CONSUMER_REQUEST.name());
On 2009/02/27 22:40:51, awiner wrote:
> might as well flip the .equals() order to prevent NPEs

Done.

http://codereview.appspot.com/23041/diff/1/9#newcode69
Line 69: Set<String> userIds = Sets.newLinkedHashSet();
On 2009/02/27 22:40:51, awiner wrote:
> why linked hash sets?  More expensive, and don't see order as relevant here.

For service implementations that have partial failures they may wan to report
last successful flush. On reflection I think thats over-engineered. Switching
back to Set

http://codereview.appspot.com/23041/diff/1/9#newcode101
Line 101: if (!resources.isEmpty()) {
On 2009/02/27 22:40:51, awiner wrote:
> check of isEmpty() not really needed

Agreed. I should get out of the business of protecting service implementors.

http://codereview.appspot.com/23041/diff/1/6
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationService.java
(right):

http://codereview.appspot.com/23041/diff/1/6#newcode50
Line 50: * @param
On 2009/02/27 22:40:51, awiner wrote:
> missing variable name

Done.

[louiscryan, 2009-03-02 21:11:51]

Updated

[panjie.pan, 2009-03-04 05:54:26]

http://codereview.appspot.com/23041/diff/3023/2032
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultInvalidationService.java
(right):

http://codereview.appspot.com/23041/diff/3023/2032#newcode71
Line 71: public void invalidateApplicationResources(Set<Uri> uris, SecurityToken
token) {
(Collection<Url> uris, SecurityToken token) should be enough.

http://codereview.appspot.com/23041/diff/3023/2032#newcode74
Line 74: httpCache.removeResponse(new HttpRequest(uri));
I think we should mark response as invalid response rather than remove it. So
that we can serve stale response when backend is unavailable.

http://codereview.appspot.com/23041/diff/3023/2032#newcode83
Line 83: */
To invalidate a user will invalidate all request/response whose owner/viewer is
the user. Shall we separate owner from viewer?

http://codereview.appspot.com/23041/diff/3023/2040
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/23041/diff/3023/2040#newcode92
Line 92: if (!request.getIgnoreCache()) {
If fetchedResponse.isError(), we will cache an error response.

http://codereview.appspot.com/23041/diff/3023/2027
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationHandler.java
(right):

http://codereview.appspot.com/23041/diff/3023/2027#newcode72
Line 72: // Assume the the viewer content is being invalidated if it is
available
Why we need to invalidate viewer by default?

[louiscryan, 2009-03-04 06:58:42]

Thanks for taking a look Pan. I will post a proposed revision to the spec based
on this work soon.

http://codereview.appspot.com/23041/diff/3023/2032
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultInvalidationService.java
(right):

http://codereview.appspot.com/23041/diff/3023/2032#newcode71
Line 71: public void invalidateApplicationResources(Set<Uri> uris, SecurityToken
token) {
On 2009/03/04 05:54:27, panjie.pan wrote:
> (Collection<Url> uris, SecurityToken token) should be enough.

Logically its a set, no need to be less specific.

http://codereview.appspot.com/23041/diff/3023/2032#newcode74
Line 74: httpCache.removeResponse(new HttpRequest(uri));
On 2009/03/04 05:54:27, panjie.pan wrote:
> I think we should mark response as invalid response rather than remove it. So
> that we can serve stale response when backend is unavailable.

Given that this is for specs and message bundles which have their own post-parse
caches this should be fine. If an app decides to flush itself it had better make
sure the replacement is available.

http://codereview.appspot.com/23041/diff/3023/2032#newcode83
Line 83: */
On 2009/03/04 05:54:27, panjie.pan wrote:
> To invalidate a user will invalidate all request/response whose owner/viewer
is
> the user. Shall we separate owner from viewer?

Thats actually what happens, take a look at how we add the marks to the
HttpResponses prior to caching them. A response fetched signed by viewer only
requires the mark for the viewer to match, a response fetched signed by owner
only requires to owner mark to match, a response fetched signed by both requires
both marks to match.

http://codereview.appspot.com/23041/diff/3023/2040
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/23041/diff/3023/2040#newcode92
Line 92: if (!request.getIgnoreCache()) {
On 2009/03/04 05:54:27, panjie.pan wrote:
> If fetchedResponse.isError(), we will cache an error response.

Yes, largely to avoid generating excessive traffic to a backend which is
repeatedly failing. This has been the behavior for quite a while. error
responses have a much shorter TTL in the cache however.

http://codereview.appspot.com/23041/diff/3023/2027
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationHandler.java
(right):

http://codereview.appspot.com/23041/diff/3023/2027#newcode72
Line 72: // Assume the the viewer content is being invalidated if it is
available
On 2009/03/04 05:54:27, panjie.pan wrote:
> Why we need to invalidate viewer by default?

This is a convenience when making the call from inside a running gadget.