Expand buf lint PROTOVALIDATE rule to lint example and predefined rules #3317
Conversation
private/bufpkg/bufcheck/bufcheckserver/internal/bufcheckserverhandle/lint.go
Outdated
Show resolved
Hide resolved
private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/field.go
Outdated
Show resolved
Hide resolved
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| func celTypeForStandardRuleMessageDescriptor( | ||
| ruleMessageDescriptor protoreflect.MessageDescriptor, | ||
| ) *cel.Type { |
There was a problem hiding this comment.
@rodaine Just wondering if there is a better way to do this and/or if protovalidate-go exposes some mechanism for me to resolve the "type of rule" based on the rule message descriptor (e.g. StringRules). The context of this is that, we are parsing predefined rules from our files, and then checking their CEL expressions. And so we need the type for the extension itself (which is easy, we use the celext function you and @oliversun9 exported a while ago) and we also need the type for the rule that the extension is extending.
| exampleValues []protoreflect.Value, | ||
| extensionTypeResolver protoencoding.Resolver, | ||
| ) error { | ||
| if err := protoencoding.ReparseExtensions(extensionTypeResolver, typeRulesMessage); err != nil { |
There was a problem hiding this comment.
This feels like a pretty deep level to be calling protoencoding.ReparseExtensions, to do this per field seems unnecessary, can you provide some documentation as to why we're calling this here, and why it can't be called at a higher level?
There was a problem hiding this comment.
Added a comment to clarify this, but basically it's to ensure that predefined rules from imported files are properly resolved.
There was a problem hiding this comment.
This comment doesn't make sense to me as a reader - I am still confused why I need to call ReparseExtensions for every field. Can you make sure this comment would make sense to another reader without context by verifying with someone else what their understanding of it is?
| return r.StandardConstraintResolver.ResolveFieldConstraints(desc) | ||
| } | ||
|
|
||
| func (r *constraintsResolverForTargetField) ResolveMessageConstraints(desc protoreflect.MessageDescriptor) *validate.MessageConstraints { |
There was a problem hiding this comment.
nit: no need to have r name for struct, and no need for desc either
| return nil | ||
| } | ||
|
|
||
| func (r *constraintsResolverForTargetField) ResolveOneofConstraints(desc protoreflect.OneofDescriptor) *validate.OneofConstraints { |
| // for marshalling and unmarshalling. We also added error handling for marshal/unmarshal. | ||
| // | ||
| // This resolves the given extension and returns the constraints for the extension. | ||
| func resolveExt[C proto.Message]( |
There was a problem hiding this comment.
other methods called *Extension, call this resolveExtension
| resolver protoencoding.Resolver, | ||
| ) (constraints C, retErr error) { | ||
| num := extType.TypeDescriptor().Number() | ||
| var msg proto.Message |
bufdev
changed the title
This updates the
PROTOVALIDATErule inbuf lintto provide checks for two new features:buf.validate.*Rulescompiles)The
protovalidatetest dependency has also been updated and some previous integration tests have also been updated to reflect this.