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

[pulkitgoyal2000]

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java
(right):

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode38
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:38:
import org.apache.commons.lang.RandomStringUtils;
On 2011/03/04 08:06:00, anupama.dutta wrote:

Remove unused import.

Done.

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode119
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:119:
List<Uri> batchUris = Lists.newArrayList();
On 2011/03/04 08:06:00, anupama.dutta wrote:

Add a comment here indicating that batchUris holds uris for the

current batch of

uris being concatenated.

Done.

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode120
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:120:
List<Uri> uris = Lists.newArrayList();
On 2011/03/04 08:06:00, anupama.dutta wrote:

Add a comment before this line indicating that uris holds the

concatenated uris

formed from batches which satisfy the GET URL limit constraint.

Done.

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java
(left):

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#oldcode124
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:124:
ImmutableList.of(RESOURCES_ONE, RESOURCES_TWO, RESOURCES_ONE);
On 2011/03/04 08:06:00, anupama.dutta wrote:

As discussed offline, let us retain RESOURCES_ONE and do the necessary

checks.

Done.

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java
(right):

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode55
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:55:

On 2011/03/04 08:06:00, anupama.dutta wrote:

Remove inserted tab?

Done.

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode134
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:134:
assertFalse(jsonParams.contains(json));
On 2011/03/04 08:06:00, anupama.dutta wrote:

Maybe assert that jsonParams begins with json.

Done.

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode314
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:314:
Uri urlA =
Uri.parse(generateUrl(ConcatUriManager.ConcatData.getMaxUrlLength() /
2));
On 2011/03/04 08:06:00, anupama.dutta wrote:

A more generic test would be one where we deal with atleast 3 URLs

(with the

second one being slightly shorter). And 2 of these urls get

concatenated into

one concat-uri and the 3rd one moves to the 2nd concat-uri. Could we

modify both

the tests here for the url limit, to accommodate this?

Done.

http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode332
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:332:
assertNull(concatUris.get(0).getUris().get(0).getQueryParameter("2"));
On 2011/03/04 08:06:00, anupama.dutta wrote:

I think we should do the usual set of asserts that we always do in

other tests

(see block 144 - 158). If convenient, please move these asserts to a

helper

routine to help simplify your code.

Done.

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