Update buf and use protovalidate #2596

tstirrat15 · 2025-10-08T19:40:44Z

Description

We'd like to use protovalidate for protobuf validation instead of protoc-gen-validate. This is a first step towards that in SpiceDB, replacing the validate generated code with use of protovalidate.

This also updates our generation code, using v2 of buf.gen.yaml.

Changes

Run formatting
Fix some lint issues around the naming of functions that match a Printf signature
Ran buf config migrate
Removed protoc-gen-validate from buf.yaml and added protovalidate instead
Use remote generation, like we're doing with the client libraries, instead of invoking protoc locally
Use protovalidate.Validate calls where we were previously calling .Validate() on proto messages

Testing

Review. See that tests pass.

tstirrat15

See comments

tstirrat15 · 2025-10-08T19:45:02Z

cmd/spicedb/serve_integration_test.go

-	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
-	"github.com/authzed/grpcutil"


There's a number of these changes. This is the formatter.

I don't understand, which formatter do you mean? You didn't change the Go linter rules..

golangci-lint fmt is what i'm using locally

tstirrat15 · 2025-10-08T19:45:16Z

e2e/newenemy/newenemy_test.go

 		}
 	}
-	return
+	return vulnerableFn, protectedFn


There's also several of these changes, which are also the formatter.

let's do this in a separate PR #2600

tstirrat15 · 2025-10-08T19:45:50Z

