Uniform error handling through ? in handlers and FromDataSimple

[RalfJung]

Feature Request / Question

I would like to use uniform error handling throughout my handlers and FromDataSimple implementations. To this end, I have an error enum:

#[derive(Debug, Fail)]
pub(crate) enum RequestError {
    #[fail(display = "Missing header: {}", header)]
    HeaderMissing { header: &'static str },
    #[fail(display = "Invalid date format")]
    InvalidDate(chrono::ParseError),
    #[fail(display = "Invalid syntax")]
    NomError(nom::ErrorKind),
    #[fail(display = "DB error")]
    Postgres(postgres::Error),
}

To use this for error handling in route functions, I have a Responder implementaion:

impl RequestError {
    fn status(&self) -> Status {
        use self::RequestError::*;
        match self {
            &Postgres(_) => Status::InternalServerError,
            &_ => Status::BadRequest,
        }
    }
}

impl<'r> rocket::response::Responder<'r> for RequestError {
    fn respond_to(self, request: &rocket::Request) -> Result<rocket::Response<'r>, Status> {
        // more detailed error message for the log
        println!("Error handling request {}: {:?}", request, self);
        // `Display` error message for the user
        // (this is obviously rather crude, but it's just a PoC)
        rocket::Response::build()
            .header(rocket::http::ContentType::Plain)
            .status(self.status())
            .sized_body(Cursor::new(format!("{}\n", self)))
            .ok()
    }
}

This means my handlers can return Result<..., RequestError>, and ? works just perfectly.

However, even with this boilerplate, I still cannot use ? in a FromDataSimple implementation. To make that work, I have to also add

// this makes "?" work in from_data
impl From<RequestError> for Result<rocket::Data, (Status, RequestError)> {
    fn from(e: RequestError) -> Self {
        Err((e.status(), e))
    }
}

then I can do

impl rocket::data::FromDataSimple for T {
    type Error = RequestError;

    fn from_data(
        req : &rocket::Request,
        data : rocket::Data,
    ) -> rocket::data::Outcome<T, Self::Error> {
    // ...
    }
}

This generally works, and the errors with the expected body and status code are returned to the user.

