fix(internal/retry): Simplify gRPC status code mapping of retry error (#8196)

atollena · web-flow · commit e8b224a3bcb0 · 2023-07-05T18:22:12.000Z
gRPC v1.55.0 that this project depends on came with a change in semantic for status.FromError such that the gRPC status of wrapped errors is returned. The implementation of GRPCStatus in internal/retry code was doing just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment, status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately obvious why this can't result in infinite recursion. A previous change I made to this code made sure this method never returns Status.OK (https://togithub.com/googleapis/google-cloud-go/pull/8128), but I failed to realize that since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle unwrapping. Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think otherwise (for the same reasons discussed in https://togithub.com/grpc/grpc-go/pull/6150).
diff --git a/internal/retry.go b/internal/retry.go
@@ -20,8 +20,6 @@ import (
 	"time"
 
 	gax "github.com/googleapis/gax-go/v2"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 // Retry calls the supplied function f repeatedly according to the provided
@@ -76,11 +74,3 @@ func (e wrappedCallErr) Unwrap() error {
 func (e wrappedCallErr) Is(err error) bool {
 	return e.ctxErr == err || e.wrappedErr == err
 }
-
-// GRPCStatus allows the wrapped error to be used with status.FromError.
-func (e wrappedCallErr) GRPCStatus() *status.Status {
-	if s, ok := status.FromError(e.wrappedErr); ok {
-		return s
-	}
-	return status.New(codes.Unknown, e.Error())
-}
diff --git a/internal/retry_test.go b/internal/retry_test.go
@@ -88,7 +88,7 @@ func TestRetryPreserveError(t *testing.T) {
 	if g, w := got.Code(), codes.NotFound; g != w {
 		t.Errorf("got code %v, want %v", g, w)
 	}
-	wantMessage := "not found"
+	wantMessage := "retry failed with context deadline exceeded; last error: rpc error: code = NotFound desc = not found"
 	if g, w := got.Message(), wantMessage; g != w {
 		t.Errorf("got message %q, want %q", g, w)
 	}
@@ -111,10 +111,7 @@ func TestRetryWrapsErrorWithStatusUnknown(t *testing.T) {
 	if g, w := err.Error(), wantError; g != w {
 		t.Errorf("got error %q, want %q", g, w)
 	}
-	got, ok := status.FromError(err)
-	if !ok {
-		t.Fatal("expected error to implement a gRPC status")
-	}
+	got, _ := status.FromError(err)
 	if g, w := got.Code(), codes.Unknown; g != w {
 		t.Errorf("got code %v, want %v", g, w)
 	}

Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ func TestRetryPreserveError(t *testing.T) {`
`88`	`88`	`if g, w := got.Code(), codes.NotFound; g != w {`
`89`	`89`	`t.Errorf("got code %v, want %v", g, w)`
`90`	`90`	`}`
`91`		`- wantMessage := "not found"`
	`91`	`+ wantMessage := "retry failed with context deadline exceeded; last error: rpc error: code = NotFound desc = not found"`
`92`	`92`	`if g, w := got.Message(), wantMessage; g != w {`
`93`	`93`	`t.Errorf("got message %q, want %q", g, w)`
`94`	`94`	`}`
`@@ -111,10 +111,7 @@ func TestRetryWrapsErrorWithStatusUnknown(t *testing.T) {`
`111`	`111`	`if g, w := err.Error(), wantError; g != w {`
`112`	`112`	`t.Errorf("got error %q, want %q", g, w)`
`113`	`113`	`}`
`114`		`- got, ok := status.FromError(err)`
`115`		`- if !ok {`
`116`		`- t.Fatal("expected error to implement a gRPC status")`
`117`		`- }`
	`114`	`+ got, _ := status.FromError(err)`
`118`	`115`	`if g, w := got.Code(), codes.Unknown; g != w {`
`119`	`116`	`t.Errorf("got code %v, want %v", g, w)`
`120`	`117`	`}`