Unreasonably hard to recover internal errors from Rejection.

[tobz]

At present, for any custom recovery method, a user is forced to manually call Rejection::find for every possible internal error if they want to be able to properly shuttle back a suitable response for internal errors as well as their own errors.

It would be incredibly useful if we could simply expose the internal status code for a rejection that it would otherwise assign when handling rejections using the default error handler. This unlocks more than enough (namely, being able to call StatusCode::canonical_reason) as the hard work has already been mapping the known errors to a suitable HTTP status code.

Given that you actually deprecated/removed the ability to do this, can you add some color as to the why and could we talk about either bringing it back or some other solution that doesn't involve a ton of boilerplate?

[seanmonstar]

Given that you actually deprecated/removed the ability to do this, can you add some color as to the why?

Sure! I removed the Rejection::status because it felt wrong in isolation. The status code it returned was only true if used by the default recovery stage. Additionally, a Rejection may be made of many rejections, and the status returned was selected by an internal ordering. By having the user write the find code, they could express which rejection types were of higher priority.

---

I admit the Rejection system may have rough edges, it's not a very common design. The inspiration comes from akka-http, with minor differences.

What's the biggest pain point at the moment? You're trying to handle certain rejections and use the default for others?

[tobz]

Exactly, yeah!

You've already done the good work of expressing the common errors -- including ones that are specific to Warp's filtering design -- and I want to be able to fall back to that when the rejection I'm being handed isn't one of my own.

[seanmonstar]

But you wish to tweak the response somehow beyond the default? So just returning Err(rejection) isn't enough?

[tobz]

Ah, sorry, yes. I'm specifically spitting out JSend-compatible messages, which is just JSON with fields that basically emulate status code/body/etc.

On Wed, Feb 19, 2020, 2:13 PM Sean McArthur [email protected] wrote:

But you wish to tweak the response somehow beyond the default? So just returning Err(rejection) isn't enough?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/seanmonstar/warp/issues/451?email_source=notifications&email_token=AABWLF72RVVZSEEXIG2QCNDRDWAFPA5CNFSM4KXPV2HKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMJDM4A#issuecomment-588396144, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABWLF42GV3FY2MDTOYDYJ3RDWAFPANCNFSM4KXPV2HA .

[seanmonstar]

Would having a function available that converts the Rejection into its default http::Response be something you're looking for? And then you can modify any headers and the body?

[tobz]

I think conceptually that works? I guess either a http::Response or StatusCode are suitable for handing back as the response, but I'm probably biased here in that I really only need the status code so I can get the canonical reason.

Just to sort of rubber duck it out here, the difference would be something like...

async fn handle_rejection(err: Rejection) -> Result<impl Reply, Infallible> {
    let mut code = None;
    let mut message = None;

    if let Some(DivideByZero) = err.find() {
        code = Some(StatusCode::BAD_REQUEST);
        message = Some("DIVIDE_BY_ZERO");
    }
    // more if conditions here
    
    let mut response = err.into_response();
    if let Some(sc) = code {
        *response.status_mut() = code;
    }
    if let Some(m) = message {
        *response.body_mut() = m.into();
    }
    
    Ok(response)
}

vs

async fn handle_rejection(err: Rejection) -> Result<impl Reply, Infallible> {
    let code;
    let message;

    if let Some(DivideByZero) = err.find() {
        code = StatusCode::BAD_REQUEST;
        message = "DIVIDE_BY_ZERO";
    } else {
        code = err.status();
        message = code.canonical_reason();
    }
    // more if conditions here
    
    Ok(warp::reply::with_status(message, code))
}

This is, of course, ignoring the customized structure/encoding, but I think it captures the spirit of what I'm proposing here. To my eyes, having to mutate/extract from a pre-baked response feels like extra work, although admittedly I suppose it's just exposing the allocation/construction of the response that would still otherwise happen?

[cjbassi]

I agree that having an easy way to convert from a rejection to a custom Json error response would be really nice. This feature, plus adding #458 would fix all the issues I've been having with errors.

[peterhuene]

I've attempted to create a simple HTTP basic auth filter but ran into this issue. I can implement a filter that checks against the Authorization header and returns one of two custom rejections (i.e. "missing authorization header" and "incorrect auth"). A recovery function is then used to return a proper response for those custom rejections.

However, the issue here then forces me to return a different (500?) response for the known (built-in) rejections since I have no access to the reply machinery for Rejection.

I thought perhaps to try using a wrapping filter instead, but the implementation of those seem entirely private.

Even if we fix this issue so that we can get at a response representation of Rejection, it still means that the transformation of a rejection to a proper response would still take place in a recovery function, which is something that makes using a filter that checks auth a bit unergonomic, as omitting the recovery function would get back, by default, 500 responses for the custom rejections.

I might be way off base, though, and perhaps there's a simple way to make an auth filter that plays nice with the built-in filters, but I'm currently at a loss as how to move forward.

[alexkornitzer]

@seanmonstar any progress on this? Not being able to extract the status codes from warp's 'known' errors means you would have to either manually find all 16 known types to return the correct code or just blanket return a 500.

https://github.com/seanmonstar/warp/blob/fbbbc5b8377914463e4411ef6fc48bfd97d15a6d/examples/rejections.rs#L61 The above will return incorrect codes as the rejection is not passed along even thought the comment states it is.

[Shadow53]

I'm having my own issue related to this one, where I have a custom error type with associated data that I want to impl Reply and Reject on (to easily forward the error to the client). I cannot recover the error, though, because the trait bounds require it has a 'static lifetime, which my error struct does not.

