Convert ConcatProxyServlet to use ConcatUriManager

Restructures ConcatProxyServlet so that it uses ConcatUriManager to process a request.

This CL also dissociates ConcatProxyServlet from its dependency on ProxyHandler as well, favoring direct use of RequestPipeline instead. In doing so, it breaks the need to use RequestWrapper along with its somewhat indirect rendering logic. Instead, all output is handled by a ConcatServletOutputStream instance, either JSON or "direct"/verbatim.

One downstream effect in terms of output is that Exception cases and 404s continue to result in 400-responses but also yield invalid JSON. This behavior somewhat existed before but is now consistently held.

As well, small tweaks are made to output, in particular start/end markers only being rendered for successful verbatim output. Error output is now marked directly w/ the file causing the error.

This CL depends on completion and commit of: http://codereview.appspot.com/217091/show

[johnfargo, 2010-02-27 00:28:33]

Note: In addition to depending on the other referenced CL, this CL will be
updated to augment ConcatUriManager with
@ImplementedBy(DefaultConcatUriManager.class), and will include container config
updates.

[johnfargo, 2010-03-02 01:15:40]

API updates.

[Paul Lindner, 2010-03-02 09:36:29]

some simple nits..

otherwise looks good.

http://codereview.appspot.com/224074/diff/5/1004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/224074/diff/5/1004#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:62:

Any reason to use transients?  I don't think this class will ever be
serialized...

http://codereview.appspot.com/224074/diff/5/1004#newcode108
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:108:
if (jsonVar.matches("^\\w*$")) {
consider creating a static pattern or use a better word matching pattern
StringUtils.isAlphaNum(jsonVar)|| jsonVar.contains("_") may be equiv...

http://codereview.appspot.com/224074/diff/5/1004#newcode121
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:121:

null check for cos perhaps...

[chirag, 2010-03-03 06:11:41]

http://codereview.appspot.com/224074/diff/5/1004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/224074/diff/5/1004#newcode223
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:223:
write(data.getBytes("UTF8"));
Shoudn't the charsetName passed to getBytes be "UTF-8" instead.

http://codereview.appspot.com/224074/diff/5/1004#newcode237
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:237:
write(uri.toString().getBytes("UTF8"));
Ditto

http://codereview.appspot.com/224074/diff/5/1004#newcode239
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:239:
write(StringEscapeUtils.escapeJavaScript(data).getBytes("UTF8"));
Ditto

[zhoresh, 2010-03-03 17:02:05]

http://codereview.appspot.com/224074/diff/5/1004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/224074/diff/5/1004#newcode182
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:182:
println(formatHttpError(resp.getHttpStatusCode(), resp.getResponseAsString(),
uri));
ServletOutputStream.println is limited to ascii while getResponseAsString return
utf-8. So this line has the potential to cause an exception.
Try to use write instead.

http://codereview.appspot.com/224074/diff/5/1004#newcode184
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:184:
outputJs(uri, resp.getResponseAsString());
Why convert to string here, the original code in proxy handler used IOUtil to
copy bytes for the regular case (faster)

http://codereview.appspot.com/224074/diff/5/1004#newcode190
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:190:
print(formatError(e, uri));
Same, use write instead of print
Also maybe use formatHttpError (basically put error in html comment block)

[zhoresh, 2010-03-03 17:50:47]

http://codereview.appspot.com/224074/diff/5/1004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/224074/diff/5/1004#newcode184
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:184:
outputJs(uri, resp.getResponseAsString());
On 2010/03/03 17:02:05, zhoresh wrote:
> Why convert to string here, the original code in proxy handler used IOUtil to
> copy bytes for the regular case (faster)

Second thought I think I understand why - if the different resources has
different encoding, the conversion to string and back will make them all utf-8. 
Maybe just put a comment here that explain this.

[johnfargo, 2010-03-03 19:13:59]

On Tue, Mar 2, 2010 at 1:36 AM, <lindner@inuus.com> wrote:

> some simple nits..
>
> otherwise looks good.
>
>
>
> http://codereview.appspot.com/224074/diff/5/1004
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
>
> http://codereview.appspot.com/224074/diff/5/1004#newcode62
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:62:
>
> Any reason to use transients?  I don't think this class will ever be
> serialized...
>

Not that I'm aware of... I thought it odd members were marked transient
before. Removed.


>
> http://codereview.appspot.com/224074/diff/5/1004#newcode108
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:108:
> if (jsonVar.matches("^\\w*$")) {
> consider creating a static pattern or use a better word matching pattern
> StringUtils.isAlphaNum(jsonVar)|| jsonVar.contains("_") may be equiv...
>

Good idea; changed to static Pattern for the moment.


>
> http://codereview.appspot.com/224074/diff/5/1004#newcode121
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:121:
>
> null check for cos perhaps...


This does look weird, but AFAICT there's no possible way for cos to be null
since all conditional branches above it either set its value or return.


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

[johnfargo, 2010-03-03 19:18:12]

This one's weird. According to
http://java.sun.com/javase/6/docs/technotes/guides/intl/encoding.doc.htmland
other docs, UTF8 is the canonical string reference for java.lang
("UTF-8" for java.nio). Painful.

On Tue, Mar 2, 2010 at 10:11 PM, <chiragshah1@gmail.com> wrote:

>
> http://codereview.appspot.com/224074/diff/5/1004
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
>
> http://codereview.appspot.com/224074/diff/5/1004#newcode223
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:223:
> write(data.getBytes("UTF8"));
> Shoudn't the charsetName passed to getBytes be "UTF-8" instead.
>
> http://codereview.appspot.com/224074/diff/5/1004#newcode237
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:237:
> write(uri.toString().getBytes("UTF8"));
> Ditto
>
> http://codereview.appspot.com/224074/diff/5/1004#newcode239
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:239:
> write(StringEscapeUtils.escapeJavaScript(data).getBytes("UTF8"));
> Ditto
>
>
> http://codereview.appspot.com/224074/show
>

[johnfargo, 2010-03-03 19:22:32]

@write vs. println, where is it noted that println doesn't support anything
but ASCII?

@UTF8, that's the idea here. I've added a comment.

On Wed, Mar 3, 2010 at 9:50 AM, <zhoresh@gmail.com> wrote:

>
> http://codereview.appspot.com/224074/diff/5/1004
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
>
> http://codereview.appspot.com/224074/diff/5/1004#newcode184
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:184:
> outputJs(uri, resp.getResponseAsString());
> On 2010/03/03 17:02:05, zhoresh wrote:
>
>> Why convert to string here, the original code in proxy handler used
>>
> IOUtil to
>
>> copy bytes for the regular case (faster)
>>
>
> Second thought I think I understand why - if the different resources has
> different encoding, the conversion to string and back will make them all
> utf-8.
> Maybe just put a comment here that explain this.
>
>
> http://codereview.appspot.com/224074/show
>

[johnfargo, 2010-03-03 19:42:00]

NM, found the reference, good catch! Fixed and submitted.

On Wed, Mar 3, 2010 at 11:22 AM, John Hjelmstad <johnfargo@gmail.com> wrote:

> @write vs. println, where is it noted that println doesn't support anything
> but ASCII?
>
> @UTF8, that's the idea here. I've added a comment.
>
>
> On Wed, Mar 3, 2010 at 9:50 AM, <zhoresh@gmail.com> wrote:
>
>>
>> http://codereview.appspot.com/224074/diff/5/1004
>> File
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
>> (right):
>>
>> http://codereview.appspot.com/224074/diff/5/1004#newcode184
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:184:
>> outputJs(uri, resp.getResponseAsString());
>> On 2010/03/03 17:02:05, zhoresh wrote:
>>
>>> Why convert to string here, the original code in proxy handler used
>>>
>> IOUtil to
>>
>>> copy bytes for the regular case (faster)
>>>
>>
>> Second thought I think I understand why - if the different resources has
>> different encoding, the conversion to string and back will make them all
>> utf-8.
>> Maybe just put a comment here that explain this.
>>
>>
>> http://codereview.appspot.com/224074/show
>>
>
>