helper: Fix error messages for wrapped gRPC errorspks-helper-fix-wrapped-grpc-error-messages

Our `Errorf()`-style error-wrapping functions know to propagate gRPC
error codes from any wrapped `status.Status` error. While this in
general works alright and does what it's intended to do, one thing that
doesn't work is that the resulting error message contains a reference to
the error code. This makes for hard-to-read error messages.

Fix this by wrapping all gRPC errors into a new error message wrapper.
Instead of returning the string returned by `status.Error()`, which
contains the reference to the error code, we only return the error
message itself.

4 files changed, 50 insertions, 16 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 72a5353f9..f57befb50 100644
--- a/internal/helper/error_test.go
+++ b/internal/helper/error_test.go
@@ -142,52 +142,52 @@ func TestErrorf(t *testing.T) {
 				require.NotEqual(t, tc.expectedCode, codes.Unauthenticated)
 
 				err := tc.errorf("top-level: %v", status.Errorf(codes.Unauthenticated, "deeply: %s", "nested"))
-				require.EqualError(t, err, "top-level: rpc error: code = Unauthenticated desc = deeply: 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: rpc error: code = Unauthenticated desc = deeply: nested"), s)
+				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: rpc error: code = Unauthenticated desc = deeply: 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: rpc error: code = Unauthenticated desc = deeply: nested"), s)
+				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: rpc error: code = Unauthenticated desc = 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: rpc error: code = Unauthenticated desc = nested"), s)
+				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: rpc error: code = Unauthenticated desc = 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: rpc error: code = Unauthenticated desc = nested"), s)
+				require.Equal(t, status.New(codes.Unauthenticated, "top-level: nested"), s)
 			})
 
 			t.Run("multi-nesting gRPC errors", func(t *testing.T) {
@@ -198,13 +198,13 @@ func TestErrorf(t *testing.T) {
 						status.Error(codes.Unauthenticated, "third"),
 					),
 				)
-				require.EqualError(t, err, "first: second: rpc error: code = Unauthenticated desc = 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: rpc error: code = Unauthenticated desc = third"), s)
+				require.Equal(t, status.New(codes.Unauthenticated, "first: second: third"), s)
 			})
 		})
 	}