However, notice that there are now two impls that need to figure out a status code for an error: the impl Responder, and the impl From for from_data. This problem occurs because a from_data, in case of a failure, needs to return both a Status and some error type. Given an error type that implements Responder (which I'd assume to be the common case in a Rocket app), that is rather redundant.

And this is not enough: to actually get the errors from from_data to the user, I also need to write my handlers as follows:

#[post("/", data="<request_data>")]
pub(crate) fn build_times(
    request_data: Result<RequestData, RequestError>,
    conn: DbConn,
) -> Result<String, RequestError> {
    let request_data = request_data?; // make sure the error from FromData is shown to the user
    // ...
}

And even with this extra boilerplate, error handling in from_data is still not as nice as I'd like it to be: For example, calling a function that returns Result<..., chrono::ParseError> looks like

let date = header.parse().map_err(RequestError::InvalidDate)?;

I would usually add impl From<chrono::ParserError> for RequestError and then do header.parse()?, but here I would need impl From<chrono::ParseError> for Result<rocket::Data, (Status, RequestError)> and that is rejected by rustc ("impl doesn't use types inside crate").

How Rocket could help

Generally I am asking for advice on how to improve the above situation. :D I also feel I am probably not using all these pieces the way they are intended to be used, but I couldn't find a more convenient approach to error handling. The Rocket guide has a chapter about error catchers, but I couldn't find anything on how to set up your error types such that coding the actual functionality is as convenient as possible (aka, maximal use of ?).

Beyond extended documentation, maybe there are things Rocket could do different to help, so I am also asking if you could consider such error handling in the overall Rocket design (in case there's no better way to do this currently). Not having a redundant Status in data::Outcome and instead relying on a Responder error type would help. And maybe the impl Try for outcome::Outcome could be improved to make it work better with custom error types; the fact that it has type Error = Result<F, E> means that as a user, I can only compose this with my own conversion functions to a very limited extend. type Error = E would help, together with removing the redundant Status it would mean that I can add my own impl From<...> for E. Or maybe the impl Try could add explicit support for doing some conversion before producing the E, by being generic over some Into<E>?
I am sure things are the way they are in Rocket for a reason, so none of these suggestions may make any sense.

[jebrosen]

When I'm at a real keyboard I can explain further, but here a few highlights:

• A new error handling design Sergio came up with is blocked for now because TypeId doesn't work on non-'static types, but IIRC the design would address most of your issues/questions -- including responding automatically if the error is a Responder. It's definitely a goal to "fix" error handling, but other things have been more important recently.
• The Try trait and its implementation for Outcome are a bit painful, I can see that. Have you seen IntoOutcome? I think it helps somewhat with this exact case.
• Error catchers are in an awkward spot: They currently don't (and I think can't, again because of some typing details) get any actual error values. They currently work okay for replacing the plain default error pages, but can't do much more exciting and I don't think they are what you want in this particular case.

[SergioBenitez]

Everything @jebrosen said is exactly on point. Regarding the Try implementation, we currently can't make it better due to the way the trait is defined. For details, see #597.

However, IntoOutcome should still help here and would remove the need for the From impl in some cases. You'd still need to pass the Status in to the into_outcome call, however.

Given an error type that implements Responder (which I'd assume to be the common case in a Rocket app), that is rather redundant.

I don't think this is the case at the moment because returning a Responder is only marginally more useful that not returning a Responder. With the error catcher redesign, returning a Responder would indeed be meaningful, but as @jebrosen states, it's currently not possible to implement that design. (You can review that design here.)

In all, we have plans to ameliorate all of the issues you've raised, but all of them are blocked on upstream Rust changes. The Try changes are blocked on a redesigned Try trait in rust-lang/rust#42327, while the catcher redesign is blocked in rust-lang/rust#41875.

[RalfJung]

Thanks for your responses, good to hear that I didn't just miss something obvious!

I don't think this is the case at the moment because returning a Responder is only marginally more useful that not returning a Responder.

In my case, it's about sharing the error type between handlers and from_data.

However, IntoOutcome should still help here and would remove the need for the From impl in some cases. You'd still need to pass the Status in to the into_outcome call, however.

I am looking at IntoOutcome now, and I don't see how it helps. First I thought I would aim for something like header.parse().into_outcome()?, but that doesn't make any sense because into_outcome takes two parameters -- self and Failure.

So I guess the idea is I would do header.parse().into_outcome(Status::BadRequest)? or so? But one key design goal in my setup was to only have one place that maps errors to status codes, so this is not a direction I would like to take.

[johnbcoughlin]

I managed to find a satisfactory solution to this issue by defining a status method on my custom error type. Then you can write something like this:

let result: Result<ParsedData, MyError> = fallible_expr();
result.into_outcome(result.err().map(|e| e.status()).unwrap_or(Status::Ok))

Then you can reuse the .status() method in the Responder impl that you'll use in the handler.

[RalfJung]

Yeah, that seem similar to the status method I have in the OP. I wouldn't call this "satisfactory" though.

[SergioBenitez]

Closing in favor of #597 and #749 which together resolve these issues.

[oren0e]

I'm not sure if this is related to this, but dealing with the same issue as the OP, I find a strange behaviour - if I implement FromDataSimple for my type and in the from_data() method I return somewhere a Failure with my custom error - I still get the default rocket error response (the one that comes with a catcher), although I never intended to use a catcher...

so in the following:

impl FromDataSimple for KnownIssue {
    type Error = DispatcherrError;

    fn from_data(req: &Request, data: Data) -> data::Outcome<Self, Self::Error> {
        // Ensure content type is json
        if req.content_type() != Some(&ContentType::JSON) {
            return Outcome::Forward(data);
        }

        let mut issue_str = String::new();
        if let Err(e) = data.open().take(LIMIT).read_to_string(&mut issue_str) {
            return Failure((
                Status::InternalServerError,
                DispatcherrError::InternalServerError(anyhow::anyhow!(e)),
            ));
        }

        let issue_to_insert = match serde_json::from_str(&issue_str) {
            Ok(issue) => issue,
            Err(e) => {
                return Failure((
                    Status::InternalServerError,
                    DispatcherrError::InternalServerError(e.into()),
                ))
            }
        };
        Success(issue_to_insert)
    }
}

If serde_json deserialization fails, I expect to see also the error which comes from the variant DispatcherrError::InternalServerError
But all I get is the default rocket internal server error html.