diff options
author | Quang-Minh Nguyen <qmnguyen@gitlab.com> | 2023-03-17 15:21:57 +0300 |
---|---|---|
committer | Quang-Minh Nguyen <qmnguyen@gitlab.com> | 2023-03-17 15:21:57 +0300 |
commit | 076d3b6bbfc3fac82dc9430205667f442b252c57 (patch) | |
tree | 7c7bdf1711d0fc2d3a0f6e4d9f336cb756125608 | |
parent | 5d96e4d2a6b2776e28d75b5545edeee268698101 (diff) | |
parent | c7e2b60d4a619e96a8a91a4f54f5ca204b979db3 (diff) |
Merge branch 'pks-structerr-unknown-by-default' into 'master'
structerr: Convert `New()` to return `Unknown` error codes
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5521
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
-rw-r--r-- | STYLE.md | 5 | ||||
-rw-r--r-- | internal/gitaly/service/diff/patch_id_test.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/operations/tags.go | 6 | ||||
-rw-r--r-- | internal/gitaly/service/remote/find_remote_root_ref_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/repository/apply_gitattributes_test.go | 5 | ||||
-rw-r--r-- | internal/gitaly/service/repository/fetch_remote_test.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/repository/object_format_test.go | 2 | ||||
-rw-r--r-- | internal/praefect/remove_all_test.go | 5 | ||||
-rw-r--r-- | internal/praefect/remove_repository_test.go | 4 | ||||
-rw-r--r-- | internal/praefect/repository_exists_test.go | 6 | ||||
-rw-r--r-- | internal/structerr/error.go | 26 | ||||
-rw-r--r-- | internal/structerr/error_test.go | 45 | ||||
-rw-r--r-- | internal/structerr/grpc_server_test.go | 6 |
13 files changed, 65 insertions, 55 deletions
@@ -88,6 +88,11 @@ func CheckFrobnicated(s string) error { } ``` +Unless the root cause of an error can be determined and categorized, you should +use `structerr.New()` to generate an error with an `Unknown` error code. This +code will automatically get overridden when the error is wrapped with a specific +error code. + ### Error wrapping You should use wrapping directives `"%w"` to wrap errors. This diff --git a/internal/gitaly/service/diff/patch_id_test.go b/internal/gitaly/service/diff/patch_id_test.go index 386ad45c9..ddf29bc9b 100644 --- a/internal/gitaly/service/diff/patch_id_test.go +++ b/internal/gitaly/service/diff/patch_id_test.go @@ -240,7 +240,7 @@ func TestGetPatchID(t *testing.T) { NewRevision: []byte(fmt.Sprintf("%s:file", newCommit)), }, //nolint:gitaly-linters - expectedErr: structerr.New("waiting for git-diff: exit status 128"). + expectedErr: structerr.NewInternal("waiting for git-diff: exit status 128"). WithInterceptedMetadata("stderr", fmt.Sprintf("fatal: path 'file' does not exist in '%s'\n", oldCommit)), } }, @@ -258,7 +258,7 @@ func TestGetPatchID(t *testing.T) { OldRevision: []byte(gittest.DefaultObjectHash.ZeroOID), NewRevision: []byte(newRevision), }, - expectedErr: structerr.New("waiting for git-diff: exit status 128"). + expectedErr: structerr.NewInternal("waiting for git-diff: exit status 128"). WithInterceptedMetadata("stderr", fmt.Sprintf("fatal: bad object %s\n", gittest.DefaultObjectHash.ZeroOID)), } }, diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 437f33d67..caa14794f 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -281,9 +281,9 @@ func (s *Server) createTag( if makingTag { tagObjectID, err := repo.WriteTag(ctx, targetObjectID, targetObjectType, tagName, message, committer, committerTime) if err != nil { - var FormatTagError localrepo.FormatTagError - if errors.As(err, &FormatTagError) { - return nil, "", structerr.NewUnknown("Rugged::InvalidError: failed to parse signature - expected prefix doesn't match actual") + var formatTagErr localrepo.FormatTagError + if errors.As(err, &formatTagErr) { + return nil, "", structerr.NewInvalidArgument("formatting tag: %w", formatTagErr) } var MktagError localrepo.MktagError diff --git a/internal/gitaly/service/remote/find_remote_root_ref_test.go b/internal/gitaly/service/remote/find_remote_root_ref_test.go index 42e20dcfb..196a8fabc 100644 --- a/internal/gitaly/service/remote/find_remote_root_ref_test.go +++ b/internal/gitaly/service/remote/find_remote_root_ref_test.go @@ -125,7 +125,7 @@ func TestFindRemoteRootRef(t *testing.T) { Repository: localRepo, RemoteUrl: "file://" + testhelper.TempDir(t), }, - expectedErr: structerr.New("exit status 128"), + expectedErr: structerr.NewInternal("exit status 128"), } }, }, diff --git a/internal/gitaly/service/repository/apply_gitattributes_test.go b/internal/gitaly/service/repository/apply_gitattributes_test.go index 227d8e36e..a3ab4aca8 100644 --- a/internal/gitaly/service/repository/apply_gitattributes_test.go +++ b/internal/gitaly/service/repository/apply_gitattributes_test.go @@ -3,7 +3,6 @@ package repository import ( "bytes" "context" - "errors" "os" "path/filepath" "testing" @@ -164,11 +163,11 @@ func TestApplyGitattributes_transactional(t *testing.T) { desc: "failing vote does not write gitattributes", revision: []byte(commitWithGitattributes), voteFn: func(t *testing.T, request *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) { - return nil, errors.New("foobar") + return nil, structerr.NewFailedPrecondition("foobar") }, shouldExist: false, expectedErr: func() error { - return structerr.NewUnknown("committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar") + return structerr.NewFailedPrecondition("committing gitattributes: voting on locked file: preimage vote: rpc error: code = FailedPrecondition desc = foobar") }(), expectedVotes: 1, }, diff --git a/internal/gitaly/service/repository/fetch_remote_test.go b/internal/gitaly/service/repository/fetch_remote_test.go index 3ae37edca..01bb7feb5 100644 --- a/internal/gitaly/service/repository/fetch_remote_test.go +++ b/internal/gitaly/service/repository/fetch_remote_test.go @@ -641,7 +641,7 @@ func TestFetchRemote(t *testing.T) { "refs/heads/main": localDivergingID, "refs/heads/branch": remoteUpdatedID, }, - expectedErr: structerr.New("fetch remote: exit status 1"), + expectedErr: structerr.NewInternal("fetch remote: exit status 1"), }, }, } @@ -678,7 +678,7 @@ func TestFetchRemote(t *testing.T) { "refs/heads/main": localDivergingID, "refs/tags/v1.0.0": remoteTagID, }, - expectedErr: structerr.New("fetch remote: exit status 1"), + expectedErr: structerr.NewInternal("fetch remote: exit status 1"), }, }, } diff --git a/internal/gitaly/service/repository/object_format_test.go b/internal/gitaly/service/repository/object_format_test.go index 80d41c36f..83053c899 100644 --- a/internal/gitaly/service/repository/object_format_test.go +++ b/internal/gitaly/service/repository/object_format_test.go @@ -151,7 +151,7 @@ func TestObjectFormat(t *testing.T) { request: &gitalypb.ObjectFormatRequest{ Repository: repoProto, }, - expectedErr: structerr.New("detecting object hash: reading object format: exit status 128").WithInterceptedMetadata( + expectedErr: structerr.NewInternal("detecting object hash: reading object format: exit status 128").WithInterceptedMetadata( "stderr", fmt.Sprintf("error: invalid value for 'extensions.objectformat': 'blake2b'\n"+ "fatal: bad config line 5 in file %s\n", filepath.Join(repoPath, "config"), diff --git a/internal/praefect/remove_all_test.go b/internal/praefect/remove_all_test.go index 7b7d43586..add4fc3f3 100644 --- a/internal/praefect/remove_all_test.go +++ b/internal/praefect/remove_all_test.go @@ -15,15 +15,14 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/grpc-proxy/proxy" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/protoregistry" + "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testdb" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc" - "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials/insecure" - "google.golang.org/grpc/status" ) func TestRemoveAllHandler(t *testing.T) { @@ -32,7 +31,7 @@ func TestRemoveAllHandler(t *testing.T) { const virtualStorage, relativePath = "virtual-storage", "relative-path" - errServedByGitaly := status.Error(codes.Unknown, "request passed to Gitaly") + errServedByGitaly := structerr.NewInternal("request passed to Gitaly") db := testdb.New(t) const gitaly1Storage = "gitaly-1" diff --git a/internal/praefect/remove_repository_test.go b/internal/praefect/remove_repository_test.go index 7bdce8a76..2d5c4d34d 100644 --- a/internal/praefect/remove_repository_test.go +++ b/internal/praefect/remove_repository_test.go @@ -22,16 +22,14 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc" - "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials/insecure" - "google.golang.org/grpc/status" ) func TestRemoveRepositoryHandler(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) - errServedByGitaly := status.Error(codes.Unknown, "request passed to Gitaly") + errServedByGitaly := structerr.NewInternal("request passed to Gitaly") const virtualStorage, relativePath = "virtual-storage", "relative-path" db := testdb.New(t) diff --git a/internal/praefect/repository_exists_test.go b/internal/praefect/repository_exists_test.go index ba005cc84..29f919c6b 100644 --- a/internal/praefect/repository_exists_test.go +++ b/internal/praefect/repository_exists_test.go @@ -13,18 +13,18 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/grpc-proxy/proxy" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/protoregistry" + "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testdb" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc" - "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials/insecure" - "google.golang.org/grpc/status" ) func TestRepositoryExistsHandler(t *testing.T) { t.Parallel() - errServedByGitaly := status.Error(codes.Unknown, "request passed to Gitaly") + + errServedByGitaly := structerr.NewInternal("request passed to Gitaly") db := testdb.New(t) for _, tc := range []struct { diff --git a/internal/structerr/error.go b/internal/structerr/error.go index fdaf132df..752075661 100644 --- a/internal/structerr/error.go +++ b/internal/structerr/error.go @@ -81,10 +81,17 @@ func newError(code codes.Code, format string, a ...any) Error { formattedErr := fmt.Errorf(format, a...) // When we wrap an Error, we retain its error code. The intent of this is to retain the most - // specific error code we have in the general case. + // specific error code we have in the general case. As `Unknown` does not really count as a + // specific error code, we will ignore these errors. + // + // Note that this impacts our middleware status handler, where we wrap non-context-errors + // via `structerr.NewInternal()`. The result is that the caller should never see any + // `Unknown` errors. var wrappedErr Error if errors.As(formattedErr, &wrappedErr) { - code = wrappedErr.code + if wrappedErr.code != codes.Unknown { + code = wrappedErr.code + } } return Error{ @@ -93,11 +100,12 @@ func newError(code codes.Code, format string, a ...any) Error { } } -// New returns a new Error with the default error code, which is Internal. When this function is -// used to wrap another Error, then the error code of that wrapped Error will be retained. The -// intent of this is to always retain the most specific error code in the general case. +// New returns a new Error with an Unknown error code. This constructor should be used in the +// general case where it is not clear what the specific error category is. As Unknown errors get +// treated specially, they will be overridden when wrapped with an error that has a more specific +// error code. func New(format string, a ...any) Error { - return newError(codes.Internal, format, a...) + return newError(codes.Unknown, format, a...) } // NewAborted constructs a new error code with the Aborted error code. Please refer to New for @@ -184,12 +192,6 @@ func NewUnimplemented(format string, a ...any) Error { return newError(codes.Unimplemented, format, a...) } -// NewUnknown constructs a new error code with the Unknown error code. Please refer to New for -// further details. -func NewUnknown(format string, a ...any) Error { - return newError(codes.Unknown, format, a...) -} - // Error returns the error message of the Error. func (e Error) Error() string { return e.err.Error() diff --git a/internal/structerr/error_test.go b/internal/structerr/error_test.go index 5b87cd98c..0cd2638db 100644 --- a/internal/structerr/error_test.go +++ b/internal/structerr/error_test.go @@ -30,7 +30,7 @@ func TestNew(t *testing.T) { { desc: "New", constructor: New, - expectedCode: codes.Internal, + expectedCode: codes.Unknown, }, { desc: "NewAborted", @@ -102,11 +102,6 @@ func TestNew(t *testing.T) { constructor: NewUnimplemented, expectedCode: codes.Unimplemented, }, - { - desc: "NewUnknown", - constructor: NewUnknown, - expectedCode: codes.Unknown, - }, } { t.Run(tc.desc, func(t *testing.T) { require.NotEqual(t, tc.expectedCode, unusedErrorCode) @@ -153,6 +148,18 @@ func TestNew(t *testing.T) { require.Equal(t, status.New(unusedErrorCode, "top-level: nested"), s) }) + t.Run("wrapping structerr with Unknown error", func(t *testing.T) { + err := tc.constructor("top-level: %w", newError(codes.Unknown, "unknown")) + require.EqualError(t, err, "top-level: unknown") + // We should be overriding the Unknown error code with the error + // code of the top-level error. + 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: unknown"), s) + }) + t.Run("wrapping status.Error", func(t *testing.T) { err := tc.constructor("top-level: %w", status.Error(unusedErrorCode, "nested")) require.EqualError(t, err, "top-level: nested") @@ -323,7 +330,7 @@ func TestError_Metadata(t *testing.T) { err := New("message") require.Equal(t, Error{ err: errors.New("message"), - code: codes.Internal, + code: codes.Unknown, }, err) require.Equal(t, map[string]any{}, err.Metadata()) }) @@ -332,7 +339,7 @@ func TestError_Metadata(t *testing.T) { err := New("message").WithMetadata("key", "value") require.Equal(t, Error{ err: errors.New("message"), - code: codes.Internal, + code: codes.Unknown, metadata: []metadataItem{ {key: "key", value: "value"}, }, @@ -346,7 +353,7 @@ func TestError_Metadata(t *testing.T) { err := New("message").WithMetadata("first", 1).WithMetadata("second", 2) require.Equal(t, Error{ err: errors.New("message"), - code: codes.Internal, + code: codes.Unknown, metadata: []metadataItem{ {key: "first", value: 1}, {key: "second", value: 2}, @@ -362,7 +369,7 @@ func TestError_Metadata(t *testing.T) { err := New("message").WithMetadata("first", "initial").WithMetadata("first", "overridden") require.Equal(t, Error{ err: errors.New("message"), - code: codes.Internal, + code: codes.Unknown, metadata: []metadataItem{ {key: "first", value: "overridden"}, }, @@ -377,7 +384,7 @@ func TestError_Metadata(t *testing.T) { toplevelErr := New("top-level: %w", nestedErr).WithMetadata("toplevel", "value") require.Equal(t, Error{ err: fmt.Errorf("top-level: %w", nestedErr), - code: codes.Internal, + code: codes.Unknown, metadata: []metadataItem{ {key: "toplevel", value: "value"}, }, @@ -393,7 +400,7 @@ func TestError_Metadata(t *testing.T) { toplevelErr := New("top-level: %w", nestedErr).WithMetadata("key", "top-level") require.Equal(t, Error{ err: fmt.Errorf("top-level: %w", nestedErr), - code: codes.Internal, + code: codes.Unknown, metadata: []metadataItem{ {key: "key", value: "top-level"}, }, @@ -408,7 +415,7 @@ func TestError_Metadata(t *testing.T) { toplevelErr := New("top-level: %w", nestedErr).WithMetadata("toplevel", "initial").WithMetadata("toplevel", "overridden") require.Equal(t, Error{ err: fmt.Errorf("top-level: %w", nestedErr), - code: codes.Internal, + code: codes.Unknown, metadata: []metadataItem{ {key: "toplevel", value: "overridden"}, }, @@ -426,7 +433,7 @@ func TestError_Metadata(t *testing.T) { require.Equal(t, Error{ err: fmt.Errorf("top: %w", midlevelErr), - code: codes.Internal, + code: codes.Unknown, metadata: []metadataItem{ {key: "toplevel", value: "value"}, }, @@ -462,7 +469,7 @@ func TestError_Details(t *testing.T) { }, expectedErr: Error{ err: errors.New("message"), - code: codes.Internal, + code: codes.Unknown, }, expectedMessage: "message", }, @@ -473,7 +480,7 @@ func TestError_Details(t *testing.T) { }, expectedErr: Error{ err: errors.New("message"), - code: codes.Internal, + code: codes.Unknown, details: []proto.Message{ initialPayload, }, @@ -490,7 +497,7 @@ func TestError_Details(t *testing.T) { }, expectedErr: Error{ err: errors.New("message"), - code: codes.Internal, + code: codes.Unknown, details: []proto.Message{ initialPayload, overridingPayload, @@ -510,7 +517,7 @@ func TestError_Details(t *testing.T) { }, expectedErr: Error{ err: fmt.Errorf("top-level: %w", New("nested").WithDetail(initialPayload)), - code: codes.Internal, + code: codes.Unknown, details: []proto.Message{ overridingPayload, }, @@ -536,7 +543,7 @@ func TestError_Details(t *testing.T) { s, ok := status.FromError(err) require.True(t, ok) - require.Equal(t, codes.Internal, s.Code()) + require.Equal(t, codes.Unknown, s.Code()) require.Equal(t, tc.expectedMessage, s.Message()) testhelper.ProtoEqual(t, anyDetails, s.Details()) }) diff --git a/internal/structerr/grpc_server_test.go b/internal/structerr/grpc_server_test.go index b9c48d6b9..6d2bb39b3 100644 --- a/internal/structerr/grpc_server_test.go +++ b/internal/structerr/grpc_server_test.go @@ -139,12 +139,12 @@ func TestFieldsProducer(t *testing.T) { { desc: "structured error", returnedErr: New("message"), - expectedErr: status.Error(codes.Internal, "message"), + expectedErr: status.Error(codes.Unknown, "message"), }, { desc: "structured error with metadata", returnedErr: New("message").WithMetadata("key", "value"), - expectedErr: status.Error(codes.Internal, "message"), + expectedErr: status.Error(codes.Unknown, "message"), expectedMetadata: []map[string]any{ { "key": "value", @@ -154,7 +154,7 @@ func TestFieldsProducer(t *testing.T) { { desc: "structured error with nested metadata", returnedErr: New("message: %w", New("nested").WithMetadata("nested", "value")).WithMetadata("key", "value"), - expectedErr: status.Error(codes.Internal, "message: nested"), + expectedErr: status.Error(codes.Unknown, "message: nested"), expectedMetadata: []map[string]any{ { "key": "value", |