Extend concat servlet to support json concatination

The goal is to improve script concatenation:

Input:
<script src="1.js"></script>
<div>Hello, World!</div>
<script src="2.js"></script>
<span>Goodbye, cruel world.</span>
<script src="3.js"></script>

Output: 
<script src="http://www.ggs.com/gadgets/concat?1=1.js&2=2.js&3=3.js&asJSON=1"></script>
<script>eval(_js["1.js"]);</script>
<div>Hello, World!</div>
<script>eval(_js["2.js"]);</script>
<span>Goodbye, cruel world.</span>
<script>eval(_js["3.js"]);</script>

The change here change the concat servlet to support such concatenation.

[johnfargo, 2010-01-04 22:15:25]

http://codereview.appspot.com/183045/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/183045/diff/1/2#newcode47
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:47:
public static final String ASJSON_PARAM = "asJSON";
Thinking on this more, IMO it makes sense to tweak this a little bit. Rather
than asJSON=1, I propose:

&json=<symbol>

The output will then be:
var <symbol> = <JSON>;

...rather than the fixed symbol "_js".

http://codereview.appspot.com/183045/diff/1/2#newcode101
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:101:
if (isAsJson) {
how about putting the asJson logic into the wrapper itself:
ResponseWrapper.setJsonVar("_js"); // "_js" actually from query
...the above, when non-null, does JSON-escaping; otherwise existing behavior.

[zhoresh, 2010-01-04 23:48:43]

Updated according to comments

[zhoresh, 2010-01-04 23:49:59]

On 2010/01/04 22:15:25, johnfargo wrote:
> http://codereview.appspot.com/183045/diff/1/2
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
> 
> http://codereview.appspot.com/183045/diff/1/2#newcode47
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:47:
> public static final String ASJSON_PARAM = "asJSON";
> Thinking on this more, IMO it makes sense to tweak this a little bit. Rather
> than asJSON=1, I propose:
> 
> &json=<symbol>
> 
> The output will then be:
> var <symbol> = <JSON>;
> 
> ...rather than the fixed symbol "_js".
> 
Done
> http://codereview.appspot.com/183045/diff/1/2#newcode101
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:101:
> if (isAsJson) {
> how about putting the asJson logic into the wrapper itself:
> ResponseWrapper.setJsonVar("_js"); // "_js" actually from query
> ...the above, when non-null, does JSON-escaping; otherwise existing behavior.
Moved some of the logic to wrapper, but there is still some logic that is
global.

[johnfargo, 2010-01-05 00:06:30]

Looks pretty good, but why not go the distance and stick the JSON-open and
JSON-end logic in EscapedServletOutputStream as well? You'd need only to pass
through the index (or first-ness) of the entry being placed -- @Override close()
for the ender. Thoughts?

[zhoresh, 2010-01-05 01:24:08]

The code recognize end of concat only when there is no paramater (line 96),
at this point the last wrapper is already gone.
I guess I can put the wrapper in the larger block and verify it is not null
(empty parameter list?).
but at this point is it really worth it?

On Mon, Jan 4, 2010 at 4:06 PM, <johnfargo@gmail.com> wrote:

> Looks pretty good, but why not go the distance and stick the JSON-open
> and JSON-end logic in EscapedServletOutputStream as well? You'd need
> only to pass through the index (or first-ness) of the entry being placed
> -- @Override close() for the ender. Thoughts?
>
>
> http://codereview.appspot.com/183045
>

[johnfargo, 2010-01-05 02:32:40]

I'm with you re: the wrapper logic. I'm suggesting a structure more like what
you're starting to hint at -- where the ResponseWrapper has more existing
base-class logic stuffed into it.

Then the loop would just be:
for (each referenced JS file) {
  wrapper.addJs(file);  // non-static private class has reference to
proxyHandler, does doFetch, creates wrapped JS, handles comments/encoding, etc.
}
// wrapper.close() called by framework, obviating the need for .done()

thoughts?

http://codereview.appspot.com/183045/diff/8/9
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/183045/diff/8/9#newcode85
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:85:
String jsonVar = request.getParameter(JSON_PARAM);
let's sanitize this by ensuring it matches [a-zA-Z0-9_]{0,32} or similar.

http://codereview.appspot.com/183045/diff/8/9#newcode99
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:99:
response.getOutputStream().println("/* ---- Start " + url + " ---- */");
start/end comments should be harmless, but are also redundant in the json case,
as the original URL reference is right in the object.

[zhoresh, 2010-01-06 02:52:06]

Update wrapper to handle all logic

[johnfargo, 2010-01-06 18:36:02]

Looks good, but I'm being a pain :)

http://codereview.appspot.com/183045/diff/1006/13
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/183045/diff/1006/13#newcode85
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:85:
String jsonVar = request.getParameter(JSON_PARAM);
still need to sanitize this :)

http://codereview.appspot.com/183045/diff/1006/13#newcode221
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:221:
public void closeStream() throws IOException {
I don't think you need the closeStream() operation -- seems to me you can do the
"url:\"" and "," additions in setUrl() and done()/close(), the former you could
rename processingUrl() if you find it clearer. Each such addition would be made
to super.getOutputStream().print(...).

getOutputStream() would be simpler in this case as well, since you'd only return
a single EscapedServletOutputStream, which does only and exactly one thing: JSON
escapes data it's given.

personally I'd rename setUrl(...) to processUrl(request, url) and call it
directly. The ResponseWrapper class becomes non-static, so has access to
proxyHandler. At that point, all output logic is in RW.

http://codereview.appspot.com/183045/diff/1006/13#newcode387
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:387:
outputStream.write(StringEscapeUtils.escapeJavaScript(data).getBytes());
I'm a little concerned about how this stream will be called, esp. if JS chunks
are provided to it in individual bytes or weird offset chunks. Seems better to
me to have write(...) APIs buffer data in a growing byte array, then add a
separate method (called by processUrl(...)) that flushes the buffer, escaping
the whole thing.

[zhoresh, 2010-01-06 23:15:24]

Changes according to comments, Also added tests for errors

[johnfargo, 2010-01-06 23:45:10]

Tests look awesome. Just one more niggling request.

http://codereview.appspot.com/183045/diff/18/19
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/183045/diff/18/19#newcode85
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:85:
// Check for asJSON concat
s/asJSON/json/g

http://codereview.appspot.com/183045/diff/18/19#newcode374
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:374:
super();
not needed

http://codereview.appspot.com/183045/diff/18/19#newcode377
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:377:
outputStream.print("\"" + var + "\":\"");
Let's pull the delimiter logic into the wrapper too, so the outputstream _just_
escapes.

http://codereview.appspot.com/183045/diff/18/20
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
(right):

http://codereview.appspot.com/183045/diff/18/20#newcode44
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:44:
"var v2 = { \\\"a-b\\\": 1 , c: \\\"hello!,\\\" };";
nit: you could also use the escaper itself to generate these vars

[zhoresh, 2010-01-07 00:45:08]

Clean up EscapedServletOutputStream

[zhoresh, 2010-01-07 18:42:06]

On Wed, Jan 6, 2010 at 3:45 PM, <johnfargo@gmail.com> wrote:

> Tests look awesome. Just one more niggling request.
>
>
> http://codereview.appspot.com/183045/diff/18/19
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
>
> http://codereview.appspot.com/183045/diff/18/19#newcode85
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:85:
> // Check for asJSON concat
> s/asJSON/json/g
>
Done

>
> http://codereview.appspot.com/183045/diff/18/19#newcode374
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:374:
> super();
> not needed
>
Changed

>
> http://codereview.appspot.com/183045/diff/18/19#newcode377
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:377:
> outputStream.print("\"" + var + "\":\"");
> Let's pull the delimiter logic into the wrapper too, so the outputstream
> _just_ escapes.
>
Good idea, The stream is much cleaner now

>
> http://codereview.appspot.com/183045/diff/18/20
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
> (right):
>
> http://codereview.appspot.com/183045/diff/18/20#newcode44
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:44:
> "var v2 = { \\\"a-b\\\": 1 , c: \\\"hello!,\\\" };";
> nit: you could also use the escaper itself to generate these vars

I know I don't really test the escaper, but it feel to me almost like
testing assertEqual(f(),f())
So I rather specify in the test what I expect instead of using the same
escaper to generate it.


>
>
> http://codereview.appspot.com/183045
>