chore: revert changes to handwrittenvalidation

tstirrat15 · tstirrat15 · commit 1fad59e9f4f2 · 2026-03-09T16:23:56.000-06:00
diff --git a/internal/middleware/handwrittenvalidation/handwrittenvalidation.go b/internal/middleware/handwrittenvalidation/handwrittenvalidation.go
@@ -2,128 +2,53 @@ package handwrittenvalidation
 
 import (
 	"context"
-	"errors"
-	"fmt"
 
-	"buf.build/go/protovalidate"
-	"go.opentelemetry.io/otel"
-	otelcodes "go.opentelemetry.io/otel/codes"
-	"go.opentelemetry.io/otel/trace"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
-	"google.golang.org/protobuf/proto"
 )
 
-var tracer = otel.Tracer("spicedb/internal/middleware")
-
-// mustNewProtoValidator wraps protovalidate.New() to panic
-// if the validator can't be constructed.
-func mustNewProtoValidator(opts ...protovalidate.ValidatorOption) protovalidate.Validator {
-	validator, err := protovalidate.New(opts...)
-	if err != nil {
-		wrappedErr := fmt.Errorf("could not construct validator: %w", err)
-		panic(wrappedErr)
-	}
-	return validator
-}
-
 type handwrittenValidator interface {
 	HandwrittenValidate() error
 }
 
