Rejection and anyhow

[Geobert]

Hi,

I'm trying to use anyhow but as warp::reject::Rejection doesn't impl std::error::Error, they doesn't compose well.

Am I missing something or it just can't be done (yet)?

[jxs]

Hi @Geobert, it should be the other way around, wrapping anyhow::Erroror any implementation of Into<Box<dyn dyn std::error::Error + Send + Sync>> in warp::reject::custom, and then recovering from it.
There's an example showing it

[Geobert]

I see, so we can't use anyhow::Result but have to use the standard Result to return a Rejection.

[jxs]

yea,
you can't use anyhow::Result because it is a type alias where the Error is anyhow::Error and you need to return Result<T, warp::Rejection>

[udoprog]

Might want to re-open this. It doesn't seem to be possible on master since #311

warp::reject::custom only accept types which implements Reject, but anyhow::Error doesn't implement it:

89  | ...                   .map_err(warp::reject::custom)
    |                                ^^^^^^^^^^^^^^^^^^^^ the trait `warp::reject::Reject` is not implemented for `anyhow::Error`

This seems to mean it's only possible to use custom rejections with types defined locally in your crate (Lack of blanket impl?). This is what I use as a workaround:

#[derive(Debug)]
struct CustomReject(anyhow::Error);

impl warp::reject::Reject for CustomReject {}

pub(crate) fn custom_reject(error: impl Into<anyhow::Error>) -> warp::Rejection {
    warp::reject::custom(CustomReject(error.into()))
}

[thomaseizinger]

We are using HttpApiProblem (RFC7807), an excellent standard for returning errors over HTTP, in our appliction together with warp and anyhow. Maybe it helps in this case: https://github.com/comit-network/comit-rs/blob/master/cnd/src/http_api/problem.rs

In a nutshell it works like this:

• HttpApiProblem implemented std::error::Error
• we use anyhow all the way up to the controller functions
• we extract the cause from anyhow::Error and map it to our desired HttpApiProblem (the link I've posted above)
• we use custom rejections to hand the HttpApiProblem to warp
• we use warp's recover functionality to render the HttpApiProblem to a response

[aslamplr]

I have tried to follow what @thomaseizinger has suggested. It is a really clean approach.
Unfortunately, I am getting the following error.

the trait bound `http_api_problem::HttpApiProblem: warp::reject::Reject` is not satisfied
the trait `warp::reject::Reject` is not implemented for `http_api_problem::HttpApiProblem`

I have put together sample code to recreate the issue:

use warp::Filter;

#[tokio::main]
async fn main() {
    pretty_env_logger::init();

    let sample_route = warp::path!("repo" / String).and_then(handler::get_something);
    let routes = warp::get()
        .and(sample_route)
        .recover(problem::unpack_problem);

    warp::serve(routes).run(([127, 0, 0, 1], 3030)).await;
}

mod handler {
    use warp::{self, Rejection, Reply};

    pub async fn get_something(some_arg: String) -> Result<impl Reply, Rejection> {
        super::service::get_something(&some_arg)
            .await
            .map_err(super::problem::from_anyhow)
            .map_err(|e| warp::reject::custom(e))
    }
}

mod service {
    use anyhow::{anyhow, Result};

    pub async fn get_something(some_arg: &str) -> Result<String> {
        Err(anyhow!("An error occured!"))
    }
}

mod problem {
    use http_api_problem::HttpApiProblem;
    use warp::{
        self,
        http::{self, StatusCode},
        Rejection, Reply,
    };

    pub fn from_anyhow(e: anyhow::Error) -> HttpApiProblem {
        let e = match e.downcast::<HttpApiProblem>() {
            Ok(problem) => return problem,
            Err(e) => e,
        };
        HttpApiProblem::new(format!("Internal Server Error\n{:?}", e))
            .set_status(warp::http::StatusCode::INTERNAL_SERVER_ERROR)
    }

    pub async fn unpack_problem(rejection: Rejection) -> Result<impl Reply, Rejection> {
        if let Some(problem) = rejection.find::<HttpApiProblem>() {
            let code = problem.status.unwrap_or(StatusCode::INTERNAL_SERVER_ERROR);

            let reply = warp::reply::json(problem);
            let reply = warp::reply::with_status(reply, code);
            let reply = warp::reply::with_header(
                reply,
                http::header::CONTENT_TYPE,
                http_api_problem::PROBLEM_JSON_MEDIA_TYPE,
            );

            return Ok(reply);
        }

        Err(rejection)
    }
}

Cargo.toml dependencies:

[dependencies]
tokio = { version = "0.2", features = ["macros"] }
warp = "0.2"
log = "0.4"
pretty_env_logger = "0.4"
anyhow = "1.0"
http-api-problem = "0.17.0"

Output:

error[E0277]: the trait bound `http_api_problem::HttpApiProblem: warp::reject::Reject` is not satisfied
   --> src/main.rs:22:47
    |
22  |             .map_err(|e| warp::reject::custom(e))
    |                                               ^ the trait `warp::reject::Reject` is not implemented for `http_api_problem::HttpApiProblem`
    | 
   ::: /Users/aslam/.cargo/registry/src/github.com-1ecc6299db9ec823/warp-0.2.2/src/reject.rs:115:18
    |
115 | pub fn custom<T: Reject>(err: T) -> Rejection {
    |                  ------ required by this bound in `warp::reject::custom`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `warp_hello`.

@thomaseizinger I tried going through the repo https://github.com/comit-network/comit-rs and couldn't find how you resolved this. Maybe I didn't look at the right place.

Any help is appreciated. Thank you!

[thomaseizinger]

You need to activate the optional warp feature on http-api-problem :)

[aslamplr]

Thank you @thomaseizinger

You need to activate the optional warp feature on http-api-problem :)

