Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitaly.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPavlo Strokov <pstrokov@gitlab.com>2022-10-20 14:58:32 +0300
committerPavlo Strokov <pstrokov@gitlab.com>2022-11-08 12:28:56 +0300
commit39f635b0fd6b0bbc018c2001c867fdded180d076 (patch)
treef9bf7b06420fcfb48eb3e5213c935d67d633c84f
parentac79e8163529b0b52b1691687c658c85c1b7b20f (diff)
service/commit: Improve validation of input
Gitaly should return the same error for all RPCs where the Repository input parameter is missing. The test coverage extended to cover changed code. Input validation code extracted into separate function where it makes sense. The error verification is done not only for the code, but for the message as well. It gives us confidence that a proper path is tested.
-rw-r--r--internal/gitaly/service/commit/check_objects_exist.go3
-rw-r--r--internal/gitaly/service/commit/check_objects_exist_test.go27
-rw-r--r--internal/gitaly/service/commit/commit_messages.go3
-rw-r--r--internal/gitaly/service/commit/commit_messages_test.go12
-rw-r--r--internal/gitaly/service/commit/commit_signatures.go7
-rw-r--r--internal/gitaly/service/commit/commit_signatures_test.go20
-rw-r--r--internal/gitaly/service/commit/commits_by_message.go5
-rw-r--r--internal/gitaly/service/commit/commits_by_message_test.go31
-rw-r--r--internal/gitaly/service/commit/count_commits.go5
-rw-r--r--internal/gitaly/service/commit/count_commits_test.go48
-rw-r--r--internal/gitaly/service/commit/count_diverging_commits.go3
-rw-r--r--internal/gitaly/service/commit/count_diverging_commits_test.go52
-rw-r--r--internal/gitaly/service/commit/filter_shas_with_signatures.go6
-rw-r--r--internal/gitaly/service/commit/filter_shas_with_signatures_test.go29
-rw-r--r--internal/gitaly/service/commit/find_all_commits.go9
-rw-r--r--internal/gitaly/service/commit/find_all_commits_test.go21
-rw-r--r--internal/gitaly/service/commit/find_commit.go17
-rw-r--r--internal/gitaly/service/commit/find_commit_test.go12
-rw-r--r--internal/gitaly/service/commit/find_commits.go13
-rw-r--r--internal/gitaly/service/commit/find_commits_test.go24
-rw-r--r--internal/gitaly/service/commit/isancestor.go23
-rw-r--r--internal/gitaly/service/commit/isancestor_test.go34
-rw-r--r--internal/gitaly/service/commit/languages.go13
-rw-r--r--internal/gitaly/service/commit/languages_test.go38
-rw-r--r--internal/gitaly/service/commit/last_commit_for_path.go6
-rw-r--r--internal/gitaly/service/commit/last_commit_for_path_test.go6
-rw-r--r--internal/gitaly/service/commit/list_all_commits.go4
-rw-r--r--internal/gitaly/service/commit/list_all_commits_test.go32
-rw-r--r--internal/gitaly/service/commit/list_commits.go3
-rw-r--r--internal/gitaly/service/commit/list_commits_by_oid.go5
-rw-r--r--internal/gitaly/service/commit/list_commits_by_oid_test.go29
-rw-r--r--internal/gitaly/service/commit/list_commits_by_ref_name.go4
-rw-r--r--internal/gitaly/service/commit/list_commits_by_ref_name_test.go29
-rw-r--r--internal/gitaly/service/commit/list_commits_test.go39
-rw-r--r--internal/gitaly/service/commit/list_files.go8
-rw-r--r--internal/gitaly/service/commit/list_files_test.go26
-rw-r--r--internal/gitaly/service/commit/list_last_commits_for_tree.go4
-rw-r--r--internal/gitaly/service/commit/list_last_commits_for_tree_test.go39
-rw-r--r--internal/gitaly/service/commit/raw_blame.go4
-rw-r--r--internal/gitaly/service/commit/raw_blame_test.go26
-rw-r--r--internal/gitaly/service/commit/stats.go13
-rw-r--r--internal/gitaly/service/commit/stats_test.go28
-rw-r--r--internal/gitaly/service/commit/testhelper_test.go7
-rw-r--r--internal/gitaly/service/commit/tree_entries.go4
-rw-r--r--internal/gitaly/service/commit/tree_entries_test.go6
-rw-r--r--internal/gitaly/service/commit/tree_entry.go4
-rw-r--r--internal/gitaly/service/commit/tree_entry_test.go6
47 files changed, 611 insertions, 176 deletions
diff --git a/internal/gitaly/service/commit/check_objects_exist.go b/internal/gitaly/service/commit/check_objects_exist.go
index 04b92be89..2a43ed0fc 100644
--- a/internal/gitaly/service/commit/check_objects_exist.go
+++ b/internal/gitaly/service/commit/check_objects_exist.go
@@ -4,6 +4,7 @@ import (
"context"
"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"
@@ -29,7 +30,7 @@ func (s *server) CheckObjectsExist(
}
if request.GetRepository() == nil {
- return helper.ErrInvalidArgumentf("empty Repository")
+ return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
}
objectInfoReader, cancel, err := s.catfileCache.ObjectInfoReader(
diff --git a/internal/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go
index a40264360..2f39d9ecd 100644
--- a/internal/gitaly/service/commit/check_objects_exist_test.go
+++ b/internal/gitaly/service/commit/check_objects_exist_test.go
@@ -12,6 +12,8 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestCheckObjectsExist(t *testing.T) {
@@ -39,12 +41,19 @@ func TestCheckObjectsExist(t *testing.T) {
expectedErr error
}{
{
+ desc: "no repository provided",
+ requests: []*gitalypb.CheckObjectsExistRequest{{Repository: nil}},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
desc: "no requests",
requests: []*gitalypb.CheckObjectsExistRequest{},
// Ideally, we'd return an invalid-argument error in case there aren't any
// requests. We can't do this though as this would diverge from Praefect's
// behaviour, which always returns `io.EOF`.
- expectedResults: map[string]bool{},
},
{
desc: "missing repository",
@@ -59,7 +68,6 @@ func TestCheckObjectsExist(t *testing.T) {
}
return helper.ErrInvalidArgumentf("empty Repository")
}(),
- expectedResults: map[string]bool{},
},
{
desc: "request without revisions",
@@ -68,7 +76,6 @@ func TestCheckObjectsExist(t *testing.T) {
Repository: repo,
},
},
- expectedResults: map[string]bool{},
},
{
desc: "commit ids and refs that exist",
@@ -158,8 +165,7 @@ func TestCheckObjectsExist(t *testing.T) {
},
},
},
- expectedErr: helper.ErrInvalidArgumentf("invalid revision %q: revision can't start with '-'", "-not-a-rev"),
- expectedResults: map[string]bool{},
+ expectedErr: helper.ErrInvalidArgumentf("invalid revision %q: revision can't start with '-'", "-not-a-rev"),
},
{
desc: "input with whitespace",
@@ -171,8 +177,7 @@ func TestCheckObjectsExist(t *testing.T) {
},
},
},
- expectedErr: helper.ErrInvalidArgumentf("invalid revision %q: revision can't contain whitespace", fmt.Sprintf("%s\n%s", commitID1, commitID2)),
- expectedResults: map[string]bool{},
+ expectedErr: helper.ErrInvalidArgumentf("invalid revision %q: revision can't contain whitespace", fmt.Sprintf("%s\n%s", commitID1, commitID2)),
},
{
desc: "chunked invalid input",
@@ -189,8 +194,7 @@ func TestCheckObjectsExist(t *testing.T) {
},
},
},
- expectedErr: helper.ErrInvalidArgumentf("invalid revision %q: revision can't start with '-'", "-not-a-rev"),
- expectedResults: map[string]bool{},
+ expectedErr: helper.ErrInvalidArgumentf("invalid revision %q: revision can't start with '-'", "-not-a-rev"),
},
} {
t.Run(tc.desc, func(t *testing.T) {
@@ -202,7 +206,7 @@ func TestCheckObjectsExist(t *testing.T) {
}
require.NoError(t, client.CloseSend())
- results := map[string]bool{}
+ var results map[string]bool
for {
var response *gitalypb.CheckObjectsExistResponse
response, err = client.Recv()
@@ -211,6 +215,9 @@ func TestCheckObjectsExist(t *testing.T) {
}
for _, revision := range response.GetRevisions() {
+ if results == nil {
+ results = map[string]bool{}
+ }
results[string(revision.GetName())] = revision.GetExists()
}
}
diff --git a/internal/gitaly/service/commit/commit_messages.go b/internal/gitaly/service/commit/commit_messages.go
index da82f92d6..3417f509a 100644
--- a/internal/gitaly/service/commit/commit_messages.go
+++ b/internal/gitaly/service/commit/commit_messages.go
@@ -5,6 +5,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"
@@ -56,7 +57,7 @@ func (s *server) getAndStreamCommitMessages(request *gitalypb.GetCommitMessagesR
func validateGetCommitMessagesRequest(request *gitalypb.GetCommitMessagesRequest) error {
if request.GetRepository() == nil {
- return fmt.Errorf("empty Repository")
+ return gitalyerrors.ErrEmptyRepository
}
return nil
diff --git a/internal/gitaly/service/commit/commit_messages_test.go b/internal/gitaly/service/commit/commit_messages_test.go
index 257a47892..c7731707b 100644
--- a/internal/gitaly/service/commit/commit_messages_test.go
+++ b/internal/gitaly/service/commit/commit_messages_test.go
@@ -13,6 +13,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestSuccessfulGetCommitMessagesRequest(t *testing.T) {
@@ -66,15 +67,18 @@ func TestFailedGetCommitMessagesRequest(t *testing.T) {
testCases := []struct {
desc string
request *gitalypb.GetCommitMessagesRequest
- code codes.Code
+ err error
}{
{
- desc: "empty Repository",
+ desc: "no repository provided",
request: &gitalypb.GetCommitMessagesRequest{
Repository: nil,
CommitIds: []string{"5937ac0a7beb003549fc5fd26fc247adbce4a52e"},
},
- code: codes.InvalidArgument,
+ err: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
},
}
@@ -90,7 +94,7 @@ func TestFailedGetCommitMessagesRequest(t *testing.T) {
}
}
- testhelper.RequireGrpcCode(t, err, testCase.code)
+ testhelper.RequireGrpcError(t, testCase.err, err)
})
}
}
diff --git a/internal/gitaly/service/commit/commit_signatures.go b/internal/gitaly/service/commit/commit_signatures.go
index 9045e428c..ffcb0adc8 100644
--- a/internal/gitaly/service/commit/commit_signatures.go
+++ b/internal/gitaly/service/commit/commit_signatures.go
@@ -7,20 +7,19 @@ 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"
"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 gpgSiganturePrefix = []byte("gpgsig")
func (s *server) GetCommitSignatures(request *gitalypb.GetCommitSignaturesRequest, stream gitalypb.CommitService_GetCommitSignaturesServer) error {
if err := validateGetCommitSignaturesRequest(request); err != nil {
- return status.Errorf(codes.InvalidArgument, "GetCommitSignatures: %v", err)
+ return helper.ErrInvalidArgumentf("GetCommitSignatures: %w", err)
}
return s.getCommitSignatures(request, stream)
@@ -129,7 +128,7 @@ func sendResponse(commitID string, signatureKey []byte, commitText []byte, strea
func validateGetCommitSignaturesRequest(request *gitalypb.GetCommitSignaturesRequest) error {
if request.GetRepository() == nil {
- return errors.New("empty Repository")
+ return gitalyerrors.ErrEmptyRepository
}
if len(request.GetCommitIds()) == 0 {
diff --git a/internal/gitaly/service/commit/commit_signatures_test.go b/internal/gitaly/service/commit/commit_signatures_test.go
index 7e9237329..f85a0b1bb 100644
--- a/internal/gitaly/service/commit/commit_signatures_test.go
+++ b/internal/gitaly/service/commit/commit_signatures_test.go
@@ -14,6 +14,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestSuccessfulGetCommitSignaturesRequest(t *testing.T) {
@@ -92,17 +93,20 @@ func TestFailedGetCommitSignaturesRequest(t *testing.T) {
_, repo, _, client := setupCommitServiceWithRepo(t, ctx)
testCases := []struct {
- desc string
- request *gitalypb.GetCommitSignaturesRequest
- code codes.Code
+ desc string
+ request *gitalypb.GetCommitSignaturesRequest
+ expectedErr error
}{
{
- desc: "empty Repository",
+ desc: "no repository provided",
request: &gitalypb.GetCommitSignaturesRequest{
Repository: nil,
CommitIds: []string{"5937ac0a7beb003549fc5fd26fc247adbce4a52e"},
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "GetCommitSignatures: empty Repository",
+ "repo scoped: empty Repository",
+ )),
},
{
desc: "empty CommitIds",
@@ -110,7 +114,7 @@ func TestFailedGetCommitSignaturesRequest(t *testing.T) {
Repository: repo,
CommitIds: []string{},
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "GetCommitSignatures: empty CommitIds"),
},
{
desc: "commitIDS with shorthand sha",
@@ -118,7 +122,7 @@ func TestFailedGetCommitSignaturesRequest(t *testing.T) {
Repository: repo,
CommitIds: []string{"5937ac0a7beb003549fc5fd26fc247adbce4a52e", "a17a9f6"},
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, `GetCommitSignatures: invalid object ID: "a17a9f6"`),
},
}
@@ -134,7 +138,7 @@ func TestFailedGetCommitSignaturesRequest(t *testing.T) {
}
}
- testhelper.RequireGrpcCode(t, err, testCase.code)
+ testhelper.RequireGrpcError(t, testCase.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/commits_by_message.go b/internal/gitaly/service/commit/commits_by_message.go
index 021141304..1eaeb52c7 100644
--- a/internal/gitaly/service/commit/commits_by_message.go
+++ b/internal/gitaly/service/commit/commits_by_message.go
@@ -3,6 +3,7 @@ package commit
import (
"fmt"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -69,6 +70,10 @@ func (s *server) commitsByMessage(in *gitalypb.CommitsByMessageRequest, stream g
}
func validateCommitsByMessageRequest(in *gitalypb.CommitsByMessageRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
+
if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
return err
}
diff --git a/internal/gitaly/service/commit/commits_by_message_test.go b/internal/gitaly/service/commit/commits_by_message_test.go
index a46720f81..1f91de9bb 100644
--- a/internal/gitaly/service/commit/commits_by_message_test.go
+++ b/internal/gitaly/service/commit/commits_by_message_test.go
@@ -9,6 +9,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/timestamppb"
)
@@ -166,29 +167,35 @@ func TestFailedCommitsByMessageRequest(t *testing.T) {
invalidRepo := &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}
testCases := []struct {
- desc string
- request *gitalypb.CommitsByMessageRequest
- code codes.Code
+ desc string
+ request *gitalypb.CommitsByMessageRequest
+ expectedErr error
}{
{
desc: "Invalid repository",
request: &gitalypb.CommitsByMessageRequest{Repository: invalidRepo, Query: "foo"},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `GetStorageByName: no such storage: "fake"`,
+ "repo scoped: invalid Repository",
+ )),
},
{
desc: "Repository is nil",
request: &gitalypb.CommitsByMessageRequest{Query: "foo"},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
},
{
- desc: "Query is missing",
- request: &gitalypb.CommitsByMessageRequest{Repository: repo},
- code: codes.InvalidArgument,
+ desc: "Query is missing",
+ request: &gitalypb.CommitsByMessageRequest{Repository: repo},
+ expectedErr: status.Error(codes.InvalidArgument, "empty Query"),
},
{
- desc: "Revision is invalid",
- request: &gitalypb.CommitsByMessageRequest{Repository: repo, Revision: []byte("--output=/meow"), Query: "not empty"},
- code: codes.InvalidArgument,
+ desc: "Revision is invalid",
+ request: &gitalypb.CommitsByMessageRequest{Repository: repo, Revision: []byte("--output=/meow"), Query: "not empty"},
+ expectedErr: status.Error(codes.InvalidArgument, "revision can't start with '-'"),
},
}
@@ -197,7 +204,7 @@ func TestFailedCommitsByMessageRequest(t *testing.T) {
c, err := client.CommitsByMessage(ctx, testCase.request)
require.NoError(t, err)
- testhelper.RequireGrpcCode(t, drainCommitsByMessageResponse(c), testCase.code)
+ testhelper.RequireGrpcError(t, testCase.expectedErr, drainCommitsByMessageResponse(c))
})
}
}
diff --git a/internal/gitaly/service/commit/count_commits.go b/internal/gitaly/service/commit/count_commits.go
index 12c0072e1..1d2807193 100644
--- a/internal/gitaly/service/commit/count_commits.go
+++ b/internal/gitaly/service/commit/count_commits.go
@@ -9,6 +9,7 @@ import (
"time"
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
@@ -76,6 +77,10 @@ func (s *server) CountCommits(ctx context.Context, in *gitalypb.CountCommitsRequ
}
func validateCountCommitsRequest(in *gitalypb.CountCommitsRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
+
if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
return err
}
diff --git a/internal/gitaly/service/commit/count_commits_test.go b/internal/gitaly/service/commit/count_commits_test.go
index b2f1a89ed..38a75c5f5 100644
--- a/internal/gitaly/service/commit/count_commits_test.go
+++ b/internal/gitaly/service/commit/count_commits_test.go
@@ -3,7 +3,6 @@
package commit
import (
- "fmt"
"testing"
"time"
@@ -12,6 +11,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/timestamppb"
)
@@ -164,17 +164,41 @@ func TestFailedCountCommitsRequestDueToValidationError(t *testing.T) {
revision := []byte("d42783470dc29fde2cf459eb3199ee1d7e3f3a72")
- rpcRequests := []*gitalypb.CountCommitsRequest{
- {Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision}, // Repository doesn't exist
- {Repository: nil, Revision: revision}, // Repository is nil
- {Repository: repo, Revision: nil, All: false}, // Revision is empty and All is false
- {Repository: repo, Revision: []byte("--output=/meow"), All: false}, // Revision is invalid
- }
-
- for _, rpcRequest := range rpcRequests {
- t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) {
- _, err := client.CountCommits(ctx, rpcRequest)
- testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.CountCommitsRequest
+ expectedErr error
+ }{
+ {
+ desc: "Repository doesn't exist",
+ req: &gitalypb.CountCommitsRequest{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `GetStorageByName: no such storage: "fake"`,
+ "repo scoped: invalid Repository",
+ )),
+ },
+ {
+ desc: "Repository is nil",
+ req: &gitalypb.CountCommitsRequest{Repository: nil, Revision: revision},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "CountCommits: empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
+ desc: "Revision is empty and All is false",
+ req: &gitalypb.CountCommitsRequest{Repository: repo, Revision: nil, All: false},
+ expectedErr: status.Error(codes.InvalidArgument, "CountCommits: empty Revision and false All"),
+ },
+ {
+ desc: "Revision is invalid",
+ req: &gitalypb.CountCommitsRequest{Repository: repo, Revision: []byte("--output=/meow"), All: false},
+ expectedErr: status.Error(codes.InvalidArgument, "CountCommits: revision can't start with '-'"),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ _, err := client.CountCommits(ctx, tc.req)
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/count_diverging_commits.go b/internal/gitaly/service/commit/count_diverging_commits.go
index a6d6ea27a..d47671ee9 100644
--- a/internal/gitaly/service/commit/count_diverging_commits.go
+++ b/internal/gitaly/service/commit/count_diverging_commits.go
@@ -8,6 +8,7 @@ import (
"strconv"
"strings"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -36,7 +37,7 @@ func (s *server) validateCountDivergingCommitsRequest(req *gitalypb.CountDivergi
}
if req.GetRepository() == nil {
- return errors.New("repository is empty")
+ return gitalyerrors.ErrEmptyRepository
}
if _, err := s.locator.GetRepoPath(req.GetRepository()); err != nil {
diff --git a/internal/gitaly/service/commit/count_diverging_commits_test.go b/internal/gitaly/service/commit/count_diverging_commits_test.go
index 763627010..40cc81f0e 100644
--- a/internal/gitaly/service/commit/count_diverging_commits_test.go
+++ b/internal/gitaly/service/commit/count_diverging_commits_test.go
@@ -14,6 +14,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func createRepoWithDivergentBranches(t *testing.T, ctx context.Context, cfg config.Cfg, leftCommits, rightCommits int, leftBranchName, rightBranchName string) *gitalypb.Repository {
@@ -165,17 +166,46 @@ func TestFailedCountDivergentCommitsRequestDueToValidationError(t *testing.T) {
revision := []byte("d42783470dc29fde2cf459eb3199ee1d7e3f3a72")
- rpcRequests := []*gitalypb.CountDivergingCommitsRequest{
- {Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, From: []byte("abcdef"), To: []byte("12345")}, // Repository doesn't exist
- {Repository: repo, From: nil, To: revision}, // From is empty
- {Repository: repo, From: revision, To: nil}, // To is empty
- {Repository: repo, From: nil, To: nil}, // From and To are empty
- }
-
- for _, rpcRequest := range rpcRequests {
- t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) {
- _, err := client.CountDivergingCommits(ctx, rpcRequest)
- testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.CountDivergingCommitsRequest
+ expectedErr error
+ }{
+ {
+ desc: "Repository not provided",
+ req: &gitalypb.CountDivergingCommitsRequest{Repository: nil, From: []byte("abcdef"), To: []byte("12345")},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
+ desc: "Repository doesn't exist",
+ req: &gitalypb.CountDivergingCommitsRequest{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, From: []byte("abcdef"), To: []byte("12345")},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `repository not valid: GetStorageByName: no such storage: "fake"`,
+ "repo scoped: invalid Repository",
+ )),
+ },
+ {
+ desc: "From is empty",
+ req: &gitalypb.CountDivergingCommitsRequest{Repository: repo, From: nil, To: revision},
+ expectedErr: status.Error(codes.InvalidArgument, "from and to are both required"),
+ },
+ {
+ desc: "To is empty",
+ req: &gitalypb.CountDivergingCommitsRequest{Repository: repo, From: revision, To: nil},
+ expectedErr: status.Error(codes.InvalidArgument, "from and to are both required"),
+ },
+ {
+ desc: "From and To are empty",
+ req: &gitalypb.CountDivergingCommitsRequest{Repository: repo, From: nil, To: nil},
+ expectedErr: status.Error(codes.InvalidArgument, "from and to are both required"),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ _, err := client.CountDivergingCommits(ctx, tc.req)
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/filter_shas_with_signatures.go b/internal/gitaly/service/commit/filter_shas_with_signatures.go
index 02e7b14df..b71f7fd54 100644
--- a/internal/gitaly/service/commit/filter_shas_with_signatures.go
+++ b/internal/gitaly/service/commit/filter_shas_with_signatures.go
@@ -2,9 +2,9 @@ package commit
import (
"context"
- "errors"
"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"
@@ -28,8 +28,8 @@ func (s *server) FilterShasWithSignatures(bidi gitalypb.CommitService_FilterShas
}
func validateFirstFilterShasWithSignaturesRequest(in *gitalypb.FilterShasWithSignaturesRequest) error {
- if in.Repository == nil {
- return errors.New("no repository given")
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
}
return nil
}
diff --git a/internal/gitaly/service/commit/filter_shas_with_signatures_test.go b/internal/gitaly/service/commit/filter_shas_with_signatures_test.go
index 0bc1cd4eb..b50a295ee 100644
--- a/internal/gitaly/service/commit/filter_shas_with_signatures_test.go
+++ b/internal/gitaly/service/commit/filter_shas_with_signatures_test.go
@@ -9,6 +9,8 @@ import (
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestFilterShasWithSignaturesSuccessful(t *testing.T) {
@@ -61,8 +63,31 @@ func TestFilterShasWithSignaturesSuccessful(t *testing.T) {
func TestFilterShasWithSignaturesValidationError(t *testing.T) {
t.Parallel()
- err := validateFirstFilterShasWithSignaturesRequest(&gitalypb.FilterShasWithSignaturesRequest{})
- require.Contains(t, err.Error(), "no repository given")
+ ctx := testhelper.Context(t)
+ _, client := setupCommitService(t, ctx)
+
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.FilterShasWithSignaturesRequest
+ expectedErr error
+ }{
+ {
+ desc: "no repository provided",
+ req: &gitalypb.FilterShasWithSignaturesRequest{Repository: nil},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ stream, err := client.FilterShasWithSignatures(ctx)
+ require.NoError(t, err)
+ require.NoError(t, stream.Send(tc.req))
+ _, err = stream.Recv()
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
+ })
+ }
}
func recvFSWS(stream gitalypb.CommitService_FilterShasWithSignaturesClient) ([][]byte, error) {
diff --git a/internal/gitaly/service/commit/find_all_commits.go b/internal/gitaly/service/commit/find_all_commits.go
index 837ff1318..573894744 100644
--- a/internal/gitaly/service/commit/find_all_commits.go
+++ b/internal/gitaly/service/commit/find_all_commits.go
@@ -3,6 +3,7 @@ package commit
import (
"fmt"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -10,7 +11,7 @@ import (
func (s *server) FindAllCommits(in *gitalypb.FindAllCommitsRequest, stream gitalypb.CommitService_FindAllCommitsServer) error {
if err := validateFindAllCommitsRequest(in); err != nil {
- return err
+ return helper.ErrInvalidArgument(err)
}
ctx := stream.Context()
@@ -39,8 +40,12 @@ func (s *server) FindAllCommits(in *gitalypb.FindAllCommitsRequest, stream gital
}
func validateFindAllCommitsRequest(in *gitalypb.FindAllCommitsRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
+
if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
- return helper.ErrInvalidArgument(err)
+ return err
}
return nil
diff --git a/internal/gitaly/service/commit/find_all_commits_test.go b/internal/gitaly/service/commit/find_all_commits_test.go
index 2a70f45a9..5463c2de6 100644
--- a/internal/gitaly/service/commit/find_all_commits_test.go
+++ b/internal/gitaly/service/commit/find_all_commits_test.go
@@ -12,6 +12,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestSuccessfulFindAllCommitsRequest(t *testing.T) {
@@ -166,19 +167,25 @@ func TestFailedFindAllCommitsRequest(t *testing.T) {
invalidRepo := &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}
testCases := []struct {
- desc string
- request *gitalypb.FindAllCommitsRequest
- code codes.Code
+ desc string
+ request *gitalypb.FindAllCommitsRequest
+ expectedErr error
}{
{
desc: "Invalid repository",
request: &gitalypb.FindAllCommitsRequest{Repository: invalidRepo},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `GetStorageByName: no such storage: "fake"`,
+ "repo scoped: invalid Repository",
+ )),
},
{
desc: "Repository is nil",
request: &gitalypb.FindAllCommitsRequest{},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
},
{
desc: "Revision is invalid",
@@ -186,7 +193,7 @@ func TestFailedFindAllCommitsRequest(t *testing.T) {
Repository: repo,
Revision: []byte("--output=/meow"),
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "revision can't start with '-'"),
},
}
@@ -196,7 +203,7 @@ func TestFailedFindAllCommitsRequest(t *testing.T) {
require.NoError(t, err)
err = drainFindAllCommitsResponse(c)
- testhelper.RequireGrpcCode(t, err, testCase.code)
+ testhelper.RequireGrpcError(t, testCase.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/find_commit.go b/internal/gitaly/service/commit/find_commit.go
index 05705a0ae..4b736452c 100644
--- a/internal/gitaly/service/commit/find_commit.go
+++ b/internal/gitaly/service/commit/find_commit.go
@@ -4,18 +4,27 @@ import (
"context"
"errors"
+ 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/localrepo"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
)
+func validateFindCommitRequest(in *gitalypb.FindCommitRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
+ if err := git.ValidateRevision(in.GetRevision()); err != nil {
+ return err
+ }
+ return nil
+}
+
func (s *server) FindCommit(ctx context.Context, in *gitalypb.FindCommitRequest) (*gitalypb.FindCommitResponse, error) {
- revision := in.GetRevision()
- if err := git.ValidateRevision(revision); err != nil {
+ if err := validateFindCommitRequest(in); err != nil {
return nil, helper.ErrInvalidArgument(err)
}
-
repo := s.localrepo(in.GetRepository())
var opts []localrepo.ReadCommitOpt
@@ -23,7 +32,7 @@ func (s *server) FindCommit(ctx context.Context, in *gitalypb.FindCommitRequest)
opts = []localrepo.ReadCommitOpt{localrepo.WithTrailers()}
}
- commit, err := repo.ReadCommit(ctx, git.Revision(revision), opts...)
+ commit, err := repo.ReadCommit(ctx, git.Revision(in.GetRevision()), opts...)
if err != nil {
if errors.Is(err, localrepo.ErrObjectNotFound) {
return &gitalypb.FindCommitResponse{}, nil
diff --git a/internal/gitaly/service/commit/find_commit_test.go b/internal/gitaly/service/commit/find_commit_test.go
index 5b593a6da..cb7c1deee 100644
--- a/internal/gitaly/service/commit/find_commit_test.go
+++ b/internal/gitaly/service/commit/find_commit_test.go
@@ -13,7 +13,9 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
+ "google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/timestamppb"
)
@@ -251,10 +253,18 @@ func TestFailedFindCommitRequest(t *testing.T) {
expectedErr error
}{
{
+ desc: "Repository is nil",
+ repo: nil,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
desc: "Invalid repo",
repo: invalidRepo,
revision: []byte("master"),
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
"GetStorageByName: no such storage: \"fake\"",
"repo scoped: invalid Repository",
)),
diff --git a/internal/gitaly/service/commit/find_commits.go b/internal/gitaly/service/commit/find_commits.go
index 13907b10a..1b2aa7117 100644
--- a/internal/gitaly/service/commit/find_commits.go
+++ b/internal/gitaly/service/commit/find_commits.go
@@ -12,6 +12,7 @@ import (
"strings"
"gitlab.com/gitlab-org/gitaly/v15/internal/command"
+ 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/git/trailerparser"
@@ -22,10 +23,20 @@ import (
var statsPattern = regexp.MustCompile(`\s(\d+)\sfiles? changed(,\s(\d+)\sinsertions?\(\+\))?(,\s(\d+)\sdeletions?\(-\))?`)
+func validateFindCommitsRequest(in *gitalypb.FindCommitsRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
+ if err := git.ValidateRevisionAllowEmpty(in.GetRevision()); err != nil {
+ return err
+ }
+ return nil
+}
+
func (s *server) FindCommits(req *gitalypb.FindCommitsRequest, stream gitalypb.CommitService_FindCommitsServer) error {
ctx := stream.Context()
- if err := git.ValidateRevisionAllowEmpty(req.Revision); err != nil {
+ if err := validateFindCommitsRequest(req); err != nil {
return helper.ErrInvalidArgument(err)
}
diff --git a/internal/gitaly/service/commit/find_commits_test.go b/internal/gitaly/service/commit/find_commits_test.go
index 406cd171e..e3521bd83 100644
--- a/internal/gitaly/service/commit/find_commits_test.go
+++ b/internal/gitaly/service/commit/find_commits_test.go
@@ -15,6 +15,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/timestamppb"
)
@@ -515,17 +516,28 @@ func TestFailureFindCommitsRequest(t *testing.T) {
_, repo, _, client := setupCommitServiceWithRepo(t, ctx)
testCases := []struct {
- desc string
- request *gitalypb.FindCommitsRequest
- code codes.Code
+ desc string
+ request *gitalypb.FindCommitsRequest
+ expectedErr error
}{
{
+ desc: "no repository provided",
+ request: &gitalypb.FindCommitsRequest{
+ Repository: nil,
+ Revision: []byte("HEAD"),
+ },
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
desc: "empty path string",
request: &gitalypb.FindCommitsRequest{
Repository: repo,
Paths: [][]byte{[]byte("")},
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "path is empty string"),
},
{
desc: "invalid revision",
@@ -534,7 +546,7 @@ func TestFailureFindCommitsRequest(t *testing.T) {
Revision: []byte("--output=/meow"),
Limit: 1,
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "revision can't start with '-'"),
},
}
@@ -547,7 +559,7 @@ func TestFailureFindCommitsRequest(t *testing.T) {
_, err = stream.Recv()
}
- testhelper.RequireGrpcCode(t, err, tc.code)
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/isancestor.go b/internal/gitaly/service/commit/isancestor.go
index 3fa8bdccc..a4b993da5 100644
--- a/internal/gitaly/service/commit/isancestor.go
+++ b/internal/gitaly/service/commit/isancestor.go
@@ -2,21 +2,34 @@ package commit
import (
"context"
+ "errors"
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
log "github.com/sirupsen/logrus"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+ "gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
-func (s *server) CommitIsAncestor(ctx context.Context, in *gitalypb.CommitIsAncestorRequest) (*gitalypb.CommitIsAncestorResponse, error) {
- if in.AncestorId == "" {
- return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty ancestor sha)")
+func validateCommitIsAncestorRequest(in *gitalypb.CommitIsAncestorRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
+ if in.GetAncestorId() == "" {
+ return errors.New("Bad Request (empty ancestor sha)") //nolint:stylecheck
+ }
+ if in.GetChildId() == "" {
+ return errors.New("Bad Request (empty child sha)") //nolint:stylecheck
}
- if in.ChildId == "" {
- return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty child sha)")
+ return nil
+}
+
+func (s *server) CommitIsAncestor(ctx context.Context, in *gitalypb.CommitIsAncestorRequest) (*gitalypb.CommitIsAncestorResponse, error) {
+ if err := validateCommitIsAncestorRequest(in); err != nil {
+ return nil, helper.ErrInvalidArgument(err)
}
ret, err := s.commitIsAncestorName(ctx, in.Repository, in.AncestorId, in.ChildId)
diff --git a/internal/gitaly/service/commit/isancestor_test.go b/internal/gitaly/service/commit/isancestor_test.go
index 413815ecf..90309528d 100644
--- a/internal/gitaly/service/commit/isancestor_test.go
+++ b/internal/gitaly/service/commit/isancestor_test.go
@@ -10,23 +10,22 @@ import (
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
- "gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestCommitIsAncestorFailure(t *testing.T) {
t.Parallel()
ctx := testhelper.Context(t)
- _, repo, _, client := setupCommitServiceWithRepo(t, ctx)
+ cfg, repo, _, client := setupCommitServiceWithRepo(t, ctx)
queries := []struct {
- Request *gitalypb.CommitIsAncestorRequest
- ErrorCode codes.Code
- ErrMsg string
+ Request *gitalypb.CommitIsAncestorRequest
+ expectedErr error
}{
{
Request: &gitalypb.CommitIsAncestorRequest{
@@ -34,8 +33,10 @@ func TestCommitIsAncestorFailure(t *testing.T) {
AncestorId: "b83d6e391c22777fca1ed3012fce84f633d7fed0",
ChildId: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
},
- ErrorCode: codes.InvalidArgument,
- ErrMsg: "Expected to throw invalid argument got: %s",
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
},
{
Request: &gitalypb.CommitIsAncestorRequest{
@@ -43,8 +44,7 @@ func TestCommitIsAncestorFailure(t *testing.T) {
AncestorId: "",
ChildId: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
},
- ErrorCode: codes.InvalidArgument,
- ErrMsg: "Expected to throw invalid argument got: %s",
+ expectedErr: status.Error(codes.InvalidArgument, "Bad Request (empty ancestor sha)"),
},
{
Request: &gitalypb.CommitIsAncestorRequest{
@@ -52,8 +52,7 @@ func TestCommitIsAncestorFailure(t *testing.T) {
AncestorId: "b83d6e391c22777fca1ed3012fce84f633d7fed0",
ChildId: "",
},
- ErrorCode: codes.InvalidArgument,
- ErrMsg: "Expected to throw invalid argument got: %s",
+ expectedErr: status.Error(codes.InvalidArgument, "Bad Request (empty child sha)"),
},
{
Request: &gitalypb.CommitIsAncestorRequest{
@@ -61,18 +60,17 @@ func TestCommitIsAncestorFailure(t *testing.T) {
AncestorId: "b83d6e391c22777fca1ed3012fce84f633d7fed0",
ChildId: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
},
- ErrorCode: codes.NotFound,
- ErrMsg: "Expected to throw internal got: %s",
+ expectedErr: status.Error(codes.NotFound, testhelper.GitalyOrPraefect(
+ `GetRepoPath: not a git repository: "`+cfg.Storages[0].Path+`/fake-path"`,
+ `accessor call: route repository accessor: consistent storages: repository "default"/"fake-path" not found`,
+ )),
},
}
for _, v := range queries {
t.Run(fmt.Sprintf("%v", v.Request), func(t *testing.T) {
- if _, err := client.CommitIsAncestor(ctx, v.Request); err == nil {
- t.Error("Expected to throw an error")
- } else if helper.GrpcCode(err) != v.ErrorCode {
- t.Errorf(v.ErrMsg, err)
- }
+ _, err := client.CommitIsAncestor(ctx, v.Request)
+ testhelper.RequireGrpcError(t, v.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/languages.go b/internal/gitaly/service/commit/languages.go
index be2116cc9..10eb971e8 100644
--- a/internal/gitaly/service/commit/languages.go
+++ b/internal/gitaly/service/commit/languages.go
@@ -9,6 +9,7 @@ import (
"sort"
"strings"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/linguist"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -18,8 +19,18 @@ import (
var errAmbigRef = errors.New("ambiguous reference")
-func (s *server) CommitLanguages(ctx context.Context, req *gitalypb.CommitLanguagesRequest) (*gitalypb.CommitLanguagesResponse, error) {
+func (s *server) validateCommitLanguagesRequest(req *gitalypb.CommitLanguagesRequest) error {
+ if req.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevisionAllowEmpty(req.Revision); err != nil {
+ return err
+ }
+ return nil
+}
+
+func (s *server) CommitLanguages(ctx context.Context, req *gitalypb.CommitLanguagesRequest) (*gitalypb.CommitLanguagesResponse, error) {
+ if err := s.validateCommitLanguagesRequest(req); err != nil {
return nil, helper.ErrInvalidArgument(err)
}
diff --git a/internal/gitaly/service/commit/languages_test.go b/internal/gitaly/service/commit/languages_test.go
index df96ae3fe..485ec6ed1 100644
--- a/internal/gitaly/service/commit/languages_test.go
+++ b/internal/gitaly/service/commit/languages_test.go
@@ -13,6 +13,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestLanguages(t *testing.T) {
@@ -118,7 +119,7 @@ func testInvalidCommitLanguagesRequestRevisionFeatured(t *testing.T, ctx context
Repository: repo,
Revision: []byte("--output=/meow"),
})
- testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+ testhelper.RequireGrpcError(t, status.Error(codes.InvalidArgument, "revision can't start with '-'"), err)
}
func TestAmbiguousRefCommitLanguagesRequestRevision(t *testing.T) {
@@ -140,3 +141,38 @@ func testAmbiguousRefCommitLanguagesRequestRevisionFeatured(t *testing.T, ctx co
})
require.NoError(t, err)
}
+
+func TestCommitLanguages_validateRequest(t *testing.T) {
+ t.Parallel()
+ testhelper.NewFeatureSets(featureflag.GoLanguageStats).Run(t, func(t *testing.T, ctx context.Context) {
+ _, repo, _, client := setupCommitServiceWithRepo(t, ctx)
+
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.CommitLanguagesRequest
+ expectedErr error
+ }{
+ {
+ desc: "no repository provided",
+ req: &gitalypb.CommitLanguagesRequest{Repository: nil},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
+ desc: "invalid revision provided",
+ req: &gitalypb.CommitLanguagesRequest{
+ Repository: repo,
+ Revision: []byte("invalid revision"),
+ },
+ expectedErr: status.Error(codes.InvalidArgument, "revision can't contain whitespace"),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ _, err := client.CommitLanguages(ctx, tc.req)
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
+ })
+ }
+ })
+}
diff --git a/internal/gitaly/service/commit/last_commit_for_path.go b/internal/gitaly/service/commit/last_commit_for_path.go
index 48e87c726..0ed2eb3e3 100644
--- a/internal/gitaly/service/commit/last_commit_for_path.go
+++ b/internal/gitaly/service/commit/last_commit_for_path.go
@@ -3,6 +3,7 @@ package commit
import (
"context"
+ 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/log"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -57,8 +58,11 @@ func (s *server) lastCommitForPath(ctx context.Context, in *gitalypb.LastCommitF
}
func validateLastCommitForPathRequest(in *gitalypb.LastCommitForPathRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevision(in.Revision); err != nil {
- return helper.ErrInvalidArgument(err)
+ return err
}
return nil
}
diff --git a/internal/gitaly/service/commit/last_commit_for_path_test.go b/internal/gitaly/service/commit/last_commit_for_path_test.go
index 33261dadb..28e19c7d0 100644
--- a/internal/gitaly/service/commit/last_commit_for_path_test.go
+++ b/internal/gitaly/service/commit/last_commit_for_path_test.go
@@ -92,7 +92,7 @@ func TestFailedLastCommitForPathRequest(t *testing.T) {
Repository: invalidRepo,
Revision: []byte("some-branch"),
},
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
"GetStorageByName: no such storage: \"fake\"",
"repo scoped: invalid Repository",
)),
@@ -102,8 +102,8 @@ func TestFailedLastCommitForPathRequest(t *testing.T) {
request: &gitalypb.LastCommitForPathRequest{
Revision: []byte("some-branch"),
},
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
- "GetStorageByName: no such storage: \"\"",
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
+ "empty Repository",
"repo scoped: empty Repository",
)),
},
diff --git a/internal/gitaly/service/commit/list_all_commits.go b/internal/gitaly/service/commit/list_all_commits.go
index ff368f6d7..9cba824b8 100644
--- a/internal/gitaly/service/commit/list_all_commits.go
+++ b/internal/gitaly/service/commit/list_all_commits.go
@@ -1,9 +1,9 @@
package commit
import (
- "errors"
"fmt"
+ 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/git/gitpipe"
@@ -14,7 +14,7 @@ import (
func verifyListAllCommitsRequest(request *gitalypb.ListAllCommitsRequest) error {
if request.GetRepository() == nil {
- return errors.New("empty repository")
+ return gitalyerrors.ErrEmptyRepository
}
return nil
}
diff --git a/internal/gitaly/service/commit/list_all_commits_test.go b/internal/gitaly/service/commit/list_all_commits_test.go
index b5368f57d..a828df9c8 100644
--- a/internal/gitaly/service/commit/list_all_commits_test.go
+++ b/internal/gitaly/service/commit/list_all_commits_test.go
@@ -13,6 +13,8 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestListAllCommits(t *testing.T) {
@@ -136,6 +138,36 @@ func TestListAllCommits(t *testing.T) {
})
}
+func TestListAllCommits_validate(t *testing.T) {
+ t.Parallel()
+ ctx := testhelper.Context(t)
+ _, client := setupCommitService(t, ctx)
+
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.ListAllCommitsRequest
+ expectedErr error
+ }{
+ {
+ desc: "no repository provided",
+ req: &gitalypb.ListAllCommitsRequest{
+ Repository: nil,
+ },
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ stream, err := client.ListAllCommits(ctx, tc.req)
+ require.NoError(t, err)
+ _, err = stream.Recv()
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
+ })
+ }
+}
+
func BenchmarkListAllCommits(b *testing.B) {
b.StopTimer()
ctx := testhelper.Context(b)
diff --git a/internal/gitaly/service/commit/list_commits.go b/internal/gitaly/service/commit/list_commits.go
index 7fc5858fe..cff5cd83a 100644
--- a/internal/gitaly/service/commit/list_commits.go
+++ b/internal/gitaly/service/commit/list_commits.go
@@ -5,6 +5,7 @@ import (
"fmt"
"strings"
+ 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/git/gitpipe"
@@ -15,7 +16,7 @@ import (
func verifyListCommitsRequest(request *gitalypb.ListCommitsRequest) error {
if request.GetRepository() == nil {
- return errors.New("empty repository")
+ return gitalyerrors.ErrEmptyRepository
}
if len(request.GetRevisions()) == 0 {
return errors.New("missing revisions")
diff --git a/internal/gitaly/service/commit/list_commits_by_oid.go b/internal/gitaly/service/commit/list_commits_by_oid.go
index 9bd423a8e..fa830121b 100644
--- a/internal/gitaly/service/commit/list_commits_by_oid.go
+++ b/internal/gitaly/service/commit/list_commits_by_oid.go
@@ -3,8 +3,10 @@ package commit
import (
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
+ 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/internal/helper/chunk"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/protobuf/proto"
@@ -23,6 +25,9 @@ var listCommitsbyOidHistogram = promauto.NewHistogram(
func (s *server) ListCommitsByOid(in *gitalypb.ListCommitsByOidRequest, stream gitalypb.CommitService_ListCommitsByOidServer) error {
ctx := stream.Context()
+ if in.GetRepository() == nil {
+ return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
+ }
repo := s.localrepo(in.GetRepository())
objectReader, cancel, err := s.catfileCache.ObjectReader(ctx, repo)
diff --git a/internal/gitaly/service/commit/list_commits_by_oid_test.go b/internal/gitaly/service/commit/list_commits_by_oid_test.go
index b597b0c32..bb7f64499 100644
--- a/internal/gitaly/service/commit/list_commits_by_oid_test.go
+++ b/internal/gitaly/service/commit/list_commits_by_oid_test.go
@@ -8,6 +8,8 @@ import (
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestSuccessfulListCommitsByOidRequest(t *testing.T) {
@@ -172,3 +174,30 @@ func TestSuccessfulListCommitsByOidLargeRequest(t *testing.T) {
require.Equal(t, masterCommitids[i], actual.Id, "commit ID must match, entry %d", i)
}
}
+
+func TestListCommitsByOid_validate(t *testing.T) {
+ t.Parallel()
+ ctx := testhelper.Context(t)
+ _, client := setupCommitService(t, ctx)
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.ListCommitsByOidRequest
+ expectedErr error
+ }{
+ {
+ desc: "repository not provided",
+ req: &gitalypb.ListCommitsByOidRequest{Repository: nil},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ stream, err := client.ListCommitsByOid(ctx, tc.req)
+ require.NoError(t, err)
+ _, err = stream.Recv()
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
+ })
+ }
+}
diff --git a/internal/gitaly/service/commit/list_commits_by_ref_name.go b/internal/gitaly/service/commit/list_commits_by_ref_name.go
index 77c8b3e61..12b866730 100644
--- a/internal/gitaly/service/commit/list_commits_by_ref_name.go
+++ b/internal/gitaly/service/commit/list_commits_by_ref_name.go
@@ -1,6 +1,7 @@
package commit
import (
+ 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"
@@ -11,6 +12,9 @@ import (
func (s *server) ListCommitsByRefName(in *gitalypb.ListCommitsByRefNameRequest, stream gitalypb.CommitService_ListCommitsByRefNameServer) error {
ctx := stream.Context()
+ if in.GetRepository() == nil {
+ return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
+ }
repo := s.localrepo(in.GetRepository())
objectReader, cancel, err := s.catfileCache.ObjectReader(ctx, repo)
diff --git a/internal/gitaly/service/commit/list_commits_by_ref_name_test.go b/internal/gitaly/service/commit/list_commits_by_ref_name_test.go
index ea5546ce8..226858b15 100644
--- a/internal/gitaly/service/commit/list_commits_by_ref_name_test.go
+++ b/internal/gitaly/service/commit/list_commits_by_ref_name_test.go
@@ -9,6 +9,8 @@ import (
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestSuccessfulListCommitsByRefNameRequest(t *testing.T) {
@@ -193,6 +195,33 @@ func TestSuccessfulListCommitsByRefNameLargeRequest(t *testing.T) {
}
}
+func TestListCommitsByRefName_validate(t *testing.T) {
+ t.Parallel()
+ ctx := testhelper.Context(t)
+ _, client := setupCommitService(t, ctx)
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.ListCommitsByRefNameRequest
+ expectedErr error
+ }{
+ {
+ desc: "repository not provided",
+ req: &gitalypb.ListCommitsByRefNameRequest{Repository: nil},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ stream, err := client.ListCommitsByRefName(ctx, tc.req)
+ require.NoError(t, err)
+ _, err = stream.Recv()
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
+ })
+ }
+}
+
func consumeGetByRefNameResponse(t *testing.T, c gitalypb.CommitService_ListCommitsByRefNameClient) []*gitalypb.ListCommitsByRefNameResponse_CommitForRef {
var receivedCommitRefs []*gitalypb.ListCommitsByRefNameResponse_CommitForRef
for {
diff --git a/internal/gitaly/service/commit/list_commits_test.go b/internal/gitaly/service/commit/list_commits_test.go
index 302cdcef3..5a582944e 100644
--- a/internal/gitaly/service/commit/list_commits_test.go
+++ b/internal/gitaly/service/commit/list_commits_test.go
@@ -11,6 +11,8 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/timestamppb"
)
@@ -315,3 +317,40 @@ func TestListCommits(t *testing.T) {
})
}
}
+
+func TestListCommits_verify(t *testing.T) {
+ t.Parallel()
+ ctx := testhelper.Context(t)
+ _, repo, _, client := setupCommitServiceWithRepo(t, ctx)
+ for _, tc := range []struct {
+ desc string
+ req *gitalypb.ListCommitsRequest
+ expectedErr error
+ }{
+ {
+ desc: "no repository provided",
+ req: &gitalypb.ListCommitsRequest{Repository: nil},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
+ desc: "no revisions",
+ req: &gitalypb.ListCommitsRequest{Repository: repo},
+ expectedErr: status.Error(codes.InvalidArgument, "missing revisions"),
+ },
+ {
+ desc: "invalid revision",
+ req: &gitalypb.ListCommitsRequest{Repository: repo, Revisions: []string{"asdf", "-invalid"}},
+ expectedErr: status.Error(codes.InvalidArgument, `invalid revision: "-invalid"`),
+ },
+ } {
+ t.Run(tc.desc, func(t *testing.T) {
+ stream, err := client.ListCommits(ctx, tc.req)
+ require.NoError(t, err)
+ _, err = stream.Recv()
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
+ })
+ }
+}
diff --git a/internal/gitaly/service/commit/list_files.go b/internal/gitaly/service/commit/list_files.go
index 9a1da2197..5ba492fe4 100644
--- a/internal/gitaly/service/commit/list_files.go
+++ b/internal/gitaly/service/commit/list_files.go
@@ -5,6 +5,7 @@ import (
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
log "github.com/sirupsen/logrus"
+ 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/lstree"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -19,7 +20,7 @@ func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.Commit
}).Debug("ListFiles")
if err := validateListFilesRequest(in); err != nil {
- return err
+ return helper.ErrInvalidArgument(err)
}
repo := s.localrepo(in.GetRepository())
@@ -58,8 +59,11 @@ func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.Commit
}
func validateListFilesRequest(in *gitalypb.ListFilesRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
- return helper.ErrInvalidArgument(err)
+ return err
}
return nil
}
diff --git a/internal/gitaly/service/commit/list_files_test.go b/internal/gitaly/service/commit/list_files_test.go
index 5352a271d..dd71af94d 100644
--- a/internal/gitaly/service/commit/list_files_test.go
+++ b/internal/gitaly/service/commit/list_files_test.go
@@ -11,6 +11,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
var defaultFiles = [][]byte{
@@ -201,27 +202,36 @@ func TestListFiles_failure(t *testing.T) {
t.Parallel()
ctx := testhelper.Context(t)
- _, _, _, client := setupCommitServiceWithRepo(t, ctx)
+ _, client := setupCommitService(t, ctx)
tests := []struct {
- desc string
- repo *gitalypb.Repository
- code codes.Code
+ desc string
+ repo *gitalypb.Repository
+ expectedErr error
}{
{
desc: "nil repo",
repo: nil,
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
},
{
desc: "empty repo object",
repo: &gitalypb.Repository{},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `GetStorageByName: no such storage: ""`,
+ "repo scoped: invalid Repository",
+ )),
},
{
desc: "non-existing repo",
repo: &gitalypb.Repository{StorageName: "foo", RelativePath: "bar"},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `GetStorageByName: no such storage: "foo"`,
+ "repo scoped: invalid Repository",
+ )),
},
}
@@ -235,7 +245,7 @@ func TestListFiles_failure(t *testing.T) {
require.NoError(t, err)
err = drainListFilesResponse(c)
- testhelper.RequireGrpcCode(t, err, tc.code)
+ testhelper.RequireGrpcError(t, tc.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/list_last_commits_for_tree.go b/internal/gitaly/service/commit/list_last_commits_for_tree.go
index 7e1c35b83..ba3f6535e 100644
--- a/internal/gitaly/service/commit/list_last_commits_for_tree.go
+++ b/internal/gitaly/service/commit/list_last_commits_for_tree.go
@@ -6,6 +6,7 @@ import (
"sort"
"gitlab.com/gitlab-org/gitaly/v15/internal/command"
+ 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/log"
"gitlab.com/gitlab-org/gitaly/v15/internal/git/lstree"
@@ -139,6 +140,9 @@ func sendCommitsForTree(batch []*gitalypb.ListLastCommitsForTreeResponse_CommitF
}
func validateListLastCommitsForTreeRequest(in *gitalypb.ListLastCommitsForTreeRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevision([]byte(in.Revision)); err != nil {
return err
}
diff --git a/internal/gitaly/service/commit/list_last_commits_for_tree_test.go b/internal/gitaly/service/commit/list_last_commits_for_tree_test.go
index 16f7dcba1..e60b92096 100644
--- a/internal/gitaly/service/commit/list_last_commits_for_tree_test.go
+++ b/internal/gitaly/service/commit/list_last_commits_for_tree_test.go
@@ -13,6 +13,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
type commitInfo struct {
@@ -218,9 +219,9 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
invalidRepo := &gitalypb.Repository{StorageName: "broken", RelativePath: "path"}
testCases := []struct {
- desc string
- request *gitalypb.ListLastCommitsForTreeRequest
- code codes.Code
+ desc string
+ request *gitalypb.ListLastCommitsForTreeRequest
+ expectedErr error
}{
{
desc: "Revision is missing",
@@ -231,7 +232,7 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
Offset: 0,
Limit: 25,
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "empty revision"),
},
{
desc: "Invalid repository",
@@ -242,7 +243,10 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
Offset: 0,
Limit: 25,
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `GetStorageByName: no such storage: "broken"`,
+ "repo scoped: invalid Repository",
+ )),
},
{
desc: "Repository is nil",
@@ -252,17 +256,10 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
Offset: 0,
Limit: 25,
},
- code: codes.InvalidArgument,
- },
- {
- desc: "Revision is missing",
- request: &gitalypb.ListLastCommitsForTreeRequest{
- Repository: repo,
- Path: []byte("/"),
- Offset: 0,
- Limit: 25,
- },
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
},
{
desc: "Ambiguous revision",
@@ -272,7 +269,7 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
Offset: 0,
Limit: 25,
},
- code: codes.Internal,
+ expectedErr: status.Error(codes.Internal, "exit status 128"),
},
{
desc: "Invalid revision",
@@ -282,7 +279,7 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
Offset: 0,
Limit: 25,
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "revision can't start with '-'"),
},
{
desc: "Negative offset",
@@ -292,7 +289,7 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
Offset: -1,
Limit: 25,
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "revision can't start with '-'"),
},
{
desc: "Negative limit",
@@ -302,7 +299,7 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
Offset: 0,
Limit: -1,
},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "revision can't start with '-'"),
},
}
@@ -312,7 +309,7 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
require.NoError(t, err)
_, err = stream.Recv()
- testhelper.RequireGrpcCode(t, err, testCase.code)
+ testhelper.RequireGrpcError(t, testCase.expectedErr, err)
})
}
}
diff --git a/internal/gitaly/service/commit/raw_blame.go b/internal/gitaly/service/commit/raw_blame.go
index 41c3c83c8..ebaf02c70 100644
--- a/internal/gitaly/service/commit/raw_blame.go
+++ b/internal/gitaly/service/commit/raw_blame.go
@@ -6,6 +6,7 @@ import (
"regexp"
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
"gitlab.com/gitlab-org/gitaly/v15/streamio"
@@ -60,6 +61,9 @@ func (s *server) RawBlame(in *gitalypb.RawBlameRequest, stream gitalypb.CommitSe
}
func validateRawBlameRequest(in *gitalypb.RawBlameRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevision(in.Revision); err != nil {
return err
}
diff --git a/internal/gitaly/service/commit/raw_blame_test.go b/internal/gitaly/service/commit/raw_blame_test.go
index aa2375a3d..9d7f3f7c2 100644
--- a/internal/gitaly/service/commit/raw_blame_test.go
+++ b/internal/gitaly/service/commit/raw_blame_test.go
@@ -12,6 +12,7 @@ import (
"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"
)
func TestSuccessfulRawBlameRequest(t *testing.T) {
@@ -86,15 +87,26 @@ func TestFailedRawBlameRequest(t *testing.T) {
repo *gitalypb.Repository
revision, path []byte
blameRange []byte
- code codes.Code
+ expectedErr error
}{
{
+ description: "No repository provided",
+ repo: nil,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "RawBlame: empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
description: "Invalid repo",
repo: invalidRepo,
revision: []byte("master"),
path: []byte("a/b/c"),
blameRange: []byte{},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ `GetStorageByName: no such storage: "fake"`,
+ "repo scoped: invalid Repository",
+ )),
},
{
description: "Empty revision",
@@ -102,7 +114,7 @@ func TestFailedRawBlameRequest(t *testing.T) {
revision: []byte(""),
path: []byte("a/b/c"),
blameRange: []byte{},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "RawBlame: empty revision"),
},
{
description: "Empty path",
@@ -110,7 +122,7 @@ func TestFailedRawBlameRequest(t *testing.T) {
revision: []byte("abcdef"),
path: []byte(""),
blameRange: []byte{},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "RawBlame: empty Path"),
},
{
description: "Invalid revision",
@@ -118,7 +130,7 @@ func TestFailedRawBlameRequest(t *testing.T) {
revision: []byte("--output=/meow"),
path: []byte("a/b/c"),
blameRange: []byte{},
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "RawBlame: revision can't start with '-'"),
},
{
description: "Invalid range",
@@ -126,7 +138,7 @@ func TestFailedRawBlameRequest(t *testing.T) {
revision: []byte("abcdef"),
path: []byte("a/b/c"),
blameRange: []byte("foo"),
- code: codes.InvalidArgument,
+ expectedErr: status.Error(codes.InvalidArgument, "RawBlame: invalid Range"),
},
}
@@ -141,7 +153,7 @@ func TestFailedRawBlameRequest(t *testing.T) {
c, err := client.RawBlame(ctx, &request)
require.NoError(t, err)
- testhelper.RequireGrpcCode(t, drainRawBlameResponse(c), testCase.code)
+ testhelper.RequireGrpcError(t, testCase.expectedErr, drainRawBlameResponse(c))
})
}
}
diff --git a/internal/gitaly/service/commit/stats.go b/internal/gitaly/service/commit/stats.go
index 1b9ba0286..1a9873f4a 100644
--- a/internal/gitaly/service/commit/stats.go
+++ b/internal/gitaly/service/commit/stats.go
@@ -7,13 +7,24 @@ import (
"strconv"
"strings"
+ gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
"gitlab.com/gitlab-org/gitaly/v15/internal/git"
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
)
-func (s *server) CommitStats(ctx context.Context, in *gitalypb.CommitStatsRequest) (*gitalypb.CommitStatsResponse, error) {
+func validateCommitStatsRequest(in *gitalypb.CommitStatsRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevision(in.Revision); err != nil {
+ return err
+ }
+ return nil
+}
+
+func (s *server) CommitStats(ctx context.Context, in *gitalypb.CommitStatsRequest) (*gitalypb.CommitStatsResponse, error) {
+ if err := validateCommitStatsRequest(in); err != nil {
return nil, helper.ErrInvalidArgument(err)
}
diff --git a/internal/gitaly/service/commit/stats_test.go b/internal/gitaly/service/commit/stats_test.go
index 744e76a15..cce0d5d93 100644
--- a/internal/gitaly/service/commit/stats_test.go
+++ b/internal/gitaly/service/commit/stats_test.go
@@ -12,6 +12,8 @@ import (
"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func TestCommitStatsSuccess(t *testing.T) {
@@ -90,6 +92,28 @@ func TestCommitStatsFailure(t *testing.T) {
expectedErr error
}{
{
+ desc: "no repository provided",
+ request: &gitalypb.CommitStatsRequest{Repository: nil},
+ expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+ "empty Repository",
+ "repo scoped: empty Repository",
+ )),
+ },
+ {
+ desc: "repo not found",
+ request: &gitalypb.CommitStatsRequest{
+ Repository: &gitalypb.Repository{
+ StorageName: repo.GetStorageName(),
+ RelativePath: "bar.git",
+ },
+ Revision: []byte("test-do-not-touch"),
+ },
+ expectedErr: helper.ErrNotFoundf(testhelper.GitalyOrPraefect(
+ fmt.Sprintf("GetRepoPath: not a git repository: %q", filepath.Join(cfg.Storages[0].Path, "bar.git")),
+ "accessor call: route repository accessor: consistent storages: repository \"default\"/\"bar.git\" not found",
+ )),
+ },
+ {
desc: "repo not found",
request: &gitalypb.CommitStatsRequest{
Repository: &gitalypb.Repository{
@@ -98,7 +122,7 @@ func TestCommitStatsFailure(t *testing.T) {
},
Revision: []byte("test-do-not-touch"),
},
- expectedErr: helper.ErrNotFoundf(gitalyOrPraefect(
+ expectedErr: helper.ErrNotFoundf(testhelper.GitalyOrPraefect(
fmt.Sprintf("GetRepoPath: not a git repository: %q", filepath.Join(cfg.Storages[0].Path, "bar.git")),
"accessor call: route repository accessor: consistent storages: repository \"default\"/\"bar.git\" not found",
)),
@@ -112,7 +136,7 @@ func TestCommitStatsFailure(t *testing.T) {
},
Revision: []byte("test-do-not-touch"),
},
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
"GetStorageByName: no such storage: \"foo\"",
"repo scoped: invalid Repository",
)),
diff --git a/internal/gitaly/service/commit/testhelper_test.go b/internal/gitaly/service/commit/testhelper_test.go
index ececb79e2..2adeda0a7 100644
--- a/internal/gitaly/service/commit/testhelper_test.go
+++ b/internal/gitaly/service/commit/testhelper_test.go
@@ -142,10 +142,3 @@ func getAllCommits(tb testing.TB, getter func() (gitCommitsGetter, error)) []*gi
commits = append(commits, resp.GetCommits()...)
}
}
-
-func gitalyOrPraefect(gitalyMsg, praefectMsg string) string {
- if testhelper.IsPraefectEnabled() {
- return praefectMsg
- }
- return gitalyMsg
-}
diff --git a/internal/gitaly/service/commit/tree_entries.go b/internal/gitaly/service/commit/tree_entries.go
index 5549631a0..1834980e0 100644
--- a/internal/gitaly/service/commit/tree_entries.go
+++ b/internal/gitaly/service/commit/tree_entries.go
@@ -10,6 +10,7 @@ import (
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
log "github.com/sirupsen/logrus"
+ 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/git/localrepo"
@@ -27,6 +28,9 @@ const (
)
func validateGetTreeEntriesRequest(in *gitalypb.GetTreeEntriesRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevision(in.Revision); err != nil {
return err
}
diff --git a/internal/gitaly/service/commit/tree_entries_test.go b/internal/gitaly/service/commit/tree_entries_test.go
index 0b0cc2270..261b5f638 100644
--- a/internal/gitaly/service/commit/tree_entries_test.go
+++ b/internal/gitaly/service/commit/tree_entries_test.go
@@ -707,7 +707,7 @@ func TestGetTreeEntries_validation(t *testing.T) {
Revision: revision,
Path: path,
},
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
"GetStorageByName: no such storage: \"fake\"",
"repo scoped: invalid Repository",
)),
@@ -719,8 +719,8 @@ func TestGetTreeEntries_validation(t *testing.T) {
Revision: revision,
Path: path,
},
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
- "GetStorageByName: no such storage: \"\"",
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
+ "TreeEntry: empty Repository",
"repo scoped: empty Repository",
)),
},
diff --git a/internal/gitaly/service/commit/tree_entry.go b/internal/gitaly/service/commit/tree_entry.go
index 4ab6afb3a..c3216c15f 100644
--- a/internal/gitaly/service/commit/tree_entry.go
+++ b/internal/gitaly/service/commit/tree_entry.go
@@ -5,6 +5,7 @@ import (
"io"
"strings"
+ 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"
@@ -148,6 +149,9 @@ func (s *server) TreeEntry(in *gitalypb.TreeEntryRequest, stream gitalypb.Commit
}
func validateRequest(in *gitalypb.TreeEntryRequest) error {
+ if in.GetRepository() == nil {
+ return gitalyerrors.ErrEmptyRepository
+ }
if err := git.ValidateRevision(in.Revision); err != nil {
return err
}
diff --git a/internal/gitaly/service/commit/tree_entry_test.go b/internal/gitaly/service/commit/tree_entry_test.go
index ead25eabc..649c09f61 100644
--- a/internal/gitaly/service/commit/tree_entry_test.go
+++ b/internal/gitaly/service/commit/tree_entry_test.go
@@ -168,7 +168,7 @@ func TestFailedTreeEntry(t *testing.T) {
{
name: "Repository doesn't exist",
req: &gitalypb.TreeEntryRequest{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision, Path: path},
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
"GetStorageByName: no such storage: \"fake\"",
"repo scoped: invalid Repository",
)),
@@ -176,8 +176,8 @@ func TestFailedTreeEntry(t *testing.T) {
{
name: "Repository is nil",
req: &gitalypb.TreeEntryRequest{Repository: nil, Revision: revision, Path: path},
- expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect(
- "GetStorageByName: no such storage: \"\"",
+ expectedErr: helper.ErrInvalidArgumentf(testhelper.GitalyOrPraefect(
+ "TreeEntry: empty Repository",
"repo scoped: empty Repository",
)),
},