-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add authority check for forUrl #430
Conversation
…a client side mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hm. Wouldn't a toUri be more effective here? Have not tested if that would fail. |
https://www.baeldung.com/java-validate-url Just want to avoid that we build our own url validation |
Yeah. To Uri should work. public void testing() {
Exception exception = assertThrows(URISyntaxException.class, () -> {
var url_1 = new URL("https://").toURI();
});
} |
A different question is: Why not just accept URIs instead of URLs as parameter. |
I don't think this makes sense. For example, this
is a valid URI as per RFC 3986 (it's a URN, which is also a type of URI). However, it is not a URL. URIs are supersets of URLs and include much more than just URLs. However, we only want to support URLs (and not even all types of URLs, e.g. not file URLs) |
But than at least we are not consistent |
But the marked usage of type URI makes kind of sense there because a few lines blow the HTTPRequest builder expects an URI. Otherwise you would create an URI there, convert it to URL and then convert it back to the supertype URI for use. |
Add authority check for forUrl to get a VaasClientException if it is a client side mistake