Merge branch 'ps-popup-code' into 'master'

helper: Preserve status code of underlying error

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4895

Merged-by: James Fargher <proglottis@gmail.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Co-authored-by: Pavlo Strokov <pstrokov@gitlab.com>

2 files changed, 114 insertions, 11 deletions

diff --git a/internal/helper/error.go b/internal/helper/error.go
index dd3e17f08..7a6d16f3f 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -57,10 +57,20 @@ func ErrAborted(err error) error { return wrapError(codes.Aborted, err) }
 // wrapError wraps the given error with the error code unless it's already a gRPC error. If given
 // nil it will return nil.
 func wrapError(code codes.Code, err error) error {
-	if GrpcCode(err) == codes.Unknown {
-		return statusWrapper{err, status.New(code, err.Error())}
+	if err == nil {
+		return nil
+	}
+
+	_, ok := status.FromError(err)
+	if ok {
+		return err
+	}
+
+	if foundCode := GrpcCode(err); foundCode != codes.OK && foundCode != codes.Unknown {
+		code = foundCode
 	}
-	return err
+
+	return statusWrapper{error: err, status: status.New(code, err.Error())}
 }
 
 // ErrCanceledf wraps a formatted error with codes.Canceled, unless the formatted error is a
@@ -165,9 +175,11 @@ func formatError(code codes.Code, format string, a ...interface{}) error {
 
 	err := fmt.Errorf(format, args...)
 
-	nestedCode := GrpcCode(errors.Unwrap(err))
-	if nestedCode != codes.OK && nestedCode != codes.Unknown {
-		code = nestedCode
+	for current := err; current != nil; current = errors.Unwrap(current) {
+		nestedCode := GrpcCode(current)
+		if nestedCode != codes.OK && nestedCode != codes.Unknown {
+			code = nestedCode
+		}
 	}
 
 	return statusWrapper{err, status.New(code, err.Error())}
@@ -197,16 +209,22 @@ func ErrWithDetails(err error, details ...proto.Message) (error, error) {
 	return statusWrapper{err, status.FromProto(proto)}, nil
 }
 
-// GrpcCode emulates the old grpc.Code function: it translates errors into codes.Code values.
+// GrpcCode translates errors into codes.Code values.
+// It unwraps the nested errors until it finds the most nested one that returns the codes.Code.
+// If err is nil it returns codes.OK.
+// If no codes.Code found it returns codes.Unknown.
 func GrpcCode(err error) codes.Code {
 	if err == nil {
 		return codes.OK
 	}
 
-	st, ok := status.FromError(err)
-	if !ok {
-		return codes.Unknown
+	code := codes.Unknown
+	for ; err != nil; err = errors.Unwrap(err) {
+		st, ok := status.FromError(err)
+		if ok {
+			code = st.Code()
+		}
 	}
 
-	return st.Code()
+	return code
 }
diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go
index 44df756cd..c0e7d6b5d 100644
--- a/internal/helper/error_test.go
+++ b/internal/helper/error_test.go
@@ -4,8 +4,10 @@ package helper
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -19,6 +21,7 @@ func TestError(t *testing.T) {
 	input := errors.New(errorMessage)
 	inputGRPCCode := codes.Unauthenticated
 	inputGRPC := status.Error(inputGRPCCode, errorMessage)
+	inputInternalGRPC := ErrAbortedf(errorMessage)
 
 	for _, tc := range []struct {
 		desc   string
@@ -93,6 +96,26 @@ func TestError(t *testing.T) {
 			require.False(t, errors.Is(err, input))
 			require.Equal(t, inputGRPCCode, status.Code(err))
 			require.NotEqual(t, tc.code, status.Code(inputGRPC))
+
+			// Wrapped gRPC error (internal.status.Error) code will get
+			// preserved, instead of the one corresponding to the function's
+			// name.
+			err = tc.errorf(fmt.Errorf("outer: %w", inputGRPC))
+			require.True(t, errors.Is(err, inputGRPC))
+			require.False(t, errors.Is(err, input))
+			require.Equal(t, inputGRPCCode, status.Code(err))
+			require.NotEqual(t, tc.code, status.Code(inputGRPC))
+
+			if tc.code != codes.Aborted {
+				// Wrapped gRPC error code constructed with helpers will get
+				// preserved, instead of the one corresponding to the function's
+				// name.
+				err = tc.errorf(fmt.Errorf("outer: %w", inputInternalGRPC))
+				require.True(t, errors.Is(err, inputInternalGRPC))
+				require.False(t, errors.Is(err, input))
+				require.Equal(t, codes.Aborted, status.Code(err))
+				require.NotEqual(t, tc.code, status.Code(inputInternalGRPC))
+			}
 		})
 	}
 }
@@ -223,6 +246,23 @@ func TestErrorf(t *testing.T) {
 				require.True(t, ok)
 				require.Equal(t, status.New(codes.Unauthenticated, "first: second: third"), s)
 			})
+
+			t.Run("multi-nesting with standard error wrapping", func(t *testing.T) {
+				require.NotEqual(t, tc.expectedCode, codes.Unauthenticated)
+
+				err := tc.errorf("first: %w",
+					fmt.Errorf("second: %w",
+						formatError(codes.Unauthenticated, "third"),
+					),
+				)
+				require.EqualError(t, err, "first: second: third")
+
+				// We should be reporting the error code of the nested error.
+				require.Equal(t, codes.Unauthenticated, status.Code(err))
+				s, ok := status.FromError(err)
+				require.True(t, ok)
+				require.Equal(t, status.New(codes.Unauthenticated, "first: second: third"), s)
+			})
 		})
 	}
 }
@@ -324,3 +364,48 @@ func TestErrWithDetails(t *testing.T) {
 		})
 	}
 }
+
+func TestGrpcCode(t *testing.T) {
+	t.Parallel()
+	for desc, tc := range map[string]struct {
+		in  error
+		exp codes.Code
+	}{
+		"unwrapped status": {
+			in:  status.Error(codes.NotFound, ""),
+			exp: codes.NotFound,
+		},
+		"wrapped status": {
+			in:  fmt.Errorf("context: %w", status.Error(codes.NotFound, "")),
+			exp: codes.NotFound,
+		},
+		"unwrapped status created by helpers": {
+			in:  ErrNotFoundf(""),
+			exp: codes.NotFound,
+		},
+		"wrapped status created by helpers": {
+			in:  fmt.Errorf("context: %w", ErrNotFoundf("")),
+			exp: codes.NotFound,
+		},
+		"double wrapped status created by helpers": {
+			in:  fmt.Errorf("outer: %w", fmt.Errorf("context: %w", ErrNotFoundf(""))),
+			exp: codes.NotFound,
+		},
+		"double helper wrapped status": {
+			in:  ErrAbortedf("outer: %w", fmt.Errorf("context: %w", ErrNotFoundf(""))),
+			exp: codes.NotFound,
+		},
+		"nil input": {
+			in:  nil,
+			exp: codes.OK,
+		},
+		"no code defined": {
+			in:  assert.AnError,
+			exp: codes.Unknown,
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			assert.Equal(t, tc.exp, GrpcCode(tc.in))
+		})
+	}
+}