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

feat(types): Add gRPC Richer Error Model support (BadRequest) #1068

Merged
merged 6 commits into from
Sep 21, 2022

Conversation

flemosr
Copy link
Contributor

@flemosr flemosr commented Aug 26, 2022

Motivation

The gRPC Richer Error Model is quite useful to send additional feedback to clients, and is supported by many gRPC libraries in other languages.

Solution

This PR is the first step to add richer error model support to tonic-types, as per #1060. It introduces the WithErrorDetails trait and adds support to the BadRequest standard error message type.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

This looks like a fantastic start! Thanks for getting this going. Left a few comments lmk if you have any questions. I am looking forward to getting these series of PRs merged.

tonic-types/build.rs Outdated Show resolved Hide resolved
tonic-types/src/error_details.rs Outdated Show resolved Hide resolved
tonic-types/src/error_details.rs Outdated Show resolved Hide resolved
tonic-types/src/error_details.rs Outdated Show resolved Hide resolved
tonic-types/src/error_details_vec.rs Outdated Show resolved Hide resolved
tonic-types/src/lib.rs Outdated Show resolved Hide resolved
tonic-types/src/lib.rs Outdated Show resolved Hide resolved
tonic-types/src/lib.rs Outdated Show resolved Hide resolved
tonic-types/src/lib.rs Show resolved Hide resolved
tonic-types/src/lib.rs Outdated Show resolved Hide resolved
Adjustments following suggestions by @LucioFranco in hyperium#1068.

Adjust style, remove unecessary prints, avoid glob imports, apply
`non_exhaustive` to `ErrorDetails` and `ErrorDetail`, avoid pub
fields in `ErrorDetails`, adjust
`WithErrorDetails::with_error_details_vec` args, add
`gen_details_bytes`.
As suggested by @LucioFranco in hyperium#1068 (comment).

This avoids the need for consumers to have `protoc` in their path.
Implemented following changes in hyperium#1065.
This allows consumers to provide custom metadata when creating a
`Status` with error details.
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on the review, this looks really good. I have just a few smaller nits then I think we can merge!

//! in [`tonic::Status`], allowing the implementation of the
//! [gRPC Richer Error Model] with [`tonic`] in a convenient way.
//!
//! [`tonic::Status`]: https://docs.rs/tonic/0.8.0/tonic/struct.Status.html
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we will forget to update this version number. I think we may want to link to latest instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I addressed this in the latest adjustments commit.


/// Used to implement associated functions and methods on `tonic::Status`, that
/// allow the addition and extraction of standard error details.
pub trait WithErrorDetails {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about naming this StatusExt since its an extension trait and that way people know its methods implemented ontop of Status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's a better name, thanks! Renamed it in the latest adjustments commit.

tonic-types/src/lib.rs Show resolved Hide resolved

#[cfg(test)]
mod tests {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Adjustments following suggestions by @LucioFranco in hyperium#1068.

Move `error_details_vec` mod into `error_details` mod, adjust doc
comments, rename `WithErrorDetails` trait to `StatusExt`.
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM thanks for being patient with the reviews!

@LucioFranco LucioFranco merged commit 3e40d81 into hyperium:master Sep 21, 2022
@flemosr
Copy link
Contributor Author

flemosr commented Sep 23, 2022

Thanks for all improvement suggestions! I will start working on the next PR, that will add support for one more standard error message type.

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