Empty segments aren't handled correctly

[GoogleCodeExporter]

Reported by a user on the forum:

http://ocpsoft.com/support/topic/url-rewrite-with-empty-parameters-in-the-url

I had a look at this issue and built a test that reproduces this:

  assertEquals("/test//", new URL("/test//").decode().toURL());

This test fails! The full test is attached to the ticket. I think the root 
cause is this line in the URL constructor:

  String[] segments = trimmedUrl.split("/");

It seems like split() doesn't work as expected here (because the / at the end 
of the string) and so the empty segment is lost.

Unfortunately I've currently not much time to look into this. Hopefully later. 
:)

Original issue reported on code.google.com by chkalt on 24 Oct 2011 at 4:59

Attachments:

• empty-segments-bug-testcase.patch

[GoogleCodeExporter]

Would you mind if I take care of this issue?

Original comment by haa...@gmail.com on 7 Jan 2012 at 2:13

[GoogleCodeExporter]

Sure! It would be great to get some help on this one!

Do you need any further information? The failing test case is attached to the 
ticket. I think String.split() doesn't work here very well. Perhaps there are 
ways to work around this issue using the java.util.regex API?

Original comment by chkalt on 7 Jan 2012 at 2:45

[GoogleCodeExporter]

It looks that there are two causes of this bug. 

First is split which omits leading and trailing empty strings. This can be 
easily fixed by using split(String regex, int limit) with negative limit. 

Second is if condition in line 58 of com.ocpsoft.pretty.faces.url.Metadata. It 
says 'hasTrailingSlash() && !result.toString().endsWith("/")'. It looks like it 
should be without '!result.toString().endsWith("/")'.

I have this felling that using Pattern for splitting string using single 
character is very ineffective. What do you think of writting static method in 
URL named splitBySlash and implementing it effective way?

Original comment by haa...@gmail.com on 7 Jan 2012 at 3:29

[GoogleCodeExporter]

Sure, writing a custom method to perform the split would be a good way. By 
doing so we could fix this bug and additionally get a performance improvement.

My suggestion is to add some more unit tests verifying the expected behavior of 
the class. Especially for all the corner cases. Then you could implement the 
splitting using a new custom method and use the tests to verify that everything 
is working fine.

Thanks for helping us with this.

Original comment by chkalt on 7 Jan 2012 at 4:40

[GoogleCodeExporter]

@haaawk,

Thanks so much for looking at this. Glad to have you involved! Are you on the 
dev list? 

prettyfaces-dev@googlegroups.com

~Lincoln

Original comment by lincolnb...@gmail.com on 7 Jan 2012 at 5:16

[GoogleCodeExporter]

I belive this issue can be closed after my pull request was merged.

Original comment by haa...@gmail.com on 17 Jan 2012 at 6:52

[GoogleCodeExporter]

Original comment by lincolnb...@gmail.com on 17 Jan 2012 at 7:30

• Changed state: Fixed