From 7921a13d32e8b4860727d87c0237eb9ea2dfc4fe Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 7 Jul 2022 12:48:19 +0200 Subject: helper: Convert gRPC `Errorf()` tests to use more subtests The tests for `Errorf()`-style gRPC error wrapping check various different cases, but do so in-line without the use of subtests. This makes it hard to grasp the scope and that we are testing different functionality. Use subtests to clearly highlight the scope. --- internal/helper/error_test.go | 62 ++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 27 deletions(-) diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go index 0a0d473c8..f08f9d051 100644 --- a/internal/helper/error_test.go +++ b/internal/helper/error_test.go @@ -147,35 +147,43 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { 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") - // 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)) - - // 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 { + t.Run("with non-gRPC error", func(t *testing.T) { + // 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)) - } - require.NotEqual(t, tc.code, status.Code(inputGRPC)) + }) - // 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)) + t.Run("with status.Errorf error", func(t *testing.T) { + // 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.Error error", func(t *testing.T) { + // The same as above, except that we test with an error returned by + // `status.Error()`. + err := tc.errorf(errorFormat, inputGRPC) + require.Equal(t, errors.Is(err, inputGRPC), isFormatW) // .True() for non-f + 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.Equal(t, inputGRPCCode, status.Code(inputGRPC)) + }) }) } } -- cgit v1.2.3 From e3d479a4132f861f90a7c841df517916bfd61533 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 7 Jul 2022 12:58:55 +0200 Subject: helper: Rename variable that holds expected code in `Errorf()` tests Rename the variable that holds the expected code in `Errorf()`-style tests. This makes it easier to spot and understand what's going on. --- internal/helper/error_test.go | 52 +++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go index f08f9d051..c5d789edc 100644 --- a/internal/helper/error_test.go +++ b/internal/helper/error_test.go @@ -109,43 +109,43 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { inputGRPCFmt := status.Errorf(inputGRPCCode, errorFormat, errorMessage) 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") + require.NotEqual(t, tc.expectedCode, inputGRPCCode, "canary test code and tc.code may not be the same") t.Run("with non-gRPC error", func(t *testing.T) { // When not re-throwing an error we get the GRPC error code @@ -154,7 +154,7 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { 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)) + require.Equal(t, tc.expectedCode, status.Code(err)) }) t.Run("with status.Errorf error", func(t *testing.T) { @@ -166,9 +166,9 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { if isFormatW { require.Equal(t, inputGRPCCode, status.Code(err)) } else { - require.Equal(t, tc.code, status.Code(err)) + require.Equal(t, tc.expectedCode, status.Code(err)) } - require.NotEqual(t, tc.code, status.Code(inputGRPC)) + require.NotEqual(t, tc.expectedCode, status.Code(inputGRPC)) }) t.Run("with status.Error error", func(t *testing.T) { @@ -180,7 +180,7 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { if isFormatW { require.Equal(t, inputGRPCCode, status.Code(err)) } else { - require.Equal(t, tc.code, status.Code(err)) + require.Equal(t, tc.expectedCode, status.Code(err)) } require.Equal(t, inputGRPCCode, status.Code(inputGRPC)) }) -- cgit v1.2.3 From 6d0749657251b7f9fbe952357167a73e6de1b7c5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 7 Jul 2022 13:11:31 +0200 Subject: helper: Inline input and expectations for `Errorf()` tests The tests for `Errorf()`-style error wrapping are quite hard to understand because of the different input and output variables. It may just be me, but I think it's hard to understand what the difference between `input`, `inputGRPC`, `inputGRPCFmt` and `inputGRPCCode` is at a detailed level. Let's inline these variables to make the testcases mostly self-contained and thus easier to understand. --- internal/helper/error_test.go | 62 +++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go index c5d789edc..a13401716 100644 --- a/internal/helper/error_test.go +++ b/internal/helper/error_test.go @@ -2,7 +2,6 @@ package helper import ( "errors" - "fmt" "strings" "testing" @@ -93,20 +92,15 @@ func TestError(t *testing.T) { } func TestErrorF_withVFormat(t *testing.T) { - testErrorfFormat(t, "expected %v", "expected %v") + testErrorfFormat(t, "top-level: %v", "top-level: %v") } func TestErrorF_withWFormat(t *testing.T) { - testErrorfFormat(t, "expected %w", "expected %s") + testErrorfFormat(t, "top-level: %w", "top-level: %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) for _, tc := range []struct { desc string @@ -145,44 +139,54 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { }, } { t.Run(tc.desc, func(t *testing.T) { - require.NotEqual(t, tc.expectedCode, inputGRPCCode, "canary test code and tc.code may not be the same") - t.Run("with non-gRPC error", func(t *testing.T) { - // 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)) + err := tc.errorf(errorFormat, errors.New("nested")) + require.EqualError(t, err, "top-level: nested") 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.Errorf error", func(t *testing.T) { - // When wrapping an existing gRPC error, then the error code will - // stay the same. + require.NotEqual(t, tc.expectedCode, codes.Unauthenticated) - err := tc.errorf(errorFormat, inputGRPCFmt) - require.False(t, errors.Is(err, input)) + err := tc.errorf(errorFormat, status.Errorf(codes.Unauthenticated, "deeply: %s", "nested")) + require.EqualError(t, err, "top-level: rpc error: code = Unauthenticated desc = deeply: nested") if isFormatW { - require.Equal(t, inputGRPCCode, status.Code(err)) + require.Equal(t, codes.Unauthenticated, status.Code(err)) } else { require.Equal(t, tc.expectedCode, status.Code(err)) } - require.NotEqual(t, tc.expectedCode, status.Code(inputGRPC)) + + s, ok := status.FromError(err) + require.True(t, ok) + if isFormatW { + require.Equal(t, status.New(codes.Unauthenticated, "top-level: rpc error: code = Unauthenticated desc = deeply: nested"), s) + } else { + require.Equal(t, status.New(tc.expectedCode, "top-level: rpc error: code = Unauthenticated desc = deeply: nested"), s) + } }) t.Run("with status.Error error", func(t *testing.T) { - // The same as above, except that we test with an error returned by - // `status.Error()`. - err := tc.errorf(errorFormat, inputGRPC) - require.Equal(t, errors.Is(err, inputGRPC), isFormatW) // .True() for non-f - require.False(t, errors.Is(err, input)) + require.NotEqual(t, tc.expectedCode, codes.Unauthenticated) + + err := tc.errorf(errorFormat, status.Error(codes.Unauthenticated, "nested")) + require.EqualError(t, err, "top-level: rpc error: code = Unauthenticated desc = nested") if isFormatW { - require.Equal(t, inputGRPCCode, status.Code(err)) + require.Equal(t, codes.Unauthenticated, status.Code(err)) } else { require.Equal(t, tc.expectedCode, status.Code(err)) } - require.Equal(t, inputGRPCCode, status.Code(inputGRPC)) + + s, ok := status.FromError(err) + require.True(t, ok) + if isFormatW { + require.Equal(t, status.New(codes.Unauthenticated, "top-level: rpc error: code = Unauthenticated desc = nested"), s) + } else { + require.Equal(t, status.New(tc.expectedCode, "top-level: rpc error: code = Unauthenticated desc = nested"), s) + } }) }) } -- cgit v1.2.3 From adb908088a9bf52b0d093b8ed98b7b43c3845e24 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 7 Jul 2022 13:15:17 +0200 Subject: helper: Use separate tests for `Errorf()` with "%w" and "%v" The conditional testing we have for `Errorf()`-style error wrapping with either `"%w"` or `"%v"` formatters makes tests hard to read. Refactor them to instead use separate subtests. While this increases duplication, it does make the tests a lot more readable. --- internal/helper/error_test.go | 75 ++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go index a13401716..80b0d429e 100644 --- a/internal/helper/error_test.go +++ b/internal/helper/error_test.go @@ -2,7 +2,6 @@ package helper import ( "errors" - "strings" "testing" "github.com/stretchr/testify/require" @@ -91,17 +90,7 @@ func TestError(t *testing.T) { } } -func TestErrorF_withVFormat(t *testing.T) { - testErrorfFormat(t, "top-level: %v", "top-level: %v") -} - -func TestErrorF_withWFormat(t *testing.T) { - testErrorfFormat(t, "top-level: %w", "top-level: %s") -} - -func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { - isFormatW := strings.Contains(errorFormat, "%w") - +func TestErrorf(t *testing.T) { for _, tc := range []struct { desc string errorf func(format string, a ...interface{}) error @@ -140,7 +129,7 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { } { t.Run(tc.desc, func(t *testing.T) { t.Run("with non-gRPC error", func(t *testing.T) { - err := tc.errorf(errorFormat, errors.New("nested")) + err := tc.errorf("top-level: %w", errors.New("nested")) require.EqualError(t, err, "top-level: nested") require.Equal(t, tc.expectedCode, status.Code(err)) @@ -149,44 +138,56 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { require.Equal(t, status.New(tc.expectedCode, "top-level: nested"), s) }) - t.Run("with status.Errorf error", func(t *testing.T) { + t.Run("with status.Errorf error and %v", func(t *testing.T) { require.NotEqual(t, tc.expectedCode, codes.Unauthenticated) - err := tc.errorf(errorFormat, status.Errorf(codes.Unauthenticated, "deeply: %s", "nested")) + 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") - if isFormatW { - require.Equal(t, codes.Unauthenticated, status.Code(err)) - } else { - require.Equal(t, tc.expectedCode, status.Code(err)) - } + // 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) + }) + + 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") + + // 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) + }) + + 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") + + // 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) - if isFormatW { - require.Equal(t, status.New(codes.Unauthenticated, "top-level: rpc error: code = Unauthenticated desc = deeply: nested"), s) - } else { - 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: rpc error: code = Unauthenticated desc = nested"), s) }) - t.Run("with status.Error error", func(t *testing.T) { + t.Run("with status.Error error and %w", func(t *testing.T) { require.NotEqual(t, tc.expectedCode, codes.Unauthenticated) - err := tc.errorf(errorFormat, status.Error(codes.Unauthenticated, "nested")) + err := tc.errorf("top-level: %w", status.Error(codes.Unauthenticated, "nested")) require.EqualError(t, err, "top-level: rpc error: code = Unauthenticated desc = nested") - if isFormatW { - require.Equal(t, codes.Unauthenticated, status.Code(err)) - } else { - require.Equal(t, tc.expectedCode, status.Code(err)) - } + // 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) - if isFormatW { - require.Equal(t, status.New(codes.Unauthenticated, "top-level: rpc error: code = Unauthenticated desc = nested"), s) - } else { - require.Equal(t, status.New(tc.expectedCode, "top-level: rpc error: code = Unauthenticated desc = nested"), s) - } + require.Equal(t, status.New(codes.Unauthenticated, "top-level: rpc error: code = Unauthenticated desc = nested"), s) }) }) } -- cgit v1.2.3 From f3ece0197d95d786e2b61b54ce3efad632a5720b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 7 Jul 2022 13:52:38 +0200 Subject: helper: Add test to verify formatting with nested gRPC errors Add a testcase to verify that formatting of gRPC errors works as expected with nested errors. --- internal/helper/error_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go index 80b0d429e..72a5353f9 100644 --- a/internal/helper/error_test.go +++ b/internal/helper/error_test.go @@ -189,6 +189,23 @@ func TestErrorf(t *testing.T) { require.True(t, ok) require.Equal(t, status.New(codes.Unauthenticated, "top-level: rpc error: code = Unauthenticated desc = 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: rpc error: code = Unauthenticated desc = 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) + }) }) } } -- cgit v1.2.3 From 898ea2a2d517463206309b3c8973f75ab363d0ca Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 7 Jul 2022 12:38:40 +0200 Subject: helper: Fix error messages for wrapped gRPC errors 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. --- .../gitaly/service/repository/fullpath_test.go | 6 ++-- internal/gitaly/service/repository/license_test.go | 2 +- internal/helper/error.go | 38 +++++++++++++++++++++- internal/helper/error_test.go | 20 ++++++------ 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) }) }) } -- cgit v1.2.3