Expand buf lint PROTOVALIDATE rule to lint example and predefined rul…

…es (#3317)

.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:"

CHANGELOG.md

@@ -2,7 +2,10 @@
 
 ## [Unreleased]
 
-- No changes yet.
+- Update the `PROTOVALIDATE` lint rule to check example field options. Examples will be checked that
+  they satisfy the field constraints, and are only present if constraints are present.
+- Update the `PROTOVALIDATE` lint rule to check predefined rules. Predefined rules will be checked
+  that they compile.
 
 ## [v1.43.0] - 2024-09-30
 

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