diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2022-08-26 10:51:51 +0300 |
---|---|---|
committer | Pavlo Strokov <pstrokov@gitlab.com> | 2022-09-05 10:34:18 +0300 |
commit | a79eb7dfa8880e6c8d0c09e0fa7d793a3f531f31 (patch) | |
tree | ccfb222135e445d5c4e2e8254666a7ca5f3dcd62 | |
parent | ea920a303a90a5cb9d0f2aaea64ed782a9dd800d (diff) |
blob: Standardize returned errors
Replace different representation of the missing repository
argument error with the pre-defined ErrEmptyRepository.
Fix order of arguments validation with localrepo creation in GetBlob.
Replace status.Error(f) with helper.ErrXXX to have a common pattern
and omit possible double-wrapping of the gRPC Status.
-rw-r--r-- | internal/gitaly/service/blob/blobs.go | 5 | ||||
-rw-r--r-- | internal/gitaly/service/blob/get_blob.go | 11 | ||||
-rw-r--r-- | internal/gitaly/service/blob/get_blobs.go | 38 | ||||
-rw-r--r-- | internal/gitaly/service/blob/lfs_pointers.go | 12 |
4 files changed, 36 insertions, 30 deletions
diff --git a/internal/gitaly/service/blob/blobs.go b/internal/gitaly/service/blob/blobs.go index f8b72177f..73cef0866 100644 --- a/internal/gitaly/service/blob/blobs.go +++ b/internal/gitaly/service/blob/blobs.go @@ -7,6 +7,7 @@ import ( "io" "strings" + gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors" "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gitpipe" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" @@ -19,7 +20,7 @@ import ( func verifyListBlobsRequest(req *gitalypb.ListBlobsRequest) error { if req.GetRepository() == nil { - return errors.New("empty repository") + return gitalyerrors.ErrEmptyRepository } if len(req.GetRevisions()) == 0 { return errors.New("missing revisions") @@ -237,7 +238,7 @@ func (s *server) ListAllBlobs(req *gitalypb.ListAllBlobsRequest, stream gitalypb ctx := stream.Context() if req.GetRepository() == nil { - return helper.ErrInvalidArgumentf("empty repository") + return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository) } repo := s.localrepo(req.GetRepository()) diff --git a/internal/gitaly/service/blob/get_blob.go b/internal/gitaly/service/blob/get_blob.go index b3f593e68..8b4046fe0 100644 --- a/internal/gitaly/service/blob/get_blob.go +++ b/internal/gitaly/service/blob/get_blob.go @@ -4,6 +4,7 @@ import ( "fmt" "io" + gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v15/internal/helper" @@ -14,12 +15,12 @@ import ( func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobService_GetBlobServer) error { ctx := stream.Context() - repo := s.localrepo(in.GetRepository()) - if err := validateRequest(in); err != nil { - return helper.ErrInvalidArgumentf("GetBlob: %v", err) + return helper.ErrInvalidArgument(err) } + repo := s.localrepo(in.GetRepository()) + objectReader, cancel, err := s.catfileCache.ObjectReader(stream.Context(), repo) if err != nil { return helper.ErrInternalf("GetBlob: %v", err) @@ -70,6 +71,10 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic } func validateRequest(in *gitalypb.GetBlobRequest) error { + if in.GetRepository() == nil { + return gitalyerrors.ErrEmptyRepository + } + if len(in.GetOid()) == 0 { return fmt.Errorf("empty Oid") } diff --git a/internal/gitaly/service/blob/get_blobs.go b/internal/gitaly/service/blob/get_blobs.go index 18694930f..0417d504e 100644 --- a/internal/gitaly/service/blob/get_blobs.go +++ b/internal/gitaly/service/blob/get_blobs.go @@ -2,14 +2,16 @@ package blob import ( "bytes" + "errors" + "fmt" "io" + gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v15/streamio" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) var treeEntryToObjectType = map[gitalypb.TreeEntry_EntryType]gitalypb.ObjectType{ @@ -38,14 +40,14 @@ func sendGetBlobsResponse( treeEntry, err := tef.FindByRevisionAndPath(ctx, revision, string(path)) if err != nil { - return err + return helper.ErrInternal(err) } response := &gitalypb.GetBlobsResponse{Revision: revision, Path: path} if treeEntry == nil || len(treeEntry.Oid) == 0 { if err := stream.Send(response); err != nil { - return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err) + return helper.ErrUnavailablef("GetBlobs: send: %v", err) } continue @@ -59,7 +61,7 @@ func sendGetBlobsResponse( response.Type = gitalypb.ObjectType_COMMIT if err := stream.Send(response); err != nil { - return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err) + return helper.ErrUnavailablef("GetBlobs: send: %v", err) } continue @@ -67,7 +69,7 @@ func sendGetBlobsResponse( objectInfo, err := objectInfoReader.Info(ctx, git.Revision(treeEntry.Oid)) if err != nil { - return status.Errorf(codes.Internal, "GetBlobs: %v", err) + return helper.ErrInternalf("GetBlobs: %v", err) } response.Size = objectInfo.Size @@ -81,7 +83,7 @@ func sendGetBlobsResponse( if response.Type != gitalypb.ObjectType_BLOB { if err := stream.Send(response); err != nil { - return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err) + return helper.ErrUnavailablef("GetBlobs: send: %v", err) } continue } @@ -114,22 +116,22 @@ func sendBlobTreeEntry( // blobObj. if readLimit == 0 { if err := stream.Send(response); err != nil { - return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err) + return helper.ErrUnavailablef("GetBlobs: send: %v", err) } return nil } blobObj, err := objectReader.Object(ctx, git.Revision(response.Oid)) if err != nil { - return status.Errorf(codes.Internal, "GetBlobs: %v", err) + return helper.ErrInternalf("GetBlobs: %v", err) } defer func() { if _, err := io.Copy(io.Discard, blobObj); err != nil && returnedErr == nil { - returnedErr = status.Errorf(codes.Internal, "GetBlobs: discarding data: %v", err) + returnedErr = helper.ErrInternalf("GetBlobs: discarding data: %v", err) } }() if blobObj.Type != "blob" { - return status.Errorf(codes.Internal, "blob got unexpected type %q", blobObj.Type) + return helper.ErrInternalf("blob got unexpected type %q", blobObj.Type) } sw := streamio.NewWriter(func(p []byte) error { @@ -146,7 +148,7 @@ func sendBlobTreeEntry( _, err = io.CopyN(sw, blobObj, readLimit) if err != nil { - return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err) + return helper.ErrUnavailablef("GetBlobs: send: %v", err) } return nil @@ -154,20 +156,20 @@ func sendBlobTreeEntry( func (s *server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobService_GetBlobsServer) error { if err := validateGetBlobsRequest(req); err != nil { - return err + return helper.ErrInvalidArgument(err) } repo := s.localrepo(req.GetRepository()) objectReader, cancel, err := s.catfileCache.ObjectReader(stream.Context(), repo) if err != nil { - return err + return helper.ErrInternal(err) } defer cancel() objectInfoReader, cancel, err := s.catfileCache.ObjectInfoReader(stream.Context(), repo) if err != nil { - return err + return helper.ErrInternal(err) } defer cancel() @@ -176,16 +178,16 @@ func (s *server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobSer func validateGetBlobsRequest(req *gitalypb.GetBlobsRequest) error { if req.Repository == nil { - return status.Errorf(codes.InvalidArgument, "GetBlobs: empty Repository") + return gitalyerrors.ErrEmptyRepository } if len(req.RevisionPaths) == 0 { - return status.Errorf(codes.InvalidArgument, "GetBlobs: empty RevisionPaths") + return errors.New("GetBlobs: empty RevisionPaths") } for _, rp := range req.RevisionPaths { if err := git.ValidateRevision([]byte(rp.Revision)); err != nil { - return status.Errorf(codes.InvalidArgument, "GetBlobs: %v", err) + return fmt.Errorf("GetBlobs: %v", err) } } diff --git a/internal/gitaly/service/blob/lfs_pointers.go b/internal/gitaly/service/blob/lfs_pointers.go index 9c66782fb..28c231fa0 100644 --- a/internal/gitaly/service/blob/lfs_pointers.go +++ b/internal/gitaly/service/blob/lfs_pointers.go @@ -13,8 +13,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -31,14 +29,14 @@ func (s *server) ListLFSPointers(in *gitalypb.ListLFSPointersRequest, stream git ctx := stream.Context() if in.GetRepository() == nil { - return status.Error(codes.InvalidArgument, "empty repository") + return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository) } if len(in.Revisions) == 0 { - return status.Error(codes.InvalidArgument, "missing revisions") + return helper.ErrInvalidArgumentf("missing revisions") } for _, revision := range in.Revisions { if strings.HasPrefix(revision, "-") && revision != "--all" && revision != "--not" { - return status.Errorf(codes.InvalidArgument, "invalid revision: %q", revision) + return helper.ErrInvalidArgumentf("invalid revision: %q", revision) } } @@ -82,7 +80,7 @@ func (s *server) ListAllLFSPointers(in *gitalypb.ListAllLFSPointersRequest, stre ctx := stream.Context() if in.GetRepository() == nil { - return status.Error(codes.InvalidArgument, "empty repository") + return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository) } repo := s.localrepo(in.GetRepository()) @@ -126,7 +124,7 @@ func (s *server) GetLFSPointers(req *gitalypb.GetLFSPointersRequest, stream gita ctx := stream.Context() if err := validateGetLFSPointersRequest(req); err != nil { - return status.Errorf(codes.InvalidArgument, "GetLFSPointers: %v", err) + return helper.ErrInvalidArgument(err) } repo := s.localrepo(req.GetRepository()) |