JSON-RPC equivalent of makeRequest

This is an implementation of the makeRequest endpoint in JSON-RPC form. 
See documentation in HttpRequestHandler for request format. 

The intent of this patch is not to switch gadgets.io.makeRequest to use this format but rather to make it available within oslite as oslite.http.* A later CL will be created to allow using this for gadgets.io.makeRequest

The following JSON-RPC operations are now available @ /gadgets/rpc :
 http.get, http.post, http.put, http.delete, http.head, cache.invalidate, system.listMethods

Includes a refactoring to allow binding different handlers into ApiServlets based on servlet config

[brian, 2009-03-04 20:01:30]

Can we share more code with MakeRequestHandler?

http://codereview.appspot.com/25058/diff/1/25
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthArguments.java
(right):

http://codereview.appspot.com/25058/diff/1/25#newcode151
Line 151: useToken = parseUseToken(auth, getAuthInfoParam(attrs,
USE_TOKEN_PARAM, ""));
I think it would be a good idea to unify the RequestAuthenticationInfo
constructor with this constructor.

http://codereview.appspot.com/25058/diff/1/25#newcode156
Line 156: origClientState = null;
JSON-RPC will need origClientState in order for 3-legged OAuth to work.

origClientState is a performance improvement (saves a Kansas lookup) for
2-legged OAuth.

http://codereview.appspot.com/25058/diff/1/28
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpRequestHandler.java
(right):

http://codereview.appspot.com/25058/diff/1/28#newcode53
Line 53: *    url : <endpoint to fetch content from>,
I think you could get rid of some boiler plate if this was switched to 

