Add IntoResponseParts

[davidpdrsn]

Note: Design has changed quite a bit from the description here. See comments for details.

---

This is a proposal that changes how responses are made by introducing a new trait:

pub trait IntoResponseParts {
    fn into_response_parts(self, res: &mut ResponseParts);
}

Where ResponseParts is:

pub struct ResponseParts { ... }

impl ResponseParts {
    pub fn set_version(&mut self, version: Version) {}
    pub fn set_status(&mut self, status: StatusCode) {}
    pub fn insert_header<K, V>(&mut self, key: K, value: V) {}
    pub fn insert_extension<T>(&mut self, extension: T) {}
}

And these impls

// this makes `impl IntoResponse` work like before,
impl<T> IntoResponse for T
where
    T: IntoResponseParts,
{}

// for tuples
impl<T1> IntoResponseParts for (T1,)
where
    T: IntoResponseParts,
{}

impl<T1, T2> IntoResponseParts for (T1, T2)
where
    T1: IntoResponseParts,
    T2: IntoResponseParts,
{}

// etc...

This means you can return tuples of response parts like (status, headers, body) just like before, but the order no longer matters. So (body, headers, status) also works.

I've also implemented IntoResponseParts for Extension and [(HeaderNameIsh, HeaderValueIsh); N] so (Extension(1), Extension(2), [("content-type", "text/plain")], "Hello, World!") works!

Overall I'm quite happy with this approach. I think its more consistent since Extension works and I like that the order of things in tuples no longer matters. I also like that there is only trait to implement vs the previous IntoResponseHeaders and IntoResponse.

The only downside I see is that users might be confused by having to implement IntoResponseParts but use IntoResponse. We should help by clearly documenting the blanket impl and how that works.

Things to note

• IntoResponse is now sealed, so users will have to implement IntoResponseParts instead.
• http::Extensions doesn't implement IntoResponseParts since we cannot merge extensions.
• Response and http::response::Parts implements IntoResponse and not IntoResponseParts, again because we cannot merge extensions.
• Result<T, E> implements IntoResponse and not IntoResponseParts. This makes Result<impl IntoResponse, E> work. Means you cannot have results in tuples of parts but thats probably fine.
• Headers has been removed. [(HeaderNameIsh, HeaderValueIsh); N] works instead and doesn't require a wrapper type.

Fixes #770

TODO

• Docs, including auditing existing docs and explaining more clearly more things compose
• changelog

Future improvements

• Move Extension into the root because it, like Json, is both an extractor and response part. I'll do that in a follow up PR. Will be re-exported from axum::response and axum::extract like Json. Same for TypedHeader.

[jplatte]

Small overall comment after reading through (only) the description: What I meant in #770 was a trait for just the headers and extensions, i.e. the parts that can added onto. The impls would still work the same way as the ones for tuples containing IntoResponseHeaders right now (i.e. one optional status code at the start, response body or other IntoResponse type at the end).

Maybe this more general approach is the better solution though. I'll look at the code.

[davidpdrsn]

Also gonna add some tests.

[jplatte]

Code looks good, but I'm still not entirely sure whether the arbitrary order of tuple elements is worth it considering the potential confusion / pitfalls around Result or opaque impl IntoResponse types in tuples¹. We'd also lose the static verification of only a single status code and body being set.

¹ in particular, I wouldn't be surprised if some people are currently writing handlers that call other handlers (where the fn signatures are likely -> impl IntoResponse) and override the status code or add headers

[davidpdrsn]

Code looks good, but I'm still not entirely sure whether the arbitrary order of tuple elements is worth it considering the potential confusion / pitfalls around Result or opaque impl IntoResponse types in tuples¹. We'd also lose the static verification of only a single status code and body being set.

I thought about as well. We kinda had the problem previously since one could do (StatusCode::NOT_FOUND, (StatusCode::OK, "")). This would actually work differently with this new setup. The final status would be OK since thats the right-most status. Maybe the order should be reversed 🤔

You're right that it wasn't possible to accidentally override the body previously.

I'm gonna experiment and see if I can find a way around these issues while still being able to put headers and extensions anywhere in the tuples without having to wrap the headers in Headers.

¹ in particular, I wouldn't be surprised if some people are currently writing handlers that call other handlers (where the fn signatures are likely -> impl IntoResponse) and override the status code or add headers

Changing those functions to impl IntoResponseParts should work.

[davidpdrsn]

I think I've cracked it. Now IntoResponseParts is only allowed to insert extensions or headers. It cannot touch the status or response.

Additionally I was able to tweak the impls such that:

• (StatusCode, impl IntoResponse) works like before. The status code overrides the one from the response.
• (T1, ..., Tn, impl IntoResponse) where T1, ..., Tn: IntoResponseParts can be used to insert headers and extensions but not the status
• (StatusCode, T1, ..., Tn, impl IntoResponse) where T1, ..., Tn: IntoResponseParts can be used to insert headers and extensions and the status.

Thus a status code must be the first element in the tuple, and there can only be one status code. The body must be the last element. Anything in the middle must implement IntoResponseParts and cannot touch the status or body.

How that doesn't give conflicting impls is magical to me, but it works! Result<impl IntoResponse, E> works like before.

I think this addresses the footguns.

See for a bunch of examples https://github.com/tokio-rs/axum/blob/extension-into-response/axum/src/response/mod.rs#L89

[davidpdrsn]

Still have to update the docs but I'll do that if you think the code makes sense.

[davidpdrsn]

Kinda surprised no one has asked why Version didn't implement IntoResponse earlier 🤔

[jplatte]

Why would you ever want to override that? I feel like the http version should always be set by hyper.

[davidpdrsn]

Not sure that you need to actually. But tonic does set it, although I think hyper would handle that as well.

[davidpdrsn]

The rule with these impls is that if things can be appended to a response, then they should implement IntoResponseParts and not IntoResponse. That only applies to headers and extensions.

Extension implements IntoResponseParts in another file.

[davidpdrsn]

I'm thinking if we should even implement tower::Layer for Extension? Then we wouldn't need AddExtension layer and users would only deal with Extension. But maybe thats odd? 🤔

[jplatte]

Oohh, nice idea! I don't really see why not. I'm personally going to continue using ServiceBuilderExt::add_extension either way, but as this would reduce axum's API surface a little bit, I'd be in favor.

[jplatte]

To be clear, I think this isn't the only instance where it was worded this way.

[jplatte]

That's all my nitpicks voiced.

[jplatte]

Should we open a separate issue about whether or not to keep the Version impl? Or are you certain enough that it's useful?

[davidpdrsn]

Yeah lets do that 👍