Expand buf lint PROTOVALIDATE rule to lint example and predefined rules

go.mod

@@ -120,3 +120,9 @@ require (
  google.golang.org/genproto/googleapis/rpc v0.0.0-20240903143218-8af14fe29dc1 // indirect
  google.golang.org/grpc v1.66.0 // indirect
 )
+
+// TODO: remove
+replace github.com/bufbuild/protovalidate-go => ../pvgojohn // commit e474f9b456a57181c0c8ad6785be9c9c2ab0a0be
+
+// TODO: remove
+replace buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go => ../pv-gen // commit 4fd7a369f04577fc71d8ff33ed733105aae84894

go.sum

@@ -1,7 +1,5 @@
 buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go v1.34.2-20240904181154-a0be11449112.2 h1:X9qBPcvWGOJs/CeRVLoxxLJwC/eKyWDS/G4nj+3KGMY=
 buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go v1.34.2-20240904181154-a0be11449112.2/go.mod h1:B+9TKHRYqoAUW57pLjhkLOnBCu0DQYMV+f7imQ9nXwI=
-buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.34.2-20240717164558-a6c49f84cc0f.2 h1:SZRVx928rbYZ6hEKUIN+vtGDkl7uotABRWGY4OAg5gM=
-buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.34.2-20240717164558-a6c49f84cc0f.2/go.mod h1:ylS4c28ACSI59oJrOdW4pHS4n0Hw4TgSPHn8rpHl4Yw=
 buf.build/gen/go/bufbuild/registry/connectrpc/go v1.16.2-20240821192916-45ba72cdd479.1 h1:QaJ6UkpvlGo4dBXR41vLRfPiKungbg7brjmbBC/k6Ig=
 buf.build/gen/go/bufbuild/registry/connectrpc/go v1.16.2-20240821192916-45ba72cdd479.1/go.mod h1:oQsMFNU3YzxxjRS6O68UkcF/A+pXdXqQNcUfQEBTWcw=
 buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.34.2-20240821192916-45ba72cdd479.2 h1:C3CTZTucEUm7i0O2tAM8GSlg23GnQYcljX1b1Jcpsro=
@@ -30,8 +28,6 @@ github.com/bufbuild/protocompile v0.14.1 h1:iA73zAf/fyljNjQKwYzUHD6AD4R8KMasmwa/
 github.com/bufbuild/protocompile v0.14.1/go.mod h1:ppVdAIhbr2H8asPk6k4pY7t9zB1OU5DoEw9xY/FUi1c=
 github.com/bufbuild/protoplugin v0.0.0-20240323223605-e2735f6c31ee h1:E6ET8YUcYJ1lAe6ctR3as7yqzW2BNItDFnaB5zQq/8M=
 github.com/bufbuild/protoplugin v0.0.0-20240323223605-e2735f6c31ee/go.mod h1:HjGFxsck9RObrTJp2hXQZfWhPgZqnR6sR1U5fCA/Kus=
-github.com/bufbuild/protovalidate-go v0.6.5 h1:WucDKXIbK22WjkO8A8J6Yyxxy0jl91Oe9LSMduq3YEE=
-github.com/bufbuild/protovalidate-go v0.6.5/go.mod h1:LHDiGCWSM3GagZEnyEZ1sPtFwi6Ja4tVTi/DCc+iDFI=
 github.com/bufbuild/protoyaml-go v0.1.12 h1:tIJrwvGxumVpNwLsw/AevT1QnkPDBuAObBSuBAdmAWY=
 github.com/bufbuild/protoyaml-go v0.1.12/go.mod h1:Xmz3wct+08Va+g9gjIuLTAmxW2w6sre5Wrgw7K3gn0I=
 github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8=

private/bufpkg/bufcheck/bufcheckserver/internal/bufcheckserverhandle/lint.go

@@ -31,6 +31,7 @@ import (
  "github.com/bufbuild/buf/private/pkg/stringutil"
  "github.com/bufbuild/bufplugin-go/check"
  "google.golang.org/protobuf/reflect/protoreflect"
+ "google.golang.org/protobuf/reflect/protoregistry"
  "google.golang.org/protobuf/types/descriptorpb"
 )
 
@@ -935,56 +936,90 @@ func handleLintPackageVersionSuffix(
 }
 
 // HandleLintProtovalidate is a handle function.
