helper: Preserve status code of underlying errorps-popup-code

The variations of the Err<StatusCode>() and Err<StatusCode>f()
functions don't verify if the passed in error wraps gRPC error
already on some nested level except the upper one only.
That is why initially InvalidArgument code error could be
returned with Internal code and wrapped inside the message, so
it would look like there was another RPC method invocation.

The GrpcCode is refactored here as well, to follow the same approach.
Now it unwraps the errors and looks for the initial status most
deeply nested. If found it becomes the final for the RPC invocation
and send to the caller.

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))
+		})
+	}
+}