Adding ProxyingRewriter which will only proxy resources

First step towards separating out Css, Js concatenation and resource proxying.

[gagan.goku, 2010-07-07 23:10:19]

more stuff

[gagan.goku, 2010-07-07 23:30:15]

removing iframe src

[vikaas.arora, 2010-07-08 07:17:20]

http://codereview.appspot.com/1711053/diff/9002/13001
File common/conf/shindig.properties (right):

http://codereview.appspot.com/1711053/diff/9002/13001#newcode51
common/conf/shindig.properties:51:
shindig.content-rewrite.include-tags=link,script,embed,img,style,input,body,script,object
We found some issues with 'embed' & 'object' tags w.r.t rewriting src/data URLs.
We can add these two TAGs later after more validation for these two TAGS.
FYI: The issues that we found w.r.t rewriten resources is that the dynamically
generated resources from within these embedded resource had some weird issues.

[gagan.goku, 2010-07-08 17:56:09]

removed embed and object tags

[gagan.goku, 2010-07-08 17:58:11]

fixing bad things

[gagan.goku, 2010-07-08 17:59:23]

http://codereview.appspot.com/1711053/diff/9002/13001
File common/conf/shindig.properties (right):

http://codereview.appspot.com/1711053/diff/9002/13001#newcode51
common/conf/shindig.properties:51:
shindig.content-rewrite.include-tags=link,script,embed,img,style,input,body,script,object
On 2010/07/08 07:17:20, vikaas.arora wrote:
> We found some issues with 'embed' & 'object' tags w.r.t rewriting src/data
URLs.
> We can add these two TAGs later after more validation for these two TAGS.
> FYI: The issues that we found w.r.t rewriten resources is that the dynamically
> generated resources from within these embedded resource had some weird issues.

Done.

[gagan.goku, 2010-07-08 18:38:46]

fixing tests

[gagan.goku, 2010-07-10 13:40:24]

+anupama

PTAL.

On Fri, Jul 9, 2010 at 12:08 AM, <gagan.goku@gmail.com> wrote:

> fixing tests
>
>
> http://codereview.appspot.com/1711053/show
>

[gagan.goku, 2010-07-15 19:24:04]

Guys please take a look.

On Sat, Jul 10, 2010 at 7:10 PM, Gagandeep singh <gagan.goku@gmail.com>wrote:

> +anupama
>
> PTAL.
>
>
> On Fri, Jul 9, 2010 at 12:08 AM, <gagan.goku@gmail.com> wrote:
>
>> fixing tests
>>
>>
>> http://codereview.appspot.com/1711053/show
>>
>
>

[anupama.dutta, 2010-07-17 03:55:24]

http://codereview.appspot.com/1711053/diff/35001/31002
File common/conf/shindig.properties (right):

http://codereview.appspot.com/1711053/diff/35001/31002#newcode51
common/conf/shindig.properties:51:
shindig.content-rewrite.include-tags=link,script,img,style,input,body
Since http://codereview.appspot.com/1806044/show is already addressing this
issue, you could revert this change?

http://codereview.appspot.com/1711053/diff/35001/31003
File
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31003#newcode60
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:60:
.put("iframe", "src").build());
Since http://codereview.appspot.com/1806044/show is already addressing this set
of changes, and we don't want to actually rewrite iframe srcs, I think you
should revert this change?

