Re: Concatenated URLs should not exceed GET URL size limit (issue3734041)

[johnfargo]

I wrote a few little nits, but my primary feedback is to ask if you
considered adding the feature in DefaultConcatUriManager rather than the
rewriter.

ConcatUriManager under the hood may (and often does) actually request
data for versioning purposes, so using it to calculate the base URL
length heuristically  can be inefficient. Within Google we have a
per-request fetch cache, so this issue is minimized however.

The alternate approach is simply to turn the Uri field in ConcatData
into List<Uri>.

Then, in DefaultConcatUriManager change this section:
    for (Uri resource : resourceUris) {
      uriBuilder.addQueryParameter(i.toString(), resource.toString());
      i++;
      if (doSplit) {
        snippets.put(resource, getJsSnippet(splitParam, resource));
      }
    }

to something like this:
List<Uri> uris = Lists.newArrayList();
    for (Uri resource : resourceUris) {
      uriBuilder.addQueryParameter(i.toString(), resource.toString());
      if (uriBuilder.toString().length() > injectedMaxUrlLength) {
        uriBuilder.removeQueryParameter(i.toString());
        uris.add(uriBuilder.toUri());
        uriBuilder = ctx.makeQueryParams(null, version);
        i = Integer.valueOf(START_INDEX);
      } else {
        i++;
      }
      if (doSplit) {
        snippets.put(resource, getJsSnippet(splitParam, resource));
      }
    }

This isn't the exact impl, since version would need to be accommodated
(it does in your impl as well), but the idea remains that code
generating the URI controls its length. WDYT?


http://codereview.appspot.com/3734041/diff/100001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/3734041/diff/100001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode266
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:266:
String url =
Utf8UrlCoder.encode(element.getAttribute(type.getSrcAttrib()));
nit: extra space

http://codereview.appspot.com/3734041/diff/100001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode267
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:267:
int extraPadding = Integer.toString(index).length() + 2;
to be conservative, use offset=3 rather than 2 for index>=10

http://codereview.appspot.com/3734041/diff/100001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode280
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:280:
if (baseConcatUrlLength + url.length() + extraPadding >
allowedMaxUrlLength) {
nit: for clarity, put parens around addition

http://codereview.appspot.com/3734041/