Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to get the original error message inside a catch registered via catchers!() #1234

Closed
silvioprog opened this issue Feb 19, 2020 · 6 comments
Labels
enhancement A minor feature request request Request for new functionality

Comments

@silvioprog
Copy link

silvioprog commented Feb 19, 2020

Hi.

Consider the following route:

// route to update a taxpayer
pub fn update(
    conn: &PgConnection,
    id: i64,
    taxpayer: &UpdatableTaxpayer,
) -> Result<QueryableTaxpayer, DbError> {
    Ok(diesel::update(taxpayers::table.find(id))
        .set(taxpayer)
        .get_result(conn)?)
}

and the following registered catcher:

// serializes an error
macro_rules! json_error {
    ($reason:expr) => {
        rocket_contrib::json!({ "status": "error", "reason": $reason })
    };
    ($reason:expr, $detail:expr) => {
        rocket_contrib::json!({ "status": "error", "reason": $reason, "detail": $detail })
    };
}

#[catch(422)]
fn unprocessable_entity(req: &Request) -> JsonValue {
    json_error!("Unprocessable entity")
}

rocket::custom(config)
...
        .register(catchers![
...
            unprocessable_entity,
...
        ]))

When we send a JSON missing some field, it raises an:

{"status":"error","reason":"Unprocessable entity"}

however, it would be nice to show something like this instead:

{"status":"error","reason":"Unprocessable entity","detail":"missing field \"manager\"", line: 1, column: 12463"}

but the catcher should be something like, i.e. some new Rocket feature:

#[catch(422)]
fn unprocessable_entity(req: &Request, error: &Error) -> JsonValue {
    json_error!("Unprocessable entity", error)
}

Notice my custom error type DbError in the route, it contains an own Responder, btw it is never called, because it seems Rocket handles the error internally and refuse my responder. =/

TIA for any help!

@jbr
Copy link

jbr commented Feb 20, 2020

I also ran into this and am currently working around it in each route handler by accepting a Result<Json<T>, JsonError<'_>> and handling the possible JsonErrors at the top (JsonError::Io and JsonError::Parse, the latter of which is probably what matters) but it would be really nice if Rocket handled this for us, or at least passed the JsonError to the handler somehow

@PeterUlb
Copy link

Above solution is correct, see #953 (comment)
Right now, if you want to handle those errors yourself you have to wrap them in a Result.

Sadly this does not work for errors occurring in RequestGuards, where you can only define a custom Status and no body whatsoever (unless you define a status for every error that can occur and map it in a catcher to an standard status code with your static custom body)

@jebrosen
Copy link
Collaborator

See my response at #1232 (comment), which largely applies here as well. In particular:

Supporting functionality like #[catch(422)] fn catch_db_error(error: DbError) is an error-handling goal, but it would require that all request guard error types be 'static and/or that we get the non_static_type_id functionality in rust (#749 (comment))


Notice my custom error type DbError in the route, it contains an own Responder, btw it is never called, because it seems Rocket handles the error internally and refuse my responder. =/

That is unexpected - in rocket 0.4 Result<T, E> will use the impl Responder for E implementation. Can you share some more details, in particular the log output that you see when a DbError happens and your code for impl Responder for DbError (or the struct if you #[derive]d it).

@jebrosen jebrosen added enhancement A minor feature request request Request for new functionality labels Feb 29, 2020
@jlindsey
Copy link

jlindsey commented Jun 4, 2020

Any updates on where this enhancement stands now that the non_static_type_id RFC has been rejected (rust-lang/rust#41875 (comment))?

@jebrosen
Copy link
Collaborator

Any updates on where this enhancement stands now that the non_static_type_id RFC has been rejected

That's unfortunate. If I remember right the issue was that we don't want to bound all Error types by 'static, because some error sources such as Form borrow from the input date. I have an (admittedly rather vague) sense that it should be possible for error sources (FromRequest, FromParam, etc.) to opt in to being "catchable" errors by converting the error type into something with Any:

impl FromRequest for Type {
    type Error = ...;
    fn upcast_error(e: Self::Error) -> Option<Box<dyn Any + Send>> { Box::new(e) as Box<dyn Any + Send> }
    fn from_request(request: &Request) -> Result<Self, Self::Error> { ... }
}

This is nothing more than a vague idea at this point, but I'll think on it more when we take the issue up.

@SergioBenitez
Copy link
Member

Let's track this in #749.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A minor feature request request Request for new functionality
Projects
None yet
Development

No branches or pull requests

6 participants