-var HandleLintProtovalidate = bufcheckserverutil.NewMultiHandler(
- // NOTE: Oneofs also have protovalidate support, but they only have a "required" field, so nothing to lint.
- bufcheckserverutil.NewLintMessageRuleHandler(handleLintMessageProtovalidate),
- bufcheckserverutil.NewLintFieldRuleHandler(handleLintFieldProtovalidate),
+var HandleLintProtovalidate = bufcheckserverutil.NewRuleHandler(
+ func(
+ ctx context.Context,
+ responseWriter bufcheckserverutil.ResponseWriter,
+ request bufcheckserverutil.Request,
+ ) error {
+ // TODO: refactor so that add func is no longer needed
+ addAnnotationFunc := func(
+ _ bufprotosource.Descriptor,
+ location bufprotosource.Location,
+ _ []bufprotosource.Location,
+ format string,
+ args ...interface{},
+ ) {
+ responseWriter.AddProtosourceAnnotation(
+ location,
+ nil,
+ format,
+ args...,
+ )
+ }
+ extensionTypesFromRequest := new(protoregistry.Types)
+ // This for-loop checks that shared rules' cel expressions compile and add
+ // those that compile to the extension types, as a side effect. These types
+ // will be useful later on when example values are checked. Therefore, this
+ // loop must run before field and messages are checked.
+ for _, file := range request.ProtosourceFiles() {
+ // Regardless whether its file is an import, we want to add a shared rule
+ // extension to the types registry, as long as the extension's cel
+ // expressions compile.
+ // TODO: pass a nop addFunc the file is an import,
+ if err := bufprotosource.ForEachMessage(
+ func(message bufprotosource.Message) error {
+ for _, extension := range message.Extensions() {
+ if err := buflintvalidate.CheckAndRegisterSharedRuleExtension(
+ addAnnotationFunc,
+ extension,
+ extensionTypesFromRequest,
+ ); err != nil {
+ return nil
+ }
+ }
+ return nil
+ },
+ file,
+ ); err != nil {
+ return err
+ }
+ for _, extension := range file.Extensions() {
+ if err := buflintvalidate.CheckAndRegisterSharedRuleExtension(
+ addAnnotationFunc,
+ extension,
+ extensionTypesFromRequest,
+ ); err != nil {
+ return nil
+ }
+ }
+ }
+ if err := bufcheckserverutil.NewLintMessageRuleHandler(
+ func(
+ _ bufcheckserverutil.ResponseWriter,
+ _ bufcheckserverutil.Request,
+ message bufprotosource.Message,
+ ) error {
+ return buflintvalidate.CheckMessage(addAnnotationFunc, message)
+ }).Handle(ctx, responseWriter, request); err != nil {
+ return err
+ }
+ // At this point the extension types are already populated.
+ if err := bufcheckserverutil.NewLintFieldRuleHandler(
+ func(
+ _ bufcheckserverutil.ResponseWriter,
+ _ bufcheckserverutil.Request,
+ field bufprotosource.Field,
+ ) error {
+ return buflintvalidate.CheckField(addAnnotationFunc, field, extensionTypesFromRequest)
+ }).Handle(ctx, responseWriter, request); err != nil {
+ return err
+ }
+ // NOTE: Oneofs also have protovalidate support, but they only have a "required" field, so nothing to lint.
+ return nil
+ },
 )
 
-func handleLintMessageProtovalidate(
- responseWriter bufcheckserverutil.ResponseWriter,
- _ bufcheckserverutil.Request,
- message bufprotosource.Message,
-) error {
- addAnnotationFunc := func(
- _ bufprotosource.Descriptor,
- location bufprotosource.Location,
- _ []bufprotosource.Location,
- format string,
- args ...interface{},
- ) {
- responseWriter.AddProtosourceAnnotation(
- location,
- nil,
- format,
- args...,
- )
- }
- return buflintvalidate.CheckMessage(addAnnotationFunc, message)
-}
-
-func handleLintFieldProtovalidate(
- responseWriter bufcheckserverutil.ResponseWriter,
- _ bufcheckserverutil.Request,
- field bufprotosource.Field,
-) error {
- addAnnotationFunc := func(
- _ bufprotosource.Descriptor,
- location bufprotosource.Location,
- _ []bufprotosource.Location,
- format string,
- args ...interface{},
- ) {
- responseWriter.AddProtosourceAnnotation(
- location,
- nil,
- format,
- args...,
- )
- }
- return buflintvalidate.CheckField(addAnnotationFunc, field)
-}
-
 // HandleLintRPCNoClientStreaming is a handle function.
 var HandleLintRPCNoClientStreaming = bufcheckserverutil.NewLintMethodRuleHandler(handleLintRPCNoClientStreaming)
 

private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/buflintvalidate.go

@@ -17,12 +17,31 @@
 import (
  "buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate"
  "github.com/bufbuild/buf/private/bufpkg/bufprotosource"
  "github.com/bufbuild/protovalidate-go/resolver"
+ "google.golang.org/protobuf/reflect/protoreflect"
+ "google.golang.org/protobuf/reflect/protoregistry"
 )
 
 // https://buf.build/bufbuild/protovalidate/docs/v0.5.1:buf.validate#buf.validate.MessageConstraints
 const disabledFieldNumberInMesageConstraints = 1
 
+// ExtensionTypeResolver is an extension resolver, the same type as the Resolver in proto.UnmarshalOptions.
+type ExtensionTypeResolver interface {
+ FindExtensionByName(field protoreflect.FullName) (protoreflect.ExtensionType, error)
+ FindExtensionByNumber(message protoreflect.FullName, field protoreflect.FieldNumber) (protoreflect.ExtensionType, error)
+}
+
+// CheckAndRegisterSharedRuleExtension checks whether an extension extending a protovalidate rule
+// is valid, checking that all of its CEL expressionus compile. If so, the extension type is added to
+// the extension types passed in.
+func CheckAndRegisterSharedRuleExtension(
+ addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),
+ field bufprotosource.Field,
+ extensionTypesToPopulate *protoregistry.Types,
+) error {
+ return checkAndRegisterSharedRuleExtension(addAnnotationFunc, field, extensionTypesToPopulate)
+}
+
 // CheckMessage validates that all rules on the message are valid, and any CEL expressions compile.
 //
 // addAnnotationFunc adds an annotation with the descriptor and location for check results.
