Fallible handlers for custom rejections

[SabrinaJewson]

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

Related: #1116

Feature Request

Motivation

axum-extra’s WithRejection is a great tool for handling custom rejections, but what I would really like is if I could know at compile time that there will be none of Axum’s default rejections returned by my service.

Proposal

The central concept of this proposal is that of the fallible handler. We first remove the Rejection: IntoResponse bound, and add the following trait:

pub trait ErrorHandler {
    type Error;
}
impl<E> ErrorHandler for E {
    type Error = Self;
}
pub struct DefaultErrors(Infallible, [()]);
impl ErrorHandler for DefaultErrors {
    type Error = Infallible;
}

This allows for specialization over whether something uses the default approach of plaintext errors: because DefaultErrors is ?Sized and Sized is a #[fundamental] trait, Rust knows that it cannot overlap with E: Sized. We then modify the Handler trait:

pub trait Handler<T, S, E: ?Sized + ErrorHandler = DefaultErrors>: Clone + Send + Sized + 'static {
    type Future: Future<Output = Result<Response, E::Error>> + Send;
    fn call(self, req: Request, state: S) -> Self::Future;
}

The old impls are kept the same (other than requiring Rejection: IntoResponse), but we add new ones for handlers with custom error handlers:

impl<F, Fut, S, Res, E, M, T1, T2> Handler<(M, T1, T2), S, E> for F
where
    F: FnOnce(T1, T2) -> Fut + Clone + Send + 'static,
    Fut: Future<Output = Result<Res, E>> + Send,
    S: Send + Sync + 'static,
    Res: IntoResponse,
    T1: FromRequestParts<S, Rejection: Into<E>> + Send,
    T2: FromRequest<S, M, Rejection: Into<E>> + Send,
{ /* … */ }

Functions like routing::get are modified to take in E: ?Sized + ErrorHandler as a generic parameter. Then, as long as the chosen E does not implement IntoResponse, type inference will be able to figure out whether a given handler is supposed to use custom error handling or not.

The expectation here is that users define their own error type, implement From for whatever rejection types they want to deal with and return -> Result<impl IntoResponse, MyError> from handlers. They could do something like impl<E: Display + IntoResponse> From<E> to automatically cover existing rejection types; this provides extra reason for ErrorHandlers to not implement IntoResponse, as such an implementation may conflict with the blanket implementation.

One option is to stop here, and convert ErrorHandlers into full Responses at the handler level. This would involve renaming ErrorHandler to ErrorHandlerTypes, making a new trait ErrorHandler with an into_response method and implementing it for Infallible, and adding type Error: ErrorHandler to ErrorHandlerTypes and then converting everything to a Response inside Route.

But a disadvantage of this is that error handling can’t be done at the level of middleware. axum already has tools for dealing with this – HandleError – and it would be nice if we could hook into that. So, I suggest we go further, replacing the E = Infallible parameter of MethodRouter with E: ?Sized + ErrorHandler = DefaultErrors, also adding the same parameter to Router, and forwarding that to handlers – services are similarly allowed to be fallible, with the error type as E::Error. Router will still only implement Service when E = DefaultErrors, to prevent accidentally handing the errors to Hyper.

Fallbacks

Rejections aren’t the only kind of response that Axum generates automatically; default fallbacks are another, and if I’m adding ten MethodRouters to my service I would like assurance that they all have custom fallbacks.

One way to achieve this is to add rejection types NotFound and MethodNotAllowed and modify ErrorHandler like so:

pub trait ErrorHandler {
    type Error;
    fn not_found() -> Result<Response, Self::Error>;
    fn method_not_allowed() -> Result<Response, Self::Error>;
}
impl<E: From<NotFound> + From<MethodNotAllowed>> ErrorHandler for E {
    type Error = Self;
    fn not_found() -> Result<Response, Self::Error> {
        Err(Self::from(NotFound))
    }
    fn method_not_allowed() -> Result<Response, Self::Error> {
        Err(Self::from(MethodNotAllowed))
    }
}
pub struct DefaultErrors(Infallible, [()]);
impl ErrorHandler for DefaultErrors {
    type Error = Infallible;
    fn not_found() -> Result<Response, Self::Error> {
        Ok(StatusCode::NOT_FOUND.into_response())
    }
    fn method_not_allowed() -> Result<Response, Self::Error> {
        Ok(StatusCode::METHOD_NOT_ALLOWED.into_response())
    }
}

The default fallbacks of Router and MethodRouter will call E::not_found() and E::method_not_allowed() respectively.

The disadvantage of this approach is that your custom error type is required to implement From<NotFound> + From<MethodNotAllowed> even in cases where you plan on adding custom fallbacks to your routers. On the other hand, with this design handle_error layers mostly eliminate the need for custom fallbacks in the first place. At the same time this creätes asymmetry in how custom fallbacks look for when an app is using default error handling versus custom error handling.

A different option is to add a third generic parameter to Router and MethodRouter which is either DefaultFallback or CustomFallback. If the routers are used with an ErrorHandler other than DefaultErrors, then if DefaultFallback is used, it requires the bounds E: From<NotFound> and E: From<MethodNotAllowed> respectively; otherwise, .fallback is required to be called. If the routers are used with E = DefaultError, .fallback is not required, and calling it does not change the type.

For now, I’m leaning toward the first approach for its simplicitly, but I’m unsure.

Other cases of default errors

There are other instances where Axum generates its own errors, for example in Json’s IntoResponse implementation. It’s possible we could make IntoResponse into a fallible trait, but I’m not sure if it’s worth the breakage since basically only Json benefits from that, and JSON serialization errors are extremely rare anyway. There might be some others I’ve missed too, I’m unsure.

[mladedav]

Functions like routing::get are modified to take in E: ?Sized + ErrorHandler as a generic parameter. Then, as long as the chosen E does not implement IntoResponse, type inference will be able to figure out whether a given handler is supposed to use custom error handling or not.

If we went with this, I think we could remove the impl IntoResponse for Result and then this wouldn't be an issue, right?

Also I don't think I've noticed mentioned how would be the ErrorHandler::Error converted into an actual HTTP response, shouldn't that be a method on the trait? Or is there an expectation to use a HandleErrors layer to recover these? In either case, why are NotFound and MethodNotAllowed special-cased and aren't simply errors where we could even require ErrorHandler: From<NotFound>?

This seems sketched out in a way that you maybe have a branch somewhere where you experimented with this? Can I look?

[SabrinaJewson]

If we went with this, I think we could remove the impl IntoResponse for Result and then this wouldn't be an issue, right?

Right. My thinking was just that we don’t want to break too much code that relies on the current behaviour.

Also I don't think I've noticed mentioned how would be the ErrorHandler::Error converted into an actual HTTP response, shouldn't that be a method on the trait? Or is there an expectation to use a HandleErrors layer to recover these?

Indeed, the suggestion is to use a .handle_error layer for this.

In either case, why are NotFound and MethodNotAllowed special-cased and aren't simply errors where we could even require ErrorHandler: From<NotFound>?

What special case does that refer to – isn’t that what this proposal is? From the user’s perspective, implementing From<NotFound> and From<MethodNotAllowed> will cause ErrorHandler to automatically be implemented. The only special case is when E = DefaultErrors, which allows the error type to be Infallible, and that’s for compatibility with existing code.

This seems sketched out in a way that you maybe have a branch somewhere where you experimented with this? Can I look?

I don’t have that, unfortunately. So it is possible there are things I’m overlooking. I opened this for some initial feedback, I should probably make the branch.

---

So yes, overall this proposal is made more complex by the desire to avoid breaking existing code. The idea is that larger applications can be piecemeal moved to the “fallible handler” design, but the default errors mode is still there for simpler use cases, where it would be annoying to have to define one’s own error type.

Possibly, if I were designing it from scratch, then it would somewhat different:

• IntoResponse for Result would be removed;
• Router, by default, uses a type PlaintextError, and is an infallible service that makes use of PlaintextError: IntoResponse.

The breakage points would be that:

• layers have to handle fallibility, and
• existing handlers that return Result now require the error type to implement From for more rejections.

But now I think about it, this probably is not as bad as I initially thought.

Edit: On the other hand, it may be a footgun, because while something like .layer(SetResponseHeaderLayer::overriding(…)) would previously apply to all responses, it now only applies to successful responses. Hard to know what’s right, I guess.

[mladedav]

By special casing I meant having

    fn not_found() -> Result<Response, Self::Error>;
    fn method_not_allowed() -> Result<Response, Self::Error>;

on the trait. Why call those and not directly E::from(NotFound)?

something like .layer(SetResponseHeaderLayer::overriding(…)) would previously apply to all responses, it now only applies to successful responses

I think that depends on how we'd handle the fallibility of responses in the layers. And since the users could call handle_errors before a given layer, they could then just use the same layers as they do now and they would apply to all responses.

It definitely is very intriguing and I've also be bitten by having a handler returning an unexpected response so I do think it would be great if we can make this work.

[SabrinaJewson]

Why call those and not directly E::from(NotFound)?

Well, we can’t have E::from(NotFound) because E is not necessarily even Sized. We also can’t have E::Error::from(NotFound), because Infallible does not (and cannot) implement NotFound. Note that for the non-infallible case, we do just use E::from(NotFound) – those functions are basically private API and are just there to make the infallible case work.

---

I’ve started hacking this up on a branch, which I’ll post once I’m done. I opted against the original proposal with the ?Sized hacks, instead goïng for a far simpler design in which Router is a synonym for Router<(), PlaintextError> (by default). If we really do think the breakage isn’t worth it then we can go back to the original, but I now believe it’s not nearly as bad as I thought initially.

[jplatte]

To set some expectations here - this proposal seems like a pretty big change to me, and I don't think we should have it in 0.8, if at all.

Really, I wasn't planning to do any changes of this size for a long time, if ever. (to me, all of #2475, #2468 and what we already merged for 0.8 seem like smaller changes in terms of churn)

I've got a much more experimental potential axum successor project in the works - lmk if you want to be involved. I'd love to explore different error handling approaches there at some point, and maybe it could be used as a testing ground for changes to axum too, though it's pretty different in some ways already like replacing the tower traits with its own HTTP-specific ones.

[SabrinaJewson]

Then fair! I opened the issue with the expectation that that would very likely be the response :smile:

[jplatte]

I don't think this needs to be outright closed (maybe it's something we should consider for 0.9 or beyond) - unless you feel like it's no longer worth exploring given the situation?

[SabrinaJewson]

Oh, fair, I was just less motivated somehow. We can keep it open!

[jplatte]

Understandable ^^

[potatotech]

If we went with this, I think we could remove the impl IntoResponse for Result and then this wouldn't be an issue, right?

I'd love to explore different error handling approaches there at some point

Related: #2587 The centralized error handler and observer design used in pavex can potentially be applied to extractor rejections as well.

[SabrinaJewson]

Right! I think that issue is quite similar, this is just a much more concrete proposal.