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.

Sign up for GitHub

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

Jump to bottom

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

Open
1 task done
reivilibre opened this issue Oct 24, 2023 · 5 comments · May be fixed by #2468
Open
1 task done

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

reivilibre opened this issue Oct 24, 2023 · 5 comments · May be fixed by #2468
Labels
A-axum-core I-needs-decision Issues in need of decision.
Milestone
0.9

Comments

@reivilibre
Copy link

reivilibre commented Oct 24, 2023 •
edited
Loading

  • 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
Copy link
Member

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.

@davidpdrsn davidpdrsn added I-needs-decision Issues in need of decision. A-axum-core labels Oct 25, 2023
@davidpdrsn davidpdrsn added this to the 0.7 milestone Oct 25, 2023
@jplatte
Copy link
Member

jplatte commented Oct 25, 2023 •
edited
Loading

  1. 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.

@davidpdrsn davidpdrsn modified the milestones: 0.7, 0.8 Nov 10, 2023
@davidpdrsn davidpdrsn linked a pull request Dec 30, 2023 that will close this issue
Draft
4 tasks
@jplatte jplatte modified the milestones: 0.8, 0.9 Nov 30, 2024
@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 27, 2024

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
Copy link
Member

jplatte commented Dec 28, 2024

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
Copy link
Collaborator

mladedav commented Jan 2, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-axum-core I-needs-decision Issues in need of decision.
Projects
None yet
Milestone
0.9
Development

Successfully merging a pull request may close this issue.

Fix IntoResponse for tuples overriding error response codes tokio-rs/axum
5 participants
@Turbo87 @davidpdrsn @jplatte @mladedav @reivilibre