Rewrite html content in DomWalker.Rewriter only if builder.getContent() is nonempty

For pages which return status code 200 and content type text/html but no content, we throw an exception in the parser and then this exception goes all the way to proxy servlet.

We should not even attempt to rewrite something using DomWalker if the original content itself is empty.

[anupama.dutta, 2010-07-30 10:18:31]

http://codereview.appspot.com/1689055/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/1689055/diff/1/2#newcode141
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:141:
if (RewriterUtils.isHtml(request, builder) &&
!StringUtils.isEmpty(builder.getContent())) {
Please add a test for this.

[gagan.goku, 2010-07-31 11:26:58]

http://codereview.appspot.com/1689055/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/1689055/diff/1/2#newcode141
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:141:
if (RewriterUtils.isHtml(request, builder) &&
!StringUtils.isEmpty(builder.getContent())) {
On 2010/07/30 10:18:31, anupama.dutta wrote:
> Please add a test for this.

Done.

[anupama.dutta, 2010-08-02 13:25:32]

LGTM. Please send to dev.remailer for review + commit.

[zhoresh, 2010-08-02 17:37:18]

LGTM, but please update the test. Thanks

http://codereview.appspot.com/1689055/diff/8001/9001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java
(right):

http://codereview.appspot.com/1689055/diff/8001/9001#newcode272
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java:272:
DomWalker.Rewriter rewriter = getRewriter();
The test now will succeed even without the change. I think you need to add an
actual visitor. Have the test fail without your change.

[gagan.goku, 2010-08-02 18:06:23]

http://codereview.appspot.com/1689055/diff/8001/9001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java
(right):

http://codereview.appspot.com/1689055/diff/8001/9001#newcode272
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java:272:
DomWalker.Rewriter rewriter = getRewriter();
On 2010/08/02 17:37:18, zhoresh wrote:
> The test now will succeed even without the change. I think you need to add an
> actual visitor. Have the test fail without your change.

With or without adding a visitor, this test fails with
UnsupportedOperationException because no html parser has been specified (if we
dont check for empty content)

[zhoresh, 2010-08-02 18:49:35]

Thanks for updating the test, LGTM, committed (r981658)

[gagan.goku, 2010-08-02 19:29:15]

Thanks Ziv.

Gagan

-- 
The only thing missing in life is background music.
-- Gagandeep Singh


On Tue, Aug 3, 2010 at 12:19 AM, <zhoresh@gmail.com> wrote:

> Thanks for updating the test, LGTM, committed (r981658)
>
>
> http://codereview.appspot.com/1689055/show
>

[johnfargo, 2010-08-02 23:06:12]

Just noticed this was committed -- rollback w/ alternate solution in the
following (also committed):
http://codereview.appspot.com/1902046/show

Comments welcome.

http://codereview.appspot.com/1689055/diff/13001/11003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/1689055/diff/13001/11003#newcode143
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:143:
if (RewriterUtils.isHtml(request, builder) &&
!StringUtils.isEmpty(builder.getContent())) {
sadly this change is very problematic from a performance perspective.

Every DomWalker-based rewriter does a getDocument(), which does a DOM parse.
This parse is cached until getContent() is called, at which time the content is
re-serialized. The cycle will return for every rewriter, leading to N re-parses
per rendering pass, where N = number of rewriters.

I'd recommend putting this logic in AccelResponseRewriterRegistry, where doing a
getContent() check is harmless since subsequent rewriters are likely to make
that call anyway, which by that time will be cached.

[gagan.goku, 2010-08-03 04:01:31]

On 2010/08/02 23:06:12, johnfargo wrote:
> Just noticed this was committed -- rollback w/ alternate solution in the
> following (also committed):
> http://codereview.appspot.com/1902046/show
> 
> Comments welcome.
> 
> http://codereview.appspot.com/1689055/diff/13001/11003
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
> (right):
> 
> http://codereview.appspot.com/1689055/diff/13001/11003#newcode143
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:143:
> if (RewriterUtils.isHtml(request, builder) &&
> !StringUtils.isEmpty(builder.getContent())) {
> sadly this change is very problematic from a performance perspective.
> 
> Every DomWalker-based rewriter does a getDocument(), which does a DOM parse.
> This parse is cached until getContent() is called, at which time the content
is
> re-serialized. The cycle will return for every rewriter, leading to N
re-parses
> per rendering pass, where N = number of rewriters.
> 
> I'd recommend putting this logic in AccelResponseRewriterRegistry, where doing
a
> getContent() check is harmless since subsequent rewriters are likely to make
> that call anyway, which by that time will be cached.

Hi John

Comment well taken. Making the change in AccelResponseRegistry is fine, except
the original exception occured while proxying a resource. This change would be
more suited in DefaultResponseRewriterRegistry.

[johnfargo, 2010-08-03 09:33:47]

Irrespective the patch, or even which registry it should go into, what was
the specific Exception that was thrown?


Cheers,
John

On Mon, Aug 2, 2010 at 9:01 PM, <gagan.goku@gmail.com> wrote:

> On 2010/08/02 23:06:12, johnfargo wrote:
>
>> Just noticed this was committed -- rollback w/ alternate solution in
>>
> the
>
>> following (also committed):
>> http://codereview.appspot.com/1902046/show
>>
>
>  Comments welcome.
>>
>
>  http://codereview.appspot.com/1689055/diff/13001/11003
>> File
>>
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
>
>> (right):
>>
>
>  http://codereview.appspot.com/1689055/diff/13001/11003#newcode143
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:143:
>
>> if (RewriterUtils.isHtml(request, builder) &&
>> !StringUtils.isEmpty(builder.getContent())) {
>> sadly this change is very problematic from a performance perspective.
>>
>
>  Every DomWalker-based rewriter does a getDocument(), which does a DOM
>>
> parse.
>
>> This parse is cached until getContent() is called, at which time the
>>
> content is
>
>> re-serialized. The cycle will return for every rewriter, leading to N
>>
> re-parses
>
>> per rendering pass, where N = number of rewriters.
>>
>
>  I'd recommend putting this logic in AccelResponseRewriterRegistry,
>>
> where doing a
>
>> getContent() check is harmless since subsequent rewriters are likely
>>
> to make
>
>> that call anyway, which by that time will be cached.
>>
>
> Hi John
>
> Comment well taken. Making the change in AccelResponseRegistry is fine,
> except the original exception occured while proxying a resource. This
> change would be more suited in DefaultResponseRewriterRegistry.
>
>
>
> http://codereview.appspot.com/1689055/show
>