Consider making it easier to construct ErrorResponse from a non-IntoResponse error

[jplatte]

Feature Request

Motivation

People ask about short-circuiting from handler functions with non-IntoResponse errors somewhat frequently.

We already have ErrorResponse in axum::response for returning different error types from a handler function conveniently. However, right now it is quite annoying to construct one from a non-IntoResponse error type (see alternatives).

Proposal

Add

impl ErrorResponse {
    fn internal_server_error(message: impl Display) -> Self {
        (StatusCode::INTERNAL_SERVER_ERROR, message.to_string()).into()
    }
}

to allow users to write

try_thing().map_err(ErrorResponse::internal_server_error)?;

Alternatives

1. Expose a free function, same thing but less verbose

   try_thing().map_err(internal_server_error)?;

2. Do nothing. One can already do

   try_thing().map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?;

Open questions

Should the internal_server_error function log the error as well?

[Lachstec]

Hi, is this still relevant?

I found this and read up on the previous PR and would agree that the Proposal here would be a nice quality of life improvement for most users to safe some characters when using the library. Only aspect that im not sure of is the standing question about whether or not to log the error.

[jplatte]

Yeah, this is still relevant. I guess now that we have more maintainers / regular contributors, it would be nice to hear everybody's opinion though. @yanns @mladedav @SabrinaJewson how do you feel about this proposal, the alternatives and the open question of whether to log something?

[SabrinaJewson]

Another alternative is to expose a struct InternalServerError:

pub struct InternalServerError<T>(pub T);
impl<T: Display> IntoResponse for InternalServerError<T> {
    // …
}

Thus users would write .map_err(InternalServerError)? instead. I argue this maybe fits better with axum’s current model of “all responses come from an IntoResponse”.

Another thing to consider is whether it should use Display or Error; if it used the latter, then it could instead print out the error chain, i.e. build the body with a loop like this:

let mut body = format!("{error}");
let mut e: &dyn Error = &error;
while let Some(new_e) = e.source() {
    e = new_e;
    write!(body, ": {e}").unwrap();
}

which would likely be more helpful for tracking down the root cause of the error.

And given that Json doesn’t currently log errors for internal server errors, I don’t think this should either. I’m not opposed to adding logging, but it should be consistent with Json.

[Lachstec]

I like the approach from @SabrinaJewson. Regarding the logging of errors I would also suggest that we don't implement it for now to keep consistency. Maybe open a separate Issue and discuss whether logging is desired and add it later?

But that is just my humble opinion, I am open to any suggestions 😀

[yanns]

another addition based on the idea from @SabrinaJewson.
Showing the root cause of an error can potentially leak from important information, like libraries being used, that can be used by malicious hackers.
We could have a flag to show or not the root cause.
Or we could use the compilation mode:

• in debug mode, we show the root cause of the error, as it's useful for the developer
• in release mode, we only show the error message.

[SabrinaJewson]

I think that showing the error at all is probably a leak; public-facing services should use other methods to ensure that 500 responses have no body (such as middleware or a custom error type). Showing the full chain makes it clear that this is to be used for development or internal services only.

[Lachstec]

I've tried to put the discussed approach into a PR, would like suggestions on how to refine it further