Gracefully handling case when document parsing returns null

Currently DomWalker.rewrite() throws NullPointerException when content.getDocument() returns null.

This change handles this case and throws a GadgetException so that we can return an INTERNAL_SERVER_ERROR to the user.

[gagan.goku, 2010-07-19 13:08:33]

On 2010/07/16 18:24:04, gagan.goku wrote:
> 

Please review.

[vikaas.arora, 2010-07-19 13:33:33]

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

http://codereview.appspot.com/1867041/diff/1/2#newcode153
main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:153: Document doc =
content.getDocument();
Is there a way we can catch this at the source i.e place where content.document
get's constructed ?

http://codereview.appspot.com/1867041/diff/1/3
File main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java (right):

http://codereview.appspot.com/1867041/diff/1/3#newcode202
main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:202:
e.printStackTrace();
Do you want to print stack in the production code ?
Is there a way we can embed this under some debug options ?

http://codereview.appspot.com/1867041/diff/1/4
File test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java (right):

http://codereview.appspot.com/1867041/diff/1/4#newcode246
test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java:246:
MutableContent mc = createMock(MutableContent.class);
You can move this statement closer to the first usage of 'mc' i.e line#250

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

addressing comments

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

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

http://codereview.appspot.com/1867041/diff/1/2#newcode153
main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:153: Document doc =
content.getDocument();
On 2010/07/19 13:33:34, vikaas.arora wrote:
> Is there a way we can catch this at the source i.e place where
content.document
> get's constructed ?

We can. But gadgets ppl seem to be catching exceptions and returning null there.
I thought there was a reason for it.
See MutableContent.getDocument

http://codereview.appspot.com/1867041/diff/1/3
File main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java (right):

http://codereview.appspot.com/1867041/diff/1/3#newcode202
main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:202:
e.printStackTrace();
On 2010/07/19 13:33:34, vikaas.arora wrote:
> Do you want to print stack in the production code ?
> Is there a way we can embed this under some debug options ?

I think this exception should be printed because its an exception raised by our
code. The html parser should be able to correct incorrect html and gracefully
return the correct string. This exception is thrown here when caja fails to
parse.
Anyways, we can always disable exception logging for MutableContent as a whole.
What do you have in mind for this exception logging ?

http://codereview.appspot.com/1867041/diff/1/4
File test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java (right):

http://codereview.appspot.com/1867041/diff/1/4#newcode246
test/java/org/apache/shindig/gadgets/rewrite/DomWalkerTest.java:246:
MutableContent mc = createMock(MutableContent.class);
On 2010/07/19 13:33:34, vikaas.arora wrote:
> You can move this statement closer to the first usage of 'mc' i.e line#250

Done.

[johnfargo, 2010-07-26 22:14:55]

LGTM, committing shortly.

[johnfargo, 2010-07-26 22:22:56]

FYI

Committed as http://svn.apache.org/viewvc?view=revision&revision=979472

On 2010/07/26 22:14:55, johnfargo wrote:
> LGTM, committing shortly.

[gagan.goku, 2010-07-27 05:36:59]

Thanks John

On Tue, Jul 27, 2010 at 3:52 AM, <johnfargo@gmail.com> wrote:

> FYI
>
> Committed as http://svn.apache.org/viewvc?view=revision&revision=979472
>
>
> On 2010/07/26 22:14:55, johnfargo wrote:
>
>> LGTM, committing shortly.
>>
>
>
>
> http://codereview.appspot.com/1867041/show
>