Skip to content
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

Remove request body type param #1154

Closed
wants to merge 3 commits into from
Closed

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Jul 11, 2022

Fixes #1110

This removes the request body type param from everything (Router, FromRequest, RequestParts, etc...). The goal being to improve usability and solve issues like #1110.

The general approach is to implement Service like this:

impl<B> Service<Request<B>> for Router
where
    B: http_body::Body,
{
    fn call(&mut self, req: Request<B>) -> Self::Future {
        let req = req.map(|body| hyper::Body::wrap_body(body));
        // ...
    }
}

This allows us to accept any impl http_body::Body on the outside but convert that to a hyper::Body on the inside.

We need to accept any impl http_body::Body instead of only hyper::Body to support middleware that change the request body (such as RequestBodyLimit).

I also considered using BoxBody rather than hyper::Body but that means users cannot extract Request<hyper::Body> which I think would confuse many users.

Note that hyper::Body:wrap_body isn't shipped yet. I have made a PR to hyper that adds that. If hyper decides not to merge that then our only option is to use BoxBody. We need that to construct a hyper::Body from a generic impl http_body::Body.

This is also a win in another elsewhere because it removes a type param from FromRequest and RequestParts. In #1121 and a WIP fix for #1142 I'm adding two more type params to those types. So by removing the body we'll only have two type params instead of three.

TODO

  • Update docs
  • Update changelog, including guide on how to upgrade

use http_body::Body as _;

#[doc(no_inline)]
pub use hyper::Body;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this makes hyper a public dependency of axum-core. I think thats a little unfortunate but its required to make impl FromRequest for Request<hyper::Body> work. I think thats an okay trade-off.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also does mean that any crate that depends on axum-core now also depends on hyper. Thats pretty unfortunate 🤔 But yeah not sure there is a way around it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One solution would be to keep the type param on FromRequest and RequestParts but that would mean they get three type params in 0.6 😬

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps its also worth considering adding our own body type to axum-core, to avoid the dependency on hyper. I believe hyper's Body will be moved into hyper-utils for 1.0 and we don't wanna have a public dependency on hyper-utils. So we'll probably need our own body type eventually.

Though we have to consider compatibility with things like reqwest. I probably should be possible to extract the body with axum for send it somewhere else with reqwest or a hyper::Client.

@@ -55,7 +58,7 @@ where
// THE SOFTWARE.
pub(crate) async fn to_bytes<T>(body: T) -> Result<Bytes, T::Error>
where
T: Body,
T: http_body::Body,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: Wo can remove this method and just use the one in hyper.

@davidpdrsn
Copy link
Member Author

I wrote about why hyper::Body::wrap_body is necessary here.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never used a non-default body type so I don't really have a good overview of what kinds of things this makes harder / pessimizes (if any). If this doesn't have any serious downsides I'd be in favor of course :)

}

/// The type used with [`FromRequest`] to extract data from requests.
///
/// Has several convenience methods for getting owned parts of the request.
#[derive(Debug)]
pub struct RequestParts<B> {
pub struct RequestParts {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, why is there a request_parts module, but RequestParts is defined elsewhere?

@davidpdrsn
Copy link
Member Author

The main downside is that it locks us into one body type for extractors. I think a lot depends on the hyper/hyper-util split for their 1.0 and where stuff ends up. We cannot publicly depend on the body from hyper-util and hyper probably won't have a single body type.

@davidpdrsn
Copy link
Member Author

I'm gonna close this. All this is gonna have to change anyway when we get closer to hyper 1.0 so I'd rather deal with it then. See #1110 (comment).

This PR is also too large to review so if/when we do this we have to do it in smaller chunks.

@davidpdrsn davidpdrsn closed this Aug 22, 2022
@davidpdrsn davidpdrsn deleted the remove-request-body-generic branch August 22, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Middleware that change the request body have poor usability
2 participants