http://codereview.appspot.com/1711053/diff/35001/31004
File
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31004#newcode1
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java:1:
/*
Did you try svn cp for creating this file? If that works, it can really help
illustrate the changes from the ProxyingContentRewriter class to this one.

http://codereview.appspot.com/1711053/diff/35001/31004#newcode45
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java:45:
protected List<DomWalker.Visitor> makeVisitors(Gadget context, Uri gadgetUri) {
You could import DomWalker.Visitor directly?

http://codereview.appspot.com/1711053/diff/35001/31005
File
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31005#newcode47
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:47:
.put("script", "src").build();
Revert this change?

http://codereview.appspot.com/1711053/diff/35001/31006
File gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31006#newcode95
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:95:
@Provides
The unified diff works fine for this file, but the side-by-side one does not.
When you re-upload the patch after addressing comments, could you see what the
problem is?

http://codereview.appspot.com/1711053/diff/35001/31007
File
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31007#newcode63
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java:63:
+ "<img src=\"//localhost:8080/gadgets/proxy?container=default"
A more readable way to right this test might be to use constants such as
String IMG_1 = "helo.img";
String REWRITTEN_IMG_1 =
"//localhost:8080/gadgets/proxy?container...url=helo.img";
and have a small method called getHtmlSnippet() that takes the urls to be
embedded and adds in the remaining common portions of the html snippet.
That way, it is very clear that only the 3 relevant urls are getting rewritten
and no other mistake is made in the test or the code.

http://codereview.appspot.com/1711053/diff/35001/31008
File
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31008#newcode59
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java:59:
public void embedVisitNotReserved() throws Exception {
Revert these changes?

[gagan.goku, 2010-07-17 05:14:24]

addressing comments

[gagan.goku, 2010-07-17 05:14:33]

http://codereview.appspot.com/1711053/diff/35001/31002
File common/conf/shindig.properties (right):

http://codereview.appspot.com/1711053/diff/35001/31002#newcode51
common/conf/shindig.properties:51:
shindig.content-rewrite.include-tags=link,script,img,style,input,body
On 2010/07/17 03:55:24, anupama.dutta wrote:
> Since http://codereview.appspot.com/1806044/show is already addressing this
> issue, you could revert this change?

Done.

http://codereview.appspot.com/1711053/diff/35001/31003
File
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31003#newcode60
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:60:
.put("iframe", "src").build());
On 2010/07/17 03:55:24, anupama.dutta wrote:
> Since http://codereview.appspot.com/1806044/show is already addressing this
set
> of changes, and we don't want to actually rewrite iframe srcs, I think you
> should revert this change?

Done.

http://codereview.appspot.com/1711053/diff/35001/31004
File
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31004#newcode1
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java:1:
/*
On 2010/07/17 03:55:24, anupama.dutta wrote:
> Did you try svn cp for creating this file? If that works, it can really help
> illustrate the changes from the ProxyingContentRewriter class to this one.

i dont have svn cp on windoze :(

http://codereview.appspot.com/1711053/diff/35001/31004#newcode45
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java:45:
protected List<DomWalker.Visitor> makeVisitors(Gadget context, Uri gadgetUri) {
On 2010/07/17 03:55:24, anupama.dutta wrote:
> You could import DomWalker.Visitor directly?

reverted

http://codereview.appspot.com/1711053/diff/35001/31005
File
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31005#newcode47
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:47:
.put("script", "src").build();
On 2010/07/17 03:55:24, anupama.dutta wrote:
> Revert this change?

Done.

http://codereview.appspot.com/1711053/diff/35001/31006
File gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31006#newcode95
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:95:
@Provides
On 2010/07/17 03:55:24, anupama.dutta wrote:
> The unified diff works fine for this file, but the side-by-side one does not.
> When you re-upload the patch after addressing comments, could you see what the
> problem is?

Done.

http://codereview.appspot.com/1711053/diff/35001/31007
File
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31007#newcode63
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java:63:
+ "<img src=\"//localhost:8080/gadgets/proxy?container=default"
On 2010/07/17 03:55:24, anupama.dutta wrote:
> A more readable way to right this test might be to use constants such as
> String IMG_1 = "helo.img";
> String REWRITTEN_IMG_1 =
> "//localhost:8080/gadgets/proxy?container...url=helo.img";
> and have a small method called getHtmlSnippet() that takes the urls to be
> embedded and adds in the remaining common portions of the html snippet.
> That way, it is very clear that only the 3 relevant urls are getting rewritten
> and no other mistake is made in the test or the code.

Done.

http://codereview.appspot.com/1711053/diff/35001/31008
File
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java
(right):

http://codereview.appspot.com/1711053/diff/35001/31008#newcode59
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java:59:
public void embedVisitNotReserved() throws Exception {
On 2010/07/17 03:55:24, anupama.dutta wrote:
> Revert these changes?

Done.

[gagan.goku, 2010-07-17 05:18:26]

addressing comments

[anupama.dutta, 2010-07-17 06:25:31]

http://codereview.appspot.com/1711053/diff/49004/51003
File main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java (right):

http://codereview.appspot.com/1711053/diff/49004/51003#newcode29
main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java:29: public
ProxyingRewriter(ContentRewriterFeature.DefaultConfig config,
The ProxyingContentRewriter seems to use the featureConfigFactory.get(gadgetUri)
call to initialize the config - should we not do the same here?

http://codereview.appspot.com/1711053/diff/49004/51005
File test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java
(right):

http://codereview.appspot.com/1711053/diff/49004/51005#newcode65
test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java:65:
String expected =
This repetition is what would be good to avoid. What I was suggesting was:
String getTestHtml(String imgPath, String inputPath, String scriptPath, String
embedPath) {
   return "<html><head><style>" +
            .... + <img src=\"" + imgPath + "\"><img>" ....
}

For getting the original html, you would ask for getHtml(IMG, INPUT, SCRIPT,
EMBED) and for getting the rewritten html, you would ask for
getHtml(REWRITTEN_IMG, INPUT, REWRITTEN_SCRIPT, REWRITTEN_EMBED);

Hope this makes sense?

Also, to emphasize the fact that we don't do concatenation in this rewriter,
should we add one more script and show that these are not concatenated into one?

[anupama.dutta, 2010-07-17 08:23:26]

http://codereview.appspot.com/1711053/diff/49004/51003
File main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java (right):

http://codereview.appspot.com/1711053/diff/49004/51003#newcode29
main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java:29: public
ProxyingRewriter(ContentRewriterFeature.DefaultConfig config,
On 2010/07/17 06:25:31, anupama.dutta wrote:
> The ProxyingContentRewriter seems to use the
featureConfigFactory.get(gadgetUri)
> call to initialize the config - should we not do the same here?


After looking around the codebase, it appears as if the config could carry
important details such as what tags to include for processing etc. I think it is
best to use the factory rather than using the default config.

[gagan.goku, 2010-07-17 12:00:37]

addressing comments

[gagan.goku, 2010-07-17 12:00:46]

http://codereview.appspot.com/1711053/diff/49004/51003
File main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java (right):

http://codereview.appspot.com/1711053/diff/49004/51003#newcode29
main/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriter.java:29: public
ProxyingRewriter(ContentRewriterFeature.DefaultConfig config,
On 2010/07/17 08:23:26, anupama.dutta wrote:
> On 2010/07/17 06:25:31, anupama.dutta wrote:
> > The ProxyingContentRewriter seems to use the
> featureConfigFactory.get(gadgetUri)
> > call to initialize the config - should we not do the same here?
> 
> 
> After looking around the codebase, it appears as if the config could carry
> important details such as what tags to include for processing etc. I think it
is
> best to use the factory rather than using the default config.

Please look again :)

http://codereview.appspot.com/1711053/diff/49004/51005
File test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java
(right):

http://codereview.appspot.com/1711053/diff/49004/51005#newcode65
test/java/org/apache/shindig/gadgets/rewrite/ProxyingRewriterTest.java:65:
String expected =
On 2010/07/17 06:25:31, anupama.dutta wrote:
> This repetition is what would be good to avoid. What I was suggesting was:
> String getTestHtml(String imgPath, String inputPath, String scriptPath, String
> embedPath) {
>    return "<html><head><style>" +
>             .... + <img src=\"" + imgPath + "\"><img>" ....
> }
> 
> For getting the original html, you would ask for getHtml(IMG, INPUT, SCRIPT,
> EMBED) and for getting the rewritten html, you would ask for
> getHtml(REWRITTEN_IMG, INPUT, REWRITTEN_SCRIPT, REWRITTEN_EMBED);
> 
> Hope this makes sense?
> 
> Also, to emphasize the fact that we don't do concatenation in this rewriter,
> should we add one more script and show that these are not concatenated into
one?

Done.

[anupama.dutta, 2010-07-18 04:28:41]

Please add John and Ziv as reviewers after addressing the below comment.

Thanks,
Anupama.

http://codereview.appspot.com/1711053/diff/63001/64002
File main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java (right):

http://codereview.appspot.com/1711053/diff/63001/64002#newcode71
main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:71:
ProxyingContentRewriter proxyingContentRewriter,
Should we not change this as well? This is part of the accel flow and not proxy
flow, correct?

[gagan.goku, 2010-07-18 04:34:28]

http://codereview.appspot.com/1711053/diff/63001/64002
File main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java (right):

http://codereview.appspot.com/1711053/diff/63001/64002#newcode71
main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:71:
ProxyingContentRewriter proxyingContentRewriter,
On 2010/07/18 04:28:41, anupama.dutta wrote:
> Should we not change this as well? This is part of the accel flow and not
proxy
> flow, correct?

right now this is not part of any flow :( Dead code.
Il add a todo to clean this up as well

[gagan.goku, 2010-07-18 04:35:05]

addressing comments

[anupama.dutta, 2010-07-18 04:43:41]

LGTM.

[gagan.goku, 2010-07-18 04:49:33]

really addressing comments

[gagan.goku, 2010-07-18 04:50:10]

On 2010/07/18 04:43:41, anupama.dutta wrote:
> LGTM.

thanks :)

[johnfargo, 2010-07-19 17:40:46]

Can you expand on the problem here? The stated point of this CL runs in direct
opposition to the design ethos of ProxyingContentRewriter w/ concat + proxy
together.

The point of doing it the way it exists is to avoid concatenation and proxying
stepping over one another - ie. proxying an already-concatenated URI or vice
versa.

If the idea is independent configurability, would it work just offer an option
in the existing ProxyingContentRewriter to disable the concatenation piece
(and/or vice versa)? Your test cases could be used essentially verbatim for this
case.

Best,
John



On 2010/07/18 04:50:10, gagan.goku wrote:
> On 2010/07/18 04:43:41, anupama.dutta wrote:
> > LGTM.
> 
> thanks :)