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

Include error details in protovalidate responses #714

Merged
merged 3 commits into from
May 25, 2024

Conversation

akshayjshah
Copy link
Contributor

This PR improves the protovalidate middleware:

  • First, it removes a potential panic from the streaming interceptor.
  • Second, it uses error details to return structured information to callers
    when message validation fails. This allows clients to know which field(s) of
    the request were invalid and which validation rules failed.

Speaking as one of the protovalidate authors, we intended validation errors to
be used as gRPC error details. I didn't notice that we weren't sending them
over the wire when I first reviewed this code.

Changes

Verification

The info struct is shared between all these subtests - there's no need
to clutter up each test by redefining it.

Signed-off-by: Akshay Shah <[email protected]>
The streaming interceptor should match the behavior of the unary
interceptor and gracefully handle non-protobuf messages.

Signed-off-by: Akshay Shah <[email protected]>
Amend the unary and streaming interceptors to send validation errors to
the client as an error detail. This allows client code to easily parse
and work with the structured validation information: for example, a UI
might want to display validation errors next to the relevant fields in a
form.

Signed-off-by: Akshay Shah <[email protected]>
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Good to see you Akshay 😄. LGTM!

@@ -9,16 +9,19 @@ import (
"net"
"testing"

"buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice 😄

@johanbrandhorst johanbrandhorst merged commit 8036513 into grpc-ecosystem:main May 25, 2024
5 checks passed
@akshayjshah
Copy link
Contributor Author

Thanks for the quick review, @johanbrandhorst! I also noticed another bug in the streaming interceptor, but I'll fix that in a separate PR.

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