Modifying ProxyingVisitor to ignore malformed uris correctly

Currently ProxyingVisitor adds null to reservedUri list for uri's that are not parsed. It looks like its done in order to keep reserverUri list and node list in sync.
However, this causes a NullPointerException in DefaultProxyUriManager:92.

This change ignores the uri's (and corresponding nodes) which fail to parse.

[johnfargo, 2010-08-03 21:31:43]

http://codereview.appspot.com/1869052/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(right):

http://codereview.appspot.com/1869052/diff/5001/6002#newcode127
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:127:
for (Node node : reservedNodes) {
This looks fine. But it seems like a good idea to fix DefaultProxyUriManager to
be null-tolerant as well. Thoughts?

[gagan.goku, 2010-08-04 03:25:18]

http://codereview.appspot.com/1869052/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(right):

http://codereview.appspot.com/1869052/diff/5001/6002#newcode127
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:127:
for (Node node : reservedNodes) {
On 2010/08/03 21:31:43, johnfargo wrote:
> This looks fine. But it seems like a good idea to fix DefaultProxyUriManager
to
> be null-tolerant as well. Thoughts?

It is possible, but not foolproof. If we handle null in DefaultProxyUriManager,
we will also have to worry about inserting the null proxyUri back in the final
returned list so that the list remains the same size as the original
IMO, its better to ensure that we pass ProxyUriManager.make() the right input.

[anupama.dutta, 2010-08-04 06:20:48]

LGTM for the fix.

http://codereview.appspot.com/1869052/diff/5001/6001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java
(right):

http://codereview.appspot.com/1869052/diff/5001/6001#newcode83
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java:83:
public void testMalformedUri() throws Exception {
Remove this test case, since the condition tested here is already being tested
in revisitModifyValidSkipInvalid.

[Kuntal Loya, 2010-08-04 06:21:30]

http://codereview.appspot.com/1869052/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(right):

http://codereview.appspot.com/1869052/diff/5001/6002#newcode109
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:109:
proxiedUri = new ProxyUriManager.ProxyUri(gadget, Uri.parse(uriStr));
This change is good, but on a side note, shouldn't the parser be more robust to
handle such Uris.

http://codereview.appspot.com/1869052/diff/5001/6002#newcode114
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:114:
if (proxiedUri != null) {
Extra if clause is not required. Both the add statements can go in the try block
itself.

[gagan.goku, 2010-08-04 06:43:48]

http://codereview.appspot.com/1869052/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(right):

http://codereview.appspot.com/1869052/diff/5001/6002#newcode109
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:109:
proxiedUri = new ProxyUriManager.ProxyUri(gadget, Uri.parse(uriStr));
On 2010/08/04 06:21:30, Kuntal Loya wrote:
> This change is good, but on a side note, shouldn't the parser be more robust
to
> handle such Uris.

Sure, it would be nice to have a more lenient uri parser that java.net.URI.
Sadly, i don't know any as of now.
Would you like to file a jira issue ?

http://codereview.appspot.com/1869052/diff/5001/6002#newcode114
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:114:
if (proxiedUri != null) {
On 2010/08/04 06:21:30, Kuntal Loya wrote:
> Extra if clause is not required. Both the add statements can go in the try
block
> itself.

Done.

http://codereview.appspot.com/1869052/diff/5001/6001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java
(right):

http://codereview.appspot.com/1869052/diff/5001/6001#newcode83
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java:83:
public void testMalformedUri() throws Exception {
On 2010/08/04 06:20:48, anupama.dutta wrote:
> Remove this test case, since the condition tested here is already being tested
> in revisitModifyValidSkipInvalid.

Done.

[johnfargo, 2010-08-04 08:03:12]

Sounds fine -- can't go wrong w/ well-formed input.

On Tue, Aug 3, 2010 at 8:25 PM, <gagan.goku@gmail.com> wrote:

> Reviewers: dev-remailer_shindig.apache.org, johnfargo,
>
>
>
> http://codereview.appspot.com/1869052/diff/5001/6002
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
> (right):
>
> http://codereview.appspot.com/1869052/diff/5001/6002#newcode127
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:127:
> for (Node node : reservedNodes) {
> On 2010/08/03 21:31:43, johnfargo wrote:
>
>> This looks fine. But it seems like a good idea to fix
>>
> DefaultProxyUriManager to
>
>> be null-tolerant as well. Thoughts?
>>
>
> It is possible, but not foolproof. If we handle null in
> DefaultProxyUriManager, we will also have to worry about inserting the
> null proxyUri back in the final returned list so that the list remains
> the same size as the original
> IMO, its better to ensure that we pass ProxyUriManager.make() the right
> input.
>
> Description:
> Currently ProxyingVisitor adds null to reservedUri list for uri's that
> are not parsed. It looks like its done in order to keep reserverUri list
> and node list in sync.
> However, this causes a NullPointerException in
> DefaultProxyUriManager:92.
>
> This change ignores the uri's (and corresponding nodes) which fail to
> parse.
>
> Please review this at http://codereview.appspot.com/1869052/show
>
> Affected files:
>  M
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
>  M
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java
>
>
>

[johnfargo, 2010-08-04 08:18:58]

committed as r982154

On Wed, Aug 4, 2010 at 1:03 AM, John Hjelmstad <johnfargo@gmail.com> wrote:

> Sounds fine -- can't go wrong w/ well-formed input.
>
>
> On Tue, Aug 3, 2010 at 8:25 PM, <gagan.goku@gmail.com> wrote:
>
>> Reviewers: dev-remailer_shindig.apache.org, johnfargo,
>>
>>
>>
>> http://codereview.appspot.com/1869052/diff/5001/6002
>> File
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
>> (right):
>>
>> http://codereview.appspot.com/1869052/diff/5001/6002#newcode127
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:127:
>> for (Node node : reservedNodes) {
>> On 2010/08/03 21:31:43, johnfargo wrote:
>>
>>> This looks fine. But it seems like a good idea to fix
>>>
>> DefaultProxyUriManager to
>>
>>> be null-tolerant as well. Thoughts?
>>>
>>
>> It is possible, but not foolproof. If we handle null in
>> DefaultProxyUriManager, we will also have to worry about inserting the
>> null proxyUri back in the final returned list so that the list remains
>> the same size as the original
>> IMO, its better to ensure that we pass ProxyUriManager.make() the right
>> input.
>>
>> Description:
>> Currently ProxyingVisitor adds null to reservedUri list for uri's that
>> are not parsed. It looks like its done in order to keep reserverUri list
>> and node list in sync.
>> However, this causes a NullPointerException in
>> DefaultProxyUriManager:92.
>>
>> This change ignores the uri's (and corresponding nodes) which fail to
>> parse.
>>
>> Please review this at http://codereview.appspot.com/1869052/show
>>
>> Affected files:
>>  M
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
>>  M
>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java
>>
>>
>>
>

[gagan.goku, 2010-08-04 11:06:09]

Thanks John :)

On Wed, Aug 4, 2010 at 1:48 PM, John Hjelmstad <johnfargo@gmail.com> wrote:

> committed as r982154
>
>
> On Wed, Aug 4, 2010 at 1:03 AM, John Hjelmstad <johnfargo@gmail.com>wrote:
>
>> Sounds fine -- can't go wrong w/ well-formed input.
>>
>>
>> On Tue, Aug 3, 2010 at 8:25 PM, <gagan.goku@gmail.com> wrote:
>>
>>> Reviewers: dev-remailer_shindig.apache.org, johnfargo,
>>>
>>>
>>>
>>> http://codereview.appspot.com/1869052/diff/5001/6002
>>> File
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
>>> (right):
>>>
>>> http://codereview.appspot.com/1869052/diff/5001/6002#newcode127
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:127:
>>> for (Node node : reservedNodes) {
>>> On 2010/08/03 21:31:43, johnfargo wrote:
>>>
>>>> This looks fine. But it seems like a good idea to fix
>>>>
>>> DefaultProxyUriManager to
>>>
>>>> be null-tolerant as well. Thoughts?
>>>>
>>>
>>> It is possible, but not foolproof. If we handle null in
>>> DefaultProxyUriManager, we will also have to worry about inserting the
>>> null proxyUri back in the final returned list so that the list remains
>>> the same size as the original
>>> IMO, its better to ensure that we pass ProxyUriManager.make() the right
>>> input.
>>>
>>> Description:
>>> Currently ProxyingVisitor adds null to reservedUri list for uri's that
>>> are not parsed. It looks like its done in order to keep reserverUri list
>>> and node list in sync.
>>> However, this causes a NullPointerException in
>>> DefaultProxyUriManager:92.
>>>
>>> This change ignores the uri's (and corresponding nodes) which fail to
>>> parse.
>>>
>>> Please review this at http://codereview.appspot.com/1869052/show
>>>
>>> Affected files:
>>>  M
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
>>>  M
>>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitorTest.java
>>>
>>>
>>>
>>
>