service/diff: 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.
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.

6 files changed, 93 insertions, 31 deletions

diff --git a/internal/gitaly/service/diff/commit.go b/internal/gitaly/service/diff/commit.go
index de9258115..8c718dcc4 100644
--- a/internal/gitaly/service/diff/commit.go
+++ b/internal/gitaly/service/diff/commit.go
@@ -6,6 +6,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/gitaly/diff"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -14,6 +15,7 @@ import (
 )
 
 type requestWithLeftRightCommitIds interface {
+	GetRepository() *gitalypb.Repository
 	GetLeftCommitId() string
 	GetRightCommitId() string
 }
@@ -197,6 +199,9 @@ func (s *server) CommitDelta(in *gitalypb.CommitDeltaRequest, stream gitalypb.Di
 }
 
 func validateRequest(in requestWithLeftRightCommitIds) error {
+	if in.GetRepository() == nil {
+		return gitalyerrors.ErrEmptyRepository
+	}
 	if in.GetLeftCommitId() == "" {
 		return fmt.Errorf("empty LeftCommitId")
 	}
diff --git a/internal/gitaly/service/diff/commit_test.go b/internal/gitaly/service/diff/commit_test.go
index 90b7bedb3..18edfa676 100644
--- a/internal/gitaly/service/diff/commit_test.go
+++ b/internal/gitaly/service/diff/commit_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"
 )
 
 func TestSuccessfulCommitDiffRequest(t *testing.T) {
@@ -793,23 +794,47 @@ func TestFailedCommitDiffRequestDueToValidationError(t *testing.T) {
 	ctx := testhelper.Context(t)
 	_, repo, _, client := setupDiffService(t, ctx)
 
-	rightCommit := "d42783470dc29fde2cf459eb3199ee1d7e3f3a72"
-	leftCommit := rightCommit + "~" // Parent of rightCommit
+	const rightCommit = "d42783470dc29fde2cf459eb3199ee1d7e3f3a72"
+	const leftCommit = rightCommit + "~" // Parent of rightCommit
 
-	rpcRequests := []*gitalypb.CommitDiffRequest{
-		{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, RightCommitId: rightCommit, LeftCommitId: leftCommit}, // Repository doesn't exist
-		{Repository: nil, RightCommitId: rightCommit, LeftCommitId: leftCommit},                                                             // Repository is nil
-		{Repository: repo, RightCommitId: "", LeftCommitId: leftCommit},                                                                     // RightCommitId is empty
-		{Repository: repo, RightCommitId: rightCommit, LeftCommitId: ""},                                                                    // LeftCommitId is empty
-	}
-
-	for _, rpcRequest := range rpcRequests {
-		t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) {
-			c, err := client.CommitDiff(ctx, rpcRequest)
+	for _, tc := range []struct {
+		desc   string
+		req    *gitalypb.CommitDiffRequest
+		exrErr error
+	}{
+		{
+			desc: "Repository doesn't exist",
+			req:  &gitalypb.CommitDiffRequest{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, RightCommitId: rightCommit, LeftCommitId: leftCommit},
+			exrErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				`GetStorageByName: no such storage: "fake"`,
+				"repo scoped: invalid Repository",
+			)),
+		},
+		{
+			desc: "Repository is nil",
+			req:  &gitalypb.CommitDiffRequest{Repository: nil, RightCommitId: rightCommit, LeftCommitId: leftCommit},
+			exrErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"CommitDiff: empty Repository",
+				"repo scoped: empty Repository",
+			)),
+		},
+		{
+			desc:   "RightCommitId is empty",
+			req:    &gitalypb.CommitDiffRequest{Repository: repo, RightCommitId: "", LeftCommitId: leftCommit},
+			exrErr: status.Error(codes.InvalidArgument, "CommitDiff: empty RightCommitId"),
+		},
+		{
+			desc:   "LeftCommitId is empty",
+			req:    &gitalypb.CommitDiffRequest{Repository: repo, RightCommitId: rightCommit, LeftCommitId: ""},
+			exrErr: status.Error(codes.InvalidArgument, "CommitDiff: empty LeftCommitId"),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			c, err := client.CommitDiff(ctx, tc.req)
 			require.NoError(t, err)
 
 			err = drainCommitDiffResponse(c)
-			testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+			testhelper.RequireGrpcError(t, tc.exrErr, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/diff/find_changed_paths.go b/internal/gitaly/service/diff/find_changed_paths.go
index 93016168a..7420f9ab2 100644
--- a/internal/gitaly/service/diff/find_changed_paths.go
+++ b/internal/gitaly/service/diff/find_changed_paths.go
@@ -10,6 +10,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/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -195,6 +196,9 @@ func resolveObjectWithType(ctx context.Context, repo *localrepo.Repo, revision s
 
 func (s *server) validateFindChangedPathsRequestParams(ctx context.Context, in *gitalypb.FindChangedPathsRequest) error {
 	repo := in.GetRepository()
+	if repo == nil {
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
+	}
 	if _, err := s.locator.GetRepoPath(repo); err != nil {
 		return err
 	}
diff --git a/internal/gitaly/service/diff/find_changed_paths_test.go b/internal/gitaly/service/diff/find_changed_paths_test.go
index 997cd656a..761e126f5 100644
--- a/internal/gitaly/service/diff/find_changed_paths_test.go
+++ b/internal/gitaly/service/diff/find_changed_paths_test.go
@@ -12,6 +12,8 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func TestFindChangedPathsRequest_success(t *testing.T) {
@@ -478,6 +480,12 @@ func TestFindChangedPathsRequest_failing(t *testing.T) {
 		err      error
 	}{
 		{
+			desc:    "Repository not provided",
+			repo:    nil,
+			commits: []string{"e4003da16c1c2c3fc4567700121b17bf8e591c6c", "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab"},
+			err:     status.Error(codes.InvalidArgument, "empty Repository"),
+		},
+		{
 			desc:    "Repo not found",
 			repo:    &gitalypb.Repository{StorageName: repo.GetStorageName(), RelativePath: "bar.git"},
 			commits: []string{"e4003da16c1c2c3fc4567700121b17bf8e591c6c", "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab"},
diff --git a/internal/gitaly/service/diff/numstat.go b/internal/gitaly/service/diff/numstat.go
index db6442ed0..ed9290ea2 100644
--- a/internal/gitaly/service/diff/numstat.go
+++ b/internal/gitaly/service/diff/numstat.go
@@ -3,8 +3,10 @@ package diff
 import (
 	"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/gitaly/diff"
+	"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"
@@ -81,6 +83,9 @@ func sendStats(batch []*gitalypb.DiffStats, stream gitalypb.DiffService_DiffStat
 
 func (s *server) validateDiffStatsRequestParams(in *gitalypb.DiffStatsRequest) error {
 	repo := in.GetRepository()
+	if repo == nil {
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
+	}
 	if _, err := s.locator.GetRepoPath(repo); err != nil {
 		return err
 	}
diff --git a/internal/gitaly/service/diff/numstat_test.go b/internal/gitaly/service/diff/numstat_test.go
index a43dc23cb..74764927b 100644
--- a/internal/gitaly/service/diff/numstat_test.go
+++ b/internal/gitaly/service/diff/numstat_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"
 )
 
 func TestSuccessfulDiffStatsRequest(t *testing.T) {
@@ -119,82 +120,96 @@ func TestSuccessfulDiffStatsRequest(t *testing.T) {
 
 func TestFailedDiffStatsRequest(t *testing.T) {
 	ctx := testhelper.Context(t)
-	_, repo, _, client := setupDiffService(t, ctx)
+	cfg, repo, _, client := setupDiffService(t, ctx)
 
 	tests := []struct {
 		desc          string
 		repo          *gitalypb.Repository
 		leftCommitID  string
 		rightCommitID string
-		err           codes.Code
+		expectedErr   error
 	}{
 		{
-			desc:          "repo not found",
+			desc:          "repository not provided",
+			repo:          nil,
+			leftCommitID:  "e4003da16c1c2c3fc4567700121b17bf8e591c6c",
+			rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"empty Repository",
+				"repo scoped: empty Repository",
+			)),
+		},
+		{
+			desc:          "repository not found",
 			repo:          &gitalypb.Repository{StorageName: repo.GetStorageName(), RelativePath: "bar.git"},
 			leftCommitID:  "e4003da16c1c2c3fc4567700121b17bf8e591c6c",
 			rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
-			err:           codes.NotFound,
+			expectedErr: status.Error(codes.NotFound, testhelper.GitalyOrPraefect(
+				`GetRepoPath: not a git repository: "`+cfg.Storages[0].Path+`/bar.git"`,
+				`accessor call: route repository accessor: consistent storages: repository "default"/"bar.git" not found`,
+			)),
 		},
 		{
 			desc:          "storage not found",
 			repo:          &gitalypb.Repository{StorageName: "foo", RelativePath: "bar.git"},
 			leftCommitID:  "e4003da16c1c2c3fc4567700121b17bf8e591c6c",
 			rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
-			err:           codes.InvalidArgument,
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				`GetStorageByName: no such storage: "foo"`,
+				"repo scoped: invalid Repository",
+			)),
 		},
 		{
 			desc:          "left commit ID not found",
 			repo:          repo,
 			leftCommitID:  "",
 			rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
-			err:           codes.InvalidArgument,
+			expectedErr:   status.Error(codes.InvalidArgument, "DiffStats: empty LeftCommitId"),
 		},
 		{
 			desc:          "right commit ID not found",
 			repo:          repo,
 			leftCommitID:  "e4003da16c1c2c3fc4567700121b17bf8e591c6c",
 			rightCommitID: "",
-			err:           codes.InvalidArgument,
+			expectedErr:   status.Error(codes.InvalidArgument, "DiffStats: empty RightCommitId"),
 		},
 		{
 			desc:          "invalid left commit",
 			repo:          repo,
 			leftCommitID:  "invalidinvalidinvalid",
 			rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
-			err:           codes.Unavailable,
+			expectedErr:   status.Error(codes.Unavailable, "DiffStats: exit status 128"),
 		},
 		{
 			desc:          "invalid right commit",
 			repo:          repo,
 			leftCommitID:  "e4003da16c1c2c3fc4567700121b17bf8e591c6c",
 			rightCommitID: "invalidinvalidinvalid",
-			err:           codes.Unavailable,
+			expectedErr:   status.Error(codes.Unavailable, "DiffStats: exit status 128"),
 		},
 		{
 			desc:          "left commit not found",
 			repo:          repo,
 			leftCommitID:  "z4003da16c1c2c3fc4567700121b17bf8e591c6c",
 			rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab",
-			err:           codes.Unavailable,
+			expectedErr:   status.Error(codes.Unavailable, "DiffStats: exit status 128"),
 		},
 		{
 			desc:          "right commit not found",
 			repo:          repo,
 			leftCommitID:  "e4003da16c1c2c3fc4567700121b17bf8e591c6c",
 			rightCommitID: "z4003da16c1c2c3fc4567700121b17bf8e591c6c",
-			err:           codes.Unavailable,
+			expectedErr:   status.Error(codes.Unavailable, "DiffStats: exit status 128"),
 		},
 	}
 
 	for _, tc := range tests {
-		rpcRequest := &gitalypb.DiffStatsRequest{Repository: tc.repo, RightCommitId: tc.rightCommitID, LeftCommitId: tc.leftCommitID}
-		stream, err := client.DiffStats(ctx, rpcRequest)
-		require.NoError(t, err)
-
 		t.Run(tc.desc, func(t *testing.T) {
-			_, err := stream.Recv()
-
-			testhelper.RequireGrpcCode(t, err, tc.err)
+			rpcRequest := &gitalypb.DiffStatsRequest{Repository: tc.repo, RightCommitId: tc.rightCommitID, LeftCommitId: tc.leftCommitID}
+			stream, err := client.DiffStats(ctx, rpcRequest)
+			require.NoError(t, err)
+			_, err = stream.Recv()
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
 		})
 	}
 }