-// UnaryServerInterceptor returns a function that performs standard proto validation and handwritten validation (if any) on the incoming request.
-func UnaryServerInterceptor() grpc.UnaryServerInterceptor {
-	protovalidator := mustNewProtoValidator()
-	return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {
-		_, span := tracer.Start(ctx, "protovalidate")
-		err := standardValidate(req, protovalidator, span)
-		if err != nil {
-			span.End()
-			return nil, err
-		}
-
-		err = handwrittenValidate(req, span)
+// UnaryServerInterceptor returns a new unary server interceptor that runs the handwritten validation
+// on the incoming request, if any.
+func UnaryServerInterceptor(ctx context.Context, req any, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {
+	validator, ok := req.(handwrittenValidator)
+	if ok {
+		err := validator.HandwrittenValidate()
 		if err != nil {
-			span.End()
-			return nil, err
+			return nil, status.Errorf(codes.InvalidArgument, "%s", err)
 		}
-
-		span.End()
-
-		return handler(ctx, req)
 	}
+
+	return handler(ctx, req)
 }
 
-// StreamServerInterceptor returns a function that performs standard proto validation and handwritten validation (if any) on the incoming request.
+// StreamServerInterceptor returns a new stream server interceptor that runs the handwritten validation
+// on the incoming request messages, if any.
 func StreamServerInterceptor(srv any, stream grpc.ServerStream, _ *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
-	wrapper := &recvWrapper{ServerStream: stream, protovalidator: mustNewProtoValidator()}
+	wrapper := &recvWrapper{stream}
 	return handler(srv, wrapper)
 }
 
 type recvWrapper struct {
 	grpc.ServerStream
-	protovalidator protovalidate.Validator
 }
 
 func (s *recvWrapper) RecvMsg(m any) error {
 	if err := s.ServerStream.RecvMsg(m); err != nil {
 		return err
 	}
 
-	_, span := tracer.Start(s.Context(), "protovalidate")
-	err := standardValidate(m, s.protovalidator, span)
-	if err != nil {
-		span.End()
-		return err
-	}
-
-	err = handwrittenValidate(m, span)
-	if err != nil {
-		span.End()
-		return err
-	}
-
-	span.End()
-
-	return nil
-}
-
-// standardValidate was copied from https://github.com/grpc-ecosystem/go-grpc-middleware/blob/ab2131d954af9580c1b49a3d9475f6adbe5de9d3/interceptors/protovalidate/protovalidate.go#L68
-// it validates the proto and if an error occurs, marks the span as errored.
-func standardValidate(m any, validator protovalidate.Validator, span trace.Span) error {
-	msg, ok := m.(proto.Message)
-	if !ok {
-		return status.Errorf(codes.Internal, "unsupported message type: %T", m)
-	}
-	err := validator.Validate(msg)
-	if err == nil {
-		return nil
-	}
-	var valErr *protovalidate.ValidationError
-	if errors.As(err, &valErr) {
-		span.SetStatus(otelcodes.Error, err.Error())
-		span.RecordError(err)
-		st := status.New(codes.InvalidArgument, err.Error())
-		ds, detErr := st.WithDetails(valErr.ToProto())
-		if detErr != nil {
-			return st.Err()
-		}
-		return ds.Err()
-	}
-
-	return status.Error(codes.Internal, err.Error())
-}
-
-func handwrittenValidate(req any, span trace.Span) error {
-	validator, ok := req.(handwrittenValidator)
+	validator, ok := m.(handwrittenValidator)
 	if ok {
 		err := validator.HandwrittenValidate()
 		if err != nil {
-			span.SetStatus(otelcodes.Error, err.Error())
-			span.RecordError(err)
-			return status.Errorf(codes.InvalidArgument, "%s", err)
+			return err
 		}
 	}
+
 	return nil
 }
diff --git a/internal/middleware/interceptorwrapper/interceptorwrapper.go b/internal/middleware/interceptorwrapper/interceptorwrapper.go
@@ -7,15 +7,19 @@ import (
 	"google.golang.org/grpc"
 )
 
+var tracer = otel.Tracer("spicedb/internal/middleware")
+
 // WrapUnaryServerInterceptorWithSpans returns a new interceptor that wraps the given interceptor
 // with a span, measuring the duration of the interceptor's pre-handler logic.
 func WrapUnaryServerInterceptorWithSpans(
 	inner grpc.UnaryServerInterceptor,
-	tracerName, spanName string,
+	spanName string,
 ) grpc.UnaryServerInterceptor {
-	t := otel.Tracer(tracerName)
 	return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {
-		ctx, span := t.Start(ctx, spanName)
+		ctx, span := tracer.Start(ctx, spanName)
+		// NOTE: this shim is what lets us measure how long the interceptor is doing work.
+		// It's the handler that we pass to the wrapped interceptor, so `span.End()` will be
+		// called when the handler itself is called.
 		shimHandler := func(ctx context.Context, req any) (any, error) {
 			span.End()
 			return handler(ctx, req)
@@ -27,32 +31,3 @@ func WrapUnaryServerInterceptorWithSpans(
 		return resp, err
 	}
 }
-
-// WrapStreamServerInterceptorWithSpans returns a new interceptor that wraps the given interceptor
-// with a span, measuring the duration of the interceptor's pre-handler logic.
-func WrapStreamServerInterceptorWithSpans(
-	inner grpc.StreamServerInterceptor,
-	tracerName, spanName string,
-) grpc.StreamServerInterceptor {
-	t := otel.Tracer(tracerName)
-	return func(srv any, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
-		ctx, span := t.Start(ss.Context(), spanName)
-		wrappedStream := &contextStream{ServerStream: ss, ctx: ctx}
-		shimHandler := func(srv any, stream grpc.ServerStream) error {
-			span.End()
-			return handler(srv, stream)
-		}
-		err := inner(srv, wrappedStream, info, shimHandler)
-		if span.IsRecording() {
-			span.End()
-		}
-		return err
-	}
-}
-
-type contextStream struct {
-	grpc.ServerStream
-	ctx context.Context
-}
-
-func (s *contextStream) Context() context.Context { return s.ctx }
diff --git a/internal/services/v1/experimental.go b/internal/services/v1/experimental.go
@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/ccoveille/go-safecast/v2"
+	grpcvalidate "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/protovalidate"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/protobuf/types/known/timestamppb"
@@ -23,6 +24,7 @@ import (
 	log "github.com/authzed/spicedb/internal/logging"
 	"github.com/authzed/spicedb/internal/middleware"
 	"github.com/authzed/spicedb/internal/middleware/handwrittenvalidation"
+	"github.com/authzed/spicedb/internal/middleware/interceptorwrapper"
 	"github.com/authzed/spicedb/internal/middleware/perfinsights"
 	"github.com/authzed/spicedb/internal/middleware/streamtimeout"
 	"github.com/authzed/spicedb/internal/middleware/usagemetrics"
@@ -35,6 +37,7 @@ import (
 	"github.com/authzed/spicedb/pkg/datastore"
 	dsoptions "github.com/authzed/spicedb/pkg/datastore/options"
 	"github.com/authzed/spicedb/pkg/datastore/queryshape"
+	"github.com/authzed/spicedb/pkg/genutil"
 	"github.com/authzed/spicedb/pkg/middleware/consistency"
 	core "github.com/authzed/spicedb/pkg/proto/core/v1"
 	dispatchv1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
@@ -92,14 +95,18 @@ func NewExperimentalServer(dispatch dispatch.Dispatcher, permServerConfig Permis
 		chunkSize = 100
 	}
 
+	validator := genutil.MustNewProtoValidator()
+
 	return &experimentalServer{
 		WithServiceSpecificInterceptors: shared.WithServiceSpecificInterceptors{
 			Unary: middleware.ChainUnaryServer(
-				handwrittenvalidation.UnaryServerInterceptor(),
+				interceptorwrapper.WrapUnaryServerInterceptorWithSpans(grpcvalidate.UnaryServerInterceptor(validator), "protovalidate"),
+				handwrittenvalidation.UnaryServerInterceptor,
 				usagemetrics.UnaryServerInterceptor(),
 				perfinsights.UnaryServerInterceptor(permServerConfig.PerformanceInsightMetricsEnabled),
 			),
 			Stream: middleware.ChainStreamServer(
+				grpcvalidate.StreamServerInterceptor(validator),
 				handwrittenvalidation.StreamServerInterceptor,
 				usagemetrics.StreamServerInterceptor(),
 				streamtimeout.MustStreamServerInterceptor(config.StreamReadTimeout),
diff --git a/internal/services/v1/relationships.go b/internal/services/v1/relationships.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"time"
 
+	grpcvalidate "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/protovalidate"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"go.opentelemetry.io/otel/trace"
@@ -18,6 +19,7 @@ import (
 	"github.com/authzed/spicedb/internal/dispatch"
 	"github.com/authzed/spicedb/internal/middleware"
 	"github.com/authzed/spicedb/internal/middleware/handwrittenvalidation"
+	"github.com/authzed/spicedb/internal/middleware/interceptorwrapper"
 	"github.com/authzed/spicedb/internal/middleware/perfinsights"
 	"github.com/authzed/spicedb/internal/middleware/streamtimeout"
 	"github.com/authzed/spicedb/internal/middleware/usagemetrics"
@@ -145,16 +147,20 @@ func NewPermissionsServer(
 		ExperimentalQueryPlan:              config.ExperimentalQueryPlan,
 	}
 
+	validator := genutil.MustNewProtoValidator()
+
 	return &permissionServer{
 		dispatch: dispatch,
 		config:   configWithDefaults,
 		WithServiceSpecificInterceptors: shared.WithServiceSpecificInterceptors{
 			Unary: middleware.ChainUnaryServer(
-				handwrittenvalidation.UnaryServerInterceptor(),
+				interceptorwrapper.WrapUnaryServerInterceptorWithSpans(grpcvalidate.UnaryServerInterceptor(validator), "protovalidate"),
+				handwrittenvalidation.UnaryServerInterceptor,
 				usagemetrics.UnaryServerInterceptor(),
 				perfinsights.UnaryServerInterceptor(configWithDefaults.PerformanceInsightMetricsEnabled),
 			),
 			Stream: middleware.ChainStreamServer(
+				grpcvalidate.StreamServerInterceptor(validator),
 				handwrittenvalidation.StreamServerInterceptor,
 				usagemetrics.StreamServerInterceptor(),
 				streamtimeout.MustStreamServerInterceptor(configWithDefaults.StreamingAPITimeout),
diff --git a/internal/services/v1/schema.go b/internal/services/v1/schema.go
@@ -5,11 +5,13 @@ import (
 	"sort"
 	"strings"
 
+	grpcvalidate "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/protovalidate"
+
 	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
 
 	log "github.com/authzed/spicedb/internal/logging"
 	"github.com/authzed/spicedb/internal/middleware"
-	"github.com/authzed/spicedb/internal/middleware/handwrittenvalidation"
+	"github.com/authzed/spicedb/internal/middleware/interceptorwrapper"
 	"github.com/authzed/spicedb/internal/middleware/perfinsights"
 	"github.com/authzed/spicedb/internal/middleware/usagemetrics"
 	"github.com/authzed/spicedb/internal/services/shared"
@@ -44,15 +46,17 @@ type SchemaServerConfig struct {
 func NewSchemaServer(config SchemaServerConfig) v1.SchemaServiceServer {
 	cts := caveattypes.TypeSetOrDefault(config.CaveatTypeSet)
 
+	validator := genutil.MustNewProtoValidator()
+
 	return &schemaServer{
 		WithServiceSpecificInterceptors: shared.WithServiceSpecificInterceptors{
 			Unary: middleware.ChainUnaryServer(
-				handwrittenvalidation.UnaryServerInterceptor(),
+				interceptorwrapper.WrapUnaryServerInterceptorWithSpans(grpcvalidate.UnaryServerInterceptor(validator), "protovalidate"),
 				usagemetrics.UnaryServerInterceptor(),
 				perfinsights.UnaryServerInterceptor(config.PerformanceInsightMetricsEnabled),
 			),
 			Stream: middleware.ChainStreamServer(
-				handwrittenvalidation.StreamServerInterceptor,
+				grpcvalidate.StreamServerInterceptor(validator),
 				usagemetrics.StreamServerInterceptor(),
 				perfinsights.StreamServerInterceptor(config.PerformanceInsightMetricsEnabled),
 			),
diff --git a/internal/services/v1/watch.go b/internal/services/v1/watch.go
@@ -5,17 +5,18 @@ import (
 	"slices"
 	"time"
 
+	grpcvalidate "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/protovalidate"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/structpb"
 
 	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
 
-	"github.com/authzed/spicedb/internal/middleware/handwrittenvalidation"
 	"github.com/authzed/spicedb/internal/middleware/usagemetrics"
 	"github.com/authzed/spicedb/internal/services/shared"
 	"github.com/authzed/spicedb/pkg/datalayer"
 	"github.com/authzed/spicedb/pkg/datastore"
+	"github.com/authzed/spicedb/pkg/genutil"
 	"github.com/authzed/spicedb/pkg/genutil/mapz"
 	dispatchv1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
 	"github.com/authzed/spicedb/pkg/tuple"
@@ -31,9 +32,11 @@ type watchServer struct {
 
 // NewWatchServer creates an instance of the watch server.
 func NewWatchServer(heartbeatDuration time.Duration) v1.WatchServiceServer {
+	validator := genutil.MustNewProtoValidator()
+
 	s := &watchServer{
 		WithStreamServiceSpecificInterceptor: shared.WithStreamServiceSpecificInterceptor{
-			Stream: handwrittenvalidation.StreamServerInterceptor,
+			Stream: grpcvalidate.StreamServerInterceptor(validator),
 		},
 		heartbeatDuration: heartbeatDuration,
 	}
@@ -49,7 +52,7 @@ func (ws *watchServer) Watch(req *v1.WatchRequest, stream v1.WatchService_WatchS
 		}
 	}
 
-	objectTypes := mapz.NewSet[string](req.GetOptionalObjectTypes()...)
+	objectTypes := mapz.NewSet(req.GetOptionalObjectTypes()...)
 
 	ctx := stream.Context()
 	dl := datalayer.MustFromContext(ctx)
diff --git a/pkg/genutil/protovalidate.go b/pkg/genutil/protovalidate.go
@@ -0,0 +1,18 @@
+package genutil
+
+import (
+	"fmt"
+
+	"buf.build/go/protovalidate"
+)
+
+// MustNewProtoValidator wraps protovalidate.New() to panic
+// if the validator can't be constructed.
+func MustNewProtoValidator(opts ...protovalidate.ValidatorOption) protovalidate.Validator {
+	validator, err := protovalidate.New(opts...)
+	if err != nil {
+		wrappedErr := fmt.Errorf("could not construct validator: %w", err)
+		panic(wrappedErr)
+	}
+	return validator
+}
