Skip to content

rename interceptors appripriately #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pitabwire merged 1 commit into from
Nov 19, 2025
Merged

rename interceptors appripriately #600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion security/interceptors/connect/authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var (
ErrInvalidToken = errors.New("invalid authorization token")
)

// AuthInterceptor implements connect.Interceptor for JWT authentication.
// AuthInterceptor implements connect.ValidationInterceptor for JWT authentication.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This comment is incorrect. AuthInterceptor implements the connect.Interceptor interface. ValidationInterceptor is a different struct in this package that also implements connect.Interceptor. This change appears to be an artifact of a search-and-replace operation and should be reverted to refer to connect.Interceptor to avoid confusion.

Suggested change
// AuthInterceptor implements connect.ValidationInterceptor for JWT authentication.
// AuthInterceptor implements connect.Interceptor for JWT authentication.

type AuthInterceptor struct {
authenticator security.Authenticator
}
Expand Down
54 changes: 27 additions & 27 deletions security/interceptors/connect/pbvalidation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,22 @@ const (
maxStructFields = 200
)

// An Option configures an [Interceptor].
// An Option configures an [ValidationInterceptor].
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's a grammatical error here. The article should be "a" instead of "an" before "ValidationInterceptor".

Suggested change
// An Option configures an [ValidationInterceptor].
// An Option configures a [ValidationInterceptor].

type Option interface {
apply(*Interceptor)
apply(*ValidationInterceptor)
}

