Issue 3734041: Concatenated URLs should not exceed GET URL size limit (Closed)
|
Created:
15 years, 2 months ago by pulkitgoyal2000 Modified:
14 years, 12 months ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionCurrently there is no size limit of the concat url. But, it can lead to error if concat url length exceeds GET url size limit. There should be check on concat url size maximum to GET url size limit.
Patch Set 1 #
Total comments: 8
Patch Set 2 : Updated Patch #
Total comments: 4
Patch Set 3 : Updated Patch {made splitting of url more efficient} #
Total comments: 28
Patch Set 4 : After Satya Comments #
Total comments: 33
Patch Set 5 : Addressing Satya's Comment #
Total comments: 12
Patch Set 6 : Updated Patch #Patch Set 7 : Moving url offset to a function #
Total comments: 10
Patch Set 8 : Realignment #Patch Set 9 : Adding Comments #
Total comments: 4
Patch Set 10 : Addressing Satya's Comment #Patch Set 11 : Removed basichttpfetcher.java #
Total comments: 30
Patch Set 12 : Addressing Anupama's Comments #
Total comments: 4
Patch Set 13 : Alternate approach #
Total comments: 16
Patch Set 14 : Addressing Comments #
Total comments: 2
Patch Set 15 : Addressing Comments #
Total comments: 1
Patch Set 16 : Addressing Comments #Patch Set 17 : Addressing Comments #Patch Set 18 : SplitParam Problem solved #Patch Set 19 : Resolving error mismatch #
Total comments: 10
Patch Set 20 : Addressing Comments #
Total comments: 16
Patch Set 21 : Addressing Anupama's Comments #Patch Set 22 : Remove Unused import Statements #
Total comments: 10
Patch Set 23 : Addressing Comments #Patch Set 24 : Removing Static Injection #Patch Set 25 : Remove empty Limes #MessagesTotal messages: 36
Sign in to reply to this message.
will go through ur cl multiple times because im scared of concat visitor. Small nit for you till then :) http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:109: public static final Integer URL_MAX_LENGTH = 2083; make it a flag so that its value can be changed at runtime, 2083 is actually the limit for IE, while other browsers allow for longer urls.
Sign in to reply to this message.
looks good...a couple of nits http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:226: */ add comments for params http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:273: Integer splitPoint = getOptimalSplitPoint(gadget, elems); keep indentation same as above http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:301: } move a space to the left
Sign in to reply to this message.
Updated Patch
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:171: List<ConcatUriManager.ConcatData> concatUrisWithinLimit = Lists.newLinkedList(); would be more efficient to split the batches based on the url length so you dont need to create the concat uri and then see if its length exceeds the limit. Just generate some dummy concat uri to get the length of the host/gadgets/concat?gadget=blah&container=xxx part, and subtract that length from the urlLimit to figure out the number of bytes that can be used by the urls. And you can split into another batch as soon as the sum of escaped urls till a point becomes larger than the limit. http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:137: revert this file
Sign in to reply to this message.
Updated Patch {made splitting of url more efficient}
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:226: */ On 2010/12/20 06:46:45, nikhilmadan23 wrote: > add comments for params Done. http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:273: Integer splitPoint = getOptimalSplitPoint(gadget, elems); On 2010/12/20 06:46:45, nikhilmadan23 wrote: > keep indentation same as above Done. http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:301: } On 2010/12/20 06:46:45, nikhilmadan23 wrote: > move a space to the left Done. http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:109: public static final Integer URL_MAX_LENGTH = 2083; On 2010/12/18 08:10:24, gagan.goku wrote: > make it a flag so that its value can be changed at runtime, 2083 is actually the > limit for IE, while other browsers allow for longer urls. Done. http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:171: List<ConcatUriManager.ConcatData> concatUrisWithinLimit = Lists.newLinkedList(); On 2010/12/22 03:52:30, gagan.goku wrote: > would be more efficient to split the batches based on the url length so you dont > need to create the concat uri and then see if its length exceeds the limit. > Just generate some dummy concat uri to get the length of the > host/gadgets/concat?gadget=blah&container=xxx part, and subtract that length > from the urlLimit to figure out the number of bytes that can be used by the > urls. > And you can split into another batch as soon as the sum of escaped urls till a > point becomes larger than the limit. Done. http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:137: On 2010/12/22 03:52:30, gagan.goku wrote: > revert this file Done.
Sign in to reply to this message.
Initial comments. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:166: ConcatUriManager.ConcatData uri = makeSingleBatchUri(gadget,Lists.newArrayList((Element)nodes.get(0))); > 100 char http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:167: Integer urlOffset = uri.getUriLength() - ((Element) nodes.get(0)).getAttribute(type.getSrcAttrib()).length(); > 100 char http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:219: /** we follow < 100 char for comments and code. Plz reformat the code accordingly http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:230: for (Element element : elements) { Can you check you logic inside for loop. Doesn't seem to be right, check my other commment http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: if(urlOffset + url.length() + extraPadding > .95f*ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { .95f make it a constant http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: if(urlOffset + url.length() + extraPadding > .95f*ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { extra spaces before and after operators. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: if(urlOffset + url.length() + extraPadding > .95f*ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { I understand the intuition behind this, if i am right you want to skip the very large url in the batch. But then it might lead to reordering of the styles. Ex: <small url> <big url> <small url2> the rewritten with skipping this will lead to concat of <small url> & <small url2> <big url> http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:241: index = 1; Shoudn't this be 0 http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:244: batchWithMaximumLimit = Lists.newLinkedList(); batchWithMaximumLimit.clear() http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:258: * @param concatBatches denotes collection of elems which are grouped together for concat new line before this http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:259: * @return concat uri corresponding to concatBatches @return concat uri lists corresponding to their concatBatches http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:278: * @param gadget Gadget object for the site under consideration New line before this http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:282: private ConcatUriManager.ConcatData makeSingleBatchUri(Gadget gadget, Move this ConcatUriManager.ConcatUri so that it can be used by others if they want. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:288: uriManager.make( Fix the indentation here
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:166: ConcatUriManager.ConcatData uri = makeSingleBatchUri(gadget,Lists.newArrayList((Element)nodes.get(0))); On 2010/12/28 08:59:02, satya3656 wrote: > > 100 char Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:167: Integer urlOffset = uri.getUriLength() - ((Element) nodes.get(0)).getAttribute(type.getSrcAttrib()).length(); On 2010/12/28 08:59:02, satya3656 wrote: > > 100 char Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:219: /** On 2010/12/28 08:59:02, satya3656 wrote: > we follow < 100 char for comments and code. Plz reformat the code accordingly Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:230: for (Element element : elements) { On 2010/12/28 08:59:02, satya3656 wrote: > Can you check you logic inside for loop. Doesn't seem to be right, check my > other commment Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: if(urlOffset + url.length() + extraPadding > .95f*ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { On 2010/12/28 08:59:02, satya3656 wrote: > .95f make it a constant Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: if(urlOffset + url.length() + extraPadding > .95f*ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { On 2010/12/28 08:59:02, satya3656 wrote: > extra spaces before and after operators. Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: if(urlOffset + url.length() + extraPadding > .95f*ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { On 2010/12/28 08:59:02, satya3656 wrote: > extra spaces before and after operators. Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:241: index = 1; On 2010/12/28 08:59:02, satya3656 wrote: > Shoudn't this be 0 No. This represents URL number in Concat Url which starts from 1. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:244: batchWithMaximumLimit = Lists.newLinkedList(); On 2010/12/28 08:59:02, satya3656 wrote: > batchWithMaximumLimit.clear() Clear() does not work here as it clears the data from output list too.. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:258: * @param concatBatches denotes collection of elems which are grouped together for concat On 2010/12/28 08:59:02, satya3656 wrote: > new line before this Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:259: * @return concat uri corresponding to concatBatches On 2010/12/28 08:59:02, satya3656 wrote: > @return concat uri lists corresponding to their concatBatches Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:278: * @param gadget Gadget object for the site under consideration On 2010/12/28 08:59:02, satya3656 wrote: > New line before this Done. http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:282: private ConcatUriManager.ConcatData makeSingleBatchUri(Gadget gadget, On 2010/12/28 08:59:02, satya3656 wrote: > Move this ConcatUriManager.ConcatUri so that it can be used by others if they > want. Not Feasible http://codereview.appspot.com/3734041/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:288: uriManager.make( On 2010/12/28 08:59:02, satya3656 wrote: > Fix the indentation here Done.
Sign in to reply to this message.
Will look at tests later. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:63: private static final float URL_LENGTH_BUFFER_MARGIN = .95f; move this to starting of the class http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:167: ConcatUriManager.ConcatData uri = It is better to move this calculation inside splitBatchesOnUrlLimits http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:176: splitBatchesOnUrlLimits(batchesIter.next(), concatBatchesWithinLimit,urlOffset); space after comma http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:227: * @param elements Good to add some description to the params, to provide more context http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:234: Integer urlLength = urlOffset; Please use int, it is much faster than Integer for simple arithmetic. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:234: Integer urlLength = urlOffset; Name this as concatUrlLength http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: Integer index = 1; Same here http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:238: Integer extraPadding = index.toString().length() + 2; You can just use Integer.toString(number).length(); http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:242: if(urlLength > indent http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:251: URL_LENGTH_BUFFER_MARGIN * ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { indentation http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:299: ConcatUriManager.ConcatUri.fromList(gadget, uriBatches, type), !split); Won't this fit in previous line? http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:111: private static final Integer URL_MAX_LENGTH = 2083; Can you move this URL_MAX_LENGTH to ConcatUriManager class and change it to int. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:127: public Integer getUriLength() { are you using this method? http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:136: public boolean isValidGetUrlLength() { Same here? http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java (right): http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:849: String URL = "http://www.url.com/" + RandomStringUtils.randomAlphanumeric(ConcatUriManager.ConcatData.getMaxUrlLength()) + ".css"; 100 char http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:864: String URLA = "http://www.urlA.com/" + RandomStringUtils.randomAlphanumeric(ConcatUriManager.ConcatData.getMaxUrlLength() / 2) + ".css"; 100 char
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:63: private static final float URL_LENGTH_BUFFER_MARGIN = .95f; On 2010/12/31 07:19:19, satya3656 wrote: > move this to starting of the class Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:167: ConcatUriManager.ConcatData uri = On 2010/12/31 07:19:19, satya3656 wrote: > It is better to move this calculation inside splitBatchesOnUrlLimits Dont want to calculate again & again with every call of splitBatchesOnUrlLimits. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:176: splitBatchesOnUrlLimits(batchesIter.next(), concatBatchesWithinLimit,urlOffset); On 2010/12/31 07:19:19, satya3656 wrote: > space after comma Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:227: * @param elements On 2010/12/31 07:19:19, satya3656 wrote: > Good to add some description to the params, to provide more context Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:234: Integer urlLength = urlOffset; On 2010/12/31 07:19:19, satya3656 wrote: > Please use int, it is much faster than Integer for simple arithmetic. Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:234: Integer urlLength = urlOffset; On 2010/12/31 07:19:19, satya3656 wrote: > Name this as concatUrlLength Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:235: Integer index = 1; On 2010/12/31 07:19:19, satya3656 wrote: > Same here Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:238: Integer extraPadding = index.toString().length() + 2; On 2010/12/31 07:19:19, satya3656 wrote: > You can just use Integer.toString(number).length(); Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:242: if(urlLength > On 2010/12/31 07:19:19, satya3656 wrote: > indent +4 is fine http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:251: URL_LENGTH_BUFFER_MARGIN * ConcatUriManager.ConcatData.getMaxUrlLength().intValue()) { On 2010/12/31 07:19:19, satya3656 wrote: > indentation +4 is fine http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:299: ConcatUriManager.ConcatUri.fromList(gadget, uriBatches, type), !split); On 2010/12/31 07:19:19, satya3656 wrote: > Won't this fit in previous line? Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:111: private static final Integer URL_MAX_LENGTH = 2083; On 2010/12/31 07:19:19, satya3656 wrote: > Can you move this URL_MAX_LENGTH to ConcatUriManager class > > and change it to int. Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:127: public Integer getUriLength() { On 2010/12/31 07:19:19, satya3656 wrote: > are you using this method? Yes..In ConcatVisitor Line@172 http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:136: public boolean isValidGetUrlLength() { On 2010/12/31 07:19:19, satya3656 wrote: > Same here? Removed http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java (right): http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:849: String URL = "http://www.url.com/" + RandomStringUtils.randomAlphanumeric(ConcatUriManager.ConcatData.getMaxUrlLength()) + ".css"; On 2010/12/31 07:19:19, satya3656 wrote: > 100 char Done. http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:864: String URLA = "http://www.urlA.com/" + RandomStringUtils.randomAlphanumeric(ConcatUriManager.ConcatData.getMaxUrlLength() / 2) + ".css"; On 2010/12/31 07:19:19, satya3656 wrote: > 100 char Done.
Sign in to reply to this message.
final few comments http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:167: ConcatUriManager.ConcatData uri = move it to makeSingleBatchUri and rename it. This piece looks very odd here. http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:171: uri.getUriLength() - ((Element) nodes.get(0)).getAttribute(type.getSrcAttrib()).length(); this also can be int http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:239: urlLength += url.length() + extraPadding; Integer -> int http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:38: static final int URL_MAX_LENGTH = 2083; -2 indent http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java (right): http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:864: public void dontConcatMultipleResourceBeyoundUrlLimit() throws Exception { can you add a normal url between URLA and URLB so that URLA, normal url get concatinated and URLB in other http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:867: String URLB = "http://www.urlB.com/" + RandomStringUtils.randomAlphanumeric( may be you can move ("http://www.urlA.com/" + RandomStringUtils.randomAlphanumeric(ConcatUriManager.ConcatData.getMaxUrlLength() / 2) + ".css") to convience function http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:883: // Should be left with a single CSS node child to parent. comment looks irrelevant
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:171: Integer urlOffset = On 2010/12/31 10:47:03, satya3656 wrote: > this also can be int Done. http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:239: Integer extraPadding = Integer.toString(index).length() + 2; On 2010/12/31 10:47:03, satya3656 wrote: > Integer -> int Done. http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:38: static final int URL_MAX_LENGTH = 2083; On 2010/12/31 10:47:03, satya3656 wrote: > -2 indent Done. http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java (right): http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:864: public void dontConcatMultipleResourceBeyoundUrlLimit() throws Exception { On 2010/12/31 10:47:03, satya3656 wrote: > can you add a normal url between URLA and URLB so that > URLA, normal url get concatinated and URLB in other Done. http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:867: String URLB = "http://www.urlB.com/" + RandomStringUtils.randomAlphanumeric( On 2010/12/31 10:47:03, satya3656 wrote: > may be you can move ("http://www.urlA.com/" + > RandomStringUtils.randomAlphanumeric(ConcatUriManager.ConcatData.getMaxUrlLength() > / 2) + ".css") to convience function Done. http://codereview.appspot.com/3734041/diff/44001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:883: // Should be left with a single CSS node child to parent. On 2010/12/31 10:47:03, satya3656 wrote: > comment looks irrelevant Done.
Sign in to reply to this message.
LGTM Few minor comments http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:219: * Calculat offset in Concat Uri apart from nodes uri's. Caculate http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:220: * @param gadget Gadget object for the site under consideration new line before this http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:300: private ConcatUriManager.ConcatData makeSingleBatchUri(Gadget gadget, merge this function with getUrlOffset http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java (right): http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:880: assertEquals(2, parent.getChildNodes().getLength()); Code is fine but organize all check for Node1 and then Node 2 will be simpler to understand. http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:890: assertEquals(CSS1_URL_STR, concatUri1.getQueryParameter("2")); assertNull(concatUri1.getQueryParameter("3"));
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:219: * Calculat offset in Concat Uri apart from nodes uri's. On 2011/01/05 09:58:19, satya3656 wrote: > Caculate Done. http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:220: * @param gadget Gadget object for the site under consideration On 2011/01/05 09:58:19, satya3656 wrote: > new line before this Done. http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:300: private ConcatUriManager.ConcatData makeSingleBatchUri(Gadget gadget, On 2011/01/05 09:58:19, satya3656 wrote: > merge this function with getUrlOffset Done. http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java (right): http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:880: assertEquals(2, parent.getChildNodes().getLength()); On 2011/01/05 09:58:19, satya3656 wrote: > Code is fine but organize all check for Node1 and then Node 2 will be simpler to > understand. Done. http://codereview.appspot.com/3734041/diff/53001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:890: assertEquals(CSS1_URL_STR, concatUri1.getQueryParameter("2")); On 2011/01/05 09:58:19, satya3656 wrote: > assertNull(concatUri1.getQueryParameter("3")); Done.
Sign in to reply to this message.
LGTM Send it to dev@ once you fix these minor comments. http://codereview.appspot.com/3734041/diff/67001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:262: //Split always happen when concat Uri approx crosses 95% of maximum allowed url length. space after // http://codereview.appspot.com/3734041/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:262: //Split always happen when concat Uri approx crosses 95% of maximum allowed url length. Split always happen when concat uri length exceeds maximum allowed url length.
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/67001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:262: //Split always happen when concat Uri approx crosses 95% of maximum allowed url length. On 2011/01/10 11:28:26, satya3656 wrote: > Split always happen when concat uri length exceeds maximum allowed url length. Done. http://codereview.appspot.com/3734041/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:262: //Split always happen when concat Uri approx crosses 95% of maximum allowed url length. On 2011/01/10 11:28:26, satya3656 wrote: > space after // Done.
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/88001/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/3734041/diff/88001/java/common/conf/shindig.pro... java/common/conf/shindig.properties:162: #Maximum Get Url size limit Space after # http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:135: if (nodes.isEmpty()) { Can we add a tiny test that exercises this check? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:169: List<List<Element>> concatBatchesWithinLimit = Lists.newLinkedList(); Would concatBatchesWithinUrlLimit be more clear (though long) - currently the "limit" seems quite an ambiguous term to use. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:229: * @param gadget Gadget object for the site under consideration site -> page? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:230: * @param elem element for generating any random url. element -> Element Element for generating a sample Concat Uri. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:249: * @param output elements list which are grouped together based on Url Max limit check. elements list -> List of elements http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:250: * @param urlOffset offset in Concat Uri apart from nodes uri's offset -> Offset (Always start comments with caps.) The definition of url offset (and even the term itself) is very vague in all your comments and variable/method names. Do you want to consider something along the lines of baseConcatUrlLength (because this is the length of the concat url leaving aside the lengths of the Uris of the concatenated nodes)? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:259: int extraPadding = Integer.toString(index).length() + 2; Why do we need this extra padding? Is this to account for the &? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:264: URL_LENGTH_BUFFER_MARGIN * ConcatUriManager.ConcatData.getMaxUrlLength()) { Since we don't expect this value to change during this method's execution, can we evaluate it outside the for loop? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:271: if (urlOffset + url.length() + extraPadding > Could you clarify what this check helps us in? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:279: if (index > 1) { Is the index mainly for knowing whether there are remaining nodes to be added to the output? If so, could we not check the size of batchWithMaximumLimit and decide whether to add to output or not? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:286: * For each batch present in concatBatches return the respective Uri batches. For each batch of elements present in concatBatches, return the respective Uri batches. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:291: private List<List<Uri>> geturiBatches(List<List<Element>> concatBatches) { geturiBatches -> getUriBatches http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:38: static final int URL_MAX_LENGTH = 2048; From your offline explanation, it seems that this is the DEFAULT_URL_MAX_LENGTH used if the shindig property for urlMaxLength is not defined. Can we name it as DEFAULT_URL_MAX_LENGTH for clarity? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:40: @Inject(optional=true) Space before and after =
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/88001/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/3734041/diff/88001/java/common/conf/shindig.pro... java/common/conf/shindig.properties:162: #Maximum Get Url size limit On 2011/01/25 10:13:52, anupama.dutta wrote: > Space after # Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:135: if (nodes.isEmpty()) { On 2011/01/25 10:13:52, anupama.dutta wrote: > Can we add a tiny test that exercises this check? Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:169: List<List<Element>> concatBatchesWithinLimit = Lists.newLinkedList(); On 2011/01/25 10:13:52, anupama.dutta wrote: > Would concatBatchesWithinUrlLimit be more clear (though long) - currently the > "limit" seems quite an ambiguous term to use. Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:229: * @param gadget Gadget object for the site under consideration On 2011/01/25 10:13:52, anupama.dutta wrote: > site -> page? Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:230: * @param elem element for generating any random url. On 2011/01/25 10:13:52, anupama.dutta wrote: > element -> Element > Element for generating a sample Concat Uri. Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:249: * @param output elements list which are grouped together based on Url Max limit check. On 2011/01/25 10:13:52, anupama.dutta wrote: > elements list -> List of elements Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:250: * @param urlOffset offset in Concat Uri apart from nodes uri's On 2011/01/25 10:13:52, anupama.dutta wrote: > offset -> Offset (Always start comments with caps.) > > The definition of url offset (and even the term itself) is very vague in all > your comments and variable/method names. Do you want to consider something along > the lines of baseConcatUrlLength (because this is the length of the concat url > leaving aside the lengths of the Uris of the concatenated nodes)? Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:259: int extraPadding = Integer.toString(index).length() + 2; On 2011/01/25 10:13:52, anupama.dutta wrote: > Why do we need this extra padding? Is this to account for the &? Yes..It includes '&', '=' and numerical values used with every resource uri. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:264: URL_LENGTH_BUFFER_MARGIN * ConcatUriManager.ConcatData.getMaxUrlLength()) { On 2011/01/25 10:13:52, anupama.dutta wrote: > Since we don't expect this value to change during this method's execution, can > we evaluate it outside the for loop? Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:271: if (urlOffset + url.length() + extraPadding > On 2011/01/25 10:13:52, anupama.dutta wrote: > Could you clarify what this check helps us in? Comment added. "Skip resource from concat if concat url length of individual node exceeds the max allowed url length." http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:279: if (index > 1) { On 2011/01/25 10:13:52, anupama.dutta wrote: > Is the index mainly for knowing whether there are remaining nodes to be added to > the output? If so, could we not check the size of batchWithMaximumLimit and > decide whether to add to output or not? No, this is not the main reason. index helps also in concat url length. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:286: * For each batch present in concatBatches return the respective Uri batches. On 2011/01/25 10:13:52, anupama.dutta wrote: > For each batch of elements present in concatBatches, return the respective Uri > batches. Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:291: private List<List<Uri>> geturiBatches(List<List<Element>> concatBatches) { On 2011/01/25 10:13:52, anupama.dutta wrote: > geturiBatches -> getUriBatches Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:38: static final int URL_MAX_LENGTH = 2048; On 2011/01/25 10:13:52, anupama.dutta wrote: > From your offline explanation, it seems that this is the DEFAULT_URL_MAX_LENGTH > used if > the shindig property for urlMaxLength is not defined. Can we name it as > DEFAULT_URL_MAX_LENGTH for clarity? Done. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:40: @Inject(optional=true) On 2011/01/25 10:13:52, anupama.dutta wrote: > Space before and after = Done.
Sign in to reply to this message.
LGTM. Please address the minor comment given below. http://codereview.appspot.com/3734041/diff/100001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/100001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:40: @Inject(optional = true) Indentation seems to be off on this line and the next.
Sign in to reply to this message.
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/...
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/...
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/...
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/...
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
Sign in to reply to this message.
Some comments on the alternate approach. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:187: // Regardless what happens, inject a copy of the first node, Update the comment to indicat that we inject as many copies of the first node as needed, with the new (concat) URIs. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:40: @Inject(optional = true) Fix indentation on this line and the next. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:127: /* Remove commented block. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:132: uriBuilders.add(uriBuilder); Most of the code here (from line 132 - 139) are repititions from the block before the for loop. Could we remove the code from outside the for loop and bring the first iteration in here itself? http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:153: if(batchUris != null && uriBuilder != ctx.makeQueryParams(null, null)) { Space before ( http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:166: for(UriBuilder cocnatUriBuilder : uriBuilders) { cocnatUriBuilder -> concatUriBuilder http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:168: if(version != null) { Indentation off and space needed before ( http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/test/java/... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:140: assertNull(uri.getUris().get(0).getScheme()); I think you should also assert that the size of getUris() is exactly 1 before each of these blocks? Similar asserts for size of getUris() might be needed in other tests as well.
Sign in to reply to this message.
Hi Fargo, I am still polishing up the patch, but could you take a look as to whether the changes are in the right direction or not? http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:187: // Regardless what happens, inject a copy of the first node, On 2011/02/01 13:40:30, anupama.dutta wrote: > Update the comment to indicat that we inject as many copies of the first node as > needed, with the new (concat) URIs. Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:40: @Inject(optional = true) On 2011/02/01 13:40:30, anupama.dutta wrote: > Fix indentation on this line and the next. Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:127: /* On 2011/02/01 13:40:30, anupama.dutta wrote: > Remove commented block. Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:153: if(batchUris != null && uriBuilder != ctx.makeQueryParams(null, null)) { On 2011/02/01 13:40:30, anupama.dutta wrote: > Space before ( Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:166: for(UriBuilder cocnatUriBuilder : uriBuilders) { On 2011/02/01 13:40:30, anupama.dutta wrote: > cocnatUriBuilder -> concatUriBuilder Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:168: if(version != null) { On 2011/02/01 13:40:30, anupama.dutta wrote: > Indentation off and space needed before ( Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/test/java/... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:140: assertNull(uri.getUris().get(0).getScheme()); On 2011/02/01 13:40:30, anupama.dutta wrote: > I think you should also assert that the size of getUris() is exactly 1 before > each of these blocks? Similar asserts for size of getUris() might be needed in > other tests as well. Done.
Sign in to reply to this message.
Directionally looks great. Versioning will be tricky as you finish this up. Thanks! (one little nit while I was perusing too) http://codereview.appspot.com/3734041/diff/117001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/117001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:119: this.uris = uris; nit: Collections.unmodifiableList(uris)
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:132: uriBuilders.add(uriBuilder); On 2011/02/01 13:40:30, anupama.dutta wrote: > Most of the code here (from line 132 - 139) are repititions from the block > before the for loop. Could we remove the code from outside the for loop and > bring the first iteration in here itself? Done. http://codereview.appspot.com/3734041/diff/117001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/117001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:119: this.uris = uris; On 2011/02/01 23:58:55, johnfargo wrote: > nit: Collections.unmodifiableList(uris) Done.
Sign in to reply to this message.
LGTM Looks fantastic. Just one nit, commit at will after. http://codereview.appspot.com/3734041/diff/115002/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/115002/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:115: float injectedMaxUrlLength = ConcatData.getMaxUrlLength() * URL_LENGTH_BUFFER_MARGIN; you can probably just cast this to an int
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:190: for(Uri uri : concatUri.getUris()) { Space before ( http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:130: uriBuilder = makeUriBuilder(ctx, concatHost, concatPath); Move the newline on line 131 to before line 130, since we are trying to reinitialize everything starting from line 130. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:167: if (versions != null && versions.size() > 0) { Will versions.size() ever be greater than 1 here? http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:182: while((resourceUri = uri.getQueryParameter(i.toString())) != null) { Space after while http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/test/java/... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:309: public void dontConcatMultipleResourceBeyoundUrlLimit() throws Exception { Yet to go through this test in detail, but does it assume that splitJsEnabled is true or false? Can we add tests for both scenarios because even in the non-json case, if there are enough adjacent js/css files, we can easily exceed the GET URL limit and ideally, these cases should be fixed by this patch.
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:190: for(Uri uri : concatUri.getUris()) { On 2011/02/28 08:35:06, anupama.dutta wrote: > Space before ( Done. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:130: uriBuilder = makeUriBuilder(ctx, concatHost, concatPath); On 2011/02/28 08:35:06, anupama.dutta wrote: > Move the newline on line 131 to before line 130, since we are trying to > reinitialize everything starting from line 130. Done. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:167: if (versions != null && versions.size() > 0) { On 2011/02/28 08:35:06, anupama.dutta wrote: > Will versions.size() ever be greater than 1 here? For correct versioner implementation, it can never be greater than 1. I changed this comparison from > 0 to == 1. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:182: while((resourceUri = uri.getQueryParameter(i.toString())) != null) { On 2011/02/28 08:35:06, anupama.dutta wrote: > Space after while Done. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/test/java/... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:309: public void dontConcatMultipleResourceBeyoundUrlLimit() throws Exception { On 2011/02/28 08:35:06, anupama.dutta wrote: > Yet to go through this test in detail, but does it assume that splitJsEnabled is > true or false? Can we add tests for both scenarios because even in the non-json > case, if there are enough adjacent js/css files, we can easily exceed the GET > URL limit and ideally, these cases should be fixed by this patch. Add another Test for null case. I also check the snippets value in case of json.
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/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/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:38: import org.apache.commons.lang.RandomStringUtils; Remove unused import. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:119: List<Uri> batchUris = Lists.newArrayList(); Add a comment here indicating that batchUris holds uris for the current batch of uris being concatenated. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:120: List<Uri> uris = Lists.newArrayList(); Add a comment before this line indicating that uris holds the concatenated uris formed from batches which satisfy the GET URL limit constraint. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/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/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:124: ImmutableList.of(RESOURCES_ONE, RESOURCES_TWO, RESOURCES_ONE); As discussed offline, let us retain RESOURCES_ONE and do the necessary checks. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/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/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:55: Remove inserted tab? http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:134: assertFalse(jsonParams.contains(json)); Maybe assert that jsonParams begins with json. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:314: Uri urlA = Uri.parse(generateUrl(ConcatUriManager.ConcatData.getMaxUrlLength() / 2)); 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? http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:332: assertNull(concatUris.get(0).getUris().get(0).getQueryParameter("2")); 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.
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/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/... 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/... 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/... 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/... 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/... 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/... 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/... 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/... 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/... 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/... 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.
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:133: if (jsonParams.keySet().contains(uri.getUris().get(0).toString())) { Assign uri.getUris().get(0).toString() to a variable calle currentUri for better readability in this block. http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:324: uriBasicCheck(concatUris.get(0).getUris().get(0), host, path, 9, type, "1", "1", versions[0]); Any idea why we get debug and nocache parameters as "1" here instead of "0" as in typeJsBatchSplitBatched? http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:380: private void uriBasicCheck(Uri uri, uriBasicCheck -> verifyBasicUriParameters or checkBasicUriParameters. http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:381: String host, Fix indentation for this line and the next few lines. http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:401: private void uriBasicCheck(Uri uri, uriBasicCheck -> verifyBasicUriParameters or checkBasicUriParameters.
Sign in to reply to this message.
http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:133: if (jsonParams.keySet().contains(uri.getUris().get(0).toString())) { On 2011/03/07 07:17:26, anupama.dutta wrote: > Assign uri.getUris().get(0).toString() to a variable calle currentUri for better > readability in this block. Done. http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:324: uriBasicCheck(concatUris.get(0).getUris().get(0), host, path, 9, type, "1", "1", versions[0]); On 2011/03/07 07:17:26, anupama.dutta wrote: > Any idea why we get debug and nocache parameters as "1" here instead of "0" as > in typeJsBatchSplitBatched? It is because of trues' used in line 303 for creating mockGadget. As these values dont matter us, we can choose any value. http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:380: private void uriBasicCheck(Uri uri, On 2011/03/07 07:17:26, anupama.dutta wrote: > uriBasicCheck -> verifyBasicUriParameters or checkBasicUriParameters. Done. http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:381: String host, On 2011/03/07 07:17:26, anupama.dutta wrote: > Fix indentation for this line and the next few lines. Done. http://codereview.appspot.com/3734041/diff/134002/java/gadgets/src/test/java/... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:401: private void uriBasicCheck(Uri uri, On 2011/03/07 07:17:26, anupama.dutta wrote: > uriBasicCheck -> verifyBasicUriParameters or checkBasicUriParameters. Done.
Sign in to reply to this message.
Static Injection is removed.
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
Am scared of ConcatVisitor, committing as Anupama and John have lgtm'ed.
Sign in to reply to this message.
|