I do not understand why Reject does not require a static lifetime, but Rejection::find does: especially the latter. Is it not enough to have the impl Reject object owned by the Rejection?

[coolreader18]

@Shadow53 I believe Rejection::find requires 'static because <dyn Any>::downcast() does, otherwise, would &'a T and &'b T be the same types or different types?

I do have my own issue with this rejection setup; you can either have 0 custom errors at all, or you have to decide exactly how every individual known error will show its output; there's no in-between where you can say "well, when this error happens, I want to send this with this status code, but otherwise just use the default behavior." There's no way to pass a function like Fn(Rejection) -> Option<impl Reply>, which I feel would make that "adding error handling" curve much less steep.

[jsdw]

Just to add my 2c, I wanted to be able to return errors from a JSON based API in a standard JSON format of my own choosing to the user. I had to look through the warp source to see what errors to match on, and then wrote a macro to make my life a little easier doing this.

Its pretty horrible, and it means that if new errors are exposed in warp, I'll not be handling them until I notice. A generic way to obtain the status code so that I can handle them as I wish would make things cleaner and more future proof.

For interest, here's my current error handling code to turn errors into my standard shape:

use warp::reject::*;
use http_error::HttpError;
use http::StatusCode;
use super::HttpErrorWrapper;

macro_rules! to_http_error {
    ($($ty:path => $code:path,)+) => (
        /// Attempt to convert a warp rejection into an HttpError.
        pub fn rejection_to_http_error(rejection: warp::reject::Rejection) -> HttpError {
            // Handle our HttpErrorWrapper types, and the "not found" outcome:
            if rejection.is_not_found() {
                return HttpError::path_not_found()
            } else if let Some(err) = rejection.find::<HttpErrorWrapper>() {
                return err.0.clone()
            }
            // Handle the warp types that we have access to:
            $(
                if let Some(err) = rejection.find::<$ty>() {
                    return HttpError {
                        code: $code.as_u16(),
                        internal_message: err.to_string(),
                        external_message: err.to_string(),
                        value: None
                    }
                }
            )+
            // We don't know what the error is, so handle it generically:
            return HttpError {
                code: 500,
                internal_message: format!("{:?}", rejection),
                external_message: HttpError::SERVER_ERROR.to_owned(),
                value: None
            }
        }
    );
}

to_http_error! (
    MethodNotAllowed => StatusCode::METHOD_NOT_ALLOWED,
    InvalidHeader => StatusCode::BAD_REQUEST,
    MissingHeader => StatusCode::BAD_REQUEST,
    MissingCookie => StatusCode::BAD_REQUEST,
    InvalidQuery => StatusCode::BAD_REQUEST,
    warp::body::BodyDeserializeError => StatusCode::BAD_REQUEST,
    LengthRequired => StatusCode::LENGTH_REQUIRED,
    PayloadTooLarge => StatusCode::PAYLOAD_TOO_LARGE,
    UnsupportedMediaType => StatusCode::UNSUPPORTED_MEDIA_TYPE,
    warp::cors::CorsForbidden => StatusCode::FORBIDDEN,
    warp::ext::MissingExtension => StatusCode::INTERNAL_SERVER_ERROR,
);

[jszwedko]

Just offering that I have the same issue where I basically just want to marshal the rejection to a specific JSON structure without having to catch and handle all "standard" errors (after handling my custom ones). I think being able to access status and to_string would be sufficient for my needs.

[alexkornitzer]

My approach was to unseal the IsReject trait, which seems to work well: https://github.com/AlexKornitzer/warp/commit/6ecfd99a9761956639745b505757ce08216dcef2

[sinking-point]

@alexkornitzer I just came to the same conclusion and found this issue. Have you tried opening a pull request?

[alexkornitzer]

I am happy to make a PR, but without some feedback here from @seanmonstar it seems like it would just be wasted effort.

[mike-kfed]

I am running into this aswell see PR #751

I made an ugly quickhack to not unseal the IsReject trait see here https://github.com/seanmonstar/warp/pull/751#issuecomment-787848502

I too am looking for a publicly exposed way to make responses out of rejections.

[Levyks]

any updates on this?

this is currently what I'm having to do in order to get the default status, and it's not pretty haha

fn get_default_status_code(err: &Rejection) -> StatusCode {
    if err.is_not_found() {
        StatusCode::NOT_FOUND
    } else if let Some(_) = err.find::<warp::reject::MethodNotAllowed>() {
        StatusCode::METHOD_NOT_ALLOWED
    } else if err.find::<warp::reject::InvalidHeader>().is_some() ||
        err.find::<warp::reject::MissingHeader>().is_some() ||
        err.find::<warp::reject::MissingCookie>().is_some() ||
        err.find::<warp::reject::InvalidQuery>().is_some() ||
        err.find::<warp::body::BodyDeserializeError>().is_some() ||
        err.find::<warp::ws::MissingConnectionUpgrade>().is_some()
    {
        StatusCode::BAD_REQUEST
    } else if let Some(_) = err.find::<warp::reject::LengthRequired>() {
        StatusCode::LENGTH_REQUIRED
    } else if let Some(_) = err.find::<warp::reject::PayloadTooLarge>() {
        StatusCode::PAYLOAD_TOO_LARGE
    } else if let Some(_) = err.find::<warp::reject::UnsupportedMediaType>() {
        StatusCode::UNSUPPORTED_MEDIA_TYPE
    } else if let Some(_) = err.find::<warp::cors::CorsForbidden>() {
        StatusCode::FORBIDDEN
    } else {
        StatusCode::INTERNAL_SERVER_ERROR
    }
}