It resolved the issue:

Cargo.toml dependencies entry for http-api-problem should have the with-warp features enabled for this to work with warp.

http-api-problem = { version = "0.17.0", features = ["with-warp"] }

[thomaseizinger]

The rejection system was design at a time where anyhow did not exist yet (around one full year before the first release).

I think at that time, it was really useful because there wasn't a really good way of handling several different kinds of errors nicely. Hence I guess it was acceptable to have them all implement Reject and filter them later in recover.

However, anyhow completely changed the game. It is super nice for application code to just use anyhow::Result and hence the integration with warp is quite a burden at the moment.

Are there any plans on supporting the anyhow usecase better @seanmonstar?

Given the feature overlap between the rejection system and anyhow, it might not even be too far fetched to just use anyhow internally?

[jhpratt]

Ping @seanmonstar. I see you haven't been quite active recently — would a PR from someone else along the lines of what @thomaseizinger described be acceptable?

[jxs]

The rejection system was design at a time where anyhow did not exist yet (around one full year before the first release).

I think at that time, it was really useful because there wasn't a really good way of handling several different kinds of errors nicely. Hence I guess it was acceptable to have them all implement Reject and filter them later in recover.

However, anyhow completely changed the game. It is super nice for application code to just use anyhow::Result and hence the integration with warp is quite a burden at the moment.

@thomaseizinger If you scroll to the beginning of this issue, you can see warp used to allow any impl of Into<Box<dyn std::error::Error + Send + Sync>> wrapped by warp::reject::custom namely failure and anyhow.
With #311 the Rejection system was purposefully redesigned to be more explicit, see #374 (comment) and #527 (comment).

@jhpratt there have already been submissions regarding this: #374, #527 and #588

[thomaseizinger]

The rejection system was design at a time where anyhow did not exist yet (around one full year before the first release).

I think at that time, it was really useful because there wasn't a really good way of handling several different kinds of errors nicely. Hence I guess it was acceptable to have them all implement Reject and filter them later in recover.

However, anyhow completely changed the game. It is super nice for application code to just use anyhow::Result and hence the integration with warp is quite a burden at the moment.

@thomaseizinger If you scroll to the beginning of this issue, you can see warp used to allow any impl of Into<Box<dyn std::error::Error + Send + Sync>> wrapped by warp::reject::custom namely failure and anyhow.
With #311 the Rejection system was purposefully redesigned to be more explicit, see #374 (comment) and #527 (comment).

Thanks @jxs for pointing this out! I did indeed miss that there was a re-design of the rejection system since it's first appearance.
Also, after reading this thread again, I also realized that I probably need to explain myself better 😁

