warp: Improved rejections

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.

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.

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.

The missing piece of the puzzle for me (which I discovered after writing the above comment) was Box<dyn warp::Reply>

Yes, that makes it possible to do explicit early-return of error replies, which is kind of nice. But #458 (I keep repeating that PR number) would make it possible to do that early return with the standard ? operator.

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?

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 😃

For certain types of errors, I want a 500 code and a log message. For other types of errors, I want to try a different filter. Handling this inside a recover function is a bit cumbersome.

The way I see it, the recover function is only for those cases when you you have tried all filters. When there is a matching route and that route actually results in an error, the route function should return an impl Reply whith the error code (and maybe a nice html or json error message for the user).

And the nice way of doing that would be to return an Error that is not a rejection but instead treated as a reply, as per #458.

Also, the way I see it, having a three-state special result, that may contain an Error or a Rejection would not be really helpful, as the operation should be in one of two modes: Either we are still resolving the route, in which case we need a Result<impl Reply, Rejection> or we are in the function handling one specific route and need to return an impl Reply and never a rejection. In that case, having Result<MyResponse, MyError> implement Reply would be really nice.

I find the documentation very hard to read, because a lot of the code is hidden and not documented. For example, the F::Error where F: Filter associated type is implemented by the FilterBase trait, which is not documented, so you wonder where the Error type comes from, when reading the documentation, and have to look at the source code. This makes the API harder to understand. But that is a separate issue: #742

Implementing Reply for Result<T: Reply, E:Reply> is already very helpful, but the Rejections are probably still somewhat easily confused with errors, because right now (if I understand correctly), rejections are returned either through the Error associated type in FilterBase or in the Error type in TryFuture (for async filters), which makes it seem like this is where actual errors belong.

Maybe it would be better if warp explicitly implemented a FilterResult and a FilterFuture (which resolves to FilterResult) where FilterResult could look something like this:

enum FilterResult<T> {
    Ok(T),
    Rejection(Rejection),
}

Then it would be clear that this is not an error.

Filter functions could then return some T: Into<FilterResult<T>> and FilterResult could implement From<Rejection>.

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, …)

Hi, this has been addressed multiple times, see https://github.com/seanmonstar/warp/pull/527, https://github.com/seanmonstar/warp/pull/374, https://github.com/seanmonstar/warp/issues/307,