Skip to content
This repository was 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
@jk-gan
Labels
discussion New feature discussion enhancement Enchancement for current feature
Milestone
v0.3

Comments

@jk-gan
Copy link
Member

jk-gan commented May 3, 2020 •
edited
Loading

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 the v0.3 milestone May 3, 2020
@jk-gan jk-gan mentioned this issue May 5, 2020
Open
11 tasks
@jk-gan
Copy link
Member Author

jk-gan commented May 10, 2020 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
Open
@jk-gan jk-gan linked a pull request May 24, 2020 that will close this issue
Feature/better error #64
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jk-gan jk-gan

Labels
discussion New feature discussion enhancement Enchancement for current feature
Projects
None yet
Milestone
v0.3
Development

Successfully merging a pull request may close this issue.

Feature/better error obsidian-rs/obsidian
1 participant
@jk-gan