Despite the workaround that I suggested earlier in this thread, I still believe that a different design of the rejection system could make usage of warp a lot more ergonomic. Maybe we can use this issue to get a discussion around this started like @seanmonstar recommended here?

Here is what things look like from my experience with warp:

1. Can't use ? in route handlers.
   This is probably the biggest ergonomic hit. ? makes error handling really nice concise but in the project where we are using warp, all functions called within route handlers exclusively return anyhow::Result. Given that lack of conversion between anyhow::Error and Rejection, we can't use ? and instead have to repeat ourselves with .map_err on every function call. One can work around that by extracting additional handler functions so you only have to .map_err once but that is just different verbosity then 🤷‍♂️
2. I never used the "try another route feature".
   Instead, all rejections returned from route handlers always result in an HTTP (error) response being sent to the user. I can't speak for other people using warp but my gut feeling would be that this is rather the norm than the exception.
3. Having to implement Reject on errors doesn't play well with an onion/hexagonal architecture.
   In such an architecture, the HTTP component of a software depends on a framework-agnostic "core". To enforce this separation, one can decompose the application into several crates (core, HTTP API, database, etc ...). Due to orphan rules, the impl Reject for ... has to be in the core with ought to be framework-agnostic. Alternatively, one can define a newtype within the HTTP component for each possible rejection. Again, that is just verbose for seemingly no benefit.

Generally, I can very much understand the intention for "people should think about meaningful rejections". However, I don't think the current design encourages that a whole lot as it primarily increases verbosity.

On the upside, being able to recover from errors returned in handlers in a single place is really nice. It is just that the added indirection through the Reject doesn't seem to add anything to this. Instead, embracing a lib like anyhow or a more generalized approach that also allows the use of eyre that acts as this generic error containers sounds like it would do the job equally well without the added verbosity.

[jxs]

1. Can't use ? in route handlers.
   This is probably the biggest ergonomic hit. ? makes error handling really nice concise but in the project where we are using warp, all functions called within route handlers exclusively return anyhow::Result. Given that lack of conversion between anyhow::Error and Rejection, we can't use ? and instead have to repeat ourselves with .map_err on every function call. One can work around that by extracting additional handler functions so you only have to .map_err once but that is just different verbosity then man_shrugging

but you can, you just have to impl From<Error> for warp::reject::Rejection

Having to implement Reject on errors doesn't play well with an onion/hexagonal architecture.
In such an architecture, the HTTP component of a software depends on a framework-agnostic "core". To enforce this separation, one can decompose the application into several crates (core, HTTP API, database, etc ...). Due to orphan rules, the impl Reject for ... has to be in the core with ought to be framework-agnostic. Alternatively, one can define a newtype within the HTTP component for each possible rejection. Again, that is just verbose for seemingly no benefit.

can't you have multiple rejections across domains that by themselves impl Reject?

[thomaseizinger]

1. Can't use ? in route handlers.
   This is probably the biggest ergonomic hit. ? makes error handling really nice concise but in the project where we are using warp, all functions called within route handlers exclusively return anyhow::Result. Given that lack of conversion between anyhow::Error and Rejection, we can't use ? and instead have to repeat ourselves with .map_err on every function call. One can work around that by extracting additional handler functions so you only have to .map_err once but that is just different verbosity then man_shrugging

but you can, you just have to impl From<Error> for warp::reject::Rejection

Are you referring to anyhow::Error with your Error type?
That impl doesn't compile due to orphan rules. It would either have to live in anyhow or warp. Given that anyhow targets a much more general scope, it would have to go into warp. In fact, that is actually what I tried to do here 🙃

Having to implement Reject on errors doesn't play well with an onion/hexagonal architecture.
In such an architecture, the HTTP component of a software depends on a framework-agnostic "core". To enforce this separation, one can decompose the application into several crates (core, HTTP API, database, etc ...). Due to orphan rules, the impl Reject for ... has to be in the core with ought to be framework-agnostic. Alternatively, one can define a newtype within the HTTP component for each possible rejection. Again, that is just verbose for seemingly no benefit.

can't you have multiple rejections across domains that by themselves impl Reject?

I am afraid I don't follow what you are suggesting :/
However, the above point is the least of my worries :)
Being able to use ? when my functions return anyhow::Result is pretty much all I am asking for!