Updates to handle more use cases cleanly #60
Conversation
hjaret
commented
Oct 9, 2024
hjaret
commented
- Handle case where beginQuery is called even though the given URI contained a query
- Handle other cases where methods are called out of correct order
| try { | ||
| test("http://acme.com#frag", b -> b.withSegment("bar").with("foo", "bar"), "http://acme.com/bar#frag"); | ||
| fail("Expected this to fail"); | ||
| } catch (Exception e) { | ||
| assertEquals(e.getMessage(), "You cannot add a URL segment after you have appended a # to the end of the URL"); | ||
| } |
There was a problem hiding this comment.
Ideally you'd add another method to this test harness to do this work. This will be cleaner and make it easier to add additional failure case tests in the future.
For example:
// You cannot add a segment after a fragment.
testInvalid("http://acme.com#frag", b -> b.withSegment("bar").with("foo", "bar"),
"You cannot add a URL segment after you have appended a # to the end of the URL");
private void testInvalid(String uri, Consumer<QueryStringBuilder> consumer, String expectedMessage) {
try {
QueryStringBuilder builder = QueryStringBuilder.builder(uri);
consumer.accept(builder);
fail("Expected this to fail.");
} catch (Exception e) {
assertEquals(e.getMessage(), expectedMessage);
}
}
| // If query string contains no terms, remove the question mark | ||
| if (sb.toString().endsWith("?")) { | ||
| sb.setLength(sb.length() - 1); | ||
| } | ||
|
|
||
| if (sb.indexOf("#") == -1) { | ||
| sb.append("#"); | ||
| addSeparator = false; | ||
| } else { | ||
| char lastChar = sb.charAt(sb.length() - 1); | ||
| addSeparator = lastChar != '#' && lastChar != '&'; | ||
| } |
There was a problem hiding this comment.
I don't know that this is correct. I believe you can add a # after a ? in a URL.
For example, I believe this is a valid URL: https://example.com?foo=bar#section-1
There was a problem hiding this comment.
Per our conversation, I think I misunderstood what this was doing.
There was a problem hiding this comment.
https://example.com?foo=bar#section-1 is definitely valid. I was just trying to avoid
https://example.com?#section-1 but I can certainly allow it.
robotdan
left a comment
robotdan
left a comment
There was a problem hiding this comment.
Looks like we have pretty good test coverage. But if you can make sure we have a test for any new behavior added that would be great!
| // If query string contains no terms, remove the question mark | ||
| if (sb.toString().endsWith("?")) { | ||
| sb.setLength(sb.length() - 1); | ||
| } | ||
|
|
||
| if (sb.indexOf("#") == -1) { | ||
| sb.append("#"); | ||
| addSeparator = false; | ||
| } else { | ||
| char lastChar = sb.charAt(sb.length() - 1); | ||
| addSeparator = lastChar != '#' && lastChar != '&'; | ||
| } |
There was a problem hiding this comment.
Per our conversation, I think I misunderstood what this was doing.
| // If query string contains no terms, remove the question mark | ||
| if (sb.toString().endsWith("?")) { | ||
| sb.setLength(sb.length() - 1); | ||
| } |
There was a problem hiding this comment.
I don't think it is illegal to have an empty query of ? so I would leave this. I'd rather not enforce anything here unless it is truly illegal.
| sb.append("#"); | ||
| addSeparator = false; |
There was a problem hiding this comment.
We should just double check that the URL spec never allows more than one #. Seems likely but I don't know for sure.
From my Googling it does appear to be a safe assumption.
https://www.webdevsplanet.com/post/url-parameters-vs-fragments
A URL cannot have more than one fragment.
| if (!sb.isEmpty() && (sb.indexOf("?") != -1)) { | ||
| throw new IllegalStateException(String.format(message, "?")); | ||
| } | ||
| if (!sb.isEmpty() && (sb.indexOf("#") != -1)) { | ||
| throw new IllegalStateException(String.format(message, "#")); |
There was a problem hiding this comment.
Should we combine the sb.isEmpty check?
String message = "You cannot add a URL segment after you have appended a %s to the end of the URL";
if (!sb.isEmpty()) {
if (sb.indexOf("?") != -1) {
throw new IllegalStateException(String.format(message, "?"));
}
if (sb.indexOf("#") != -1) {
throw new IllegalStateException(String.format(message, "#"));
}
}
