Expand buf lint PROTOVALIDATE rule to lint example and predefined rules

.golangci.yml

@@ -410,3 +410,9 @@ issues:
  - forbidigo
  path: private/pkg/protoencoding
  text: "prototext.Unmarshal"
+ - linters:
+ - gosec
+ # This checks the cel constraints for predefined rules from an Image, and converts loop indices to int32
+ # to set the source path for the location, this operation should be safe.
+ path: private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/predefined_rules.go
+ text: "G115:"

go.mod

@@ -16,7 +16,7 @@ require (
  connectrpc.com/otelconnect v0.7.1
  github.com/bufbuild/protocompile v0.14.1
  github.com/bufbuild/protoplugin v0.0.0-20240911180120-7bb73e41a54a
- github.com/bufbuild/protovalidate-go v0.7.0
+ github.com/bufbuild/protovalidate-go v0.7.1
  github.com/docker/docker v27.3.1+incompatible
  github.com/go-chi/chi/v5 v5.1.0
  github.com/gofrs/flock v0.12.1

go.sum

@@ -34,8 +34,8 @@ 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-20240911180120-7bb73e41a54a h1:l3RhVoG0RtC61h6TVWnkniGj4TgBebuyPQRdleFAmTg=
 github.com/bufbuild/protoplugin v0.0.0-20240911180120-7bb73e41a54a/go.mod h1:c5D8gWRIZ2HLWO3gXYTtUfw/hbJyD8xikv2ooPxnklQ=
-github.com/bufbuild/protovalidate-go v0.7.0 h1:MYU9GSZM7TSsWNywvyXoEc8y3kc1MNqD3k5mddIBEL4=
-github.com/bufbuild/protovalidate-go v0.7.0/go.mod h1:PHV5pFuWlRzdDW02/cmVyNzdiQ+RNNwo7idGxdzS7o4=
+github.com/bufbuild/protovalidate-go v0.7.1 h1:ac50NTO6+1+mKg5sP/GBPLlMkQFeI+OeaYGFdS1vu98=
+github.com/bufbuild/protovalidate-go v0.7.1/go.mod h1:PHV5pFuWlRzdDW02/cmVyNzdiQ+RNNwo7idGxdzS7o4=
 github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8=
 github.com/cenkalti/backoff/v4 v4.3.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=

make/buf/all.mk

@@ -42,7 +42,7 @@ LICENSE_HEADER_LICENSE_TYPE := apache
 LICENSE_HEADER_COPYRIGHT_HOLDER := Buf Technologies, Inc.
 LICENSE_HEADER_YEAR_RANGE := 2020-2024
 LICENSE_HEADER_IGNORES := \/testdata enterprise
-PROTOVALIDATE_VERSION := v0.7.1
+PROTOVALIDATE_VERSION := v0.8.1
 # Comment out to use released buf
 BUF_GO_INSTALL_PATH := ./cmd/buf
 
@@ -144,6 +144,9 @@ bufgeneratebuflinttestdata:
  $(BUF_BIN) export \
  buf.build/bufbuild/protovalidate:$(PROTOVALIDATE_VERSION) \
  --output private/bufpkg/bufcheck/testdata/lint/protovalidate/vendor/protovalidate
+ $(BUF_BIN) export \
+ buf.build/bufbuild/protovalidate:$(PROTOVALIDATE_VERSION) \
+ --output private/bufpkg/bufcheck/testdata/lint/protovalidate_predefined/vendor/protovalidate
 
 bufgeneratesteps:: \
  bufgeneratego \

private/buf/cmd/buf/buf_test.go

@@ -2907,8 +2907,7 @@ testdata/check_plugins/current/proto/api/v1/service.proto:17:14:RPC request type
 testdata/check_plugins/current/proto/api/v1/service.proto:17:42:RPC response type "GetFooTestResponse" should be named "GetFooResponse" or "FooServiceTestGetFooResponse".
 testdata/check_plugins/current/proto/api/v1/service.proto:26:1:"ListFooResponse" is a pagination response without a page token field named "page_token" (buf-plugin-rpc-ext)
 testdata/check_plugins/current/proto/common/v1alpha1/messages.proto:16:5:field "common.v1alpha1.Four.FourTwo.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
-testdata/check_plugins/current/vendor/protovalidate/buf/validate/expression.proto:42:3:field "buf.validate.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
-testdata/check_plugins/current/vendor/protovalidate/buf/validate/priv/private.proto:38:3:field "buf.validate.priv.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
+testdata/check_plugins/current/vendor/protovalidate/buf/validate/validate.proto:94:3:field "buf.validate.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
  `),
  "lint",
  filepath.Join("testdata", "check_plugins", "current"),
@@ -2934,8 +2933,7 @@ testdata/check_plugins/current/proto/api/v1/service.proto:17:14:RPC request type
 testdata/check_plugins/current/proto/api/v1/service.proto:17:42:RPC response type "GetFooTestResponse" should be named "GetFooResponse" or "FooServiceTestGetFooResponse".
 testdata/check_plugins/current/proto/api/v1/service.proto:26:1:"ListFooResponse" is a pagination response without a page token field named "page_token" (buf-plugin-rpc-ext)
 testdata/check_plugins/current/proto/common/v1alpha1/messages.proto:16:5:field "common.v1alpha1.Four.FourTwo.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
-testdata/check_plugins/current/vendor/protovalidate/buf/validate/expression.proto:42:3:field "buf.validate.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
-testdata/check_plugins/current/vendor/protovalidate/buf/validate/priv/private.proto:38:3:field "buf.validate.priv.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
+testdata/check_plugins/current/vendor/protovalidate/buf/validate/validate.proto:94:3:field "buf.validate.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
  `),
  "lint",
  filepath.Join("testdata", "check_plugins", "current"),
