Support proxy interceptors visiting Payloads and Failures in gRPC errors #213
Conversation
# Conflicts: # proxy/interceptor_test.go
proxy/interceptor.go
Outdated
| stat, ok := status.FromError(err) | ||
| if !ok { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Now that I think about it, I am a tad concerned about this logic because I am concerned that both response and non-status err can be non-nil since technically an interceptor can return an error with a valid response. I think we may still want to apply the logic below the overall err != nil block even on error.
There was a problem hiding this comment.
This changes existing behavior today, I think it's better if I tackle this in a separate PR with dedicated tests to validate this scenario.
There was a problem hiding this comment.
Hrmm, I see we did always eagerly return any error that happened and not affect response. Agreed can be done separately.
There was a problem hiding this comment.
aa5ebbe to
b333dc5
Compare
| newStatus := status.New(stat.Code(), stat.Message()) | ||
| var newDetails []protoadapt.MessageV1 | ||
| for _, detail := range stat.Details() { | ||
| detailAny, ok := detail.(*anypb.Any) |
There was a problem hiding this comment.
What if details contains a proto message that is not any but contains a payload?
There was a problem hiding this comment.
In theory this should be impossible looking at source, but I agree, we might as well first type assert to message v1, then optionally type assert to any (falling back to just appending the v1 message if it didn't work). also this is large enough that you could consider a private helper in this generated code that does all of this instead of inlining in the interceptor.
There was a problem hiding this comment.
by "theory this should be impossible looking at source" do you mean the server source or go protobuf?
| @@ -478,6 +571,54 @@ func (t *testGRPCServer) PollActivityTaskQueue( | |||
| }, nil | |||
| } | |||
|
|
|||
| func (t *testGRPCServer) ExecuteMultiOperation( | |||
There was a problem hiding this comment.
Did we verify this matches the real output of the server?
There was a problem hiding this comment.
No, temporalio/temporal#7487 for query, and from our previous discussion it sounded like multi op is not supported yet today?
There was a problem hiding this comment.
multi op is supported, it is used by update with start
What changed?
Added support for proxy interceptors visiting gRPC errors.
Why?
In some scenarios there can be user-provided payloads.
temporalio/sdk-go#1825
How did you test it?
Added new test to validate both these behaviors
Potential risks
Proxy interceptors could completely break?