-
Notifications
You must be signed in to change notification settings - Fork 37
Support parsing error details that aren't anypb.Any type #214
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.
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
Support parsing error details that aren't anypb.Any type #214
Conversation
oops, I kinda forgot about this PR 😅 |
} | ||
return newStatus.Err() | ||
} | ||
if s, ok := status.FromError(err); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have to convert to status if there is no inbound, might as well do status.FromError
inside of the parseGrpcPayload
after you've checked whether there even is an inbound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the status check out to allow the scenario for response
and non-status err
to also visit response
. What you say makes sense, will move the inbound check out to before the status.FromError
check outside the function
# Conflicts: # proxy/interceptor_test.go
} | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this return err
is still needed here, I don't think you want to fall through to success stuff if status.FromError
happens to not be ok
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the original desire from #213 (comment) was to not immediately return err here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if both response
and err
are non-nil, would we return the err, or return VisitPayloads(ctx, resMsg, *options.Inbound)
from processing the response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, I recall that comment chain now. It was meant to apply to all errors, not just ones that don't match FromError
or when Inbound
is nil.
So stepping back (and pardon all of this detail, thanks for sticking with it), it should probably look something like:
func NewPayloadVisitorInterceptor(options PayloadVisitorInterceptorOptions) (grpc.UnaryClientInterceptor, error) {
return func(ctx context.Context, method string, req, response interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
if reqMsg, ok := req.(proto.Message); ok && options.Outbound != nil {
err := VisitPayloads(ctx, reqMsg, *options.Outbound)
if err != nil {
return err
}
}
err := invoker(ctx, method, req, response, cc, opts...)
if err != nil && options.Inbound != nil {
if s, ok := status.FromError(err); ok {
// user provided payloads can sometimes end up in the status details of
// gRPC errors, make sure to visit those as well
err = visitGrpcErrorPayload(ctx, err, s, options.Inbound)
}
}
if resMsg, ok := response.(proto.Message); ok && options.Inbound != nil {
if visitErr := VisitPayloads(ctx, resMsg, *options.Inbound); visitErr != nil {
// We are choosing visit error over RPC error in this basically-never-should-happen case
err = visitErr
}
}
return err
}, nil
}
cmd/proxygenerator/interceptor.go
Outdated
func visitGrpcErrorPayload(ctx context.Context, err error, s *status.Status, inbound *VisitPayloadsOptions) error { | ||
p := s.Proto() | ||
for _, detail := range p.Details { | ||
payloadTypes := []string{ {{ range $i, $name := .GrpcPayload }}{{ if $i }}, {{ end }}"{{$name}}"{{ end }} } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this to a package-level var (same for failureTypes
below), no need to make this fixed, immutable slice in this function (much less every loop iteration)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoot, I thought I did this before from your previous comment, thanks for the reminder
cmd/proxygenerator/interceptor.go
Outdated
if s, ok := status.FromError(err); ok { | ||
// user provided payloads can sometimes end up in the status details of | ||
// gRPC errors, make sure to visit those as well | ||
return visitGrpcErrorPayload(ctx, err, s, options.Inbound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still preventing the ability for both response
and err
to be non-nil and have response
be visited too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry! completely missed that from the suggestion
What changed?
Add support for parsing payloads for non-Any details.
Also added in support for visiting Payload/failures when both response and non-status errors are non-nil temporalio/sdk-go#1889
Why?
This shouldn't be possible, but adding a safety net here.
Follow up to #213,
How did you test it?
Potential risks