`IntoResponse` implementation for `(StatusCode, R) where R: IntoResponse` overrides error status codes and might be a footgun

[reivilibre]

• [x] I have looked for existing issues (including closed) about this

Bug Report

Version

├── axum v0.6.20
│   ├── axum-core v0.3.4
│   ├── axum v0.6.20 (*)

(but I checked the source code for the latest version on docs.rs to notice this)

Crates

Description

I'm looking at the code in async fn hello() below.

pub async fn serve(bind: SocketAddr) -> eyre::Result<()> {
    let router = Router::new()
        .route("/", get(hello));

    info!("Listening on {bind:?}");
    axum::Server::try_bind(&bind)
        .context("could not bind listen address")?
        .serve(router.into_make_service_with_connect_info::<SocketAddr>())
        .await?;

    Ok(())
}

async fn hello() -> impl IntoResponse {
    let mut map = HashMap::new();
    map.insert((1, 2), 3);
    (StatusCode::IM_A_TEAPOT, Json(map))
}

The general idea is: we're emitting a JSON payload with a custom status code. But along the lines, something has gone wrong and the JSON serialisation fails (in this case: because the map has keys which are not strings and are not coerced into strings by serde_json).

Had I just used Json(map) as the return value, then I'd get a 500 Internal Server Error response — very reasonable!

However, instead I am getting the originally desired status code but with a text error message:

$ curl -i http://127.0.0.1:8080
HTTP/1.1 418 I'm a teapot
content-type: text/plain; charset=utf-8
content-length: 20
date: Tue, 24 Oct 2023 21:35:49 GMT

key must be a string

I find this a little bit unsettling and like it might turn out to be a bit of a footgun. It's frankly not very likely with emitting Json (but not impossible) but when writing a custom IntoResponse to render a template or something like that, it seems quite reasonable to anticipate errors!

On the other hand, I don't know how/if you can fix this with the current API surface. But it felt like it was worth documenting.

(I should note this is entirely academic only and hasn't caused me any trouble in a real project or anything, so of course don't waste too much time on me!)

[davidpdrsn]

Hm yeah that is a bit of a footgun for sure.

I think we could

1. Leave things as is but document it and provide a type that doesn't override 5xx status codes.
2. Change it to not override 5xx status codes. This would break middleware that "catch" server errors and change the status code but I don't know if that's a common thing to do. We'd probably also have to provide a OverrideAllStatusCodes(StatusCode) sorta type if you want the old behavior.

Of these I'm leaning towards 2. Feels like it helps users "do the right thing" while letting you customize things if necessary.

[jplatte]

3. Communicate "IntoResponse failed" through a response extension and check that in the given impl. This would also work with "handler returned Result::Err" (though probably should be a separate unit type for the response extension), which people have wanted to catch in middleware pretty often.

[Turbo87]

just brainstorming here: would it make sense to extract a IntoResponseResult (or IntoFallibleResponse or ...) trait, where anything that implements IntoResponse automatically implements IntoResponseResult<Error = Infallible>? then we could keep combining responses until one fails, in which case the error response wins and we return that without caring about the other tuple contents 🤔

[jplatte]

It should work, but I'm uncertain whether it is better than what was proposed above. I guess it is in a way less magical, but we need two new traits (also for response parts), plus docs explaining which one to implement when.

[mladedav]

We could also change IntoResponse::into_response to fn into_response(self) -> Result<Response, Response>. It would be closer to IntoResponseParts::into_response_parts(self, res: ResponseParts) -> Result<ResponseParts, Self::Error>; and we could return directly whatever the implementation considers an error.

Note that in (StatusCode, impl IntoResponseParts, impl IntoResponse), any error the intermediate IntoResponseParts return is directly returned (discarding the original status code and any previously resolved headers/extensions). Having something similar for IntoResponse seems like something we might want.