diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-06-27 12:40:50 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-06-28 08:18:46 +0300 |
commit | b1919c1ec2ef38c62ee6563e4eb6c0a93e8eddb7 (patch) | |
tree | 274f9ad32cd51807af03c272d4f617c3530455da | |
parent | 13c9342533169e26f40af638e76d9d5b1782bcd8 (diff) |
gitaly/service: Drop error prefixes when validating requests
Some of our RPCs provide error prefixes when validating the incoming
request. This is not best practice, and most of our RPCs will return
validation errors directly.
Remove the error prefixes. This is a preparatory step to unify Gitaly
and Praefect errors.
16 files changed, 33 insertions, 34 deletions
diff --git a/internal/gitaly/service/commit/commit_signatures.go b/internal/gitaly/service/commit/commit_signatures.go index f054497e3..a4a83b560 100644 --- a/internal/gitaly/service/commit/commit_signatures.go +++ b/internal/gitaly/service/commit/commit_signatures.go @@ -19,7 +19,7 @@ var gpgSiganturePrefix = []byte("gpgsig") func (s *server) GetCommitSignatures(request *gitalypb.GetCommitSignaturesRequest, stream gitalypb.CommitService_GetCommitSignaturesServer) error { if err := validateGetCommitSignaturesRequest(s.locator, request); err != nil { - return structerr.NewInvalidArgument("GetCommitSignatures: %w", err) + return structerr.NewInvalidArgument("%w", err) } return s.getCommitSignatures(request, stream) diff --git a/internal/gitaly/service/commit/commit_signatures_test.go b/internal/gitaly/service/commit/commit_signatures_test.go index 1d858b502..3ddafa13e 100644 --- a/internal/gitaly/service/commit/commit_signatures_test.go +++ b/internal/gitaly/service/commit/commit_signatures_test.go @@ -107,7 +107,7 @@ func TestFailedGetCommitSignaturesRequest(t *testing.T) { CommitIds: []string{"5937ac0a7beb003549fc5fd26fc247adbce4a52e"}, }, expectedErr: testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("GetCommitSignatures: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), }, @@ -117,7 +117,7 @@ func TestFailedGetCommitSignaturesRequest(t *testing.T) { Repository: repo, CommitIds: []string{}, }, - expectedErr: status.Error(codes.InvalidArgument, "GetCommitSignatures: empty CommitIds"), + expectedErr: status.Error(codes.InvalidArgument, "empty CommitIds"), }, { desc: "commitIDS with shorthand sha", @@ -125,7 +125,7 @@ func TestFailedGetCommitSignaturesRequest(t *testing.T) { Repository: repo, CommitIds: []string{"5937ac0a7beb003549fc5fd26fc247adbce4a52e", "a17a9f6"}, }, - expectedErr: status.Error(codes.InvalidArgument, fmt.Sprintf(`GetCommitSignatures: invalid object ID: "a17a9f6", expected length %v, got 7`, gittest.DefaultObjectHash.EncodedLen())), + expectedErr: status.Error(codes.InvalidArgument, fmt.Sprintf(`invalid object ID: "a17a9f6", expected length %v, got 7`, gittest.DefaultObjectHash.EncodedLen())), }, } diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index e4108f4cb..6e6cc1e0e 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -119,7 +119,7 @@ func validateUserCreateTag(locator storage.Locator, req *gitalypb.UserCreateTagR //nolint:revive // This is unintentionally missing documentation. func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) { if err := validateUserCreateTag(s.locator, req); err != nil { - return nil, structerr.NewInvalidArgument("validating request: %w", err) + return nil, structerr.NewInvalidArgument("%w", err) } targetRevision := git.Revision(req.TargetRevision) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 4eb891c4b..85da22421 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -656,7 +656,7 @@ func TestUserCreateTag_message(t *testing.T) { { desc: "error: contains null byte", message: "\000", - expectedErr: structerr.NewInvalidArgument("validating request: tag message contains NUL byte"), + expectedErr: structerr.NewInvalidArgument("tag message contains NUL byte"), }, { desc: "annotated: some control characters", @@ -1321,7 +1321,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { targetRevision: "main", user: gittest.TestUser, expectedErr: testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("validating request: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), }, @@ -1331,7 +1331,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { tagName: "shiny-new-tag", targetRevision: "", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: empty target revision"), + expectedErr: structerr.NewInvalidArgument("empty target revision"), }, { desc: "empty user", @@ -1339,7 +1339,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { tagName: "shiny-new-tag", targetRevision: "main", user: nil, - expectedErr: structerr.NewInvalidArgument("validating request: empty user"), + expectedErr: structerr.NewInvalidArgument("empty user"), }, { desc: "empty starting point", @@ -1347,7 +1347,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { tagName: "new-tag", targetRevision: "", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: empty target revision"), + expectedErr: structerr.NewInvalidArgument("empty target revision"), }, { desc: "non-existing starting point", @@ -1363,7 +1363,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { tagName: "a tag", targetRevision: "main", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: invalid tag name: revision can't contain whitespace"), + expectedErr: structerr.NewInvalidArgument("invalid tag name: revision can't contain whitespace"), }, { desc: "space in annotated tag name", @@ -1372,7 +1372,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { targetRevision: "main", message: "a message", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: invalid tag name: revision can't contain whitespace"), + expectedErr: structerr.NewInvalidArgument("invalid tag name: revision can't contain whitespace"), }, { desc: "newline in lightweight tag name", @@ -1380,7 +1380,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { tagName: "a\ntag", targetRevision: "main", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: invalid tag name: revision can't contain whitespace"), + expectedErr: structerr.NewInvalidArgument("invalid tag name: revision can't contain whitespace"), }, { desc: "newline in annotated tag name", @@ -1389,7 +1389,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { targetRevision: "main", message: "a message", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: invalid tag name: revision can't contain whitespace"), + expectedErr: structerr.NewInvalidArgument("invalid tag name: revision can't contain whitespace"), }, { desc: "injection in lightweight tag name", @@ -1397,7 +1397,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { tagName: injectedTag, targetRevision: "main", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: invalid tag name: revision can't contain whitespace"), + expectedErr: structerr.NewInvalidArgument("invalid tag name: revision can't contain whitespace"), }, { desc: "injection in annotated tag name", @@ -1406,7 +1406,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { targetRevision: "main", message: "a message", user: gittest.TestUser, - expectedErr: structerr.NewInvalidArgument("validating request: invalid tag name: revision can't contain whitespace"), + expectedErr: structerr.NewInvalidArgument("invalid tag name: revision can't contain whitespace"), }, } { t.Run(tc.desc, func(t *testing.T) { diff --git a/internal/gitaly/service/repository/backup_repository.go b/internal/gitaly/service/repository/backup_repository.go index 8e9eea55f..c3b927125 100644 --- a/internal/gitaly/service/repository/backup_repository.go +++ b/internal/gitaly/service/repository/backup_repository.go @@ -16,7 +16,7 @@ func (s *server) BackupRepository(ctx context.Context, in *gitalypb.BackupReposi return nil, structerr.NewFailedPrecondition("backup repository: server-side backups are not configured") } if err := s.validateBackupRepositoryRequest(in); err != nil { - return nil, structerr.NewInvalidArgument("backup repository: %w", err) + return nil, structerr.NewInvalidArgument("%w", err) } manager := backup.NewManagerLocal( diff --git a/internal/gitaly/service/repository/backup_repository_test.go b/internal/gitaly/service/repository/backup_repository_test.go index 82d76ce91..441df4a34 100644 --- a/internal/gitaly/service/repository/backup_repository_test.go +++ b/internal/gitaly/service/repository/backup_repository_test.go @@ -70,7 +70,7 @@ func TestServerBackupRepository(t *testing.T) { backupID: "", } }, - expectedErr: structerr.NewInvalidArgument("backup repository: empty BackupId"), + expectedErr: structerr.NewInvalidArgument("empty BackupId"), }, { desc: "missing repository", @@ -88,7 +88,7 @@ func TestServerBackupRepository(t *testing.T) { } }, expectedErr: structerr.NewInvalidArgument(testhelper.GitalyOrPraefect( - "backup repository: repository not set", + "repository not set", "repo scoped: repository not set", )), }, diff --git a/internal/gitaly/service/repository/create_bundle.go b/internal/gitaly/service/repository/create_bundle.go index 9faa2d7f7..1e2733504 100644 --- a/internal/gitaly/service/repository/create_bundle.go +++ b/internal/gitaly/service/repository/create_bundle.go @@ -12,7 +12,7 @@ func (s *server) CreateBundle(req *gitalypb.CreateBundleRequest, stream gitalypb repository := req.GetRepository() if err := s.locator.ValidateRepository(repository); err != nil { - return structerr.NewInvalidArgument("CreateBundle: %w", err) + return structerr.NewInvalidArgument("%w", err) } repo := s.localrepo(repository) diff --git a/internal/gitaly/service/repository/create_bundle_test.go b/internal/gitaly/service/repository/create_bundle_test.go index c34dc8e4a..d798694e7 100644 --- a/internal/gitaly/service/repository/create_bundle_test.go +++ b/internal/gitaly/service/repository/create_bundle_test.go @@ -77,7 +77,7 @@ func TestFailedCreateBundleRequestDueToValidations(t *testing.T) { desc: "empty repository", request: &gitalypb.CreateBundleRequest{}, expectedErr: testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("CreateBundle: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), }, diff --git a/internal/gitaly/service/repository/create_repository_from_bundle.go b/internal/gitaly/service/repository/create_repository_from_bundle.go index c079a3f71..4e97ccb92 100644 --- a/internal/gitaly/service/repository/create_repository_from_bundle.go +++ b/internal/gitaly/service/repository/create_repository_from_bundle.go @@ -20,7 +20,7 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr repo := firstRequest.GetRepository() if err := s.locator.ValidateRepository(repo, storage.WithSkipRepositoryExistenceCheck()); err != nil { - return structerr.NewInvalidArgument("CreateRepositoryFromBundle: %w", err) + return structerr.NewInvalidArgument("%w", err) } ctx := stream.Context() diff --git a/internal/gitaly/service/repository/create_repository_from_bundle_test.go b/internal/gitaly/service/repository/create_repository_from_bundle_test.go index 69edaedbd..8403df23d 100644 --- a/internal/gitaly/service/repository/create_repository_from_bundle_test.go +++ b/internal/gitaly/service/repository/create_repository_from_bundle_test.go @@ -255,7 +255,7 @@ func TestCreateRepositoryFromBundle_invalidArgument(t *testing.T) { _, err = stream.CloseAndRecv() testhelper.RequireGrpcError(t, testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("CreateRepositoryFromBundle: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), err) } diff --git a/internal/gitaly/service/repository/create_repository_from_url.go b/internal/gitaly/service/repository/create_repository_from_url.go index 9479b42fa..4ed9daed5 100644 --- a/internal/gitaly/service/repository/create_repository_from_url.go +++ b/internal/gitaly/service/repository/create_repository_from_url.go @@ -78,7 +78,7 @@ func (s *server) cloneFromURLCommand( func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.CreateRepositoryFromURLRequest) (*gitalypb.CreateRepositoryFromURLResponse, error) { if err := validateCreateRepositoryFromURLRequest(s.locator, req); err != nil { - return nil, structerr.NewInvalidArgument("CreateRepositoryFromURL: %w", err) + return nil, structerr.NewInvalidArgument("%w", err) } if err := repoutil.Create(ctx, s.locator, s.gitCmdFactory, s.txManager, req.GetRepository(), func(repo *gitalypb.Repository) error { diff --git a/internal/gitaly/service/repository/create_repository_from_url_test.go b/internal/gitaly/service/repository/create_repository_from_url_test.go index 4074f4214..4a732c4a0 100644 --- a/internal/gitaly/service/repository/create_repository_from_url_test.go +++ b/internal/gitaly/service/repository/create_repository_from_url_test.go @@ -21,7 +21,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) func TestCreateRepositoryFromURL_successful(t *testing.T) { @@ -339,14 +338,14 @@ func TestServer_CloneFromURLCommand_validate(t *testing.T) { desc: "no repository provided", req: &gitalypb.CreateRepositoryFromURLRequest{Repository: nil}, expectedErr: testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("CreateRepositoryFromURL: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), }, { desc: "no URL provided", req: &gitalypb.CreateRepositoryFromURLRequest{Repository: &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: "new"}, Url: ""}, - expectedErr: status.Error(codes.InvalidArgument, "CreateRepositoryFromURL: empty Url"), + expectedErr: structerr.NewInvalidArgument("empty Url"), }, } diff --git a/internal/gitaly/service/repository/get_custom_hooks.go b/internal/gitaly/service/repository/get_custom_hooks.go index a3ab82195..0e67f353c 100644 --- a/internal/gitaly/service/repository/get_custom_hooks.go +++ b/internal/gitaly/service/repository/get_custom_hooks.go @@ -14,7 +14,7 @@ func (s *server) GetCustomHooks(in *gitalypb.GetCustomHooksRequest, stream gital ctx := stream.Context() if err := s.locator.ValidateRepository(in.GetRepository()); err != nil { - return structerr.NewInvalidArgument("validating repository: %w", err) + return structerr.NewInvalidArgument("%w", err) } writer := streamio.NewWriter(func(p []byte) error { @@ -35,7 +35,7 @@ func (s *server) BackupCustomHooks(in *gitalypb.BackupCustomHooksRequest, stream ctx := stream.Context() if err := s.locator.ValidateRepository(in.GetRepository()); err != nil { - return structerr.NewInvalidArgument("validating repository: %w", err) + return structerr.NewInvalidArgument("%w", err) } writer := streamio.NewWriter(func(p []byte) error { diff --git a/internal/gitaly/service/repository/get_custom_hooks_test.go b/internal/gitaly/service/repository/get_custom_hooks_test.go index 7d60a6fc0..645ad9115 100644 --- a/internal/gitaly/service/repository/get_custom_hooks_test.go +++ b/internal/gitaly/service/repository/get_custom_hooks_test.go @@ -211,7 +211,7 @@ func TestGetCustomHooks_validate(t *testing.T) { desc: "repository not provided", req: &gitalypb.GetCustomHooksRequest{Repository: nil}, expectedErr: testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("validating repository: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), }, @@ -240,7 +240,7 @@ func TestBackupCustomHooks_validate(t *testing.T) { desc: "repository not provided", req: &gitalypb.BackupCustomHooksRequest{Repository: nil}, expectedErr: testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("validating repository: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), }, diff --git a/internal/gitaly/service/repository/set_custom_hooks.go b/internal/gitaly/service/repository/set_custom_hooks.go index 1d2d1eab9..517f73732 100644 --- a/internal/gitaly/service/repository/set_custom_hooks.go +++ b/internal/gitaly/service/repository/set_custom_hooks.go @@ -20,7 +20,7 @@ func (s *server) SetCustomHooks(stream gitalypb.RepositoryService_SetCustomHooks repo := firstRequest.GetRepository() if err := s.locator.ValidateRepository(repo); err != nil { - return structerr.NewInvalidArgument("validating repo: %w", err) + return structerr.NewInvalidArgument("%w", err) } reader := streamio.NewReader(func() ([]byte, error) { @@ -54,7 +54,7 @@ func (s *server) RestoreCustomHooks(stream gitalypb.RepositoryService_RestoreCus repo := firstRequest.GetRepository() if err := s.locator.ValidateRepository(repo); err != nil { - return structerr.NewInvalidArgument("validating repo: %w", err) + return structerr.NewInvalidArgument("%w", err) } reader := streamio.NewReader(func() ([]byte, error) { diff --git a/internal/gitaly/service/repository/set_custom_hooks_test.go b/internal/gitaly/service/repository/set_custom_hooks_test.go index bc48b9df3..f48ec9ca7 100644 --- a/internal/gitaly/service/repository/set_custom_hooks_test.go +++ b/internal/gitaly/service/repository/set_custom_hooks_test.go @@ -170,7 +170,7 @@ func TestSetCustomHooks_failedValidation(t *testing.T) { err := tc.streamSender(t, ctx, client) testhelper.RequireGrpcError(t, testhelper.GitalyOrPraefect( - structerr.NewInvalidArgument("validating repo: %w", storage.ErrRepositoryNotSet), + structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), structerr.NewInvalidArgument("repo scoped: %w", storage.ErrRepositoryNotSet), ), err) }) |