Expand buf lint PROTOVALIDATE rule to lint example and predefined rules

.golangci.yml

@@ -342,3 +342,9 @@ issues:
       # to set the source path for the location, this operation should be safe.
       path: private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/cel.go
       text: "G115:"
+    - 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

@@ -7,8 +7,8 @@ toolchain go1.23.1
 require (
 	buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go v1.34.2-20240920201003-06b7587d0fdf.2
 	buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.34.2-20240920164238-5a7b106cbb87.2
-	buf.build/gen/go/bufbuild/registry/connectrpc/go v1.17.0-20240821192916-45ba72cdd479.1
-	buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.34.2-20240821192916-45ba72cdd479.2
+	buf.build/gen/go/bufbuild/registry/connectrpc/go v1.17.0-20240925012807-1610ffa05635.1
+	buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.34.2-20240925012807-1610ffa05635.2
 	buf.build/go/bufplugin v0.3.0
 	buf.build/go/protoyaml v0.2.0
 	connectrpc.com/connect v1.17.0
@@ -114,7 +114,7 @@ require (
 	go.uber.org/mock v0.4.0 // indirect
 	golang.org/x/sys v0.25.0 // indirect
 	golang.org/x/text v0.18.0 // indirect
-	google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 // indirect
-	google.golang.org/genproto/googleapis/rpc v0.0.0-20240903143218-8af14fe29dc1 // indirect
+	google.golang.org/genproto/googleapis/api v0.0.0-20240924160255-9d4c2d233b61 // indirect
+	google.golang.org/genproto/googleapis/rpc v0.0.0-20240924160255-9d4c2d233b61 // indirect
 	google.golang.org/grpc v1.67.0 // indirect
 )

go.sum

@@ -2,10 +2,10 @@ buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go v1.34.2-20240920201003-06
 buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go v1.34.2-20240920201003-06b7587d0fdf.2/go.mod h1:B+9TKHRYqoAUW57pLjhkLOnBCu0DQYMV+f7imQ9nXwI=
 buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.34.2-20240920164238-5a7b106cbb87.2 h1:hl0FrmGlNpQZIGvU1/jDz0lsPDd0BhCE0QDRwPfLZcA=
 buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.34.2-20240920164238-5a7b106cbb87.2/go.mod h1:ylS4c28ACSI59oJrOdW4pHS4n0Hw4TgSPHn8rpHl4Yw=
-buf.build/gen/go/bufbuild/registry/connectrpc/go v1.17.0-20240821192916-45ba72cdd479.1 h1:XFW5E+4pdF3SY0MRtnID+Ycv2QdH4OKVtoLCLsQoXac=
-buf.build/gen/go/bufbuild/registry/connectrpc/go v1.17.0-20240821192916-45ba72cdd479.1/go.mod h1:uYzs3OHgVkINg9WZ/ISnGzOUaKfQ9Q70n6dascgz5lo=
-buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.34.2-20240821192916-45ba72cdd479.2 h1:C3CTZTucEUm7i0O2tAM8GSlg23GnQYcljX1b1Jcpsro=
-buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.34.2-20240821192916-45ba72cdd479.2/go.mod h1:psseUmlKRo9v5LZJtR/aTpdTLuyp9o3X7rnLT87SZEo=
+buf.build/gen/go/bufbuild/registry/connectrpc/go v1.17.0-20240925012807-1610ffa05635.1 h1:p4A9QnhBrKjCquBt1mKqfO37QseLwgWqQp+Wb9ZjasE=
+buf.build/gen/go/bufbuild/registry/connectrpc/go v1.17.0-20240925012807-1610ffa05635.1/go.mod h1:7WtU+waNF+dyxDsuNaqmG3d0w3y2poNju8cvun1/jLs=
+buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.34.2-20240925012807-1610ffa05635.2 h1:3sSS9z8k6zVe7rNNt9R6DN2fOFBVClEflmICIjbXwms=
+buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.34.2-20240925012807-1610ffa05635.2/go.mod h1:psseUmlKRo9v5LZJtR/aTpdTLuyp9o3X7rnLT87SZEo=
 buf.build/gen/go/pluginrpc/pluginrpc/protocolbuffers/go v1.34.2-20240828222655-5345c0a56177.2 h1:oSi+Adw4xvIjXrW8eY8QGR3sBdfWeY5HN/RefnRt52M=
 buf.build/gen/go/pluginrpc/pluginrpc/protocolbuffers/go v1.34.2-20240828222655-5345c0a56177.2/go.mod h1:GjH0gjlY/ns16X8d6eaXV2W+6IFwsO5Ly9WVnzyd1E0=
 buf.build/go/bufplugin v0.3.0 h1:0kxad9/q3nCEbHVYR02crapmRyVT0zTqC/gNkzxuWSs=
@@ -355,10 +355,10 @@ google.golang.org/appengine v1.4.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7
 google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
 google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55/go.mod h1:DMBHOl98Agz4BDEuKkezgsaosCRResVns1a3J2ZsMNc=
 google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013/go.mod h1:NbSheEEYHJ7i3ixzK3sjbqSGDJWnxyFXZblF3eUsNvo=
-google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 h1:hjSy6tcFQZ171igDaN5QHOw2n6vx40juYbC/x67CEhc=
-google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1/go.mod h1:qpvKtACPCQhAdu3PyQgV4l3LMXZEtft7y8QcarRsp9I=
-google.golang.org/genproto/googleapis/rpc v0.0.0-20240903143218-8af14fe29dc1 h1:pPJltXNxVzT4pK9yD8vR9X75DaWYYmLGMsEvBfFQZzQ=
-google.golang.org/genproto/googleapis/rpc v0.0.0-20240903143218-8af14fe29dc1/go.mod h1:UqMtugtsSgubUsoxbuAoiCXvqvErP7Gf0so0mK9tHxU=
+google.golang.org/genproto/googleapis/api v0.0.0-20240924160255-9d4c2d233b61 h1:pAjq8XSSzXoP9ya73v/w+9QEAAJNluLrpmMq5qFJQNY=
+google.golang.org/genproto/googleapis/api v0.0.0-20240924160255-9d4c2d233b61/go.mod h1:O6rP0uBq4k0mdi/b4ZEMAZjkhYWhS815kCvaMha4VN8=
+google.golang.org/genproto/googleapis/rpc v0.0.0-20240924160255-9d4c2d233b61 h1:N9BgCIAUvn/M+p4NJccWPWb3BWh88+zyL0ll9HgbEeM=
+google.golang.org/genproto/googleapis/rpc v0.0.0-20240924160255-9d4c2d233b61/go.mod h1:UqMtugtsSgubUsoxbuAoiCXvqvErP7Gf0so0mK9tHxU=
 google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c=
 google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg=
 google.golang.org/grpc v1.25.1/go.mod h1:c3i+UQWmh7LiEpx4sFZnkU36qjEYZ0imhYfXVyQciAY=

make/buf/all.mk

@@ -43,7 +43,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
 

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

@@ -31,6 +31,7 @@ import (
 	"github.com/bufbuild/buf/private/pkg/slicesext"
 	"github.com/bufbuild/buf/private/pkg/stringutil"
 	"google.golang.org/protobuf/reflect/protoreflect"
+	"google.golang.org/protobuf/reflect/protoregistry"
 	"google.golang.org/protobuf/types/descriptorpb"
 )
 
@@ -935,17 +936,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 +962,72 @@ 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...,
-		)
+	// Predefined rules are checked first because predefined rules from all files, are added
+	// to an extension resolver and used to resolve rules when checking fields.
+	extensionTypesFromRequest := new(protoregistry.Types)
+	// This for-loop checks that predefined rules have cel expressions that compile and adds
+	// the ones that compile to the extension resolver, as a side effect. These types are relied
+	// on to check the example values for fields.
+	for _, file := range request.ProtosourceFiles() {
+		// We check all predefined rules for all files and add them to the extension resolver
+		// if they compile, regardless if the file is an import or not. This is because a non-import
+		// file may use a predefined rule from an import file.
+		// However, we only add check annotations for non-import files.
+		if err := bufprotosource.ForEachMessage(
+			func(message bufprotosource.Message) error {
+				for _, extension := range message.Extensions() {
+					if err := buflintvalidate.CheckAndRegisterPredefinedRuleExtension(
+						addAnnotationFunc,
+						extension,
+						extensionTypesFromRequest,
+						file.IsImport(),
+					); err != nil {
+						return err
+					}
+				}
+				return nil
+			},
+			file,
+		); err != nil {
+			return err
+		}
+		for _, extension := range file.Extensions() {
+			if err := buflintvalidate.CheckAndRegisterPredefinedRuleExtension(
+				addAnnotationFunc,
+				extension,
+				extensionTypesFromRequest,
+				file.IsImport(),
+			); err != nil {
+				return err
+			}
+		}
+	}
+	if err := bufcheckserverutil.NewLintMessageRuleHandler(
+		func(
+			// The responseWriter is being passed in through the shared addAnnotationFunc, so we
+			// do not pass in responseWriter and request again. This should be addressed in a refactor.
+			_ bufcheckserverutil.ResponseWriter,
+			_ bufcheckserverutil.Request,
+			message bufprotosource.Message,
+		) error {
+			return buflintvalidate.CheckMessage(addAnnotationFunc, message)
+		}).Handle(ctx, nil, nil); err != nil {
+		return err
+	}
+	// At this point the extension types are already populated.
+	if err := bufcheckserverutil.NewLintFieldRuleHandler(
+		func(
+			// The responseWriter is being passed in through the shared addAnnotationFunc, so we
+			// do not pass in responseWriter and request again. This should be addressed in a refactor.
+			_ bufcheckserverutil.ResponseWriter,
+			_ bufcheckserverutil.Request,
+			field bufprotosource.Field,
+		) error {
+			return buflintvalidate.CheckField(addAnnotationFunc, field, extensionTypesFromRequest)
+		}).Handle(ctx, nil, nil); err != nil {
+		return err
 	}
-	return buflintvalidate.CheckField(addAnnotationFunc, field)
+	return nil
 }
 
 // HandleLintRPCNoClientStreaming is a handle function.

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

@@ -18,11 +18,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)
+}
+
+// CheckAndRegisterPredefinedRuleExtension 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 CheckAndRegisterPredefinedRuleExtension(
+	addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),
+	field bufprotosource.Field,
+	extensionTypesToPopulate *protoregistry.Types,
+	fileIsImport bool,
+) error {
+	return checkAndRegisterPredefinedRuleExtension(addAnnotationFunc, field, extensionTypesToPopulate, fileIsImport)
+}
+
 // 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.
@@ -55,13 +75,14 @@ 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 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

@@ -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