Merge branch 'pks-helper-fix-wrapped-grpc-error-messages' into 'master'

helper: Fix error messages for wrapped gRPC errors

See merge request gitlab-org/gitaly!4692

4 files changed, 137 insertions, 73 deletions

diff --git a/internal/gitaly/service/repository/fullpath_test.go b/internal/gitaly/service/repository/fullpath_test.go
index 36b3947f7..cbe89f8f7 100644
--- a/internal/gitaly/service/repository/fullpath_test.go
+++ b/internal/gitaly/service/repository/fullpath_test.go
@@ -72,8 +72,7 @@ func TestSetFullPath(t *testing.T) {
 
 		require.Nil(t, response)
 
-		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = setting config: rpc "+
-			"error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath)
+		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = setting config: GetRepoPath: not a git repository: %q", repoPath)
 		if testhelper.IsPraefectEnabled() {
 			expectedErr = `rpc error: code = NotFound desc = mutator call: route repository mutator: get repository id: repository "default"/"/path/to/repo.git" not found`
 		}
@@ -176,8 +175,7 @@ func TestFullPath(t *testing.T) {
 		})
 		require.Nil(t, response)
 
-		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = fetch config: rpc "+
-			"error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath)
+		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = fetch config: GetRepoPath: not a git repository: %q", repoPath)
 		if testhelper.IsPraefectEnabled() {
 			expectedErr = `rpc error: code = NotFound desc = accessor call: route repository accessor: consistent storages: repository "default"/"/path/to/repo.git" not found`
 		}
