Consider using http::{Request, Response} for easier conversion

[MarijnS95]

The http crate was added in 398d3a2, at which point the HttpRequest and HttpResponse types are mostly built up from http types. Would it be (un)reasonable to switch to http Request and Response types entirely? This, in addition to being a more commonly used type, already has builtin conversions for ureq and reqwest:

algesten/ureq#669
https://docs.rs/reqwest/latest/reqwest/struct.Request.html#impl-TryFrom%3CRequest%3CT%3E%3E-for-Request

But perhaps I am overlooking some shortcomings with the http types that make this infeasible? Thanks for considering this at least (and for the record, I'm willing to help out with the conversion)!

I could not find http conversions for the curl crate, unfortunately.

[MarijnS95]

For completeness, unfortunately reqwest does not currently provide a reqwest::Response -> http::Response conversion yet, but it's being "worked on":

seanmonstar/reqwest#1954 (comment)

[ramosbugs]

Hey @MarijnS95,

My concern with using http's request/response types is that they include a lot more functionality than the barebones types defined by this crate. Different HTTP client implementations may not have full feature parity, and that means this crate could end up setting some value in http::Request that isn't supported by a particular client, and gets silently ignored. Defining simple HttpRequest/HttpResponse structs in this crate makes it obvious to users which fields they need to care about when writing their own HTTP clients, including when adding shims around the popular ones for telemetry, to support off-spec behavior, etc.

That said, implementing From<HttpRequest> for http::Request and From<http::Response> for HttpResponse in this crate should be reasonable as an ergonomics improvement. That doesn't change the request/request_async interface, which means it should still be easy for HTTP client implementors to understand what fields they need to care about, while also making it easier for users relying on clients that already use http's types. It also avoids breaking changes.

Thoughts?

[MarijnS95]

I wouldn't be too afraid of that happening. Both of the structs seem to solely map what's in a HTTP request/response (method, uri, version, headers, response code, body), plus an Extensions struct holding unique types used to communicate extra (optional, afaik) features between the caller (oauth2) and the implementation. If oauth2 doesn't care about any extension, it simply doesn't read/write them.

If a client-implementer does care about an extension, they could insert/extract one before/after going down into the HTTP client implementation?

---

If that's something you want to avoid at all costs, I believe we could implement those From<> conversions in one place for library consumers and the provided backend implementations.

Regardless I started replacing the types last week just to see what it'd look like (don't remember, would have to check if this is the most recent push):
main...Traverse-Research:oauth2-rs:http-interop

One significant difference is the use of the url crate vs HTTP's builtin Uri type.

[ramosbugs]

I'm leaning toward accepting this change at the same time as #237 since they both require a major version bump. After looking closely at http::Request and http::Response, I agree that they don't contain much additional information that would cause parity issues between HTTP client implementations.

I also don't think we need to wait for reqwest to implement http::Request -> reqwest::Request and reqwest::Response -> http::Response conversions since this crate can do that within oauth2::reqwest::[async_]http_client. Once reqwest supports the conversion(s), we can just remove the redundant code from this crate. However, we do need to wait for reqwest to support http 1.0 (see #237), since having reqwest::Error in the public interface for oauth2::reqwest::[async_]http_client means this crate needs a major version bump whenever reqwest has one.

One significant difference is the use of the url crate vs HTTP's builtin Uri type.

This is less than ideal, but probably not a blocker. In particular, having to use http::Uri in each request introduces a new source of fallible conversions. I think we should continue using url::Url within types like AuthUrl that must specifically be URLs and not other types of URIs, and no infallible conversion seems to be planned within hyper (see hyperium/hyper#1219). I assume that any stringified url::Url should be parsed successfully as a http::Uri, and we can just return oauth2::RequestTokenError::Other in the unlikely event that http::Uri parsing fails.

[MarijnS95]

The diff linked above should compile now for most/all backends, but I still have to:

• Look into the Uri vs Url story more closely;
• Make the examples and tests compile again (mostly comparisons between Url types);
• Few other bits to clean up and improve.

For now a version bump should not be needed, we can incorporate both as separate PRs (but land them in the same breaking oauth2 release) 👍. For example ureq 2.9.1 just released with support for both http 1.0 as well as http 0.2.

[ramosbugs]

One other breaking change I think we should make in the same release is to define AsyncHttpClient and SyncHttpClient traits instead of using FnOnce, which someone previously found confusing while attempting to write a custom HTTP client wrapper. For convenience, we can impl those traits automatically for function types that already satisfy the current FnOnce trait bounds.

[ramosbugs]

I've started merging breaking changes into main for the future 5.0 major release, so now would be a good time to start making these changes

[ramosbugs]

This is now released in 5.0.0-alpha.1. If you can, please try it out and let me know if you find any issues that should be fixed before stabilizing the 5.0 API. There's an Upgrade Guide to help.

[ramosbugs]

I assume that any stringified url::Url should be parsed successfully as a http::Uri, and we can just return oauth2::RequestTokenError::Other in the unlikely event that http::Uri parsing fails.

Turns out this assumption doesn't always hold (seanmonstar/reqwest#668), but the http::request::Builder will return an http::Error that we'll handle when preparing each HTTP request.