Open troelsarvin opened 2 years ago
I appreciate your concern on this, and I agree that Isahc ought to handle this better. However, this would be a breaking change and would require a 2.0 release at the very least. The problem is that the http
crate is part of Isahc's public API -- isahc::Request
is just a re-export of http::Request
, so with the current design we can't choose whether or not http::Uri
is used. Only the http
crate can make that determination.
For an example that demonstrates this better:
use isahc::Request; // remember this is just `http::Request`
fn main() {
let request = Request::get("https://sejs-svejbækbiler.dk/")
.body(())
.unwrap(); // panics here with `http::Error(InvalidUri(InvalidUriChar))`
// before any Isahc code even runs
isahc::send(request).unwrap();
}
I see only two possible ways that we could handle this:
If we could push for the first approach to get resolved then that would be most ideal, since we could fix this without breaking any existing code out there using Isahc. The second approach may have its own benefits, but also some drawbacks as well as requiring a significant API redesign.
When the http crate was first introduced, it was under the hope of being a common "interface crate" that would be generic enough that both HTTP clients and servers could adopt it and allowing the use of interoperable types between otherwise independent crates. This theoretically allows for things such as middleware to be written in an entirely generic way that would allow it to be used with any HTTP client or server instead of being tied to a specific implementation. This sounded like a noble goal, and Isahc was an early adopter those years ago when it was still young. In practice, I am not sure how well this vision materialized and may be worth some investigation.
As far as the URI/URL distinction, last time I looked that was a crazy rabbit hole as to which is more technically correct per the HTTP spec. In syntax, URLs are strictly speaking, always absolute, but HTTP allows relative URLs (which is a misnomer), so it is a bit looser than just URLs, but it is stricter than URIs which are very broad indeed. Then there's a mostly semantic debate as to what we call these "URIs as allowed by the HTTP spec", which some opt to call URIs (technically correct but not very precise) and others call URLs (more colloquially used, but not strictly correct).
Personally I don't really care about the semantic debate, you can call them URLs or URIs, as long as you're following the HTTP spec. Which http::Uri
does as far as I know, in the most strict reading of the spec.
I've updated https://github.com/hyperium/http/issues/259 pointing here. Meanwhile, I wonder if it were possible to add some From/into functions to isahc, so that a url::Url could be used seamlessly with isahc through automatic conversoins between url:Url and http::Uri?
Strictly speaking, it is not possible to implement conversions between url::Url
and http::Uri
from within Isahc due to the orphan rule; only the crate that defines a type can implement common traits for the type. So either the url
crate or the http
crate would have to implement a conversion from one to the other, though I'd doubt either crate would be willing to do that.
I'm not sure that would even help much in this scenario anyway, since Uri
is still part of the public API and we can't change the http
types to also accept a Url
.
First of all, replace http::Uri
with url::Url
is not acceptable.
The best solution for us is to address it from upstream. I submit PR https://github.com/hyperium/http/pull/565 for that.
Xuanwo's attempt at having a fix accepted in the http crate was not successfull, unfortunately :-(
@sagebind, could it be time for a version 2 of isahc, allowing for the API change?
@troelsarvin I'm not currently planning on making this breaking change, even though technically yes, it could be done as part of version 2. This change would not be insignificant as it would require removing the http
crate from the Isahc public API, which currently forms the backbone of Isahc's API. I'm just not sure that the juice is worth the squeeze to upend the main crate API in order to resolve this issue right now, or if the effort required is available.
All I can say is that the changes for version 2 are already planned, and this isn't in that list.
isahc's reliance on http::Uri makes it cumbersome to work with URLs which have hostnames with non-ASCII characters. I suggest that isahc switch to using url::Url instead of http::Uri.
Motivations:
The following code shows the current state of affairs in some crates: