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

Extend-errors #15

Merged
merged 4 commits into from
Jul 4, 2024
Merged

Extend-errors #15

merged 4 commits into from
Jul 4, 2024

Conversation

BenoitRanque
Copy link
Contributor

This PR extends errors to allow users of the SDK to set the details json object.

The PR also implements IntoResponse for all the error types, so they can be returned directly from their respective routes.

This means the routes.rs file is removed. Note we add a fetch_metrics function, this contains the logic that used to live in the respective route (this was the only route with logic).

Before merging, I'll move all the error types into their own file, but wanted to keep them as-is for now, so it's easier to see what changed/stayed the same.

You may notice the IntoResponse implementation does not use the to_string method on the error types to add the prefix to the message. This is intentional, that information is conveyed with the status code.

@BenoitRanque
Copy link
Contributor Author

Note on usage/updating to the new error types

SDK users will need to update their error conversion code.
The following code examples use QueryError but the same pattern applies to all error types.

When converting from your own error types, you are encouraged to implement From<YourErrorType> for QueryError
This allows using the ? operator on any function that returns YourErrorType inside a function that returns QueryError

Error types have utility creation function that are generic over a reference to anything implementing ToString. That includes anything implementing display, including any error types. So these examples are all valid:

QueryError::new_invalid_request(&"some static error string");
QueryError::new_unsupported_operation(&SomeErrorType);

Additionally, each error type has an Other variant that can be created from anything that implements Into<Box<dyn Error>>
This error type corresponds to the 500 status code, so those errors won't be shown to the end user (but will show up in traces I believe?)

That error type is created with the new function on each error type. Example:

let json_string = serde_json::to_string(&underlying_value).map_err(QueryError::new)?;

All error types are initially created without details. Users wishing to add details to a created error should use the with_details method, which takes a json value as argument..

let json_string = serde_json::to_string(&underlying_value).map_err(|err| {
  QueryError::new_invalid_request(&"there was a problem serializing the value")
    .with_details(serde_json::json!({
        "key": "value"
    }))
})?;

Copy link
Contributor

@sordina sordina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @BenoitRanque I think stepping through with you yesterday cleared everything up for me.

Comment on lines 505 to 518
match self {
Self::InvalidRequest(err) => (StatusCode::BAD_REQUEST, Json(err)),
Self::UnprocessableContent(err) => (StatusCode::UNPROCESSABLE_ENTITY, Json(err)),
Self::UnsupportedOperation(err) => (StatusCode::NOT_IMPLEMENTED, Json(err)),
Self::Conflict(err) => (StatusCode::CONFLICT, Json(err)),
Self::ConstraintNotMet(err) => (StatusCode::FORBIDDEN, Json(err)),
Self::Other(err, details) => (
StatusCode::INTERNAL_SERVER_ERROR,
Json(models::ErrorResponse {
message: err.to_string(),
details,
}),
),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@BenoitRanque BenoitRanque added this pull request to the merge queue Jul 4, 2024
Merged via the queue into main with commit f4f6502 Jul 4, 2024
8 checks passed
@BenoitRanque BenoitRanque deleted the extend-errors branch July 4, 2024 22:27
github-merge-queue bot pushed a commit to hasura/ndc-postgres that referenced this pull request Jul 15, 2024
### What

We want to update ndc-sdk-rs to 0.2.1, and it introduces several
changes:

1. Error messages hasura/ndc-sdk-rs#15
2. Update ndc-spec to 0.1.5, which introduces breaking changes by adding
newtype wrappers around string types

### How

1. For errors, follow
hasura/ndc-sdk-rs#15 (comment)
2. For newtypes, go into type hell and fix the bugs by trying to put the
ndc-models newtypes in the data structures, as they are more
descriptive.
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

Successfully merging this pull request may close these issues.

2 participants