Improved rejections

[mankinskin]

I get the impression that the rejection system in this crate ought to be severely refactored. The rejections example does not show a simple way of handling functions which return Results:

async fn login(credentials: Credentials) -> Result<UserSession, LoginError> {
    ...
}

which is the standard way of handling Errors in Rust. The suggested way of handling this seems to be to

• implement Reject for LoginError (which requires me to own the LoginError type)
• call recover after the handling filter with a "rejection handler" function, which converts rejections into a Reply type which can be sent by warp

This causes a lot of cognitive and syntactic overhead, and I think the crate would benefit a lot from a refactor like this.

[mankinskin]

For context, this is the way that I found to handle this case:

    let login = warp::post()
        .and(warp::path!("api"/"login"))
        .and(warp::body::json())
        .and_then(|credentials: Credentials| async {

            Ok(match login(credentials).await {
                Ok(session) => warp::reply::json(&session).into_response(),
                Err(status) => warp::reply::with_status("", status).into_response(),
            }) as Result<warp::reply::Response, core::convert::Infallible>

        });

There is a lot of boilerplate here that ought to be handled by Filter::and_then (and friends) or the Reject or Reply trait.
The problem I see currently is that Filter::and_then requires a function, returning a TryFuture, resolving to a Result, where the Error implements CombinedRejection, which is a sealed trait of this crate, and is only implemented for Rejection and Infallible.

So actually returning an error from a Filter using and_then (which is the only method accepting a Future, i.e. async values btw) comes down to converting the error to the a Rejection type, which seems completely useless by itself, and does not integrate well with other types. The only way to use it with code external to warp (which is absolutely necessary given the current reject module, basically only supporting not_found) is to use reject::custom. This function however requires the input type to implement the Reject trait, which is not implemented for anything by warp itself, so you can only implement it yourself, for types defined by your own crate. This however requires you to define a new type for every error type your crate is using, which seems completely unreasonable.

My current proposal would be to remove the reject concept altogether and use the Reply interface instead. Errors can be denoted through the HTTP error codes, but are just replies otherwise. I will take a deeper look at how to implement this though, and might change the design later on.
I would be glad to talk about any feedback.

[jxs]

Hi, this has been addressed multiple times, see #527, #374, #307,

[seanmonstar]

It has been brought up multiple times, but I that also likely means the design is missing something. It might need to be easier to use rejections and stuff, but it also is the case that people see rejections and assume that all errors should be turned into rejections. That shouldn't be the case, though. Rejections are meant to say "this filter didn't accept the request, maybe another can." So things like DbError or similar shouldn't be turned into rejections.

I've seen it suggested the idea of "fatal" rejections, that don't try other filters. But I don't know... I don't yet know the best answer.

[apiraino]

Rejections are meant to say "this filter didn't accept the request, maybe another can." So things like DbError or similar shouldn't be turned into rejections.

I am glad to read this comment and wonder if I got the Rejection tool the other way around :-) I basically use Rejections to inject poison into a wrong request (e.g. wrong auth token, missing header, etc.) before any business logic kicks in, then get the request through all the remaining filters and be handled by a .recover() and finally turned into a 40x/50x error. I've based my approach, among other, also on this example.

[jxs]

So things like DbError or similar shouldn't be turned into rejections.

ah! I was also not aware of this, in my case what I have been previously doing is wrapping third party library errors in project ones eg:
https://github.com/jxs/currencies/blob/master/src/db.rs#L18

The issue i see with transforming third party library errors into Response's on each handler is that you have to do it individually whereas with rejections you can do it on recover defining a Response type for multiple Rejection's: https://github.com/jxs/currencies/blob/master/src/error.rs#L25

[jakajancar]

The rejection system makes sense for built-in and custom filters, but for the final handler, e.g. some /users endpoint, 95% of the time you want the Error of the Result be a 500, not a rejection. And this should work nicely with the ? operator.

I hope @seanmonstar you can either take the time to re-think/extend this, or invite the community to come up with something for 0.3 and trust the kids to do the right thing :)

[jakajancar]

FWIW, my suggestion is:

.and_then_reply(handler)

where handler is a function which returns Result<impl warp::Reply, std::error::Error>.

The error is automatically converted into an InternalError rejection, and such a rejection will not continue trying other filters.

This allows nice handler implementations, while the rejection can still be centrally recovered to convert to an error reply, together with 404's and such.

[m9xiuz]

It might need to be easier to use rejections and stuff, but it also is the case that people see rejections and assume that all errors should be turned into rejections. That shouldn't be the case, though. Rejections are meant to say "this filter didn't accept the request, maybe another can." So things like DbError or similar shouldn't be turned into rejections.

I also assumed that way. Maybe this explanation should be added to the documentation?
Btw, it's not clear how to customize just not found error without writing handle_rejection function that must handle every Known rejection.

[jakajancar]

Some notes from Discord yesterday, hopefully to continue here:

A lot of this Rejection == Error confusion probably comes from using Err for rejections... perhaps handlers should not return std::result::Result but some other type, e.g. warp::Result that has ::Reply or ::Rejection types, not ::Ok and ::Err.

i'm thinking:

1. non-rejecting filters are common for the final, "endpoint handling" filters. they're a large part (majority?) of app code.
2. you want ? to work
3. to get it to work, the handler must return Result<..., Error>, not Result<..., Rejection>.
4. since user cannot extend Filter (can't do an extension trait due to Func magic being private), some combinator needs to be added to it that takes Result<..., Error>

then it's just a question where to stick the Error -> Reply/Rejection handler
(is error implicitly converted to some rejection (perhaps fatal), do you pass error handler to combinator directly, ...)

[kaj]

Hm. Reading this thread gave me the idea that maybe a nice way of handling errors (not rejection) would be to impl Reply for Result<Response, MyError>. Then my code can handle my errors, while fitting nicely into the interface provided by warp. I'll do some experimentation on this.

[jakajancar]

You can't impl Reply (from warp) for Result (from std) in your lib due to coherence rules.

…

On Thu, Nov 5, 2020 at 4:23 PM Rasmus Kaj ***@***.***> wrote: Hm. Reading this thread gave me the idea that maybe a nice way of handling errors (not rejection) would be to impl Reply for Result<Response, MyError>. Then *my* code can handle *my* errors, while fitting nicely into the interface provided by warp. I'll do some experimentation on this. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#712 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABN2XOU73YR7NTGEWN55GTSOKRLRANCNFSM4RU7TBDQ> .

[kaj]

You can't impl Reply (from warp) for Result (from std) in your lib due to coherence rules.

Correct. I just found out that having my own type as an error type is not enough to impl a trait on Result.

There is an impl<T> Reply for Result<T, http::Error> where T: Reply, but I don't see a way to construct a http::Error other than for specific errors that can happen in the http crate. Maybe warp could provide an impl<T, E> Reply for Result<T, E> where T: Reply, E: Reply? I would like to be able to define how to render error messages in my application code, and I think beeing able to provide an impl Reply for MyError and then have warp provide Reply for Result<impl Reply, MyError>.

Maybe warp should provide impl<T, E> Reply for Result<T, E> where T: Reply + Send, E: Reply + Send + std::error::Error ? Or should it add another marker type on E to avoid being too generic?