method: http.fetch
params  : {
   method : <real-http-method>
   ...

http://codereview.appspot.com/25058/diff/1/28#newcode90
Line 90: static final Set<String> BAD_HEADERS = ImmutableSet.of("HOST",
"ACCEPT", "ACCEPT-ENCODING");
Can we centralize this logic somewhere so that it can be shared by
MakeRequestHandler and HttpRequestHandler?

http://codereview.appspot.com/25058/diff/1/28#newcode126
Line 126: @Operation(httpMethods = "DELETE", path = "/delete")
httpMethods should be set to "POST" for all of these, I would think?

(This is the first time I've looked at the Operation class, but it looks like
httpMethods is supposed to match the actual HTTP method received by the
servlet.)

http://codereview.appspot.com/25058/diff/1/28#newcode137
Line 137: assertNoBody(httpReq, "HEAD");
I think the assertNoBody stuff in this class is overkill, it can be caught by
downstream fetchers.

http://codereview.appspot.com/25058/diff/1/28#newcode158
Line 158: if (!"http".equals(httpApiRequest.url.getScheme()) && !"https"
Can this logic somehow be shared with ProxyBase.java?

There's also been some talk about allowing custom URL schemes, the fewer places
we need to change to do that, the better.

http://codereview.appspot.com/25058/diff/1/28#newcode216
Line 216: results = contentRewriterRegistry.rewriteHttpResponse(req, results);
Hmm.  Again, might be cool to share with MakeRequestHandler.

http://codereview.appspot.com/25058/diff/1/28#newcode465
Line 465: // TODO - Revise this for compatability
I don't understand this comment.

[louiscryan, 2009-03-09 22:25:15]

Thanks for the review Brian. Updated patch included

http://codereview.appspot.com/25058/diff/1/25
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthArguments.java
(right):

http://codereview.appspot.com/25058/diff/1/25#newcode151
Line 151: useToken = parseUseToken(auth, getAuthInfoParam(attrs,
USE_TOKEN_PARAM, ""));
On 2009/03/04 20:01:30, brian wrote:
> I think it would be a good idea to unify the RequestAuthenticationInfo
> constructor with this constructor.

Done.

http://codereview.appspot.com/25058/diff/1/25#newcode156
Line 156: origClientState = null;
On 2009/03/04 20:01:30, brian wrote:
> JSON-RPC will need origClientState in order for 3-legged OAuth to work.
> 
> origClientState is a performance improvement (saves a Kansas lookup) for
> 2-legged OAuth.

Done. Disallowed cache bypass and this for RequestAuthenticationInfo

http://codereview.appspot.com/25058/diff/1/28
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpRequestHandler.java
(right):

http://codereview.appspot.com/25058/diff/1/28#newcode53
Line 53: *    url : <endpoint to fetch content from>,
On 2009/03/04 20:01:30, brian wrote:
> I think you could get rid of some boiler plate if this was switched to 
> 
> method: http.fetch
> params  : {
>    method : <real-http-method>
>    ...
> 
> 

Really depends on what we want the calling model to be. In osapi this will
translate into

osapi.http.get(...) 
osapi.http.post(...)

which seems pretty nice. Ill leave it as is for the moment and we can resolve on
the spec list once this comes up

http://codereview.appspot.com/25058/diff/1/28#newcode90
Line 90: static final Set<String> BAD_HEADERS = ImmutableSet.of("HOST",
"ACCEPT", "ACCEPT-ENCODING");
On 2009/03/04 20:01:30, brian wrote:
> Can we centralize this logic somewhere so that it can be shared by
> MakeRequestHandler and HttpRequestHandler?

The intention is to retire MakeRequestHandler so Ill update it to refer to this.

http://codereview.appspot.com/25058/diff/1/28#newcode126
Line 126: @Operation(httpMethods = "DELETE", path = "/delete")
On 2009/03/04 20:01:30, brian wrote:
> httpMethods should be set to "POST" for all of these, I would think?
> 
> (This is the first time I've looked at the Operation class, but it looks like
> httpMethods is supposed to match the actual HTTP method received by the
> servlet.)

Yup, typo. I allow GET or POST as the incoming method for outbound GET or HEAD,
everything else is POST

http://codereview.appspot.com/25058/diff/1/28#newcode137
Line 137: assertNoBody(httpReq, "HEAD");
On 2009/03/04 20:01:30, brian wrote:
> I think the assertNoBody stuff in this class is overkill, it can be caught by
> downstream fetchers.

Some fetcher implementations may just silently ignore the body for certain
requests which seems overly tolerant

http://codereview.appspot.com/25058/diff/1/28#newcode158
Line 158: if (!"http".equals(httpApiRequest.url.getScheme()) && !"https"
On 2009/03/04 20:01:30, brian wrote:
> Can this logic somehow be shared with ProxyBase.java?
> 
> There's also been some talk about allowing custom URL schemes, the fewer
places
> we need to change to do that, the better.

Yes, its centralized in DefaultRequestPipeline now. Allows for container
specific protocol checks now too.

http://codereview.appspot.com/25058/diff/1/28#newcode216
Line 216: results = contentRewriterRegistry.rewriteHttpResponse(req, results);
On 2009/03/04 20:01:30, brian wrote:
> Hmm.  Again, might be cool to share with MakeRequestHandler.

Don't think we'd get much mileage here. Skipping.

http://codereview.appspot.com/25058/diff/1/28#newcode465
Line 465: // TODO - Revise this for compatability
On 2009/03/04 20:01:30, brian wrote:
> I don't understand this comment.

MakeRequest handler suppresses the 'metadata' field on output if its empty,
which is a little unnatural for the protocol. Having it available but empty
might make client-side coding easier if it was always provided but seeing as I
have'nt done that yet Im not sure of the value. Added comment to that effect

[louiscryan, 2009-03-10 00:42:11]

Updated patch

[louiscryan, 2009-03-10 04:16:26]

test

[beaton, 2009-03-10 21:58:31]

http://codereview.appspot.com/25058/diff/1012/1044
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/25058/diff/1012/1044#newcode106
Line 106: if (request.getUri().getScheme()== null) {
Does this ever happen?  Seems like a bug if it does.

http://codereview.appspot.com/25058/diff/1012/1045
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/RequestPipeline.java
(right):

http://codereview.appspot.com/25058/diff/1012/1045#newcode43
Line 43: void normalizeProtocol(HttpRequest request)  throws GadgetException;
extra space here

http://codereview.appspot.com/25058/diff/1012/1046
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthArguments.java
(right):

http://codereview.appspot.com/25058/diff/1012/1046#newcode133
Line 133: origClientState = null;  // Dont allow for declarative calls??
comment should be: client has no state for declarative calls

http://codereview.appspot.com/25058/diff/1012/1046#newcode134
Line 134: bypassSpecCache = false; // Ditto
// too much trouble to copy nocache=1 from the request context to here.

http://codereview.appspot.com/25058/diff/1012/1050
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpRequestHandler.java
(right):

http://codereview.appspot.com/25058/diff/1012/1050#newcode165
Line 165: }
Your reply made me think that you were moving this to RequestPipeline, but now
it is in both places.  Intentional?  Wrong patch upload?

[louiscryan, 2009-03-11 02:25:42]

New patch forthcoming...

http://codereview.appspot.com/25058/diff/1012/1044
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/25058/diff/1012/1044#newcode106
Line 106: if (request.getUri().getScheme()== null) {
On 2009/03/10 21:58:31, beaton wrote:
> Does this ever happen?  Seems like a bug if it does.

All the time in fact. Client JS doesn't require it. Current behavior just
rejects it so Ill mimick that rather than trying to do a fixup.

http://codereview.appspot.com/25058/diff/1012/1045
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/RequestPipeline.java
(right):

http://codereview.appspot.com/25058/diff/1012/1045#newcode43
Line 43: void normalizeProtocol(HttpRequest request)  throws GadgetException;
On 2009/03/10 21:58:31, beaton wrote:
> extra space here

Done.

http://codereview.appspot.com/25058/diff/1012/1046
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthArguments.java
(right):

http://codereview.appspot.com/25058/diff/1012/1046#newcode133
Line 133: origClientState = null;  // Dont allow for declarative calls??
On 2009/03/10 21:58:31, beaton wrote:
> comment should be: client has no state for declarative calls

Done.

http://codereview.appspot.com/25058/diff/1012/1046#newcode134
Line 134: bypassSpecCache = false; // Ditto
On 2009/03/10 21:58:31, beaton wrote:
> // too much trouble to copy nocache=1 from the request context to here.

Done.

http://codereview.appspot.com/25058/diff/1012/1050
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpRequestHandler.java
(right):

http://codereview.appspot.com/25058/diff/1012/1050#newcode165
Line 165: }
On 2009/03/10 21:58:31, beaton wrote:
> Your reply made me think that you were moving this to RequestPipeline, but now
> it is in both places.  Intentional?  Wrong patch upload?

Brain fart.

[louiscryan, 2009-03-11 08:09:30]

Updated with latest

[louiscryan, 2009-03-11 22:53:01]

On 2009/03/11 08:09:30, louiscryan wrote:
> Updated with latest

Forwarding test

[louiscryan, 2009-03-11 22:53:56]

On 2009/03/11 22:53:01, louiscryan wrote:
> On 2009/03/11 08:09:30, louiscryan wrote:
> > Updated with latest
> 
> Forwarding test

Forwarding test 2