@@ -2985,8 +2983,7 @@ testdata/check_plugins/current/vendor/protovalidate/buf/validate/priv/private.pr
  filepath.FromSlash(`
 testdata/check_plugins/current/proto/api/v1/service.proto:11:1:Service name "api.v1.FooServiceMock" has banned suffix "Mock". (buf-plugin-suffix)
 testdata/check_plugins/current/proto/common/v1alpha1/messages.proto:16:5:field "common.v1alpha1.Four.FourTwo.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
-testdata/check_plugins/current/vendor/protovalidate/buf/validate/expression.proto:42:3:field "buf.validate.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
-testdata/check_plugins/current/vendor/protovalidate/buf/validate/priv/private.proto:38:3:field "buf.validate.priv.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
+testdata/check_plugins/current/vendor/protovalidate/buf/validate/validate.proto:94:3:field "buf.validate.Constraint.id" does not have rule (buf.validate.field).string.tuuid set (buf-plugin-protovalidate-ext)
  `),
  "lint",
  filepath.Join("testdata", "check_plugins", "current"),

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

@@ -27,6 +27,8 @@ import (
  "github.com/bufbuild/buf/private/bufpkg/bufcheck/internal/bufcheckopt"
  "github.com/bufbuild/buf/private/bufpkg/bufprotosource"
  "github.com/bufbuild/buf/private/pkg/normalpath"
+ "github.com/bufbuild/buf/private/pkg/protodescriptor"
+ "github.com/bufbuild/buf/private/pkg/protoencoding"
  "github.com/bufbuild/buf/private/pkg/protoversion"
  "github.com/bufbuild/buf/private/pkg/slicesext"
  "github.com/bufbuild/buf/private/pkg/stringutil"
@@ -935,17 +937,18 @@ 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(handleLintProtovalidate)
 
-func handleLintMessageProtovalidate(
+// handleLintProtovalidate runs checks all predefined rules, message rules, and field rules.
+//
+// NOTE: Oneofs also have protovalidate support, but they only have a "required" field, so nothing to lint.
+func handleLintProtovalidate(
+ ctx context.Context,
  responseWriter bufcheckserverutil.ResponseWriter,
- _ bufcheckserverutil.Request,
- message bufprotosource.Message,
+ request bufcheckserverutil.Request,
 ) error {
+ // TODO: addAnnotationFunc is used to set add annotations to responseWriter. A follow-up
+ // will be made to refactor the code so we no longer need this.
  addAnnotationFunc := func(
  _ bufprotosource.Descriptor,
  location bufprotosource.Location,
@@ -960,29 +963,49 @@ func handleLintMessageProtovalidate(
  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...,
- )
+ // We create a new extension resolver using all of the files from the request, including
+ // import files. This is because there can be a case where a non-import file uses a predefined
+ // rule from an imported file.
+ extensionResolver, err := protoencoding.NewResolver(
+ slicesext.Map(
+ request.ProtosourceFiles(),
+ func(protosourceFile bufprotosource.File) protodescriptor.FileDescriptor {
+ return protosourceFile.FileDescriptor()
+ },
+ )...,
+ )
+ if err != nil {
+ return err
+ }
+ // However, we only want to check non-import files, so we can use NewLintMessageRuleHandler
+ // and NewLintFieldRuleHandler utils to check messages and fields respectively.
+ if err := bufcheckserverutil.NewLintMessageRuleHandler(
+ func(
+ _ bufcheckserverutil.ResponseWriter,
+ _ bufcheckserverutil.Request,
+ message bufprotosource.Message,
+ ) error {
+ return buflintvalidate.CheckMessage(addAnnotationFunc, message)
+ },
+ // The responseWriter is being passed in through the shared addAnnotationFunc, so we
+ // do not pass in responseWriter again. This should be addressed in a refactor.
+ ).Handle(ctx, nil, request); err != nil {
+ return err
  }
- return buflintvalidate.CheckField(addAnnotationFunc, field)
+ return bufcheckserverutil.NewLintFieldRuleHandler(
+ func(
+ _ bufcheckserverutil.ResponseWriter,
+ _ bufcheckserverutil.Request,
+ field bufprotosource.Field,
+ ) error {
+ if err := buflintvalidate.CheckPredefinedRuleExtension(addAnnotationFunc, field, extensionResolver); err != nil {
+ return err
+ }
+ return buflintvalidate.CheckField(addAnnotationFunc, field, extensionResolver)
+ },
+ // The responseWriter is being passed in through the shared addAnnotationFunc, so we
+ // do not pass in responseWriter again. This should be addressed in a refactor.
+ ).Handle(ctx, nil, request)
 }
 
 // HandleLintRPCNoClientStreaming is a handle function.

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

@@ -17,16 +17,17 @@ package buflintvalidate
 import (
  "buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate"
  "github.com/bufbuild/buf/private/bufpkg/bufprotosource"
+ "github.com/bufbuild/buf/private/pkg/protoencoding"
  "github.com/bufbuild/protovalidate-go/resolver"
 )
 
 // https://buf.build/bufbuild/protovalidate/docs/v0.5.1:buf.validate#buf.validate.MessageConstraints
 const disabledFieldNumberInMesageConstraints = 1
 
 // 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.
+// It also checks all predefined rule extensions on the messages.
 func CheckMessage(
+ // addAnnotationFunc adds an annotation with the descriptor and location for check results.
  addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),
  message bufprotosource.Message,
 ) error {
@@ -55,13 +56,23 @@ func CheckMessage(
 // CheckField validates that all rules on the field are valid, and any CEL expressions compile.
 //
 // For a set of rules to be valid, it must
-// 1. permit _some_ value
+// 1. permit _some_ value and all example values, if any
 // 2. have a type compatible with the field it validates.
-//
-// addAnnotationFunc adds an annotation with the descriptor and location for check results.
 func CheckField(
+ // addAnnotationFunc adds an annotation with the descriptor and location for check results.
+ addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),
+ field bufprotosource.Field,
+ extensionTypeResolver protoencoding.Resolver,
+) error {
+ return checkField(addAnnotationFunc, field, extensionTypeResolver)
+}
+
+// CheckPredefinedRuleExtension checks that a predefined extension is valid, and any CEL expressions compile.
+func CheckPredefinedRuleExtension(
+ // addAnnotationFunc adds an annotation with the descriptor and location for check results.
  addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),
  field bufprotosource.Field,
+ extensionResolver protoencoding.Resolver,
 ) error {
- return checkField(addAnnotationFunc, field)
+ return checkPredefinedRuleExtension(addAnnotationFunc, field, extensionResolver)
 }

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

@@ -109,14 +109,16 @@ func checkCELForField(
  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,7 +144,7 @@ func checkCEL(
  } else {
  add(i, "%s has an empty %s.id. IDs should always be specified.", parentNameCapitalized, celName)
  }
- if celConstraint.GetExpression() == "" {
+ if len(strings.TrimSpace(celConstraint.GetExpression())) == 0 {
  add(i, "%s has an empty %s.expression. Expressions should always be specified.", parentNameCapitalized, celName)
  continue
  }
@@ -179,6 +181,7 @@ func checkCEL(
  )
  }
  if compileIssues.Err() != nil {
+ allCelExpressionsCompile = false
  for _, parsedIssue := range parseCelIssuesText(compileIssues.Err().Error()) {
  add(
  i,
@@ -204,6 +207,7 @@ func checkCEL(
  )
  }
  }
+ return allCelExpressionsCompile
 }
 
 // this depends on the undocumented behavior of cel-go's error message