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

Protovalidate interceptor cleanup, Go version bump #721

Merged
merged 5 commits into from
Aug 16, 2024

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Aug 14, 2024

Changes

Please see individual commits.

Verification

Unit tests. No significant changes.

@ash2k ash2k force-pushed the protovalidate-cleanup branch 4 times, most recently from 19af624 to 839d345 Compare August 14, 2024 11:38
@ash2k ash2k changed the title Protovalidate interceptor cleanup Protovalidate interceptor cleanup, Go version bump Aug 14, 2024
@ash2k
Copy link
Contributor Author

ash2k commented Aug 14, 2024

Follow up questions:

  • I have a custom codec that I use with gRPC. It handles both proto and non-proto (a single type) messages. I'd like to use the protovalidate interceptors with it. Would you accept a feature to specify a list of Go types (as reflect.Type) to ignore, instead of erroring on them.
  • Would you accept a PR with client validating interceptors?

// UnaryServerInterceptor returns a new unary server interceptor that validates incoming messages.
// If the request is invalid, clients may access a structured representation of the validation failure as an error detail.
func UnaryServerInterceptor(validator *protovalidate.Validator, opts ...Option) grpc.UnaryServerInterceptor {
func UnaryServerInterceptor(validator Validator, opts ...Option) grpc.UnaryServerInterceptor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually a breaking change, and I'm not sure we need to make this change? If you want to propose this, please open an issue. If you remove this change I'm happy to merge the rest of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this commit.

@johanbrandhorst
Copy link
Collaborator

* I have a custom codec that I use with gRPC. It handles both proto and non-proto (a single type) messages. I'd like to use the protovalidate interceptors with it. Would you accept a feature to specify a list of Go types (as `reflect.Type`) to ignore, instead of erroring on them.

No, I'd rather users create their own interceptors for this sort of thing.

* Would you accept a PR with client validating interceptors?

For outgoing requests? For responses from servers? Not sure where that would go.

@ash2k ash2k force-pushed the protovalidate-cleanup branch from 839d345 to a415b61 Compare August 16, 2024 05:53
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.

LGTM

@johanbrandhorst johanbrandhorst merged commit 8de7e4a into grpc-ecosystem:main Aug 16, 2024
7 checks passed
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

@ash2k ash2k deleted the protovalidate-cleanup branch August 17, 2024 00:32
@ash2k
Copy link
Contributor Author

ash2k commented Aug 17, 2024

Thank you!

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