Skip to content

Conversation

yuandrew
Copy link
Contributor

@yuandrew yuandrew commented Mar 27, 2025

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

@yuandrew yuandrew requested review from a team as code owners March 27, 2025 18:24
@yuandrew
Copy link
Contributor Author

oops, I kinda forgot about this PR 😅

}
return newStatus.Err()
}
if s, ok := status.FromError(err); ok {
Copy link
Member

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

Copy link
Contributor Author

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

@yuandrew yuandrew requested a review from cretz April 28, 2025 23:50
}
return err
Copy link
Member

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.

Copy link
Contributor Author

@yuandrew yuandrew Apr 29, 2025

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.

Copy link
Contributor Author

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?

Copy link
Member

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
}

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 }} }
Copy link
Member

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)

Copy link
Contributor Author

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

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)
Copy link
Member

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

Copy link
Contributor Author

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

@yuandrew yuandrew merged commit c479868 into temporalio:master May 1, 2025
4 checks passed
@yuandrew yuandrew deleted the grpc-failure-details-any branch May 1, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants