Support validation of responses in Interceptor

[mattdowdell]

As it stands, it appears that only requests are validated by the Interceptor implementation, i.e. when the client sends the request, when the handler receives the request. In the past, I've found it useful to also validate the response as a sanity check and to protect clients from an obviously broken implementation.

Would this be a feature you'd consider?

As a rough example of what I had in mind:

// WithValidateResponseFn configures a callback for response validation failures and enables response validation.
func WithValidateResponseFn(fn func(context.Context, error) error) Option {
	return optionFunc(func(i *Interceptor) {
		i.validateResponseFn = on
	})
}

// (skip some code)

// WrapUnary implements connect.Interceptor.
func (i *Interceptor) WrapUnary(next connect.UnaryFunc) connect.UnaryFunc {
	return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) {
		if err := validate(i.validator, req.Any()); err != nil {
			return nil, err
		}

		resp, err := next(ctx, req)
		if err != nil || i.validateResponseFn == nil {
			return resp, err
		}

		if err := validate(i.validator, req.Any()); err != nil {
			// user can choose to ignore the error, capture it for debugging, transform it into an internal error, etc.
			if err := validateResponseFn(ctx, err); err != nil {
				return nil, err
			}
		}

		return resp, nil
	}
}

// (similar code for the stream client/handler here)

// (example usage)
interceptor := validate.NewInterceptor(WithValidateResponseFn(func (ctx context.Context, err error) {
	// log error for debugging
	slog.ErrorContext(ctx, "response validation failed", slog.Any("error", err)

	// record error on span
	span := trace.SpanFromContext(ctx)
	span.RecordError(err)

	// transform the error to an internal error, hide the original from the client
	connect.NewError(connect.CodeInternal, nil)
}))

// use interceptor here
_ = interceptor

[jhump]

@mattdowdell, response validation has come up more than once before, so we'll keep this on the backlog.

If/when we add such an option, it will be simpler -- just a simple WithResponseValidation() to enable it. The ability to filter the errors or even transform/customize them could be done via a wrapper around a protovalidate.Validator. We can change the signature of the WithValidator option to accept an interface instead of a concrete type, so that you can decorate a validator with that sort of filtering logic.

Normally, I might not propose that sort of change (changing a signature to accept an interface instead of the existing concrete type), since it is not backwards-compatible. However, the github.com/bufbuild/protovalidate-go repo has recently made that same breaking change already, so when the next version is released and incorporated into this module, a backwards-incompatible change is inevitable.

[mattdowdell]

@jhump thanks for the thoughtful response.

Regarding your proposal to wrap the validator for error transformation, I'm a little confused how you would differentiate between a request a response message. I can see the current interceptor behaviour is to reject invalid requests with an InvalidArgument status code, which I think is the correct choice. But I don't see anything to programmatically hook off in the Protobuf message that would let me replace the error with an Internal status code. I also don't quite see why the validator would be responsible for sending the error status when that logic lies in the interceptor here. It feels muddled to move that logic to a lower level.

I might rely on the message name's suffix to only target responses, but such suffixes are by convention and only enforced if using buf lint's RPC_RESPONSE_STANDARD_NAME. To me, using that convention feels like a little too large an assumption for this.

I wonder if I'm suffering from a lack of imagination - did you have something else in mind for implementing this feature?

[jhump]

Ah, you are right. The objective behind the suggested simplification would be to minimize the API surface area, eliding anything that isn't strictly necessary. If, for example, this could be done in a different way that composes with the validation interceptor, instead of having to push it into the validation interceptor, that is what we'd prefer. We'll need to think about this a little more.

[bufdev]

We had added WithValidateResponses in v0.4.0.