// WithValidator configures the [Interceptor] to use a customized
// WithValidator configures the [ValidationInterceptor] to use a customized
// [protovalidate.Validator]. By default, [protovalidate.GlobalInterceptor]
// is used See [protovalidate.ValidatorOption] for the range of available
// customizations.
func WithValidator(validator protovalidate.Validator) Option {
return optionFunc(func(i *Interceptor) {
return optionFunc(func(i *ValidationInterceptor) {
i.validator = validator
})
}

// WithValidateResponses configures the [Interceptor] to also validate reponses
// WithValidateResponses configures the [ValidationInterceptor] to also validate reponses
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a typo in "reponses". It should be "responses".

Suggested change
// WithValidateResponses configures the [ValidationInterceptor] to also validate reponses
// WithValidateResponses configures the [ValidationInterceptor] to also validate responses

// in addition to validating requests.
//
// By default:
Expand All @@ -45,21 +45,21 @@ func WithValidator(validator protovalidate.Validator) Option {
//
// However, these messages are all validated if this option is set.
func WithValidateResponses() Option {
return optionFunc(func(i *Interceptor) {
return optionFunc(func(i *ValidationInterceptor) {
i.validateResponses = true
})
}

// WithoutErrorDetails configures the [Interceptor] to elide error details from
// WithoutErrorDetails configures the [ValidationInterceptor] to elide error details from
// validation errors. By default, a [protovalidate.ValidationError] is added
// as a detail when validation errors are returned.
func WithoutErrorDetails() Option {
return optionFunc(func(i *Interceptor) {
return optionFunc(func(i *ValidationInterceptor) {
i.noErrorDetails = true
})
}

// Interceptor is a [connect.Interceptor] that ensures that RPC request
// ValidationInterceptor is a [connect.Interceptor] that ensures that RPC request
// messages match the constraints expressed in their Protobuf schemas. It does
// not validate response messages unless the [WithValidateResponses] option
// is specified.
Expand All @@ -79,16 +79,16 @@ func WithoutErrorDetails() Option {
// schema.
//
// [detailed representation of the error]: https://pkg.go.dev/buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate#Violations
type Interceptor struct {
type ValidationInterceptor struct {
validator protovalidate.Validator
validateResponses bool
noErrorDetails bool
}

// NewInterceptor builds an Interceptor. The default configuration is
// NewValidationInterceptor builds an ValidationInterceptor. The default configuration is
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's a grammatical error here. The article should be "a" instead of "an" before "ValidationInterceptor".

Suggested change
// NewValidationInterceptor builds an ValidationInterceptor. The default configuration is
// NewValidationInterceptor builds a ValidationInterceptor. The default configuration is

// appropriate for most use cases.
func NewInterceptor(opts ...Option) *Interceptor {
var interceptor Interceptor
func NewValidationInterceptor(opts ...Option) *ValidationInterceptor {
var interceptor ValidationInterceptor
for _, opt := range opts {
opt.apply(&interceptor)
}
Expand All @@ -100,8 +100,8 @@ func NewInterceptor(opts ...Option) *Interceptor {
return &interceptor
}

// WrapUnary implements connect.Interceptor.
func (i *Interceptor) WrapUnary(next connect.UnaryFunc) connect.UnaryFunc {
// WrapUnary implements connect.ValidationInterceptor.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This comment is misleading. WrapUnary is a method that helps ValidationInterceptor satisfy the connect.Interceptor interface. It does not implement connect.ValidationInterceptor. This seems to be an error from a search-and-replace. Please revert it to refer to connect.Interceptor.

Suggested change
// WrapUnary implements connect.ValidationInterceptor.
// WrapUnary implements connect.Interceptor.

func (i *ValidationInterceptor) WrapUnary(next connect.UnaryFunc) connect.UnaryFunc {
return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) {
if err := i.validateRequest(req.Any()); err != nil {
return nil, err
Expand All @@ -117,8 +117,8 @@ func (i *Interceptor) WrapUnary(next connect.UnaryFunc) connect.UnaryFunc {
}
}

// WrapStreamingClient implements connect.Interceptor.
func (i *Interceptor) WrapStreamingClient(next connect.StreamingClientFunc) connect.StreamingClientFunc {
// WrapStreamingClient implements connect.ValidationInterceptor.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This comment is misleading. WrapStreamingClient is a method that helps ValidationInterceptor satisfy the connect.Interceptor interface. It does not implement connect.ValidationInterceptor. This seems to be an error from a search-and-replace. Please revert it to refer to connect.Interceptor.

Suggested change
// WrapStreamingClient implements connect.ValidationInterceptor.
// WrapStreamingClient implements connect.Interceptor.

func (i *ValidationInterceptor) WrapStreamingClient(next connect.StreamingClientFunc) connect.StreamingClientFunc {
return func(ctx context.Context, spec connect.Spec) connect.StreamingClientConn {
return &streamingClientInterceptor{
StreamingClientConn: next(ctx, spec),
Expand All @@ -127,8 +127,8 @@ func (i *Interceptor) WrapStreamingClient(next connect.StreamingClientFunc) conn
}
}

// WrapStreamingHandler implements connect.Interceptor.
func (i *Interceptor) WrapStreamingHandler(next connect.StreamingHandlerFunc) connect.StreamingHandlerFunc {
// WrapStreamingHandler implements connect.ValidationInterceptor.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This comment is misleading. WrapStreamingHandler is a method that helps ValidationInterceptor satisfy the connect.Interceptor interface. It does not implement connect.ValidationInterceptor. This seems to be an error from a search-and-replace. Please revert it to refer to connect.Interceptor.

Suggested change
// WrapStreamingHandler implements connect.ValidationInterceptor.
// WrapStreamingHandler implements connect.Interceptor.

func (i *ValidationInterceptor) WrapStreamingHandler(next connect.StreamingHandlerFunc) connect.StreamingHandlerFunc {
return func(ctx context.Context, conn connect.StreamingHandlerConn) error {
return next(ctx, &streamingHandlerInterceptor{
StreamingHandlerConn: conn,
Expand All @@ -137,18 +137,18 @@ func (i *Interceptor) WrapStreamingHandler(next connect.StreamingHandlerFunc) co
}
}

func (i *Interceptor) validateRequest(msg any) error {
func (i *ValidationInterceptor) validateRequest(msg any) error {
return i.validate(msg, connect.CodeInvalidArgument)
}

func (i *Interceptor) validateResponse(msg any) error {
func (i *ValidationInterceptor) validateResponse(msg any) error {
if !i.validateResponses {
return nil
}
return i.validate(msg, connect.CodeInternal)
}

func (i *Interceptor) validate(msg any, code connect.Code) error {
func (i *ValidationInterceptor) validate(msg any, code connect.Code) error {
if msg == nil {
return nil
}
Expand All @@ -169,7 +169,7 @@ func (i *Interceptor) validate(msg any, code connect.Code) error {
return nil
}

func (i *Interceptor) wrapValidationError(originalErr error, code connect.Code) error {
func (i *ValidationInterceptor) wrapValidationError(originalErr error, code connect.Code) error {
connectErr := connect.NewError(code, originalErr)
if i.noErrorDetails {
return connectErr
Expand Down Expand Up @@ -272,7 +272,7 @@ func validateSingleStruct(s *structpb.Struct) error {
type streamingClientInterceptor struct {
connect.StreamingClientConn

interceptor *Interceptor
interceptor *ValidationInterceptor
}

func (s *streamingClientInterceptor) Send(msg any) error {
Expand All @@ -292,7 +292,7 @@ func (s *streamingClientInterceptor) Receive(msg any) error {
type streamingHandlerInterceptor struct {
connect.StreamingHandlerConn

interceptor *Interceptor
interceptor *ValidationInterceptor
}

func (s *streamingHandlerInterceptor) Send(msg any) error {
Expand All @@ -309,6 +309,6 @@ func (s *streamingHandlerInterceptor) Receive(msg any) error {
return s.interceptor.validateRequest(msg)
}

type optionFunc func(*Interceptor)
type optionFunc func(*ValidationInterceptor)

func (f optionFunc) apply(i *Interceptor) { f(i) }
func (f optionFunc) apply(i *ValidationInterceptor) { f(i) }
26 changes: 13 additions & 13 deletions security/interceptors/connect/pbvalidation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,31 @@ import (

func TestNewInterceptor(t *testing.T) {
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with the function being tested (NewValidationInterceptor), this test function should be renamed to TestNewValidationInterceptor.

Suggested change
func TestNewInterceptor(t *testing.T) {
func TestNewValidationInterceptor(t *testing.T) {

t.Run("default configuration", func(t *testing.T) {
interceptor := NewInterceptor()
interceptor := NewValidationInterceptor()
assert.NotNil(t, interceptor.validator)
assert.False(t, interceptor.validateResponses)
assert.False(t, interceptor.noErrorDetails)
})

t.Run("with custom validator", func(t *testing.T) {
customValidator := protovalidate.GlobalValidator
interceptor := NewInterceptor(WithValidator(customValidator))
interceptor := NewValidationInterceptor(WithValidator(customValidator))
assert.Equal(t, customValidator, interceptor.validator)
})

t.Run("with validate responses", func(t *testing.T) {
interceptor := NewInterceptor(WithValidateResponses())
interceptor := NewValidationInterceptor(WithValidateResponses())
assert.True(t, interceptor.validateResponses)
})

t.Run("with no error details", func(t *testing.T) {
interceptor := NewInterceptor(WithoutErrorDetails())
interceptor := NewValidationInterceptor(WithoutErrorDetails())
assert.True(t, interceptor.noErrorDetails)
})

t.Run("multiple options", func(t *testing.T) {
customValidator := protovalidate.GlobalValidator
interceptor := NewInterceptor(
interceptor := NewValidationInterceptor(
WithValidator(customValidator),
WithValidateResponses(),
WithoutErrorDetails(),
Expand All @@ -55,7 +55,7 @@ func TestNewInterceptor(t *testing.T) {
}

func TestValidateRequest(t *testing.T) {
interceptor := NewInterceptor()
interceptor := NewValidationInterceptor()

t.Run("nil message", func(t *testing.T) {
err := interceptor.validateRequest(nil)
Expand All @@ -77,14 +77,14 @@ func TestValidateRequest(t *testing.T) {

func TestValidateResponse(t *testing.T) {
t.Run("without validate responses option", func(t *testing.T) {
interceptor := NewInterceptor()
interceptor := NewValidationInterceptor()
msg := &wrapperspb.StringValue{Value: "test"}
err := interceptor.validateResponse(msg)
require.NoError(t, err) // Should not validate responses by default
})

t.Run("with validate responses option", func(t *testing.T) {
interceptor := NewInterceptor(WithValidateResponses())
interceptor := NewValidationInterceptor(WithValidateResponses())
msg := &wrapperspb.StringValue{Value: "test"}
err := interceptor.validateResponse(msg)
require.NoError(t, err)
Expand Down Expand Up @@ -224,7 +224,7 @@ func TestValidateAllStructs(t *testing.T) {

func TestWrapValidationError(t *testing.T) {
t.Run("with error details", func(t *testing.T) {
interceptor := NewInterceptor()
interceptor := NewValidationInterceptor()
originalErr := &protovalidate.ValidationError{}
wrappedErr := interceptor.wrapValidationError(originalErr, connect.CodeInvalidArgument)

Expand All @@ -234,7 +234,7 @@ func TestWrapValidationError(t *testing.T) {
})

t.Run("without error details", func(t *testing.T) {
interceptor := NewInterceptor(WithoutErrorDetails())
interceptor := NewValidationInterceptor(WithoutErrorDetails())
originalErr := &protovalidate.ValidationError{}
wrappedErr := interceptor.wrapValidationError(originalErr, connect.CodeInvalidArgument)

Expand All @@ -245,7 +245,7 @@ func TestWrapValidationError(t *testing.T) {
}

func TestUnaryInterceptor(t *testing.T) {
interceptor := NewInterceptor()
interceptor := NewValidationInterceptor()

t.Run("valid request", func(t *testing.T) {
callCount := 0
Expand Down Expand Up @@ -297,7 +297,7 @@ func TestUnaryInterceptor(t *testing.T) {
}

func TestStreamingInterceptors(t *testing.T) {
interceptor := NewInterceptor()
interceptor := NewValidationInterceptor()

t.Run("streaming client interceptor", func(t *testing.T) {
wrapped := interceptor.WrapStreamingClient(
Expand Down Expand Up @@ -378,7 +378,7 @@ func BenchmarkValidateAllStructs(b *testing.B) {
}

func BenchmarkInterceptorWrapUnary(b *testing.B) {
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with the renaming of Interceptor to ValidationInterceptor, this benchmark function should be renamed to BenchmarkValidationInterceptorWrapUnary.

Suggested change
func BenchmarkInterceptorWrapUnary(b *testing.B) {
func BenchmarkValidationInterceptorWrapUnary(b *testing.B) {

interceptor := NewInterceptor()
interceptor := NewValidationInterceptor()
next := func(_ context.Context, _ connect.AnyRequest) (connect.AnyResponse, error) {
return connect.NewResponse(&wrapperspb.StringValue{Value: "response"}), nil
}
Expand Down
Loading