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

Error trait to implement custom errors #1774

Closed
baptiste0928 opened this issue Jul 20, 2021 · 3 comments
Closed

Error trait to implement custom errors #1774

baptiste0928 opened this issue Jul 20, 2021 · 3 comments
Labels
duplicate This issue or pull request already exists request Request for new functionality

Comments

@baptiste0928
Copy link

Is your feature request motivated by a concrete problem? Please describe.

Actually, error handling with Rocket is limited to returning a Status or an error type that implements Responder, but error catchers only have access to the returned status and it's not possible to return a custom error message in places like request guards.

Ideal Solution

I think that a system to implement custom errors in a similar way to thiserror, but being able to specify the error code would be a good solution. It could look like this:

use rocket::error::RequestError;

#[derive(RequestError, Debug)]
pub enum ApiError {
    #[error("Missing authorization header", code = 400)]
    MissingAuth,
    #[error("Invalid authentication token (found {found})", code = 400)]
    InvalidToken {
        found: String
    },
    #[error("Database error", code = 500)]
    Database(#[from] DatabaseError)
}

This code will generate a type that implements std::error::Error and Display, but also a Rocket error trait (let's name it RequestError) with a .status() method to get the error code. It will also implement a Responder that forward to the appropriate error catcher.

It could be also great to allow error catchers that take the error in argument, like this :

#[catch(400)]
fn bad_request(req: &Request, error: &ApiError) {
    unimplemented!()
}

The error could also be displayed in the logs with its source.

Additional Context

If this suggestion is accepted, I would be happy to help implement it ! 😄

@baptiste0928 baptiste0928 added the request Request for new functionality label Jul 20, 2021
@the10thWiz
Copy link
Collaborator

I believe this is already possible, via the responder derive. It would look like:

#[derive(Responder)]
pub enum ApiError {
    #[response(status = 400)]
    MissingAuth(&'static str),
    #[response(status = 400)]
    InvalidTokens(String),
    #[response(status = 500)]
    Database(DatabaseError)
}

This calls to the first element's Responder impl. I think this could be implemented as an enhancement of the existing Responder derive, by adding some way to build a more complex response than just shelling out to the first parameter's impl. Something like

#[derive(Responder)]
pub enum ApiError {
    #[response(status = 400, responder = "Missing authorization header")]
    MissingAuth,
}

The idea would be that the responder param would specify how to convert the type into a type that implements Responder. I think this enhancement is much more straight forward, and likely more general.

@baptiste0928
Copy link
Author

The problem with using Responder derive is that enum fields become content and header, and custom responders cannot be used in request guards. My current error implementation looks like this :

Current custom error implementation
use thiserror::Error;
use rocket::{
    http::Status,
    response::{self, Responder},
    serde::json::Json,
    Request, Response,
};
use serde::{Deserialize, Serialize};

#[derive(Error, Debug)]
pub enum ApiError {
    #[error("Missing authorization header")]
    MissingAuth,
    #[error("Invalid authentication token (found {found})")]
    InvalidToken,
    #[error("Database error")]
    Database(#[from] DatabaseError)
}

impl Error {
    /// Get the [`Status`] associated with the error
    fn status(&self) -> Status {
        use Error::*;

        match self {
            MissingAuth | InvalidToken => Status::BadRequest,
            Database(_) => Status::InternalServerError,
        }
    }
}

impl<'r> Responder<'r, 'static> for Error {
    fn respond_to(self, request: &'r Request<'_>) -> response::Result<'static> {
        let status = self.status();
        let error_response = ErrorResponse::Response {
            code: status.code,
            reason: status.reason_lossy(),
            description: Some(self.to_string()),
        };

        let response = Response::build_from(Json(error_response).respond_to(request)?)
            .status(status)
            .finalize();

        Ok(response)
    }
}

/// Representation of errors in JSON.
#[derive(Serialize, Deserialize)]
pub enum ErrorResponse {
    #[serde(rename = "error")]
    Response {
        code: u16,
        reason: &'static str,
        #[serde(skip_serializing_if = "Option::is_none")]
        description: Option<String>,
    },
}

It works (with some downsides exposed in the issue), but it could be simplified with an appropriate derive macro. I think I'm not the only one to implement custom errors, it would be nice to have a more elegant way to do it. 🤷

@SergioBenitez
Copy link
Member

This is a duplicate of #749. That is, resolving #749 would resolve this.

@SergioBenitez SergioBenitez added the duplicate This issue or pull request already exists label Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists request Request for new functionality
Projects
None yet
Development

No branches or pull requests

3 participants