Refactor proxy handler, part #1: remove ProxyBase

First phase in getting rid of the confusing ProxyBase class. All its methods are stateless helpers, and its constants are replaced with UriCommon.Param enums.

1. Removes ProxyBase and for the most part puts its helper methods into a new util wrapper, ServletUtil.

2. Replaces helper method use with ServletUtil calls, in MakeRequest and Proxy handlers.

3. Modifies ProxyHandler to deal with HttpRequest/HttpResponse rather than Http[Servlet]Request/Response. This is a big motivator for this change, moving closer to making ProxyHandler accessible easily via JSON-RPC as well as direct HTTP request.

4. In support of #3, adds ServletUtil methods converting between HttpServletRequest and HttpRequest, and HttpResponse to HttpServletResponse.

5. Adds, but does not use, ServletUtil method converting an HttpResponse into a JSON-style HttpResponse with dataUri field and metadata.

[gagan.goku, 2010-07-24 04:11:22]

Nice change :)
Small nits:

http://codereview.appspot.com/1696056/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(left):

http://codereview.appspot.com/1696056/diff/1/3#oldcode140
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:140:
for (Map.Entry<String, String> entry : results.getHeaders().entries()) {
you should use UriUtils here.
@See http://codereview.appspot.com/1855044/show and
http://codereview.appspot.com/1855041/show.

http://codereview.appspot.com/1696056/diff/1/3#oldcode174
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:174:
IOUtils.copy(results.getResponse(), response.getOutputStream());
@See http://codereview.appspot.com/1855044/diff/45001/46005

http://codereview.appspot.com/1696056/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1696056/diff/1/3#newcode57
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:57:
public static final String OUTPUT_JS_PARAM_VALUE = "js";
maybe this should be &output=json ?

http://codereview.appspot.com/1696056/diff/1/3#newcode195
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:195:
private void emitOutputJs(String contentType, HttpResponse results,
HttpServletResponse response)
would something like emitResponseAsJSON look better ?
basically to convey that we are emitting a json object which has the actual
resource and metadata of original page.

http://codereview.appspot.com/1696056/diff/1/2
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
(right):

http://codereview.appspot.com/1696056/diff/1/2#newcode409
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:409:
assertEquals("data:" + expectedType + ";base64;charset=" + expectedEncoding +
"," + content64,
would be nice to show the response in string form for tests. Returning "data:" +
expectedType + ... is kind of exactly what the original function does, we are
not really testing it.

Maybe you can return js.getString(ProxyHandler.DATA_URI_KEY)
here and in test testOutputDataUriWithCharset, you can say
assertEquals("data:expectedType;base64;....", checkOutputDataUri("text/bar;
charset=ISO-8859-1", "text/bar", "ISO-8859-1");

Makes sense ?

[zhoresh, 2010-07-26 20:34:50]

Code looks nice but I would argue that this data should be served by the new
JSON interface - GadgetHandler and not ProxyServlet.
I think it will be clearer to the user to access one end point for JSON
interface, and another for data/html.
I plan on extending the GadgetHandler to provide Proxy url, so I think it
would be simpler (from API point of view) to extend it to what you are
doing.

-Ziv


On Fri, Jul 23, 2010 at 5:59 PM, <johnfargo@gmail.com> wrote:

> Reviewers: dev-remailer_shindig.apache.org,
>
> Description:
> Adds the ability to request, via &output=js, proxied content in dataUri
> form.
>
> In the presence of this parameter, a JSON object is emitted containing a
> 'dataUri' field w/ the URI contents, as well as any metadata that is
> associated w/ the HttpResponse (allowing augmentation in various cases
> such as image proxying, where height/width/etc. may be set).
>
> This isn't particularly configurable as yet (ie. max size of response),
> but seems to work just fine. A test page (added in this CL) demonstrates
> this. We may also consider adding JSONP and CORS support.
>
> Please review this at http://codereview.appspot.com/1696056/show
>
> Affected files:
>  content/container/datauri_proxy.html
>
> 
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
>
> 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
>
>
>

[johnfargo, 2010-07-26 20:43:36]

That seems reasonable to me: one place for all "new" metadata-driven APIs.

This is consistent w/ refactoring I'd like to do btw ProxyHandler and
ProxyServlet (making Handler an abstract logic handler rather than consuming
and processing HttpServletX objects), so I'll do that in a patch I'll add to
this review.

A brief implementation note, sidebar: once the APIs support streaming HTTP
fetch, the metadata/data URI proxy handler will be forced into buffered-read
mode. Distinctly not a really big concern (data URI isn't appropriate for
large objects anyway); just an awareness item.

Stay tuned for an updated CL. I'll respond re: Gagan's comments as well.

--j

On Mon, Jul 26, 2010 at 1:34 PM, Ziv Horesh <zhoresh@gmail.com> wrote:

> Code looks nice but I would argue that this data should be served by the
> new JSON interface - GadgetHandler and not ProxyServlet.
> I think it will be clearer to the user to access one end point for JSON
> interface, and another for data/html.
> I plan on extending the GadgetHandler to provide Proxy url, so I think it
> would be simpler (from API point of view) to extend it to what you are
> doing.
>
> -Ziv
>
>
> On Fri, Jul 23, 2010 at 5:59 PM, <johnfargo@gmail.com> wrote:
>
>> Reviewers: dev-remailer_shindig.apache.org,
>>
>> Description:
>> Adds the ability to request, via &output=js, proxied content in dataUri
>> form.
>>
>> In the presence of this parameter, a JSON object is emitted containing a
>> 'dataUri' field w/ the URI contents, as well as any metadata that is
>> associated w/ the HttpResponse (allowing augmentation in various cases
>> such as image proxying, where height/width/etc. may be set).
>>
>> This isn't particularly configurable as yet (ie. max size of response),
>> but seems to work just fine. A test page (added in this CL) demonstrates
>> this. We may also consider adding JSONP and CORS support.
>>
>> Please review this at http://codereview.appspot.com/1696056/show
>>
>> Affected files:
>>  content/container/datauri_proxy.html
>>
>> 
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
>>
>> 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
>>
>>
>>
>

[johnfargo, 2010-08-02 19:09:09]

Refactoring, per discussion w/ Ziv. This is a much more significant change, and
bears a closer look.

[gagan.goku, 2010-08-02 20:09:58]

On 2010/08/02 19:09:09, johnfargo wrote:
> Refactoring, per discussion w/ Ziv. This is a much more significant change,
and
> bears a closer look.

In the spirit of cleaning things up, have you taken a look at
http://codereview.appspot.com/1770043/show

At first glance, it seems you want most of the core functionality to deal with
HttpRequest and HttpResponse and have the conversion to/from HttpServletRquest
HttpServletResponse at the very early and last stages of processing respectively
(atleast in some files you do). I agree with this idea and its a nice
encapsulation that would enable us to do much more cool stuff.

It would be nice if you could take a look at the above mentioned patch so we
don't end up duplicating the effort.

Also, it would be nice if we could split this patch into smaller ones, like one
that adds all ProxyBase constants to UriCommon.Param. This will reduce the file
count in this patch.
Also, please add the json functionality in a separate cl as this cl is already
big enough.

[johnfargo, 2010-08-02 21:06:29]

Moved UriCommon.Param-only files to:
http://codereview.appspot.com/1883046/show

On 2010/08/02 20:09:58, gagan.goku wrote:
> On 2010/08/02 19:09:09, johnfargo wrote:
> > Refactoring, per discussion w/ Ziv. This is a much more significant change,
> and
> > bears a closer look.
> 
> In the spirit of cleaning things up, have you taken a look at
> http://codereview.appspot.com/1770043/show
> 
> At first glance, it seems you want most of the core functionality to deal with
> HttpRequest and HttpResponse and have the conversion to/from HttpServletRquest
> HttpServletResponse at the very early and last stages of processing
respectively
> (atleast in some files you do). I agree with this idea and its a nice
> encapsulation that would enable us to do much more cool stuff.
> 
> It would be nice if you could take a look at the above mentioned patch so we
> don't end up duplicating the effort.
> 
> Also, it would be nice if we could split this patch into smaller ones, like
one
> that adds all ProxyBase constants to UriCommon.Param. This will reduce the
file
> count in this patch.
> Also, please add the json functionality in a separate cl as this cl is already
> big enough.

[johnfargo, 2010-08-02 21:15:43]

Committed small UriCommon CL; many fewer files now in this patch.

[zhoresh, 2010-08-02 22:21:55]

Here couple preliminary comments, I will continue

http://codereview.appspot.com/1696056/diff/36001/8012
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1696056/diff/36001/8012#newcode112
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:112:
ByteArrayOutputStream baos = new ByteArrayOutputStream();
Don't think you really need a stream. Just pass the bytes from results to
response.

http://codereview.appspot.com/1696056/diff/36001/8014
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1696056/diff/36001/8014#newcode68
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:68:
}
Cannot see below this line, do you need to update your client to head?

http://codereview.appspot.com/1696056/diff/36001/8015
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/1696056/diff/36001/8015#newcode48
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:48:
public static HttpRequest fromHttpServletRequest(HttpServletRequest servletReq)
throws IOException {
Maybe make this a service class instead of static method. 
That will improve testability and allow override if needed.

[zhoresh, 2010-08-03 00:39:01]

http://codereview.appspot.com/1696056/diff/36001/8015
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/1696056/diff/36001/8015#newcode166
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:166:
ByteArrayOutputStream os = new ByteArrayOutputStream();
Maybe it is an overkill, but maybe you can use JSONObject instead of direct
write?

[johnfargo, 2010-08-03 01:09:27]

On Mon, Aug 2, 2010 at 3:21 PM, <zhoresh@gmail.com> wrote:

> Here couple preliminary comments, I will continue
>
>
> http://codereview.appspot.com/1696056/diff/36001/8012
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
> (right):
>
> http://codereview.appspot.com/1696056/diff/36001/8012#newcode112
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:112:
> ByteArrayOutputStream baos = new ByteArrayOutputStream();
> Don't think you really need a stream. Just pass the bytes from results
> to response.
>

Now I recall why I did this -- HttpResponse only has the following, for good
reason (it's Immutable):
InputStream getResponse();
String getResponseAsString();

I don't want to force conversion to String, so am using getResponse().

This will go away once HttpResponseBuilder supports:
setResponse(InputStream response);

...as in CL: http://codereview.appspot.com/1626044/show


>
> http://codereview.appspot.com/1696056/diff/36001/8014
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
> (right):
>
> http://codereview.appspot.com/1696056/diff/36001/8014#newcode68
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:68:
> }
> Cannot see below this line, do you need to update your client to head?
>

Evidently; stand by.


>
> http://codereview.appspot.com/1696056/diff/36001/8015
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
> (right):
>
> http://codereview.appspot.com/1696056/diff/36001/8015#newcode48
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:48:
> public static HttpRequest fromHttpServletRequest(HttpServletRequest
> servletReq) throws IOException {
> Maybe make this a service class instead of static method.
> That will improve testability and allow override if needed.


Not a bad idea... will look @ it after the sync.


>
>
> http://codereview.appspot.com/1696056/show
>

[johnfargo, 2010-08-03 01:10:13]

I was going to use JSONObject, but preferred this mechanism since it allows
for streaming writes, alleviating memory demands. In any case, I'm moving
this particular piece out of the CL since it's not used (yet).

On Mon, Aug 2, 2010 at 5:39 PM, <zhoresh@gmail.com> wrote:

>
> http://codereview.appspot.com/1696056/diff/36001/8015
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
> (right):
>
> http://codereview.appspot.com/1696056/diff/36001/8015#newcode166
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:166:
> ByteArrayOutputStream os = new ByteArrayOutputStream();
> Maybe it is an overkill, but maybe you can use JSONObject instead of
> direct write?
>
>
> http://codereview.appspot.com/1696056/show
>

[johnfargo, 2010-08-03 01:13:12]

On Mon, Aug 2, 2010 at 1:09 PM, <gagan.goku@gmail.com> wrote:

> On 2010/08/02 19:09:09, johnfargo wrote:
>
>> Refactoring, per discussion w/ Ziv. This is a much more significant
>>
> change, and
>
>> bears a closer look.
>>
>
> In the spirit of cleaning things up, have you taken a look at
> http://codereview.appspot.com/1770043/show
>
> At first glance, it seems you want most of the core functionality to
> deal with HttpRequest and HttpResponse and have the conversion to/from
> HttpServletRquest HttpServletResponse at the very early and last stages
> of processing respectively (atleast in some files you do). I agree with
> this idea and its a nice encapsulation that would enable us to do much
> more cool stuff.
>

The originating idea is to break the Handler dependency on Servlet APIs.
HttpRequest in, HttpResponse out achieves this goal, though still isn't
ideal for metadata use. Even so, I stopped this far to keep the CL's focus
fairly intact.


>
> It would be nice if you could take a look at the above mentioned patch
> so we don't end up duplicating the effort.
>

I'll take a look. This is the first I've heard of it.


>
> Also, it would be nice if we could split this patch into smaller ones,
> like one that adds all ProxyBase constants to UriCommon.Param. This will
> reduce the file count in this patch.
> Also, please add the json functionality in a separate cl as this cl is
> already big enough.


One done, the other split out soonish.


>
>
>
> http://codereview.appspot.com/1696056/show
>

[johnfargo, 2010-08-03 01:26:21]

Sync w/ @head, removed datauri_proxy.html page. Haven't removed JSON stuff yet
(pre-submit)

[johnfargo, 2010-08-03 01:29:15]

Oops, removing a few more UriCommon.Param-only changes.

[zhoresh, 2010-08-03 23:04:41]

lgtm

http://codereview.appspot.com/1696056/diff/53001/54019
File content/container/datauri_proxy.html (right):

http://codereview.appspot.com/1696056/diff/53001/54019#newcode1
content/container/datauri_proxy.html:1: <html>
I assume this is not part of the change

http://codereview.appspot.com/1696056/diff/53001/54007
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
(right):

http://codereview.appspot.com/1696056/diff/53001/54007#newcode93
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java:93:
Capture<HttpRequest> requestCapture = new Capture<HttpRequest>();
Add check to verify what you capture

[johnfargo, 2010-08-04 00:10:43]

On Tue, Aug 3, 2010 at 4:04 PM, <zhoresh@gmail.com> wrote:

> lgtm
>
>
> http://codereview.appspot.com/1696056/diff/53001/54019
> File content/container/datauri_proxy.html (right):
>
> http://codereview.appspot.com/1696056/diff/53001/54019#newcode1
> content/container/datauri_proxy.html:1: <html>
> I assume this is not part of the change
>

True, will be removed from the change before commit.


>
> http://codereview.appspot.com/1696056/diff/53001/54007
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
> (right):
>
> http://codereview.appspot.com/1696056/diff/53001/54007#newcode93
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java:93:
> Capture<HttpRequest> requestCapture = new Capture<HttpRequest>();
> Add check to verify what you capture


Done. Committing!


>
>
> http://codereview.appspot.com/1696056/show
>