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

4 files changed, 30 insertions, 34 deletions

diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go
index c0316e5eb..427603524 100644
--- a/internal/gitaly/service/conflicts/list_conflict_files.go
+++ b/internal/gitaly/service/conflicts/list_conflict_files.go
@@ -7,6 +7,7 @@ import (
 	"io"
 	"unicode/utf8"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -127,7 +128,7 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s
 
 func validateListConflictFilesRequest(in *gitalypb.ListConflictFilesRequest) error {
 	if in.GetRepository() == nil {
-		return fmt.Errorf("empty Repository")
+		return gitalyerrors.ErrEmptyRepository
 	}
 	if in.GetOurCommitOid() == "" {
 		return fmt.Errorf("empty OurCommitOid")
diff --git a/internal/gitaly/service/conflicts/list_conflict_files_test.go b/internal/gitaly/service/conflicts/list_conflict_files_test.go
index 64e67216b..a64701786 100644
--- a/internal/gitaly/service/conflicts/list_conflict_files_test.go
+++ b/internal/gitaly/service/conflicts/list_conflict_files_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"
 )
 
 type conflictFile struct {
@@ -328,9 +329,9 @@ func TestFailedListConflictFilesRequestDueToValidation(t *testing.T) {
 	theirCommitOid := "bb5206fee213d983da88c47f9cf4cc6caf9c66dc"
 
 	testCases := []struct {
-		desc    string
-		request *gitalypb.ListConflictFilesRequest
-		code    codes.Code
+		desc        string
+		request     *gitalypb.ListConflictFilesRequest
+		expectedErr error
 	}{
 		{
 			desc: "empty repo",
@@ -339,7 +340,10 @@ func TestFailedListConflictFilesRequestDueToValidation(t *testing.T) {
 				OurCommitOid:   ourCommitOid,
 				TheirCommitOid: theirCommitOid,
 			},
-			code: codes.InvalidArgument,
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"empty Repository",
+				"repo scoped: empty Repository",
+			)),
 		},
 		{
 			desc: "empty OurCommitId field",
@@ -348,7 +352,7 @@ func TestFailedListConflictFilesRequestDueToValidation(t *testing.T) {
 				OurCommitOid:   "",
 				TheirCommitOid: theirCommitOid,
 			},
-			code: codes.InvalidArgument,
+			expectedErr: status.Error(codes.InvalidArgument, "empty OurCommitOid"),
 		},
 		{
 			desc: "empty TheirCommitId field",
@@ -357,14 +361,14 @@ func TestFailedListConflictFilesRequestDueToValidation(t *testing.T) {
 				OurCommitOid:   ourCommitOid,
 				TheirCommitOid: "",
 			},
-			code: codes.InvalidArgument,
+			expectedErr: status.Error(codes.InvalidArgument, "empty TheirCommitOid"),
 		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			c, _ := client.ListConflictFiles(ctx, testCase.request)
-			testhelper.RequireGrpcCode(t, drainListConflictFilesResponse(c), testCase.code)
+			testhelper.RequireGrpcError(t, testCase.expectedErr, drainListConflictFilesResponse(c))
 		})
 	}
 }
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go
index 11de0412f..72ed7f94e 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts.go
@@ -12,6 +12,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/internal/git/conflict"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
@@ -80,7 +81,7 @@ func validateResolveConflictsHeader(header *gitalypb.ResolveConflictsRequestHead
 		return fmt.Errorf("empty OurCommitOid")
 	}
 	if header.GetRepository() == nil {
-		return fmt.Errorf("empty Repository")
+		return gitalyerrors.ErrEmptyRepository
 	}
 	if header.GetTargetRepository() == nil {
 		return fmt.Errorf("empty TargetRepository")
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts_test.go b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
index f4374784c..1d31fb785 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts_test.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
@@ -662,10 +662,9 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 	targetBranch := []byte("conflict-start")
 
 	testCases := []struct {
-		desc         string
-		header       *gitalypb.ResolveConflictsRequestHeader
-		expectedCode codes.Code
-		expectedErr  string
+		desc        string
+		header      *gitalypb.ResolveConflictsRequestHeader
+		expectedErr error
 	}{
 		{
 			desc: "empty user",
@@ -679,8 +678,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     sourceBranch,
 				TargetBranch:     targetBranch,
 			},
-			expectedCode: codes.InvalidArgument,
-			expectedErr:  "ResolveConflicts: empty User",
+			expectedErr: status.Error(codes.InvalidArgument, "ResolveConflicts: empty User"),
 		},
 		{
 			desc: "empty repo",
@@ -694,11 +692,10 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     sourceBranch,
 				TargetBranch:     targetBranch,
 			},
-			expectedCode: codes.InvalidArgument,
-			// Praefect checks for an empty repository, too, but will raise a different
-			// error message. Luckily, both Gitaly's and Praefect's error messages
-			// contain "empty Repository".
-			expectedErr: "empty Repository",
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"ResolveConflicts: empty Repository",
+				"repo scoped: empty Repository",
+			)),
 		},
 		{
 			desc: "empty target repo",
@@ -712,8 +709,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     sourceBranch,
 				TargetBranch:     targetBranch,
 			},
