Reproduce issue with client error handling + fix

[jamesrom]

One line fix is pretty self-explanatory. Update: it's still one line, but not-so self-explanatory 😅

If a client interceptor hides an error, this type assertion will fail. This is because type information is lost when unaryFunc returns nil.

I have provided a test case that reproduces the issue.

[jhump]

This doesn't seem right. A nil, nil response isn't really valid. What is the framework expected to do with this when returning a value to the caller?

[jamesrom]

See #799 (comment)

[jhump]

This causes the interceptor to return nil, nil. I don't think that should be legal. If an interceptor is "hiding" an error, it must synthesize an actual, non-nil response value.

This could be done dynamically by inspecting req.Spec.Schema(). If that is unset then a dynamic response is not possible.

We could also potentially have connect-go's internal implementation of AnyRequest implement an optional method that could help. For example, maybe something like so:

if respTyped, ok := req.(interface {
    // Could instantiate the correct response type and then run
    // any configured initializer.
    NewResponse() any
}); ok {
    return respTyped.NewResponse(), nil
}

But, now that I've suggested it, I don't love that. It is much less intuitive of course. But it also just feels like a terrible pattern to support -- always returning a zero value response seems wrong and is almost certain to violate any expected RPC contract about what the method is supposed to return. This just doesn't seem like an appropriate function for an interceptor.

It would be much better to inject this error handling much deeper -- like into the actual response handlers, so they can return a valid response value if the application wants to ignore a particular error. Another possibility would be to use a switch on the schema type or the actual procedure name so that it always returns a valid response (which may not necessarily be empty).

[jamesrom]

The client interceptor returning an actual, non-nil response value is reasonable. However, it also doesn't seem right that interceptors lose all information about the expected response in an error condition.

I don't think smuggling the type via AnyRequest would work either -- there's no reason (other than convention, I suppose) that the same request type isn't used by multiple service methods returning different response types.

Short of smuggling the type information through the Error struct, another option is that resp.Any() could do a nil check:

// Any returns the concrete response message as an empty interface, so that
// *Response implements the [AnyResponse] interface. If the response message is
// nil, Any returns a new zero value of the message type.
func (r *Response[T]) Any() any {
	if r.Msg == nil {
		return new(T)
	}
	return r.Msg
}

But this means the framework would need to ensure that resp is never nil before being passed to the next interceptor.