diff options
author | Christian Couder <chriscool@tuxfamily.org> | 2021-07-28 14:43:35 +0300 |
---|---|---|
committer | Christian Couder <chriscool@tuxfamily.org> | 2021-07-28 14:43:35 +0300 |
commit | 6c07bc7ab6c4732700f722e0dd876f51bb837f33 (patch) | |
tree | c6dc08ae400bb819db4fd072dc4f2b6c36c6f350 | |
parent | d513d220b183d83ae7219ec52f49aa3b4f7fc551 (diff) | |
parent | ab24848383b46c97e5cc138ceeb01751196a3ef0 (diff) |
Merge branch 'pks-helper-formatting-grpc-error-codes' into 'master'
helper: Improve gRPC error interfaces and retain error codes in formatting directives
See merge request gitlab-org/gitaly!3708
-rw-r--r-- | cmd/gitaly-git2go/conflicts/conflicts.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/blob/get_blob.go | 16 | ||||
-rw-r--r-- | internal/gitaly/service/commit/list_files.go | 7 | ||||
-rw-r--r-- | internal/gitaly/service/commit/tree_entry.go | 22 | ||||
-rw-r--r-- | internal/gitaly/service/conflicts/list_conflict_files.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/operations/merge.go | 11 | ||||
-rw-r--r-- | internal/gitaly/service/operations/rebase.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/repository/archive.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/repository/create_from_bundle.go | 3 | ||||
-rw-r--r-- | internal/gitaly/service/repository/rename.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/repository/search_files.go | 16 | ||||
-rw-r--r-- | internal/helper/error.go | 79 | ||||
-rw-r--r-- | internal/helper/error_test.go (renamed from internal/helper/errors_test.go) | 67 | ||||
-rw-r--r-- | internal/praefect/coordinator.go | 2 | ||||
-rw-r--r-- | internal/praefect/service/transaction/server.go | 5 |
15 files changed, 141 insertions, 105 deletions
diff --git a/cmd/gitaly-git2go/conflicts/conflicts.go b/cmd/gitaly-git2go/conflicts/conflicts.go index 625eb241d..c5a79f85e 100644 --- a/cmd/gitaly-git2go/conflicts/conflicts.go +++ b/cmd/gitaly-git2go/conflicts/conflicts.go @@ -55,7 +55,7 @@ func Merge(repo *git.Repository, conflict git.IndexConflict) (*git.MergeFileResu blob, err := repo.LookupBlob(entry.Id) if err != nil { - return nil, helper.ErrPreconditionFailedf("could not get conflicting blob: %w", err) + return nil, helper.ErrFailedPreconditionf("could not get conflicting blob: %w", err) } input.Path = entry.Path diff --git a/internal/gitaly/service/blob/get_blob.go b/internal/gitaly/service/blob/get_blob.go index 526a34b11..177a9756a 100644 --- a/internal/gitaly/service/blob/get_blob.go +++ b/internal/gitaly/service/blob/get_blob.go @@ -9,8 +9,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v14/streamio" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobService_GetBlobServer) error { @@ -19,20 +17,20 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic repo := s.localrepo(in.GetRepository()) if err := validateRequest(in); err != nil { - return status.Errorf(codes.InvalidArgument, "GetBlob: %v", err) + return helper.ErrInvalidArgumentf("GetBlob: %v", err) } c, err := s.catfileCache.BatchProcess(stream.Context(), repo) if err != nil { - return status.Errorf(codes.Internal, "GetBlob: %v", err) + return helper.ErrInternalf("GetBlob: %v", err) } objectInfo, err := c.Info(ctx, git.Revision(in.Oid)) if err != nil && !catfile.IsNotFound(err) { - return status.Errorf(codes.Internal, "GetBlob: %v", err) + return helper.ErrInternalf("GetBlob: %v", err) } if catfile.IsNotFound(err) || objectInfo.Type != "blob" { - return helper.DecorateError(codes.Unavailable, stream.Send(&gitalypb.GetBlobResponse{})) + return helper.ErrUnavailable(stream.Send(&gitalypb.GetBlobResponse{})) } readLimit := objectInfo.Size @@ -45,12 +43,12 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic } if readLimit == 0 { - return helper.DecorateError(codes.Unavailable, stream.Send(firstMessage)) + return helper.ErrUnavailable(stream.Send(firstMessage)) } blobObj, err := c.Blob(ctx, git.Revision(objectInfo.Oid)) if err != nil { - return status.Errorf(codes.Internal, "GetBlob: %v", err) + return helper.ErrInternalf("GetBlob: %v", err) } sw := streamio.NewWriter(func(p []byte) error { @@ -65,7 +63,7 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic _, err = io.CopyN(sw, blobObj.Reader, readLimit) if err != nil { - return status.Errorf(codes.Unavailable, "GetBlob: send: %v", err) + return helper.ErrUnavailablef("GetBlob: send: %v", err) } return nil diff --git a/internal/gitaly/service/commit/list_files.go b/internal/gitaly/service/commit/list_files.go index 0ab072789..ba7688f0d 100644 --- a/internal/gitaly/service/commit/list_files.go +++ b/internal/gitaly/service/commit/list_files.go @@ -1,7 +1,6 @@ package commit import ( - "fmt" "io" "github.com/golang/protobuf/proto" @@ -12,8 +11,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.CommitService_ListFilesServer) error { @@ -34,11 +31,11 @@ func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.Commit if len(revision) == 0 { defaultBranch, err := defaultBranchName(stream.Context(), repo) if err != nil { - return helper.DecorateError(codes.NotFound, fmt.Errorf("revision not found %q", revision)) + return helper.ErrNotFoundf("revision not found %q", revision) } if len(defaultBranch) == 0 { - return status.Errorf(codes.FailedPrecondition, "repository does not have a default branch") + return helper.ErrFailedPreconditionf("repository does not have a default branch") } revision = string(defaultBranch) diff --git a/internal/gitaly/service/commit/tree_entry.go b/internal/gitaly/service/commit/tree_entry.go index 6d0ab14f6..aea2f9e18 100644 --- a/internal/gitaly/service/commit/tree_entry.go +++ b/internal/gitaly/service/commit/tree_entry.go @@ -10,8 +10,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v14/streamio" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batch, revision, path string, limit, maxSize int64) error { @@ -23,7 +21,7 @@ func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batc } if treeEntry == nil || len(treeEntry.Oid) == 0 { - return status.Errorf(codes.NotFound, "not found: %s", path) + return helper.ErrNotFoundf("not found: %s", path) } if treeEntry.Type == gitalypb.TreeEntry_COMMIT { @@ -33,7 +31,7 @@ func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batc Oid: treeEntry.Oid, } if err := stream.Send(response); err != nil { - return status.Errorf(codes.Unavailable, "TreeEntry: send: %v", err) + return helper.ErrUnavailablef("TreeEntry: send: %v", err) } return nil @@ -51,17 +49,16 @@ func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batc Size: treeInfo.Size, Mode: treeEntry.Mode, } - return helper.DecorateError(codes.Unavailable, stream.Send(response)) + return helper.ErrUnavailable(stream.Send(response)) } objectInfo, err := c.Info(ctx, git.Revision(treeEntry.Oid)) if err != nil { - return status.Errorf(codes.Internal, "TreeEntry: %v", err) + return helper.ErrInternalf("TreeEntry: %v", err) } if strings.ToLower(treeEntry.Type.String()) != objectInfo.Type { - return status.Errorf( - codes.Internal, + return helper.ErrInternalf( "TreeEntry: mismatched object type: tree-oid=%s object-oid=%s entry-type=%s object-type=%s", treeEntry.Oid, objectInfo.Oid, treeEntry.Type.String(), objectInfo.Type, ) @@ -70,8 +67,7 @@ func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batc dataLength := objectInfo.Size if maxSize > 0 && dataLength > maxSize { - return status.Errorf( - codes.FailedPrecondition, + return helper.ErrFailedPreconditionf( "TreeEntry: object size (%d) is bigger than the maximum allowed size (%d)", dataLength, maxSize, ) @@ -88,7 +84,7 @@ func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batc Mode: treeEntry.Mode, } if dataLength == 0 { - return helper.DecorateError(codes.Unavailable, stream.Send(response)) + return helper.ErrUnavailable(stream.Send(response)) } blobObj, err := c.Blob(ctx, git.Revision(objectInfo.Oid)) @@ -100,7 +96,7 @@ func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batc response.Data = p if err := stream.Send(response); err != nil { - return status.Errorf(codes.Unavailable, "TreeEntry: send: %v", err) + return helper.ErrUnavailablef("TreeEntry: send: %v", err) } // Use a new response so we don't send other fields (Size, ...) over and over @@ -115,7 +111,7 @@ func sendTreeEntry(stream gitalypb.CommitService_TreeEntryServer, c catfile.Batc func (s *server) TreeEntry(in *gitalypb.TreeEntryRequest, stream gitalypb.CommitService_TreeEntryServer) error { if err := validateRequest(in); err != nil { - return status.Errorf(codes.InvalidArgument, "TreeEntry: %v", err) + return helper.ErrInvalidArgumentf("TreeEntry: %v", err) } repo := s.localrepo(in.GetRepository()) diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go index a64d38ff6..2fe1447dc 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -25,12 +25,12 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s ours, err := repo.ResolveRevision(ctx, git.Revision(request.OurCommitOid+"^{commit}")) if err != nil { - return helper.ErrPreconditionFailedf("could not lookup 'our' OID: %s", err) + return helper.ErrFailedPreconditionf("could not lookup 'our' OID: %s", err) } theirs, err := repo.ResolveRevision(ctx, git.Revision(request.TheirCommitOid+"^{commit}")) if err != nil { - return helper.ErrPreconditionFailedf("could not lookup 'their' OID: %s", err) + return helper.ErrFailedPreconditionf("could not lookup 'their' OID: %s", err) } repoPath, err := s.locator.GetPath(request.Repository) @@ -55,11 +55,11 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s for _, conflict := range conflicts.Conflicts { if !request.AllowTreeConflicts && (conflict.Their.Path == "" || conflict.Our.Path == "") { - return helper.ErrPreconditionFailedf("conflict side missing") + return helper.ErrFailedPreconditionf("conflict side missing") } if !utf8.Valid(conflict.Content) { - return helper.ErrPreconditionFailed(errors.New("unsupported encoding")) + return helper.ErrFailedPrecondition(errors.New("unsupported encoding")) } conflictFiles = append(conflictFiles, &gitalypb.ConflictFile{ diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index c56c76920..846c578e5 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -97,7 +97,7 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc return err } if !secondRequest.Apply { - return helper.ErrPreconditionFailedf("merge aborted by client") + return helper.ErrFailedPreconditionf("merge aborted by client") } if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, nil, referenceName, mergeOID, revision); err != nil { @@ -170,7 +170,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ return nil, err } if !ancestor { - return nil, helper.ErrPreconditionFailedf("not fast forward") + return nil, helper.ErrFailedPreconditionf("not fast forward") } if err := s.updateReferenceWithHooks(ctx, in.Repository, in.User, nil, referenceName, commitID, revision); err != nil { @@ -266,7 +266,7 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge var oldTargetOID git.ObjectID if targetRef, err := repo.GetReference(ctx, git.ReferenceName(request.TargetRef)); err == nil { if targetRef.IsSymbolic { - return nil, helper.ErrPreconditionFailedf("target reference is symbolic: %q", request.TargetRef) + return nil, helper.ErrFailedPreconditionf("target reference is symbolic: %q", request.TargetRef) } oid, err := git.NewObjectIDFromHex(targetRef.Target) @@ -304,7 +304,7 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge if errors.Is(err, git2go.ErrInvalidArgument) { return nil, helper.ErrInvalidArgument(err) } - return nil, helper.ErrPreconditionFailedf("Failed to create merge commit for source_sha %s and target_sha %s at %s", + return nil, helper.ErrFailedPreconditionf("Failed to create merge commit for source_sha %s and target_sha %s at %s", sourceOID, oid, string(request.TargetRef)) } @@ -316,8 +316,7 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge // ... and move branch from target ref to the merge commit. The Ruby // implementation doesn't invoke hooks, so we don't either. if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), mergeOID, oldTargetOID); err != nil { - //nolint:stylecheck - return nil, helper.ErrPreconditionFailed(fmt.Errorf("Could not update %s. Please refresh and try again", string(request.TargetRef))) + return nil, helper.ErrFailedPreconditionf("Could not update %s. Please refresh and try again", string(request.TargetRef)) } return &gitalypb.UserMergeToRefResponse{ diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go index a3fde32a8..af530206e 100644 --- a/internal/gitaly/service/operations/rebase.go +++ b/internal/gitaly/service/operations/rebase.go @@ -86,7 +86,7 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba } if !secondRequest.GetApply() { - return helper.ErrPreconditionFailedf("rebase aborted by client") + return helper.ErrFailedPreconditionf("rebase aborted by client") } if err := s.updateReferenceWithHooks( diff --git a/internal/gitaly/service/repository/archive.go b/internal/gitaly/service/repository/archive.go index 2f0197e72..51e1d4394 100644 --- a/internal/gitaly/service/repository/archive.go +++ b/internal/gitaly/service/repository/archive.go @@ -151,7 +151,7 @@ func (s *server) validateGetArchivePrecondition( if ok, err := findGetArchivePath(ctx, f, commitID, path); err != nil { return err } else if !ok { - return helper.ErrPreconditionFailedf("path doesn't exist") + return helper.ErrFailedPreconditionf("path doesn't exist") } } @@ -159,7 +159,7 @@ func (s *server) validateGetArchivePrecondition( if ok, err := findGetArchivePath(ctx, f, commitID, exclude); err != nil { return err } else if !ok { - return helper.ErrPreconditionFailedf("exclude[%d] doesn't exist", i) + return helper.ErrFailedPreconditionf("exclude[%d] doesn't exist", i) } } diff --git a/internal/gitaly/service/repository/create_from_bundle.go b/internal/gitaly/service/repository/create_from_bundle.go index 5fa6d7e62..e5005d711 100644 --- a/internal/gitaly/service/repository/create_from_bundle.go +++ b/internal/gitaly/service/repository/create_from_bundle.go @@ -2,7 +2,6 @@ package repository import ( "bytes" - "errors" "fmt" "io" "os" @@ -36,7 +35,7 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr } if !isDirEmpty(repoPath) { - return helper.ErrPreconditionFailed(errors.New("CreateRepositoryFromBundle: target directory is non-empty")) + return helper.ErrFailedPreconditionf("CreateRepositoryFromBundle: target directory is non-empty") } firstRead := false diff --git a/internal/gitaly/service/repository/rename.go b/internal/gitaly/service/repository/rename.go index f76507dd5..2557cbc2b 100644 --- a/internal/gitaly/service/repository/rename.go +++ b/internal/gitaly/service/repository/rename.go @@ -26,7 +26,7 @@ func (s *server) RenameRepository(ctx context.Context, in *gitalypb.RenameReposi } if _, err = os.Stat(toFullPath); !os.IsNotExist(err) { - return nil, helper.ErrPreconditionFailed(errors.New("destination already exists")) + return nil, helper.ErrFailedPreconditionf("destination already exists") } if err = os.MkdirAll(filepath.Dir(toFullPath), 0755); err != nil { diff --git a/internal/gitaly/service/repository/search_files.go b/internal/gitaly/service/repository/search_files.go index a95e7d9ca..2eb2d5b9e 100644 --- a/internal/gitaly/service/repository/search_files.go +++ b/internal/gitaly/service/repository/search_files.go @@ -14,8 +14,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/helper/lines" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v14/streamio" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) const ( @@ -30,12 +28,12 @@ var contentDelimiter = []byte("--\n") func (s *server) SearchFilesByContent(req *gitalypb.SearchFilesByContentRequest, stream gitalypb.RepositoryService_SearchFilesByContentServer) error { if err := validateSearchFilesRequest(req); err != nil { - return helper.DecorateError(codes.InvalidArgument, err) + return helper.ErrInvalidArgument(err) } repo := req.GetRepository() if repo == nil { - return status.Errorf(codes.InvalidArgument, "SearchFilesByContent: empty Repository") + return helper.ErrInvalidArgumentf("SearchFilesByContent: empty Repository") } ctx := stream.Context() @@ -51,11 +49,11 @@ func (s *server) SearchFilesByContent(req *gitalypb.SearchFilesByContentRequest, git.Flag{Name: "-e"}}, Args: []string{req.GetQuery(), string(req.GetRef())}}) if err != nil { - return status.Errorf(codes.Internal, "SearchFilesByContent: cmd start failed: %v", err) + return helper.ErrInternalf("SearchFilesByContent: cmd start failed: %v", err) } if err = sendSearchFilesResultChunked(cmd, stream); err != nil { - return status.Errorf(codes.Internal, "SearchFilesByContent: sending chunked response failed: %v", err) + return helper.ErrInternalf("SearchFilesByContent: sending chunked response failed: %v", err) } return nil @@ -105,7 +103,7 @@ func sendSearchFilesResultChunked(cmd *command.Command, stream gitalypb.Reposito func (s *server) SearchFilesByName(req *gitalypb.SearchFilesByNameRequest, stream gitalypb.RepositoryService_SearchFilesByNameServer) error { if err := validateSearchFilesRequest(req); err != nil { - return helper.DecorateError(codes.InvalidArgument, err) + return helper.ErrInvalidArgument(err) } var filter *regexp.Regexp @@ -122,7 +120,7 @@ func (s *server) SearchFilesByName(req *gitalypb.SearchFilesByNameRequest, strea repo := req.GetRepository() if repo == nil { - return status.Errorf(codes.InvalidArgument, "SearchFilesByName: empty Repository") + return helper.ErrInvalidArgumentf("SearchFilesByName: empty Repository") } ctx := stream.Context() @@ -134,7 +132,7 @@ func (s *server) SearchFilesByName(req *gitalypb.SearchFilesByNameRequest, strea git.Flag{Name: "--name-status"}, git.Flag{Name: "-r"}}, Args: []string{string(req.GetRef()), req.GetQuery()}}) if err != nil { - return status.Errorf(codes.Internal, "SearchFilesByName: cmd start failed: %v", err) + return helper.ErrInternalf("SearchFilesByName: cmd start failed: %v", err) } lr := func(objs [][]byte) error { diff --git a/internal/helper/error.go b/internal/helper/error.go index 53d6fe208..c42029266 100644 --- a/internal/helper/error.go +++ b/internal/helper/error.go @@ -1,6 +1,7 @@ package helper import ( + "errors" "fmt" "google.golang.org/grpc/codes" @@ -20,41 +21,77 @@ func (sw statusWrapper) Unwrap() error { return sw.error } -// DecorateError unless it's already a grpc error. -// If given nil it will return nil. -func DecorateError(code codes.Code, err error) error { - if err != nil && GrpcCode(err) == codes.Unknown { +// ErrCanceled wraps err with codes.Canceled, unless err is already a gRPC error. +func ErrCanceled(err error) error { return wrapError(codes.Canceled, err) } + +// ErrInternal wraps err with codes.Internal, unless err is already a gRPC error. +func ErrInternal(err error) error { return wrapError(codes.Internal, err) } + +// ErrInvalidArgument wraps err with codes.InvalidArgument, unless err is already a gRPC error. +func ErrInvalidArgument(err error) error { return wrapError(codes.InvalidArgument, err) } + +// ErrNotFound wraps error with codes.NotFound, unless err is already a gRPC error. +func ErrNotFound(err error) error { return wrapError(codes.NotFound, err) } + +// ErrFailedPrecondition wraps err with codes.FailedPrecondition, unless err is already a gRPC +// error. +func ErrFailedPrecondition(err error) error { return wrapError(codes.FailedPrecondition, err) } + +// ErrUnavailable wraps err with codes.Unavailable, unless err is already a gRPC error. +func ErrUnavailable(err error) error { return wrapError(codes.Unavailable, 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())} } return err } -// ErrInternal wraps err with codes.Internal, unless err is already a grpc error -func ErrInternal(err error) error { return DecorateError(codes.Internal, err) } - -// ErrInternalf wrapps a formatted error with codes.Internal, clobbering any existing grpc error +// ErrInternalf wraps a formatted error with codes.Internal, unless the formatted error is a +// wrapped gRPC error. func ErrInternalf(format string, a ...interface{}) error { - return ErrInternal(fmt.Errorf(format, a...)) + return formatError(codes.Internal, format, a...) } -// ErrInvalidArgument wraps err with codes.InvalidArgument, unless err is already a grpc error -func ErrInvalidArgument(err error) error { return DecorateError(codes.InvalidArgument, err) } - -// ErrInvalidArgumentf wraps a formatted error with codes.InvalidArgument, clobbering any existing grpc error +// ErrInvalidArgumentf wraps a formatted error with codes.InvalidArgument, unless the formatted +// error is a wrapped gRPC error. func ErrInvalidArgumentf(format string, a ...interface{}) error { - return ErrInvalidArgument(fmt.Errorf(format, a...)) + return formatError(codes.InvalidArgument, format, a...) } -// ErrPreconditionFailed wraps err with codes.FailedPrecondition, unless err is already a grpc error -func ErrPreconditionFailed(err error) error { return DecorateError(codes.FailedPrecondition, err) } +// ErrNotFoundf wraps a formatted error with codes.NotFound, unless the +// formatted error is a wrapped gRPC error. +func ErrNotFoundf(format string, a ...interface{}) error { + return formatError(codes.NotFound, format, a...) +} + +// ErrFailedPreconditionf wraps a formatted error with codes.FailedPrecondition, unless the +// formatted error is a wrapped gRPC error. +func ErrFailedPreconditionf(format string, a ...interface{}) error { + return formatError(codes.FailedPrecondition, format, a...) +} -// ErrPreconditionFailedf wraps a formatted error with codes.FailedPrecondition, clobbering any existing grpc error -func ErrPreconditionFailedf(format string, a ...interface{}) error { - return ErrPreconditionFailed(fmt.Errorf(format, a...)) +// ErrUnavailablef wraps a formatted error with codes.Unavailable, unless the +// formatted error is a wrapped gRPC error. +func ErrUnavailablef(format string, a ...interface{}) error { + return formatError(codes.Unavailable, format, a...) } -// ErrNotFound wraps error with codes.NotFound, unless err is already a grpc error -func ErrNotFound(err error) error { return DecorateError(codes.NotFound, 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...) + + nestedCode := GrpcCode(errors.Unwrap(err)) + if nestedCode != codes.OK && nestedCode != codes.Unknown { + code = nestedCode + } + + return statusWrapper{err, status.New(code, err.Error())} +} // GrpcCode emulates the old grpc.Code function: it translates errors into codes.Code values. func GrpcCode(err error) codes.Code { diff --git a/internal/helper/errors_test.go b/internal/helper/error_test.go index 7927d41f6..614b7b8b6 100644 --- a/internal/helper/errors_test.go +++ b/internal/helper/error_test.go @@ -23,6 +23,11 @@ func TestError(t *testing.T) { code codes.Code }{ { + desc: "Canceled", + errorf: ErrCanceled, + code: codes.Canceled, + }, + { desc: "Internal", errorf: ErrInternal, code: codes.Internal, @@ -33,8 +38,8 @@ func TestError(t *testing.T) { code: codes.InvalidArgument, }, { - desc: "PreconditionFailed", - errorf: ErrPreconditionFailed, + desc: "FailedPrecondition", + errorf: ErrFailedPrecondition, code: codes.FailedPrecondition, }, { @@ -42,6 +47,11 @@ func TestError(t *testing.T) { errorf: ErrNotFound, code: codes.NotFound, }, + { + desc: "Unavailable", + errorf: ErrUnavailable, + code: codes.Unavailable, + }, } { t.Run(tc.desc, func(t *testing.T) { // tc.code and our canary test code must not @@ -77,13 +87,6 @@ func TestErrorF_withWFormat(t *testing.T) { testErrorfFormat(t, "expected %w", "expected %s") } -// oldPreconditionFailedf shows ErrPreconditionFailedf looked like -// before 777a12cfd. We're testing the nature of a regression in that -// change. -func oldPreconditionFailedf(format string, a ...interface{}) error { - return DecorateError(codes.FailedPrecondition, fmt.Errorf(format, a...)) -} - func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { isFormatW := strings.Contains(errorFormat, "%w") errorMessage := "sentinel error" @@ -108,43 +111,53 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) { code: codes.InvalidArgument, }, { - desc: "PreconditionFailedf", - errorf: ErrPreconditionFailedf, + desc: "FailedPreconditionf", + errorf: ErrFailedPreconditionf, code: codes.FailedPrecondition, }, { - desc: "oldPreconditionFailedf", - errorf: oldPreconditionFailedf, - code: codes.FailedPrecondition, + desc: "NotFoundf", + errorf: ErrNotFoundf, + code: codes.NotFound, + }, + { + desc: "ErrUnavailablef", + errorf: ErrUnavailablef, + code: codes.Unavailable, }, } { t.Run(tc.desc, func(t *testing.T) { - // tc.code and our canary test code must not - // clash! - require.NotEqual(t, tc.code, inputGRPCCode) + 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. + // 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 re-throwing an error an existing GRPC - // error code will get clobbered, not the one - // corresponding to the function's name. This - // is unlike the non-f functions. + // 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)) - require.Equal(t, tc.code, status.Code(err)) + 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)) + // 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)) - require.NotEqual(t, inputGRPCCode, status.Code(errX)) // .Equal() for non-f - require.NotEqual(t, tc.code, status.Code(inputGRPC)) + 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)) }) } } diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go index 108f1e926..67e5461d7 100644 --- a/internal/praefect/coordinator.go +++ b/internal/praefect/coordinator.go @@ -32,7 +32,7 @@ import ( // ErrRepositoryReadOnly is returned when the repository is in read-only mode. This happens // if the primary does not have the latest changes. -var ErrRepositoryReadOnly = helper.ErrPreconditionFailedf("repository is in read-only mode") +var ErrRepositoryReadOnly = helper.ErrFailedPreconditionf("repository is in read-only mode") type transactionsCondition func(context.Context) bool diff --git a/internal/praefect/service/transaction/server.go b/internal/praefect/service/transaction/server.go index 086449532..5de3f3599 100644 --- a/internal/praefect/service/transaction/server.go +++ b/internal/praefect/service/transaction/server.go @@ -8,7 +8,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/transactions" "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" - "google.golang.org/grpc/codes" ) type Server struct { @@ -36,7 +35,7 @@ func (s *Server) VoteTransaction(ctx context.Context, in *gitalypb.VoteTransacti case errors.Is(err, transactions.ErrNotFound): return nil, helper.ErrNotFound(err) case errors.Is(err, transactions.ErrTransactionCanceled): - return nil, helper.DecorateError(codes.Canceled, err) + return nil, helper.ErrCanceled(err) case errors.Is(err, transactions.ErrTransactionStopped): return &gitalypb.VoteTransactionResponse{ State: gitalypb.VoteTransactionResponse_STOP, @@ -65,7 +64,7 @@ func (s *Server) StopTransaction(ctx context.Context, in *gitalypb.StopTransacti case errors.Is(err, transactions.ErrNotFound): return nil, helper.ErrNotFound(err) case errors.Is(err, transactions.ErrTransactionCanceled): - return nil, helper.DecorateError(codes.Canceled, err) + return nil, helper.ErrCanceled(err) case errors.Is(err, transactions.ErrTransactionStopped): return &gitalypb.StopTransactionResponse{}, nil default: |