-			expectedCode: codes.InvalidArgument,
-			expectedErr:  "ResolveConflicts: empty TargetRepository",
+			expectedErr: status.Error(codes.InvalidArgument, "ResolveConflicts: empty TargetRepository"),
 		},
 		{
 			desc: "empty OurCommitId repo",
@@ -727,8 +723,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     sourceBranch,
 				TargetBranch:     targetBranch,
 			},
-			expectedCode: codes.InvalidArgument,
-			expectedErr:  "ResolveConflicts: empty OurCommitOid",
+			expectedErr: status.Error(codes.InvalidArgument, "ResolveConflicts: empty OurCommitOid"),
 		},
 		{
 			desc: "empty TheirCommitId repo",
@@ -742,8 +737,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     sourceBranch,
 				TargetBranch:     targetBranch,
 			},
-			expectedCode: codes.InvalidArgument,
-			expectedErr:  "ResolveConflicts: empty TheirCommitOid",
+			expectedErr: status.Error(codes.InvalidArgument, "ResolveConflicts: empty TheirCommitOid"),
 		},
 		{
 			desc: "empty CommitMessage repo",
@@ -757,8 +751,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     sourceBranch,
 				TargetBranch:     targetBranch,
 			},
-			expectedCode: codes.InvalidArgument,
-			expectedErr:  "ResolveConflicts: empty CommitMessage",
+			expectedErr: status.Error(codes.InvalidArgument, "ResolveConflicts: empty CommitMessage"),
 		},
 		{
 			desc: "empty SourceBranch repo",
@@ -772,8 +765,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     nil,
 				TargetBranch:     targetBranch,
 			},
-			expectedCode: codes.InvalidArgument,
-			expectedErr:  "ResolveConflicts: empty SourceBranch",
+			expectedErr: status.Error(codes.InvalidArgument, "ResolveConflicts: empty SourceBranch"),
 		},
 		{
 			desc: "empty TargetBranch repo",
@@ -787,8 +779,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 				SourceBranch:     sourceBranch,
 				TargetBranch:     nil,
 			},
-			expectedCode: codes.InvalidArgument,
-			expectedErr:  "ResolveConflicts: empty TargetBranch",
+			expectedErr: status.Error(codes.InvalidArgument, "ResolveConflicts: empty TargetBranch"),
 		},
 	}
 
@@ -808,8 +799,7 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 			require.NoError(t, stream.Send(headerRequest))
 
 			_, err = stream.CloseAndRecv()
-			testhelper.RequireGrpcCode(t, err, testCase.expectedCode)
-			require.Contains(t, err.Error(), testCase.expectedErr)
+			testhelper.RequireGrpcError(t, testCase.expectedErr, err)
 		})
 	}
 }