You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@shindig.apache.org by zh...@gmail.com on 2010/03/06 03:20:26 UTC

Re: Implement AbsolutePathLinkRewriter (issue224098)

LGTM

Minor comments


http://codereview.appspot.com/224098/diff/2001/3001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriter.java
(right):

http://codereview.appspot.com/224098/diff/2001/3001#newcode58
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriter.java:58:
} catch (Exception e) {
Try to catch the specific exception you expect here.
Other exception might be sign for bugs to fix?

http://codereview.appspot.com/224098/diff/2001/3002
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriterTest.java
(right):

http://codereview.appspot.com/224098/diff/2001/3002#newcode37
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriterTest.java:37:
private static final Uri RELATIVE_URI = Uri.parse("/host/relative");
Test also a local refeence (no /)
Or same schema ("//a.com/a/b")
And a bad element ("<a/>")

http://codereview.appspot.com/224098/show

Re: Implement AbsolutePathLinkRewriter (issue224098)

Posted by John Hjelmstad <jo...@gmail.com>.

Thanks Ziv -- Responses inline.

On Fri, Mar 5, 2010 at 6:20 PM, <zh...@gmail.com> wrote:

> LGTM
>
> Minor comments
>
>
> http://codereview.appspot.com/224098/diff/2001/3001
> File
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriter.java
> (right):
>
> http://codereview.appspot.com/224098/diff/2001/3001#newcode58
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriter.java:58:
> } catch (Exception e) {
> Try to catch the specific exception you expect here.
> Other exception might be sign for bugs to fix?
>

Good call. Caught Uri.UriException instead, which is what was intended in
the first place.


>
> http://codereview.appspot.com/224098/diff/2001/3002
> File
>
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriterTest.java
> (right):
>
> http://codereview.appspot.com/224098/diff/2001/3002#newcode37
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathLinkRewriterTest.java:37:
> private static final Uri RELATIVE_URI = Uri.parse("/host/relative");
> Test also a local refeence (no /)
> Or same schema ("//a.com/a/b")
>

Added path-relative testing as well.


> And a bad element ("<a/>")


Added test to ensure that such an element is bypassed.


>
>
> http://codereview.appspot.com/224098/show
>