Refactor HtmlAccelServlet to be similar to ProxyHandler

Refactor HtmlAccelServlet to more of a "true" proxy model (ie. as
HtmlAccelHandler, similar to ProxyHandler).

https://issues.apache.org/jira/browse/SHINDIG-1351

[gagan.goku, 2010-06-08 05:44:26]

Please have an overall basic look so i can add shindig remailer to the cc list.

[johnfargo, 2010-06-08 12:21:42]

http://codereview.appspot.com/1598041/diff/6001/7010
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7010#newcode170
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:170:
// Rewrite import declerations to be passed through concat servlet
this file can be removed from this patch set

http://codereview.appspot.com/1598041/diff/6001/7009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7009#newcode147
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:147:
contentBytes = CharsetUtil.getUtf8Bytes(HtmlSerialization.serialize(document));
please upload this tiny patch as a 1-file CL separately -- I'll commit it
instantly

http://codereview.appspot.com/1598041/diff/6001/7004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7004#newcode1
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:1:
package org.apache.shindig.gadgets.servlet;
class is missing the copy-and-paste Apache license

http://codereview.appspot.com/1598041/diff/6001/7004#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:63:
// TODO: What is this for ?
This can now be removed.

It served two purposes:
1. Sets cache control headers based on the versioning status of the Uri. This is
not relevant in the Accel case since URLs are not versioned.
2. Makes it possible to force the cache control expiry of the accel'd content. I
don't think we really need this feature.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode73
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:73:
HttpRequest req = buildHttpRequest(request, proxyUri, proxyUri.getResource());
since you use proxyUri, may as well omit the getResource call -- you can make it
in buildHttpRequest.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:76:
"No url parameter in request", HttpResponse.SC_BAD_REQUEST);
let's just throw this GadgetException in buildHttpRequest.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode100
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:100:
rewriteContentType(req, response);
this code is fine, but you're welcome to omit it if you like. this behavior is
triggered by the rewriteMime=... parameter, which I doubt accel will really ever
use.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode108
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:108:
* @param uriCtx TODO: Dont know yet ? :(
this is the parsed ProxyUri object.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode152
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:152:
}
if you'd like you could achieve the same effect by having this method return
'void' and just throw new GadgetException(code, ERROR_FETCHING_DATA,
httpResponseCode); as needed.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode158
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:158:
if (results.getHttpStatusCode() == HttpServletResponse.SC_NOT_FOUND) {
nit: you could move this above the previous if:
if (SC_NOT_FOUND) {
} else (isError) {
}

http://codereview.appspot.com/1598041/diff/6001/7005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7005#newcode112
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:112:
// Do not set disposition for css.
change can be removed from this particular patch

http://codereview.appspot.com/1598041/diff/6001/7003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7003#newcode58
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:58:
HtmlAccelServlet.ACCEL_GADGET_PARAM_VALUE;
nit: 4-space indent from previous line

http://codereview.appspot.com/1598041/diff/6001/7008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7008#newcode54
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java:54:
private boolean shouldConcatenateCssResource = false;
looks like this logic sneaked into the CL -- we should remove it when sending
this out in full.

http://codereview.appspot.com/1598041/diff/6001/7006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7006#newcode15
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:15: * To
change this template use File | Settings | File Templates.
could use a class description :)

http://codereview.appspot.com/1598041/diff/6001/7006#newcode18
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:18:
protected static final Set<String> DISALLOWED_RESPONSE_HEADERS =
I like the idea of putting this into its own class.

What do you think about breaking up these headers into different classes?
Something like:
1. OUTPUT_TRANSFER_DIRECTIVES = "content-length", "transfer-encoding",
"content-encoding", "server", "accept-ranges" (directives controlled by the
serving infrastructure)
2. CACHING_DIRECTIVES = "vary", "expires", "date", "pragma", "cache-control"
3. CLIENT_STATE_DIRECTIVES = "set-cookie", "www-authenticate"

This would allow API users to pick and choose which headers they want to include
and which they don't.

http://codereview.appspot.com/1598041/diff/6001/7006#newcode26
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:26: *
Returns true if the header name is valid.
maybe include a reference to the HTTP RFC

http://codereview.appspot.com/1598041/diff/6001/7006#newcode46
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:46: *
Returns true if the header value is valid.
same

[gagan.goku, 2010-06-08 13:52:18]

Addressed all comments.

http://codereview.appspot.com/1598041/diff/6001/7010
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7010#newcode170
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:170:
// Rewrite import declerations to be passed through concat servlet
On 2010/06/08 12:21:42, johnfargo wrote:
> this file can be removed from this patch set

Done.

http://codereview.appspot.com/1598041/diff/6001/7010#newcode170
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:170:
// Rewrite import declerations to be passed through concat servlet
On 2010/06/08 12:21:42, johnfargo wrote:
> this file can be removed from this patch set

Done.

http://codereview.appspot.com/1598041/diff/6001/7009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7009#newcode147
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:147:
contentBytes = CharsetUtil.getUtf8Bytes(HtmlSerialization.serialize(document));
On 2010/06/08 12:21:42, johnfargo wrote:
> please upload this tiny patch as a 1-file CL separately -- I'll commit it
> instantly

removed from this patch.

http://codereview.appspot.com/1598041/diff/6001/7009#newcode147
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:147:
contentBytes = CharsetUtil.getUtf8Bytes(HtmlSerialization.serialize(document));
On 2010/06/08 12:21:42, johnfargo wrote:
> please upload this tiny patch as a 1-file CL separately -- I'll commit it
> instantly

Done.

