http_interop: Implement Request conversion for http::request::Parts

[MarijnS95]

Fixes #656
Closes #501
Closes #520

I found the current conversion for http::request::Builder to be rather useless when the http crate and various crates providing http interfaces like oauth2 are designed to provide an http::Request directly, and there being no way to convert from a http::Request back into its http::request::Builder. That, together with strange infallible defaults instead of providing TryFrom make the current implementation cumbersome to use.

Fortunately http provides http::Request::into_parts() to get back a Parts structure (which is wrapped in Result<> inside http::request::Builder as sole member!) together with the request body (a generic type) which the user can manually pass to send_string(), send_bytes() or call() if there's no data.

Implement a From<http::request::Parts> for ureq::Request to support this case, making ureq finally capable of sending http::Requests.
(Note that, despite exclusively consisting of Result<Parts>, http::request::Builder has no constructor from Ok(Parts {..}) which would have also facilitated this use-case somewhat)

TODO

Some more ideas and things I found while working on this crate:

• Use doc_cfg when building for docs.rs: Enable all features in documentation and utilize doc_auto_cfg #670;
• Enable all features when building for docs.rs to make the above useful (for discovery);
• Remove unnecessary UTF-8 conversions and their fallibility: http_interop: Remove requirement for header values to be UTF-8 #672;
• Wrap intradoc references properly in backticks;
• Replace From<http::request::Builder> with TryFrom<> (breaking change!).
• Or better: remove this conversion for the Builder altogether, see below!

[kade-robertson]

I agree, poor decision on my part to not implement as a TryFrom originally (and missing that Parts was a thing).

[MarijnS95]

@kade-robertson great!

By the way, apologies for the commit/PR message. I always write them very business-y and to the point to get the point across and justify the change, but realize now that it may not come across nicely on the original author.

Let's wait for @algesten to confirm whether we can make such a breaking change now or in the future.

We can anyway:

• have the current PR change
• add a TryFrom
• mark the current From as deprecated (and remove it in the future)

But then I'm not exactly sure how much value there is in providing a TryFrom. Your example seems to indicate using the http builder pattern directly, but I've found it more useful for crate interop (some crate generates a request with the builder, and gives the built request to the caller so that they can decide what implementation to use). I have anyway implemented it on a WIP branch in terms of .body(())?.into_parts() via the newly added From<http::request::Parts>. Thoughts?

[kade-robertson]

@kade-robertson great!

By the way, apologies for the commit/PR message. I always write them very business-y and to the point to get the point across and justify the change, but realize now that it may not come across nicely on the original author.

No worries! It's a very valid point -- not sure why I didn't do it that way originally.

But then I'm not exactly sure how much value there is in providing a TryFrom. Your example seems to indicate using the http builder pattern directly, but I've found it more useful for crate interop (some crate generates a request with the builder, and gives the built request to the caller so that they can decide what implementation to use).

You're totally right! When I implemented this before I even left a message in the PR that I wasn't really sure what the purpose would be. I admittedly had some bias towards the Response conversion since I needed that for my own projects and I gave less thought to the Request aspect.

I have anyway implemented it on a WIP branch in terms of .body(())?.into_parts() via the newly added From<http::request::Parts>. Thoughts?

