Skip to content
This repository has been archived by the owner on Oct 29, 2021. It is now read-only.

Proposal for better error handling #61

Open
jk-gan opened this issue May 3, 2020 · 6 comments · May be fixed by #64
Open

Proposal for better error handling #61

jk-gan opened this issue May 3, 2020 · 6 comments · May be fixed by #64
Assignees
Labels
discussion New feature discussion enhancement Enchancement for current feature
Projects
Milestone

Comments

@jk-gan
Copy link
Member

jk-gan commented May 3, 2020

1. Early return Error

Question: How do we know what response content-type to be returned by the Handler if user using ? to early return Error?

Currently, the Error will always return String which might not be what users expecting if they are developing json API.

Note: for Obsidian 0.2.x, the user only can handle this by manually write Error response instead of early return using ?.

From

pub async fn create_user(mut ctx: Context) -> ContextResult {
    #[derive(Serialize, Deserialize)]
    struct User {
        name: String,
        age: i8,
    }

    let user: User = match ctx.json().await {
        Ok(body) => body,
        Err(error) => {
            return ctx
                .build_json(error)
                .with_status(StatusCode::INTERNAL_SERVER_ERROR)
                .ok()
        }
    };

    // create user logic here

    ctx.build_json(user)
        .with_status(StatusCode::CREATED)
        .ok()
}

To

pub async fn create_user(mut ctx: Context) -> ContextResult {
    #[derive(Serialize, Deserialize)]
    struct User {
        name: String,
        age: i8,
    }

    let user: User = ctx.json().await?; // this will return a response with json content-type if Error

    // create user logic here

    ctx.build_json(user)
        .with_status(StatusCode::CREATED)
        .ok()
}

Expected response

// Http Status Code: 400
{"error":"'name' is required"}

The suggestion section is a work-in-progress

Suggestion

1. Trait

To allow the user to control error handling globally, we can provide an ErrorResponse trait.

pub trait ErrorResponse {
    fn respond_to(&self) -> Response;
}

impl ErrorResponse for SomeError {
    fn respond_to(&self) -> Response {
        // generate Response here
    }
}

2. Middleware

We can have a EarlyReturnErrorMiddleware to handle it:

#[async_trait]
impl Middleware for EarlyReturnErrorMiddleware {
    async fn handle<'a>(
        &'a self,
        context: Context,
        ep_executor: EndpointExecutor<'a>,
    ) -> ContextResult {
        if matches!(ep_executor.next(context).await, Err(error)) {
            // handle error   
        }
    }
}

3. Annotation

#[response(type="json")]
async fn get_user(ctx: Context) -> ContextResult {}
@jk-gan jk-gan self-assigned this May 3, 2020
@jk-gan jk-gan added discussion New feature discussion enhancement Enchancement for current feature labels May 3, 2020
@jk-gan jk-gan added this to To do in Obsidian via automation May 3, 2020
@jk-gan jk-gan added this to the v0.3 milestone May 3, 2020
@jk-gan jk-gan mentioned this issue May 5, 2020
11 tasks
@jk-gan
Copy link
Member Author

jk-gan commented May 10, 2020

2. Third Pary Error Type

Question: How do we support different Error type returned by different crate and return in the format users are expection?

Currently, we're expecting ObsidianError to be returned by the handler if an error occurs. However, we can't always know the Error type to be returned by different crates. It will be compilation error. This is annoying.

Technically we can allow any Error which impl std::error::Error trait to be returned in Handler, but we need an ergonomic way to allow users to modify the error response pattern as well.

Suggestion

1. Trait

To allow the user to control error handling globally, we can provide an ErrorResponse trait. All the Error return in Handler must impl this trait.

pub trait ErrorResponse {
    fn respond_to(&self) -> Response;
}

impl ErrorResponse for SomeError {
    fn respond_to(&self) -> Response {
        // generate Response here
    }
}

2. Using From trait

I would suggest we convert ObsidianError to become a struct to hold more data, or let Obsidian::ThirdPartyError to be an enum struct:

impl From<SomeError> for ObsidianError {
    fn from(error: SomeError) -> Self {
        ObsidianError::ThirdPartyError { crate: "some-crate", error }
    }
}

@jk-gan jk-gan changed the title [WIP] Proposal for better error handling Proposal for better error handling May 10, 2020
@jk-gan
Copy link
Member Author

jk-gan commented May 11, 2020

3. Error status code

Question: How do we support different http status code if the handler return Err(error)?

Currently, the endpoint_resolver will return StatusCode::INTERNAL_SERVER_ERROR(500) if the handler return Err(error). However, our framework should allow user to set proper status code based on the error: 4xx for client error and 5xx for server error.

@jk-gan
Copy link
Member Author

jk-gan commented May 11, 2020

4. Can't get Context data when the Handler return Err(error)

Question: How do we get Context data in the middleware after the Handler return Err(error)?

Currently, the context is moved into handler so there is no way to get the context if handler return Err(error). The return type for handler is pub type ContextResult<T = ObsidianError> = Result<Context, T>;.

@jk-gan
Copy link
Member Author

jk-gan commented May 18, 2020

Just found this crate (anyhow), let me try to use it for Error handling.

@jk-gan
Copy link
Member Author

jk-gan commented May 20, 2020

this is good reference too

@jk-gan
Copy link
Member Author

jk-gan commented May 23, 2020

99436742_3232543743423768_749704812307677184_o

Error will have to impl IntoErrorResponse trait in order to be handled by the framework. This is my initial thought:

pub trait IntoErrorResponse: fmt::Debug + fmt::Display {
    /// convert Error into error response
    fn into_error_response(&self) -> Result<Response<Body>, http::Error> {
        let body = Body::from(self.to_string());
        Response::builder().status(self.status_code()).body(body)
    }

    /// status code for this error response
    /// this will return Internal Server Error (500) by default
    fn status_code(&self) -> StatusCode {
        StatusCode::INTERNAL_SERVER_ERROR
    }
}

#[derive(Debug)]
pub struct ObsidianError {
    inner: Box<dyn IntoErrorResponse>,
}

impl ObsidianError {
    pub fn into_response(&self) -> Result<Response<Body>, http::Error> {
        self.into_error_response()
    }
}

impl<T: IntoErrorResponse + 'static> From<T> for ObsidianError {
    fn from(err: T) -> Self {
        ObsidianError {
            inner: Box::new(err),
        }
    }
}

@jk-gan jk-gan linked a pull request May 24, 2020 that will close this issue
@jk-gan jk-gan linked a pull request May 24, 2020 that will close this issue
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion New feature discussion enhancement Enchancement for current feature
Projects
Obsidian
  
To do
Development

Successfully merging a pull request may close this issue.

1 participant