http://codereview.appspot.com/1598041/diff/6001/7004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7004#newcode1
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:1:
package org.apache.shindig.gadgets.servlet;
On 2010/06/08 12:21:42, johnfargo wrote:
> class is missing the copy-and-paste Apache license

Done.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:63:
// TODO: What is this for ?
On 2010/06/08 12:21:42, johnfargo wrote:
> This can now be removed.
> 
> It served two purposes:
> 1. Sets cache control headers based on the versioning status of the Uri. This
is
> not relevant in the Accel case since URLs are not versioned.
> 2. Makes it possible to force the cache control expiry of the accel'd content.
I
> don't think we really need this feature.

removed

http://codereview.appspot.com/1598041/diff/6001/7004#newcode73
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:73:
HttpRequest req = buildHttpRequest(request, proxyUri, proxyUri.getResource());
On 2010/06/08 12:21:42, johnfargo wrote:
> since you use proxyUri, may as well omit the getResource call -- you can make
it
> in buildHttpRequest.

Done.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:76:
"No url parameter in request", HttpResponse.SC_BAD_REQUEST);
On 2010/06/08 12:21:42, johnfargo wrote:
> let's just throw this GadgetException in buildHttpRequest.

Done.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode100
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:100:
rewriteContentType(req, response);
On 2010/06/08 12:21:42, johnfargo wrote:
> this code is fine, but you're welcome to omit it if you like. this behavior is
> triggered by the rewriteMime=... parameter, which I doubt accel will really
ever
> use.

I think it might be useful. We should keep as much debugging options as
possible.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode108
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:108:
* @param uriCtx TODO: Dont know yet ? :(
On 2010/06/08 12:21:42, johnfargo wrote:
> this is the parsed ProxyUri object.

Done.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode152
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:152:
}
On 2010/06/08 12:21:42, johnfargo wrote:
> if you'd like you could achieve the same effect by having this method return
> 'void' and just throw new GadgetException(code, ERROR_FETCHING_DATA,
> httpResponseCode); as needed.

As discussed, im keeping it as is because ProxyBase has logic for handling
GadgetExceptions which we might not want to change now.

http://codereview.appspot.com/1598041/diff/6001/7004#newcode158
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:158:
if (results.getHttpStatusCode() == HttpServletResponse.SC_NOT_FOUND) {
On 2010/06/08 12:21:42, johnfargo wrote:
> nit: you could move this above the previous if:
> if (SC_NOT_FOUND) {
> } else (isError) {
> }

Done.

http://codereview.appspot.com/1598041/diff/6001/7005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7005#newcode112
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:112:
// Do not set disposition for css.
On 2010/06/08 12:21:42, johnfargo wrote:
> change can be removed from this particular patch

Done.

http://codereview.appspot.com/1598041/diff/6001/7005#newcode112
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:112:
// Do not set disposition for css.
On 2010/06/08 12:21:42, johnfargo wrote:
> change can be removed from this particular patch

Done.

http://codereview.appspot.com/1598041/diff/6001/7003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7003#newcode58
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:58:
HtmlAccelServlet.ACCEL_GADGET_PARAM_VALUE;
On 2010/06/08 12:21:42, johnfargo wrote:
> nit: 4-space indent from previous line

Done.

http://codereview.appspot.com/1598041/diff/6001/7008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7008#newcode54
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java:54:
private boolean shouldConcatenateCssResource = false;
On 2010/06/08 12:21:42, johnfargo wrote:
> looks like this logic sneaked into the CL -- we should remove it when sending
> this out in full.

removed from this change.

http://codereview.appspot.com/1598041/diff/6001/7006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
(right):

http://codereview.appspot.com/1598041/diff/6001/7006#newcode15
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:15: * To
change this template use File | Settings | File Templates.
On 2010/06/08 12:21:42, johnfargo wrote:
> could use a class description :)

Done.

http://codereview.appspot.com/1598041/diff/6001/7006#newcode18
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:18:
protected static final Set<String> DISALLOWED_RESPONSE_HEADERS =
On 2010/06/08 12:21:42, johnfargo wrote:
> I like the idea of putting this into its own class.
> 
> What do you think about breaking up these headers into different classes?
> Something like:
> 1. OUTPUT_TRANSFER_DIRECTIVES = "content-length", "transfer-encoding",
> "content-encoding", "server", "accept-ranges" (directives controlled by the
> serving infrastructure)
> 2. CACHING_DIRECTIVES = "vary", "expires", "date", "pragma", "cache-control"
> 3. CLIENT_STATE_DIRECTIVES = "set-cookie", "www-authenticate"
> 
> This would allow API users to pick and choose which headers they want to
include
> and which they don't.

Done.

http://codereview.appspot.com/1598041/diff/6001/7006#newcode26
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:26: *
Returns true if the header name is valid.
On 2010/06/08 12:21:42, johnfargo wrote:
> maybe include a reference to the HTTP RFC

Done.

http://codereview.appspot.com/1598041/diff/6001/7006#newcode46
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:46: *
Returns true if the header value is valid.
On 2010/06/08 12:21:42, johnfargo wrote:
> same

Done.

[gagan.goku, 2010-06-12 12:46:28]

Please review.

[johnfargo, 2010-06-14 21:16:31]

LGTM Gagan, committing shortly.

[johnfargo, 2010-06-14 21:20:09]

Submitted.

On 2010/06/14 21:16:31, johnfargo wrote:
> LGTM Gagan, committing shortly.

[gagan.goku, 2010-06-14 22:08:05]

On 2010/06/14 21:20:09, johnfargo wrote:
> Submitted.
> 
> On 2010/06/14 21:16:31, johnfargo wrote:
> > LGTM Gagan, committing shortly.

Awesome, thanks lots :)