@@ -62,6 +81,7 @@
 func CheckField(
  addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),
  field bufprotosource.Field,
+ extensionTypeResolver ExtensionTypeResolver,
 ) error {
- return checkField(addAnnotationFunc, field)
+ return checkField(addAnnotationFunc, field, extensionTypeResolver)
 }

private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/cel.go

@@ -20,7 +20,7 @@
 
  "buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate"
  "github.com/bufbuild/buf/private/bufpkg/bufprotosource"
  "github.com/bufbuild/protovalidate-go/celext"
  "github.com/google/cel-go/cel"
  "github.com/google/cel-go/common/types"
  "google.golang.org/protobuf/reflect/protoreflect"
@@ -109,14 +109,16 @@
  return nil
 }
 
+// Returns true only if all cel expressions compile
 func checkCEL(
  celEnv *cel.Env,
  celConstraints []*validate.Constraint,
  parentName string,
  parentNameCapitalized string,
  celName string,
  add func(int, string, ...interface{}),
-) {
+) bool {
+ allCelExpressionsCompile := true
  idToConstraintIndices := make(map[string][]int, len(celConstraints))
  for i, celConstraint := range celConstraints {
  if celID := celConstraint.GetId(); celID != "" {
@@ -142,14 +144,14 @@
  } else {
  add(i, "%s has an empty %s.id. IDs should always be specified.", parentNameCapitalized, celName)
  }
- if len(strings.TrimSpace(celConstraint.Expression)) == 0 {
+ if len(strings.TrimSpace(celConstraint.GetExpression())) == 0 {
  add(i, "%s has an empty %s.expression. Expressions should always be specified.", parentNameCapitalized, celName)
  continue
  }
- ast, compileIssues := celEnv.Compile(celConstraint.Expression)
+ ast, compileIssues := celEnv.Compile(celConstraint.GetExpression())
  switch {
  case ast.OutputType().IsAssignableType(cel.BoolType):
- if celConstraint.Message == "" {
+ if celConstraint.GetMessage() == "" {
  add(
  i,
  "%s has an empty %s.message for an expression that evaluates to a boolean. If an expression evaluates to a boolean, a message should always be specified.",
@@ -158,7 +160,7 @@
  )
  }
  case ast.OutputType().IsAssignableType(cel.StringType):
- if celConstraint.Message != "" {
+ if celConstraint.GetMessage() != "" {
  add(
  i,
  "%s has a %s with an expression that evaluates to a string, and also has a message. The message is redundant - since the expression evaluates to a string, its result will be printed instead of the message, so the message should be removed.",
@@ -179,6 +181,7 @@
  )
  }
  if compileIssues.Err() != nil {
+ allCelExpressionsCompile = false
  for _, parsedIssue := range parseCelIssuesText(compileIssues.Err().Error()) {
  add(
  i,
@@ -204,6 +207,7 @@
  )
  }
  }
+ return allCelExpressionsCompile
 }
 
 // this depends on the undocumented behavior of cel-go's error message