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

idiomatic way to return errors with json body and StatusCode::Ok #532

Closed
danieleades opened this issue May 23, 2020 · 7 comments
Closed

Comments

@danieleades
Copy link

i'm making a toy server that implements the alternative registries web API.
The endpoints all need to return a 200 response on errors, with a json body encoding the actual details.

I'm struggling to find a nice pattern for doing this using tide.

  • My endpoint functions have to return Result<impl Into<Response>, tide::Error>, even though the tide::Error path is never reachable.
  • i can't propagate errors with the ? operator-

what i've come up with is an "inner" endpoint function that returns a result, and then a "public" endpoint function that converts that Result into a tide::Response and Ok-wraps it. It works, but it feels slightly clumsy.

pub async fn endpoint(request: Request<State>) -> tide::Result<tide::Response> {
    Ok(
        match inner_endpoint(request).await {
            Ok(response) => response.into(),
            Err(e) => e.into()
        }
    )
}

pub async fn inner_endpoint(request: Request<State>) -> Result<MyResponse, MyError> {
    ...
}

impl From<MyError> for tide::Response {
    fn from(e: MyError) -> Self {
        ...
    }
}

impl From<MyResponse> for tide::Response {
    fn from(e: MyResponse) -> Self {
        ...
    }
}
@danieleades
Copy link
Author

i suspect this is a duplicate of #452

@sdimkov
Copy link

sdimkov commented May 24, 2020

I guess returning 2xx HTTP statuses on failure is not your choice but I also hope you agree that it is a gross anti-pattern.

Not the most helpful comment but I couldn’t resist

@danieleades
Copy link
Author

I guess returning 2xx HTTP statuses on failure is not your choice but I also hope you agree that it is a gross anti-pattern.

I don't really have a strong opinion here. It indicates that the operation executed successfully, and allows you to provide a structured response. Also the API is only ever consumed by one client, which is expecting precisely this. But as you say, it's out of my hands anyway

Even if you were returning other http codes, but wanted to choose them manually, or set a custom body or headers, you'd run into the problem I'm describing. It's in no way specific to 200 codes.

Not the most helpful comment but I couldn’t resist

Agreed.

@Fishrock123
Copy link
Member

Will this be addressed by #549?

    app.middleware(ErrorHandler::new(
        |err: tide::http::url::ParseError| async move {
            Ok(Response::new(StatusCode::Ok).body_json(json_api_error_response))
        },
    ));

@danieleades
Copy link
Author

@Fishrock123 possibly! Conceptually though, it does seem awkward to take a structured error, convert to a tide::Error, and then use a handler to turn it back into some structured error response

Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 3, 2020
Related to all of the following discussions, it would be useful for http_types's errors to optionally carry data similar to a response, particulaly Headers and/or a Body.

This allows for better handling of situations where things like middleware want to act only partialy on an error.

Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 3, 2020
Related to all of the following discussions, it would be useful for http_types's errors to optionally carry data similar to a response, particularly Headers and/or a Body.

This allows for better handling of situations where things like middleware want to act only partially on an error.

Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 5, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retreival via `error() -> Option<&Error>`.

Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 5, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retrieval via `error() -> Option<&Error>`.

Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 5, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retrieval via `error() -> Option<&Error>`.

Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 5, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retrieval via `error() -> Option<&Error>`.

Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 5, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retrieval via `error() -> Option<&Error>`.

Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 5, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retrieval via `error() -> Option<&Error>`.

PR-URL: http-rs#174
Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 16, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retrieval via `error() -> Option<&Error>`.

PR-URL: http-rs#174
Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
Fishrock123 added a commit to Fishrock123/http-types that referenced this issue Jun 16, 2020
This allows for robust creation of Response-s directly from Error-s, with error
capture for future reference, and retrieval via `error() -> Option<&Error>`.

PR-URL: http-rs#174
Refs: http-rs#169
Refs: http-rs/tide#546
Refs: http-rs/tide#532
Refs: http-rs/tide#452
@Fishrock123
Copy link
Member

Is this still an issue in Tide 0.12? (i.e. with #570)

@yoshuawuyts
Copy link
Member

It should now indeed be possible to write:

let mut app = tide::new();
app.at("/").get(|_| async { Ok(json!({ "hello": "nori" })) });
app.listen("localhost:8080").await?;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants