Fix URL parsing issue (#391) #393
Conversation
|
FYI @taer . Please give this a go if you can. |
| @@ -121,7 +121,7 @@ public boolean isFullyQualified() { | |||
| * @return true if the URL is absolute. | |||
| */ | |||
| public boolean isAbsolute() { | |||
| return path.startsWith("/"); | |||
There was a problem hiding this comment.
What is this method used for? We have changed from it's an absolute path to i_t's an absolute URI_ . What's the real purpose?
There was a problem hiding this comment.
I can only guess. I believe the original intention was to determine if the request is in origin-form or absolute-form. My gut feeling is that we don't really need this method in the URL class.
As to why this change? It is because every path component always starts with / therefore it always returned true.
There was a problem hiding this comment.
I would at least mark it as deprecated then. I think from previous discussions our goal could be to completely remove Url from the API.
| @@ -222,17 +221,23 @@ public void throwsBadRequestExceptionWhenRequestMessageContainsMoreThanOneHostHe | |||
|
|
|||
| @Test | |||
| public void callsTheEscaperForUnwiseChars() throws Exception { | |||
| UnwiseCharsEncoder encoder = mock(UnwiseCharsEncoder.class); | |||
| AtomicBoolean encoderCalled = new AtomicBoolean(); | |||
| UnwiseCharsEncoder encoder = x -> { | |||
There was a problem hiding this comment.
Why replace mocking with this approach?
There was a problem hiding this comment.
I meant to comment on the same to be honest... I think keeping mock.verify() would make our code a bit more uniform.
There was a problem hiding this comment.
By mistake. I have backed off this change.
| } | ||
|
|
||
| @Test | ||
| public void decodesOriginFormWithLowercaseHostHeader() { |
There was a problem hiding this comment.
This test is identical to the one below and the first test in the file.
There was a problem hiding this comment.
Actually it is different from the first test in the file. See the header case. But I have now made it different from the test below.
| @Test | ||
| public void decodesOriginFormWithUppercaseHostHeader() { | ||
| DefaultFullHttpRequest request = new DefaultFullHttpRequest(HTTP_1_1, GET, "/foo"); | ||
| request.headers().add("host", "example.com"); |
There was a problem hiding this comment.
Shouldn't this be uppercase?
|
|
||
| public class UrlDecoderTest { | ||
| @Test | ||
| public void decodesOriginForm() { |
There was a problem hiding this comment.
I have made some changes to the test files. Please have another look.
| } | ||
|
|
||
| @Test | ||
| public void decodesOriginFormIssue391() { |
There was a problem hiding this comment.
Is using the issue number in the name of the test something we usually do? I would still have a name that tried to explain the issue though.
There was a problem hiding this comment.
Agreed. Let's describe the scenario in an issue-agnostic way for the test name, then maybe add the issue link in a comment above the test.
Fixes issue #391.
The sample URI from #391 is incorrectly exposed to the interceptor chain:
https://test.homeaway.co.uk//www.homeaway.co.uk/p6873759//www.homeaway.co.uk/p6873759/p6873759Root cause: Styx calls Java
URI.createwhich fails to parse the URI in its origin form. The problem disappears when we temporarily convert the URI to its absolute-form before callingURI.create.