I took a look and this branch looks good (if it's desired) to me, though I guess what the exact plan to update this isn't decided on yet.

Maybe there's some way to take advantage of the fact that this was already a non-default feature? Have the "fixed" implementations exposed under a second feature and deprecate the first one until a ureq 3.x is actually desired? I'd hate to force a breaking change over an optional feature if it can be avoided. Not sure if other libraries have dealt with similar things in the past.

[MarijnS95]

Tackling the doc-issues in #670 🙂

[MarijnS95]

I admittedly had some bias towards the Response conversion since I needed that for my own projects and I gave less thought to the Request aspect.

Yeah that one seems to work fine though I have some doubts/suspicions about the UTF-8 "fallibility" part...

Have the "fixed" implementations exposed under a second feature and deprecate the first one until a ureq 3.x is actually desired?

This (adding a non-conflicting new trait implementation and #[deprecated]'ing an existing one) is not a breaking change and would actually be allowed semver-wise. Removing the symbol would.

[algesten]

H! @MarijnS95 welcome to ureq!

Instead of commenting on the sub-issues, I'll do it here.

Thank you so much for taking the time to look at this. I think all of this makes perfect sense, and I agree we should have this instead of the current variant of http interop. However, I would like to avoid a breaking change.

What do you think about deprecating the old http-interop and placing this under a new feature flag?

[MarijnS95]

@algesten glad to be here! We've been using ureq for a while now as a nice and light-weight request crate without all the async tokio bits and bobs that would clobber our dependency tree and build times, many thanks for providing and maintaining it!

It's only one implementation that doesn't really seem to be useful (IMO), the new trait conversions can all be added without adding breaking changes.

I don't think we need a new feature for this, and renaming http-interop to http (do we have a proper justification for that?) is stricly a separate change we could do.

[MarijnS95]

As it turns out a #[deprecated] annotation has no effect on trait implementations.

(Not sure why though... it could be caught in type-specific scenarios, just not in dyn scenarios?)

[algesten]

I'm probably being stupid, but apart from the fact that turning Builder -> Request is a fallible operation, why is TryFrom needed to improve the ergonomics here?

[MarijnS95]

@algesten a http::request::Builder contains Result<Parts>. If there's an Err in there the conversion becomes totally useless/meaningless, and we could represent that with TryFrom.

[algesten]

@MarijnS95 sure, I get that.

I think like this: 1) What could make Builder go wrong? 2) Is it likely to go wrong? 3) Is there another code path available if you need to detect this error? (Like completing the Builder -> Request manually and after that convert to ureq).

My goal here is to avoid a breaking change, but still have an answer for what to do. I agree TryFrom is the correct choice here, but since we started with From, can we stay with that without making some use cases impossible?

[MarijnS95]

@algesten sure thing.

1. Adding the conversion for Parts is a non-breaking change and can live in coexistence with the existing From<request::Builder> for Request;
2. The current implementation is suboptimal as it doesn't clearly make the user aware of an Err (and I don't see an easy way to check this without moving the builder by adding a body using .body(()) -> Request<()> and completing the Builder);
3. Unfortunately From implies TryFrom, so we cannot add a manual TryFrom implementation and deprecate the From implementation (also because #[deprecated] doesn't work on trait implementations).

In the end all we can do is extend the current doc-comment to explain the situation. If the user has a Builder and want to check the error they're better off with something like:

let request = builder.body(())?; // Check the error
let (parts, ()) = request.into_parts();
let request: ureq::Request = parts.into(); // Use the new conversion added in this PR

[algesten]

@MarijnS95 This sounds good. At the moment we continue to live with the suboptimal situation of From. I also will start a FUTURE.md file where we list improvements we want to make that would require a major version.

Would you mind adding the code you show above into the rust doc?

[MarijnS95]

@algesten yeah that was the intent. I've replaced the current documentation entirely as http::request::Builder does not have an "incomplete" state as described, by default it always contains a GET / request and it is only Err that would get the current impl From to return GET https://example.com.

And added a quick FUTURE.md noting the above idea just so that we don't forget, please update that as you see fit.

[algesten]

Looks good! Let's merge it!

[MarijnS95]

@algesten are there any release plans for a semver-compatible minor/patch release with these changes? I'd like to start cleaning up some user crates with the new conversion methods 🎉

[algesten]

@MarijnS95 yes!

I'm waiting for #679 but maybe that can wait. 🤔

[MarijnS95]

Cool!

Not too much hurry in hindsight, it seems the crates I'm willing to apply this to also have other outstanding problems that make this harder to achieve.