You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Ziv Horesh <zh...@gmail.com> on 2010/06/14 19:12:28 UTC
Re: Removing conversion of & to & and vice versa when generating
url (issue1632042)
If I understand correctly, the html definition call for escaping & with
&, if only for the small chance of something actually want a value like
'<' as is.
So I don't think we want the new logic in your change to disable it.
What we need is to fix the places it is not converted back correctly.
On Sat, Jun 12, 2010 at 10:20 PM, <ga...@gmail.com> wrote:
> Reviewers: johnfargo, shindig.remailer_gmail.com, zhoresh,
>
> Message:
>
> On 2010/06/09 23:04:12, zhoresh wrote:
>
>> Good catch in the MutableContent!
>>
>
> Thanks
>
>
> http://codereview.appspot.com/1632042/diff/2001/3003
>> File
>>
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java
>
>> (left):
>>
>
> http://codereview.appspot.com/1632042/diff/2001/3003#oldcode169
>>
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java:169:
>
>> elem.getNamespaceURI() == null &&
>> Can you create a test case that show it is not needed? (or maybe I
>>
> didn't see
>
>> it)
>>
>
> http://codereview.appspot.com/1632042/diff/2001/3005
>> File
>>
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
>
>> (right):
>>
>
> http://codereview.appspot.com/1632042/diff/2001/3005#newcode135
>>
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:135:
>
>> concatUri.getUri().toString());
>> I guess there is a reason why it was added, maybe the right solution
>>
> is to fix
>
>> the parser?
>>
>
> I thought i understood what was going on, but now i don't.
>
> What I think is going on is:
> Elements created using Document.createElement() method have their
> getNamespaceURI as null.
> Elements that are parsed from html or copied / cloned around here and
> there have their getNamespaceURI as non-null.
>
> In both ConcatVistor and RenderingGadgetRewriter, we create elements
> using createElement and these will have getNamespaceURI() as null. We
> manually go and replace all occurances of '&' with '&' in the url.
>
> In DefaultHtmlSerializer, we check for url's and the logic states what
> for everything other than a url, replace '&' with '&'. But the logic
> for isUrl has the check element.getNamespaceURI() which will count only
> the elements created using createElement.
> So, all other urls that were parsed from html, get their '&' replaced by
> '&'
>
> Thus, the actual behavior seems to be to replace '&' by '&' but the
> intent of the code logic states otherwise.
>
> My main question is: why do we need this escaping & to & business. We
> should really not be doing such hardcoding stuff either.
>
> To make things even worse, in StyleTagExtractorVisitor, we create a
> <link> node but we dont seem to be replacing '&' with '&' here. So
> we don't seem to be consistent either.
>
> Here is a list of all places where we use '&':
>
> http://www.google.com/codesearch?hl=en&sa=N&filter=0&num=100&q=file:gadgets+file:shindig+file
> :\.java+\%26amp;+-file:Test.java
>
>
> To make things even worser, browsers seem to be treating & in urls
> differently.
> Using the changes in current patch, i loaded
> view-source:
> http://localhost:9003/gadgets/accel?container=default&url=http://www.cnn.com/
>
> It has a concatenated script tag:
> <script
> src="
> http://localhost:9003/gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com%2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%2Fi.cdn.turner.com%2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%2Fwww.cnn.com%2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js<http://localhost:9003/gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com%2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%2Fi.cdn.turner.com%2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%2Fwww.cnn.com%2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js>
> "
> type="text/javascript"></script>
>
> Then i ran nc -l -p 9003
>
> If i click on the img src link from within firefox, the request i get is
> this:
> GET
> /gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com
> %2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%
> 2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%
> 2Fi.cdn.turner.com%2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%2Fwww.cnn.com
> %2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%
> 2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js
> HTTP/1.1
>
> that is, with & in url replaced by &
>
>
> Now if i directly copy and paste the exact link in the navigation bar,
> here is what i get:
>
> GET
> /gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com
> %2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%
> 2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%
> 2Fwww.cnn.com
> %2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%
> 2Fi.cdn.turner.com
> %2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%
> 2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js
> HTTP/1.1
>
> that is, with & as is.
> This url throws a 404 with the following error:
> MISSING_PARAMETER ..<url>.. Missing type.
>
>
> I have spent quite some time trying to think this through and come up
> with an acceptable solution. In the process, i ended up adding an option
> called gadgets.uri.escapeAmpersandToAndAmp in the shindig.properties
> file, hoping that i could encapsulate all the '&' logic with this
> flag and this would be a non breaking change. Alas, im not fluent enough
> in Guice to accomplish that [I could not inject Injector into some
> classes and ended up modifying lot of code :( ]
>
>
> My suggestion is to get rid of escaping '&' business for urls
> everywhere. Why do we need it anyways ?
> To make things worsest, codereview did not let me submit this reply and
> kept throwing invalid XSRF token or some crap.
> So end result = i'm confused and kind of giving up.
>
> Description:
> Currently we have code that converts & to & and then & in url
> back to &.
> There is a bug right now in the way that dynamic nodes are added (say
> from StyleConcatVisitor) for which the reconversion from & to & does
> not happen.
>
>
> Please review this at http://codereview.appspot.com/1632042/show
>
> Affected files:
> M .gitignore
> M config/container.js
> M java/common/conf/shindig.properties
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/CompactHtmlSerializer.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
> A
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/MessageBundle.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java
> M
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpFetcherTest.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/AbstractParsingTestBase.java
> A
> java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializerTest.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/old/SanitizingGadgetRewriterTest.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTestBase.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/RewriterTestBase.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/old/BaseRewriterTestCase.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java
> M
> java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java
>
>
>
Re: Removing conversion of & to & and vice versa when generating
url (issue1632042)
Posted by John Hjelmstad <jo...@gmail.com>.
+1 to that. The reason you can't copy-and-paste the URL into your browser
bar is that the URL is unescaped by the browser when it's the value of a
target attribute ie src="value".
Re: the XSRF issue on codereview, that happens occasionally with the tool
and is orthogonal. I've seen it occur when you start an operation but don't
complete it within a period of time (in the tool). A refresh usually fixes
the problem.
--j
On Mon, Jun 14, 2010 at 10:12 AM, Ziv Horesh <zh...@gmail.com> wrote:
>
> If I understand correctly, the html definition call for escaping & with
> &, if only for the small chance of something actually want a value like
> '<' as is.
> So I don't think we want the new logic in your change to disable it.
> What we need is to fix the places it is not converted back correctly.
>
>
>
> On Sat, Jun 12, 2010 at 10:20 PM, <ga...@gmail.com> wrote:
>
>> Reviewers: johnfargo, shindig.remailer_gmail.com, zhoresh,
>>
>> Message:
>>
>> On 2010/06/09 23:04:12, zhoresh wrote:
>>
>>> Good catch in the MutableContent!
>>>
>>
>> Thanks
>>
>>
>> http://codereview.appspot.com/1632042/diff/2001/3003
>>> File
>>>
>>
>>
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java
>>
>>> (left):
>>>
>>
>> http://codereview.appspot.com/1632042/diff/2001/3003#oldcode169
>>>
>>
>>
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java:169:
>>
>>> elem.getNamespaceURI() == null &&
>>> Can you create a test case that show it is not needed? (or maybe I
>>>
>> didn't see
>>
>>> it)
>>>
>>
>> http://codereview.appspot.com/1632042/diff/2001/3005
>>> File
>>>
>>
>>
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
>>
>>> (right):
>>>
>>
>> http://codereview.appspot.com/1632042/diff/2001/3005#newcode135
>>>
>>
>>
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:135:
>>
>>> concatUri.getUri().toString());
>>> I guess there is a reason why it was added, maybe the right solution
>>>
>> is to fix
>>
>>> the parser?
>>>
>>
>> I thought i understood what was going on, but now i don't.
>>
>> What I think is going on is:
>> Elements created using Document.createElement() method have their
>> getNamespaceURI as null.
>> Elements that are parsed from html or copied / cloned around here and
>> there have their getNamespaceURI as non-null.
>>
>> In both ConcatVistor and RenderingGadgetRewriter, we create elements
>> using createElement and these will have getNamespaceURI() as null. We
>> manually go and replace all occurances of '&' with '&' in the url.
>>
>> In DefaultHtmlSerializer, we check for url's and the logic states what
>> for everything other than a url, replace '&' with '&'. But the logic
>> for isUrl has the check element.getNamespaceURI() which will count only
>> the elements created using createElement.
>> So, all other urls that were parsed from html, get their '&' replaced by
>> '&'
>>
>> Thus, the actual behavior seems to be to replace '&' by '&' but the
>> intent of the code logic states otherwise.
>>
>> My main question is: why do we need this escaping & to & business. We
>> should really not be doing such hardcoding stuff either.
>>
>> To make things even worse, in StyleTagExtractorVisitor, we create a
>> <link> node but we dont seem to be replacing '&' with '&' here. So
>> we don't seem to be consistent either.
>>
>> Here is a list of all places where we use '&':
>>
>> http://www.google.com/codesearch?hl=en&sa=N&filter=0&num=100&q=file:gadgets+file:shindig+file
>> :\.java+\%26amp;+-file:Test.java
>>
>>
>> To make things even worser, browsers seem to be treating & in urls
>> differently.
>> Using the changes in current patch, i loaded
>> view-source:
>> http://localhost:9003/gadgets/accel?container=default&url=http://www.cnn.com/
>>
>> It has a concatenated script tag:
>> <script
>> src="
>> http://localhost:9003/gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com%2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%2Fi.cdn.turner.com%2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%2Fwww.cnn.com%2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js<http://localhost:9003/gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com%2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%2Fi.cdn.turner.com%2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%2Fwww.cnn.com%2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js>
>> "
>> type="text/javascript"></script>
>>
>> Then i ran nc -l -p 9003
>>
>> If i click on the img src link from within firefox, the request i get is
>> this:
>> GET
>> /gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com
>> %2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%
>> 2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%
>> 2Fi.cdn.turner.com%2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%
>> 2Fwww.cnn.com
>> %2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%
>> 2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js
>> HTTP/1.1
>>
>> that is, with & in url replaced by &
>>
>>
>> Now if i directly copy and paste the exact link in the navigation bar,
>> here is what i get:
>>
>> GET
>> /gadgets/concat?container=default&gadget=http%3A%2F%2Fwww.cnn.com
>> %2F&debug=0&nocache=0&type=js&json=false&1=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fomni.time.js&2=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fprotoaculous.1.8.2.min.js&3=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fmain.js&4=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fswfobject-2.2.js&5=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2FcsiManager.js&6=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2FStorageManager.js&7=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fconnect%2Fconnect-lite.js&8=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Flocal.js%3F20100421&9=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F2.0%2FcnnSkypeOpen.js&10=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp_suppl.js&11=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fvideo%2Fcvp.js&12=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fxslo%2Fcvp%2Fads%2Ffreewheel%2Fjs%2Ffwjslib_1.1.js%3Fversion%3D1.1&13=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F2.0%2Fad_head0.js&14=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2Fcnn_adspaces%2Fcnn_adspaces.js&15=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Foo_engine.js&16=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fspecials%2Fworldcup%2Fwcdata.day.js&17=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fweather.footer.js&18=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fedition.vars.js&19=http%3A%2F%
>> 2Fwww.cnn.com
>> %2F.element%2Fjs%2F3.0%2Fintl%2Fedition_footer.js&20=http%3A%2F%
>> 2Fi.cdn.turner.com
>> %2Fcnn%2F.element%2Fjs%2F3.0%2Fs_code.intl.js&21=http%3A%2F%
>> 2Fi.cdn.turner.com%2Fcnn%2F.element%2Fjs%2F3.0%2Fhpsectiontracking.js
>> HTTP/1.1
>>
>> that is, with & as is.
>> This url throws a 404 with the following error:
>> MISSING_PARAMETER ..<url>.. Missing type.
>>
>>
>> I have spent quite some time trying to think this through and come up
>> with an acceptable solution. In the process, i ended up adding an option
>> called gadgets.uri.escapeAmpersandToAndAmp in the shindig.properties
>> file, hoping that i could encapsulate all the '&' logic with this
>> flag and this would be a non breaking change. Alas, im not fluent enough
>> in Guice to accomplish that [I could not inject Injector into some
>> classes and ended up modifying lot of code :( ]
>>
>>
>> My suggestion is to get rid of escaping '&' business for urls
>> everywhere. Why do we need it anyways ?
>> To make things worsest, codereview did not let me submit this reply and
>> kept throwing invalid XSRF token or some crap.
>> So end result = i'm confused and kind of giving up.
>>
>> Description:
>> Currently we have code that converts & to & and then & in url
>> back to &.
>> There is a bug right now in the way that dynamic nodes are added (say
>> from StyleConcatVisitor) for which the reconversion from & to & does
>> not happen.
>>
>>
>> Please review this at http://codereview.appspot.com/1632042/show
>>
>> Affected files:
>> M .gitignore
>> M config/container.js
>> M java/common/conf/shindig.properties
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/CompactHtmlSerializer.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
>> A
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceRewriter.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/MessageBundle.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java
>> M
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpFetcherTest.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/AbstractParsingTestBase.java
>> A
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializerTest.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/old/SanitizingGadgetRewriterTest.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTestBase.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/RewriterTestBase.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/old/BaseRewriterTestCase.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java
>> M
>> java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java
>>
>>
>>
>