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

Implement IntoResponse for Response<()> and response::Parts #950

Merged
merged 5 commits into from
Apr 29, 2022

Conversation

neoeinstein
Copy link
Contributor

@neoeinstein neoeinstein commented Apr 19, 2022

Add a simple implementation to use Parts and Response<()> as a source for IntoResponse.

This doesn't fit neatly into the IntoResponseParts paradigm because Parts already contains a status code. The implementation itself is rather trivial.

Motivation

I have a library where I generate some generic template responses using http::response::Builder and wanted to be able to make it one step easier to use those templates (often built with a body of (), and then substituted with the intended body) as responses. This makes it so that the template can be built and specified next to the body to be placed into the response.

Solution

Add impls for Parts and Response<()>. The new Response<()> impl discards the existing () body and uses the provided body to construct the response and then moves the headers and status into the new response.

@jplatte
Copy link
Member

jplatte commented Apr 19, 2022

I think the Parts one might make sense, but

  • it should be done in the macro below, in an equivalent manner to
    impl<R, $($ty,)*> IntoResponse for (StatusCode, $($ty),*, R)
  • it should propagate response extensions as well

I think the Response<X> one is too much of a footgun though.

@davidpdrsn
Copy link
Member

davidpdrsn commented Apr 19, 2022

Hm an issue with this is that we cannot merge two Extensions. So either the Extensions from the Parts or the Response needs to be dropped. I'm a little worried about that. I think in general we should be careful to not remove stuff from either part of the response. That is at least how things work in axum today.

Maybe we should re-open hyperium/http#525?

@neoeinstein
Copy link
Contributor Author

neoeinstein commented Apr 19, 2022

Yep. The Extensions and Version were both things I looked at, but it's difficult to merge in the extensions as mentioned in that linked issue, as there's no way to iterate and extend at the moment.

I can see Response<X> being a potential footgun. It's a bit difficult because I can't provide an impl for http::response::Response<R> where R: IntoResponse, as that would conflict with the existing one that expects a fully formed http_body::Body. This was my workaround for that, but even just having Parts would make it one step easier.

One potential alternative for that is to impl this for (Response<()>, R) instead of (Response<X>, R). That would make it clear that the body of the left side has no value.

@davidpdrsn
Copy link
Member

I think this looks good as is but I don't think we should merge it until hyperium/http#546 has landed.

1 similar comment
@davidpdrsn

This comment was marked as duplicate.

@davidpdrsn davidpdrsn added S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. A-axum-core labels Apr 19, 2022
@davidpdrsn davidpdrsn changed the title feat(axum-core): add IntoResponse for http::response types Implement IntoResponse for Response<()> and response::Parts Apr 24, 2022
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

I think this is good to go now :)

@davidpdrsn davidpdrsn requested a review from jplatte April 29, 2022 15:33
@davidpdrsn davidpdrsn removed the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Apr 29, 2022
@jplatte
Copy link
Member

jplatte commented Apr 29, 2022

What about extra impls for (Parts | Request<()>, $(impl IntoResponseParts)+, impl IntoResponse)?

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.

Changes look good either way.

@davidpdrsn
Copy link
Member

Good idea! I filed #979. I'll look into it.

@davidpdrsn davidpdrsn merged commit 6e18350 into tokio-rs:main Apr 29, 2022
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.

4 participants