diff --git a/internal/gitaly/service/repository/license_test.go b/internal/gitaly/service/repository/license_test.go
index 7d6247529..0cd09ba71 100644
--- a/internal/gitaly/service/repository/license_test.go
+++ b/internal/gitaly/service/repository/license_test.go
@@ -26,7 +26,7 @@ func testSuccessfulFindLicenseRequest(t *testing.T, cfg config.Cfg, client gital
 			{
 				desc:                  "repository does not exist",
 				nonExistentRepository: true,
-				errorContains:         "rpc error: code = NotFound desc = GetRepoPath: not a git repository",
+				errorContains:         "GetRepoPath: not a git repository",
 			},
 			{
 				desc: "empty if no license file in repo",
diff --git a/internal/helper/error.go b/internal/helper/error.go
index 5196513e0..e649e5231 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -108,11 +108,47 @@ func ErrAbortedf(format string, a ...interface{}) error {
 	return formatError(codes.Aborted, format, a...)
 }
 
+// grpcErrorMessageWrapper is used to wrap a gRPC `status.Status`-style error such that it behaves
+// like a `status.Status`, except that it generates a readable error message.
+type grpcErrorMessageWrapper struct {
+	*status.Status
+}
+
+func (e grpcErrorMessageWrapper) GRPCStatus() *status.Status {
+	return e.Status
+}
+
+func (e grpcErrorMessageWrapper) Error() string {
+	return e.Message()
+}
+
+func (e grpcErrorMessageWrapper) Unwrap() error {
+	return e.Status.Err()
+}
+
 // formatError will create a new error from the given format string. If the error string contains a
 // %w verb and its corresponding error has a gRPC error code, then the returned error will keep this
 // gRPC error code instead of using the one provided as an argument.
 func formatError(code codes.Code, format string, a ...interface{}) error {
-	err := fmt.Errorf(format, a...)
+	args := make([]interface{}, 0, len(a))
+	for _, a := range a {
+		err, ok := a.(error)
+		if !ok {
+			args = append(args, a)
+			continue
+		}
+
+		status, ok := status.FromError(err)
+		if !ok {
+			args = append(args, a)
+			continue
+		}
+
+		// Wrap gRPC status errors so that the resulting error message stays readable.
+		args = append(args, grpcErrorMessageWrapper{status})
+	}
+
+	err := fmt.Errorf(format, args...)
 
 	nestedCode := GrpcCode(errors.Unwrap(err))
 	if nestedCode != codes.OK && nestedCode != codes.Unknown {
diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go
index 0a0d473c8..f57befb50 100644
--- a/internal/helper/error_test.go
+++ b/internal/helper/error_test.go
@@ -2,8 +2,6 @@ package helper
 
 import (
 	"errors"
-	"fmt"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -92,90 +90,122 @@ func TestError(t *testing.T) {
 	}
 }
 
-func TestErrorF_withVFormat(t *testing.T) {
-	testErrorfFormat(t, "expected %v", "expected %v")
-}
-
-func TestErrorF_withWFormat(t *testing.T) {
-	testErrorfFormat(t, "expected %w", "expected %s")
-}
-
-func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) {
-	isFormatW := strings.Contains(errorFormat, "%w")
-	errorMessage := "sentinel error"
-	input := errors.New(errorMessage)
-	inputGRPCCode := codes.Unauthenticated
-	inputGRPC := status.Error(inputGRPCCode, errorMessage)
-	inputGRPCFmt := status.Errorf(inputGRPCCode, errorFormat, errorMessage)
-
+func TestErrorf(t *testing.T) {
 	for _, tc := range []struct {
-		desc   string
-		errorf func(format string, a ...interface{}) error
-		code   codes.Code
+		desc         string
+		errorf       func(format string, a ...interface{}) error
+		expectedCode codes.Code
 	}{
 		{
-			desc:   "Internalf",
-			errorf: ErrInternalf,
-			code:   codes.Internal,
+			desc:         "Internalf",
+			errorf:       ErrInternalf,
+			expectedCode: codes.Internal,
 		},
 		{
-			desc:   "InvalidArgumentf",
-			errorf: ErrInvalidArgumentf,
-			code:   codes.InvalidArgument,
+			desc:         "InvalidArgumentf",
+			errorf:       ErrInvalidArgumentf,
+			expectedCode: codes.InvalidArgument,
 		},
 		{
-			desc:   "FailedPreconditionf",
-			errorf: ErrFailedPreconditionf,
-			code:   codes.FailedPrecondition,
+			desc:         "FailedPreconditionf",
+			errorf:       ErrFailedPreconditionf,
+			expectedCode: codes.FailedPrecondition,
 		},
 		{
-			desc:   "NotFoundf",
-			errorf: ErrNotFoundf,
-			code:   codes.NotFound,
+			desc:         "NotFoundf",
+			errorf:       ErrNotFoundf,
+			expectedCode: codes.NotFound,
 		},
 		{
-			desc:   "ErrUnavailablef",
-			errorf: ErrUnavailablef,
-			code:   codes.Unavailable,
+			desc:         "ErrUnavailablef",
+			errorf:       ErrUnavailablef,
+			expectedCode: codes.Unavailable,
 		},
 		{
-			desc:   "ErrAbortedf",
-			errorf: ErrAbortedf,
-			code:   codes.Aborted,
+			desc:         "ErrAbortedf",
+			errorf:       ErrAbortedf,
+			expectedCode: codes.Aborted,
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			require.NotEqual(t, tc.code, inputGRPCCode, "canary test code and tc.code may not be the same")
+			t.Run("with non-gRPC error", func(t *testing.T) {
+				err := tc.errorf("top-level: %w", errors.New("nested"))
+				require.EqualError(t, err, "top-level: nested")
+				require.Equal(t, tc.expectedCode, status.Code(err))
 
-			// When not re-throwing an error we get the GRPC error code corresponding to
-			// the function's name. Just like the non-f functions.
-			err := tc.errorf(errorFormat, input)
-			require.EqualError(t, err, fmt.Sprintf(errorFormatEqual, errorMessage))
-			require.False(t, errors.Is(err, inputGRPC))
-			require.Equal(t, tc.code, status.Code(err))
+				s, ok := status.FromError(err)
+				require.True(t, ok)
+				require.Equal(t, status.New(tc.expectedCode, "top-level: nested"), s)
+			})
 
-			// When wrapping an existing gRPC error, then the error code will stay the
-			// same.
-			err = tc.errorf(errorFormat, inputGRPCFmt)
-			require.False(t, errors.Is(err, input))
-			if isFormatW {
-				require.Equal(t, inputGRPCCode, status.Code(err))
-			} else {
-				require.Equal(t, tc.code, status.Code(err))
-			}
-			require.NotEqual(t, tc.code, status.Code(inputGRPC))
+			t.Run("with status.Errorf error and %v", func(t *testing.T) {
+				require.NotEqual(t, tc.expectedCode, codes.Unauthenticated)
 
-			// The same as above, except that we test with an error returned by
-			// `status.Error()`.
-			errX := tc.errorf(errorFormat, inputGRPC)
-			require.Equal(t, errors.Is(errX, inputGRPC), isFormatW) // .True() for non-f
-			require.False(t, errors.Is(errX, input))
-			if isFormatW {
-				require.Equal(t, inputGRPCCode, status.Code(errX))
-			} else {
-				require.Equal(t, tc.code, status.Code(errX))
-			}
-			require.Equal(t, inputGRPCCode, status.Code(inputGRPC))
+				err := tc.errorf("top-level: %v", status.Errorf(codes.Unauthenticated, "deeply: %s", "nested"))
+				require.EqualError(t, err, "top-level: deeply: nested")
+
+				// The error code of the nested error should be discarded.
+				require.Equal(t, tc.expectedCode, status.Code(err))
+				s, ok := status.FromError(err)
+				require.True(t, ok)
+				require.Equal(t, status.New(tc.expectedCode, "top-level: deeply: nested"), s)
+			})
+
+			t.Run("with status.Errorf error and %w", func(t *testing.T) {
+				require.NotEqual(t, tc.expectedCode, codes.Unauthenticated)
+
+				err := tc.errorf("top-level: %w", status.Errorf(codes.Unauthenticated, "deeply: %s", "nested"))
+				require.EqualError(t, err, "top-level: deeply: nested")
+
+				// 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, "top-level: deeply: nested"), s)
+			})
+
+			t.Run("with status.Error error and %v", func(t *testing.T) {
+				require.NotEqual(t, tc.expectedCode, codes.Unauthenticated)
+
+				err := tc.errorf("top-level: %v", status.Error(codes.Unauthenticated, "nested"))
+				require.EqualError(t, err, "top-level: nested")
+
+				// The error code of the nested error should be discarded.
+				require.Equal(t, tc.expectedCode, status.Code(err))
+				s, ok := status.FromError(err)
+				require.True(t, ok)
+				require.Equal(t, status.New(tc.expectedCode, "top-level: nested"), s)
+			})
+
+			t.Run("with status.Error error and %w", func(t *testing.T) {
+				require.NotEqual(t, tc.expectedCode, codes.Unauthenticated)
+
+				err := tc.errorf("top-level: %w", status.Error(codes.Unauthenticated, "nested"))
+				require.EqualError(t, err, "top-level: nested")
+
+				// 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, "top-level: nested"), s)
+			})
+
+			t.Run("multi-nesting gRPC errors", func(t *testing.T) {
+				require.NotEqual(t, tc.expectedCode, codes.Unauthenticated)
+
+				err := tc.errorf("first: %w",
+					ErrInternalf("second: %w",
+						status.Error(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)
+			})
 		})
 	}
 }