internal/datastore/common/schema.go


 func (si SchemaInformation) debugValidate() {
-	spiceerrors.DebugAssert(func() bool {
+	spiceerrors.DebugAssertf(func() bool {


This is the new linter complaint - because DebugAssert had a Printf-like signature, the linter wanted the function to end in f

tstirrat15 · 2025-10-08T19:46:00Z

internal/datastore/common/sql.go


 func (sqf SchemaQueryFilterer) UnderlyingQueryBuilder() sq.SelectBuilder {
-	spiceerrors.DebugAssert(func() bool {
+	spiceerrors.DebugAssertf(func() bool {


So there's lots of these changes

tstirrat15 · 2025-10-08T19:47:28Z

internal/testfixtures/validating.go


 	for _, ns := range read {
-		err := ns.Definition.Validate()
+		err := protovalidate.Validate(ns.Definition)


These are a part of the substantive changes in this PR - protovalidate uses reflection on the messages to figure out what the associated CEL is, and then runs those expressions from the outside, rather than invoking generated code (which is what protoc-gen-validate used to do)

tstirrat15 · 2025-10-08T20:16:29Z

proto/internal/core/v1/core.proto

-  ObjectAndRelation resource_and_relation = 1 [(validate.rules).message.required = true];
+  ObjectAndRelation resource_and_relation = 1 [(buf.validate.field).required = true];



These are changes brought by running the migration tool.

tstirrat15 · 2025-10-08T20:22:03Z

buf.yaml

This got moved down and out of the proto folder by the migration step. I think this is a fine place for it to be.

tstirrat15 · 2025-10-08T20:22:24Z

buf.work.yaml

@@ -1,4 +0,0 @@
---


No longer necessary with buf.yaml being in the root.

tstirrat15 · 2025-10-08T20:23:42Z

buf.gen.yaml

+  - remote: "buf.build/protocolbuffers/go:v1.36.10"
+    out: "pkg/proto"
+    opt: "paths=source_relative"
+  - remote: "buf.build/grpc/go:v1.5.1"
+    out: "pkg/proto"
+    opt: "paths=source_relative"


We're using the remote plugins now, similar to what we're doing with the client libraries.

tstirrat15 · 2025-10-08T23:19:54Z

proto/internal/core/v1/core.proto

-    SetOperation union = 1 [(validate.rules).message.required = true];
-    SetOperation intersection = 2 [(validate.rules).message.required = true];
-    SetOperation exclusion = 3 [(validate.rules).message.required = true];


This was a change called out by buf lint - the required fields don't mean anything on the options in a oneof, so the linter says they should be excluded.

codecov · 2025-10-08T23:28:15Z

Codecov Report

❌ Patch coverage is 73.00000% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.63%. Comparing base (ac244f9) to head (97925eb).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/datastore/mysql/watch.go	23.08%	10 Missing ⚠️
pkg/composableschemadsl/compiler/translator.go	33.34%	0 Missing and 4 partials ⚠️
internal/testfixtures/validating.go	66.67%	1 Missing and 2 partials ⚠️
pkg/schemadsl/compiler/translator.go	40.00%	0 Missing and 3 partials ⚠️
internal/datastore/mysql/gc.go	33.34%	2 Missing ⚠️
internal/datastore/postgres/revisions.go	75.00%	1 Missing ⚠️
internal/datastore/spanner/readwrite.go	50.00%	1 Missing ⚠️
internal/middleware/perfinsights/perfinsights.go	50.00%	1 Missing ⚠️
pkg/development/devcontext.go	50.00%	0 Missing and 1 partial ⚠️
pkg/spiceerrors/bug.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2596      +/-   ##
==========================================
+ Coverage   77.62%   77.63%   +0.01%     
==========================================
  Files         442      442              
  Lines       54880    54880              
==========================================
+ Hits        42597    42599       +2     
+ Misses       9618     9617       -1     
+ Partials     2665     2664       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

miparnisari · 2025-10-09T17:11:00Z

buf.yaml

    - "ENUM_VALUE_PREFIX"
    - "ENUM_ZERO_VALUE_SUFFIX"
+    - "FIELD_NOT_REQUIRED"
+    - "PACKAGE_NO_IMPORT_CYCLE"


😨 we have a cycle somewhere?

No - I think this was added defensively by the migration script. I removed both of these exceptions and lints still pass, so I committed that.

miparnisari · 2025-10-09T18:05:50Z

pkg/proto/impl/v1/impl.pb.go

 	DispatchVersion uint32 `protobuf:"varint,4,opt,name=dispatch_version,json=dispatchVersion,proto3" json:"dispatch_version,omitempty"`
 	// flags are flags set by the API caller.
-	Flags map[string]string `protobuf:"bytes,5,rep,name=flags,proto3" json:"flags,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`
+	Flags map[string]string `protobuf:"bytes,5,rep,name=flags,proto3" json:"flags,omitempty" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"bytes,2,opt,name=value"`


This is a difference between the remote plugin's version and the version we were previously using. I'm not expecting it to have any meaningful effect, but I also can't say for certain because I don't understand this part of protobuf generation very well.

miparnisari · 2025-10-09T18:06:42Z

pkg/proto/core/v1/core.pb.go

 	// * name represents the globally-unique identifier of the caveat *
 	Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"`
 	// * serialized_expression is the byte representation of a caveat's logic
 	SerializedExpression []byte `protobuf:"bytes,2,opt,name=serialized_expression,json=serializedExpression,proto3" json:"serialized_expression,omitempty"`
 	// * parameters_and_types is a map from parameter name to its type
-	ParameterTypes map[string]*CaveatTypeReference `protobuf:"bytes,3,rep,name=parameter_types,json=parameterTypes,proto3" json:"parameter_types,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`
+	ParameterTypes map[string]*CaveatTypeReference `protobuf:"bytes,3,rep,name=parameter_types,json=parameterTypes,proto3" json:"parameter_types,omitempty" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"bytes,2,opt,name=value"`


miparnisari

Is there any way we can test this PR from a user's point of view? For example, what if we updated the Playground to use SpiceDB pointing to this commit?

tstirrat15 · 2025-10-09T18:11:39Z

Yeah, that's not a bad idea.

tstirrat15 · 2025-10-09T21:47:31Z

Some benchmarks from the Buf folks: bufbuild/protovalidate-go#283 (comment)

Their sense is that protovalidate will be slower than executing the code generated by protoc-gen-validate, but not by enough to be a bottleneck concern. It's also using protobuf reflection, which is a separate concept from golang reflection and not as expensive.

tstirrat15 · 2025-10-09T22:03:14Z

pkg/schemadsl/compiler/compiler_test.go

 			nilPrefix,
 			`definition someTenant/fo {}`,
-			"parse error in `invalid definition name`, line 1, column 1: error in object definition someTenant/fo: invalid NamespaceDefinition.Name: value does not match regex pattern \"^([a-z][a-z0-9_]{1,62}[a-z0-9]/)*[a-z][a-z0-9_]{1,62}[a-z0-9]$\"",
+			"parse error in `invalid definition name`, line 1, column 1: error in object definition someTenant/fo: validation error:\n - name: value does not match regex pattern `^([a-z][a-z0-9_]{1,62}[a-z0-9]/)*[a-z][a-z0-9_]{1,62}[a-z0-9]$` [string.pattern]",


These changes represent something that's different from the perspective of the user - it no longer names the object being validated, and it includes the [string.pattern] bit at the end. I think this is fine.

tstirrat15 force-pushed the update-buf-and-use-protovalidate branch 4 times, most recently from afaf255 to 7e05740 Compare October 8, 2025 23:16

tstirrat15 commented Oct 8, 2025

View reviewed changes

tstirrat15 force-pushed the update-buf-and-use-protovalidate branch 3 times, most recently from 30c3e48 to 8ff28bd Compare October 9, 2025 00:04

miparnisari reviewed Oct 9, 2025

View reviewed changes

tstirrat15 force-pushed the update-buf-and-use-protovalidate branch 2 times, most recently from 2296200 to fdf13a2 Compare October 9, 2025 18:24

tstirrat15 force-pushed the update-buf-and-use-protovalidate branch 2 times, most recently from 6f0cdcc to 0bb56df Compare October 9, 2025 22:02

tstirrat15 marked this pull request as ready for review October 9, 2025 22:02

tstirrat15 requested a review from a team as a code owner October 9, 2025 22:02

tstirrat15 commented Oct 9, 2025

View reviewed changes

tstirrat15 added 2 commits October 9, 2025 16:47

chore: Migrate buf configuration

735b322

chore: Use protovalidate

05603a9

tstirrat15 added 5 commits October 9, 2025 16:47

chore: Fix lint issues

6dc7558

chore: make mage work

818c59d

chore: use the version of vtproto that we were originally using

3fd86b8

chore: fix buf lint issues

6c2ba60

chore: Fix lint issues

97925eb

tstirrat15 force-pushed the update-buf-and-use-protovalidate branch from 0bb56df to 97925eb Compare October 9, 2025 22:47

		v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
		"github.com/authzed/grpcutil"

		ObjectAndRelation resource_and_relation = 1 [(validate.rules).message.required = true];
		ObjectAndRelation resource_and_relation = 1 [(buf.validate.field).required = true];

Update buf and use protovalidate #2596

Are you sure you want to change the base?

Update buf and use protovalidate #2596

Uh oh!

Conversation

tstirrat15 commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstirrat15 Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miparnisari Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miparnisari left a comment

Choose a reason for hiding this comment

Uh oh!

tstirrat15 commented Oct 9, 2025

Uh oh!

tstirrat15 commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tstirrat15 commented Oct 8, 2025 •

edited

Loading

tstirrat15 Oct 9, 2025 •

edited

Loading

miparnisari Oct 9, 2025 •

edited

Loading

codecov bot commented Oct 8, 2025 •

edited

Loading