Make wiki commit fields backwards compatible

5 files changed, 178 insertions, 192 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2d76c573e..17f178b47 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 UNRELEASED
 
+- Make wiki commit fields backwards compatible
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/685
 - Catch CommitErrors while rebasing
   https://gitlab.com/gitlab-org/gitaly/merge_requests/680
 
diff --git a/internal/service/wiki/delete_page_test.go b/internal/service/wiki/delete_page_test.go
index 90ea91713..ec9af3199 100644
--- a/internal/service/wiki/delete_page_test.go
+++ b/internal/service/wiki/delete_page_test.go
@@ -33,30 +33,54 @@ func TestSuccessfulWikiDeletePageRequest(t *testing.T) {
 	message := []byte("Delete " + pageName)
 	content := []byte("It was the best of wikis, it was the worst of wikis")
 
-	writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: pageName, content: content})
-
-	request := &pb.WikiDeletePageRequest{
-		Repository: wikiRepo,
-		PagePath:   []byte("a-talé-of-two-wikis"),
-		CommitDetails: &pb.WikiCommitDetails{
-			Name:     authorName,
-			Email:    authorEmail,
-			Message:  message,
-			UserId:   authorID,
-			UserName: authorUserName,
+	testCases := []struct {
+		desc string
+		req  *pb.WikiDeletePageRequest
+	}{
+		{
+			desc: "with user id and username",
+			req: &pb.WikiDeletePageRequest{
+				Repository: wikiRepo,
+				PagePath:   []byte("a-talé-of-two-wikis"),
+				CommitDetails: &pb.WikiCommitDetails{
+					Name:     authorName,
+					Email:    authorEmail,
+					Message:  message,
+					UserId:   authorID,
+					UserName: authorUserName,
+				},
+			},
+		},
+		{
+			desc: "without user id and username", // deprecate in GitLab 11.0 https://gitlab.com/gitlab-org/gitaly/issues/1154
+			req: &pb.WikiDeletePageRequest{
+				Repository: wikiRepo,
+				PagePath:   []byte("a-talé-of-two-wikis"),
+				CommitDetails: &pb.WikiCommitDetails{
+					Name:    authorName,
+					Email:   authorEmail,
+					Message: message,
+				},
+			},
 		},
 	}
 
-	_, err := client.WikiDeletePage(ctx, request)
-	require.NoError(t, err)
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: pageName, content: content})
 
-	headID := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "show", "--format=format:%H", "--no-patch", "HEAD")
-	commit, err := gitlog.GetCommit(ctx, wikiRepo, string(headID), "")
-	require.NoError(t, err, "look up git commit after deleting a wiki page")
+			_, err := client.WikiDeletePage(ctx, tc.req)
+			require.NoError(t, err)
 
-	require.Equal(t, authorName, commit.Author.Name, "author name mismatched")
-	require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
-	require.Equal(t, message, commit.Subject, "message mismatched")
+			headID := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "show", "--format=format:%H", "--no-patch", "HEAD")
+			commit, err := gitlog.GetCommit(ctx, wikiRepo, string(headID), "")
+			require.NoError(t, err, "look up git commit after deleting a wiki page")
+
+			require.Equal(t, authorName, commit.Author.Name, "author name mismatched")
+			require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
+			require.Equal(t, message, commit.Subject, "message mismatched")
+		})
+	}
 }
 
 func TestFailedWikiDeletePageDueToValidations(t *testing.T) {
@@ -149,34 +173,6 @@ func TestFailedWikiDeletePageDueToValidations(t *testing.T) {
 			},
 			code: codes.InvalidArgument,
 		},
-		{
-			desc: "empty commit details' user id",
-			request: &pb.WikiDeletePageRequest{
-				Repository: wikiRepo,
-				PagePath:   []byte("does-not-matter"),
-				CommitDetails: &pb.WikiCommitDetails{
-					Name:     []byte("A name"),
-					Email:    []byte("a@b.com"),
-					Message:  []byte("A message"),
-					UserName: []byte("username"),
-				},
-			},
-			code: codes.InvalidArgument,
-		},
-		{
-			desc: "empty commit details' username",
-			request: &pb.WikiDeletePageRequest{
-				Repository: wikiRepo,
-				PagePath:   []byte("does-not-matter"),
-				CommitDetails: &pb.WikiCommitDetails{
-					Name:    []byte("A name"),
-					Email:   []byte("a@b.com"),
-					Message: []byte("A message"),
-					UserId:  int32(1),
-				},
-			},
-			code: codes.InvalidArgument,
-		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/wiki/update_page_test.go b/internal/service/wiki/update_page_test.go
index 74db3f5c2..79fcf28ba 100644
--- a/internal/service/wiki/update_page_test.go
+++ b/internal/service/wiki/update_page_test.go
@@ -28,60 +28,88 @@ func TestSuccessfulWikiUpdatePageRequest(t *testing.T) {
 
 	writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: "Instálling Gitaly", content: []byte("foobar")})
 
-	content := bytes.Repeat([]byte("Mock wiki page content"), 10000)
-
 	authorID := int32(1)
 	authorUserName := []byte("ahmad")
 	authorName := []byte("Ahmad Sherif")
 	authorEmail := []byte("ahmad@gitlab.com")
 	message := []byte("Add installation instructions")
 
-	request := &pb.WikiUpdatePageRequest{
-		Repository: wikiRepo,
-		PagePath:   []byte("//Instálling Gitaly"),
-		Title:      []byte("Instálling Gitaly"),
-		Format:     "markdown",
-		CommitDetails: &pb.WikiCommitDetails{
-			Name:     authorName,
-			Email:    authorEmail,
-			Message:  message,
-			UserId:   authorID,
-			UserName: authorUserName,
+	testCases := []struct {
+		desc    string
+		req     *pb.WikiUpdatePageRequest
+		content []byte
+	}{
+		{
+			desc: "with user id and username",
+			req: &pb.WikiUpdatePageRequest{
+				Repository: wikiRepo,
+				PagePath:   []byte("//Instálling Gitaly"),
+				Title:      []byte("Instálling Gitaly"),
+				Format:     "markdown",
+				CommitDetails: &pb.WikiCommitDetails{
+					Name:     authorName,
+					Email:    authorEmail,
+					Message:  message,
+					UserId:   authorID,
+					UserName: authorUserName,
+				},
+			},
+			content: bytes.Repeat([]byte("Mock wiki page content"), 10000),
+		},
+		{
+			desc: "without user id and username", // deprecate in gitlab 11.0 https://gitlab.com/gitlab-org/gitaly/issues/1154
+			req: &pb.WikiUpdatePageRequest{
+				Repository: wikiRepo,
+				PagePath:   []byte("//Instálling Gitaly"),
+				Title:      []byte("Instálling Gitaly"),
+				Format:     "markdown",
+				CommitDetails: &pb.WikiCommitDetails{
+					Name:    authorName,
+					Email:   authorEmail,
+					Message: message,
+				},
+			},
+			content: bytes.Repeat([]byte("Mock wiki page content 2"), 10000),
 		},
 	}
 
-	stream, err := client.WikiUpdatePage(ctx)
-	require.NoError(t, err)
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			stream, err := client.WikiUpdatePage(ctx)
+			require.NoError(t, err)
 
-	nSends, err := sendBytes(content, 1000, func(p []byte) error {
-		request.Content = p
+			request := tc.req
+			nSends, err := sendBytes(tc.content, 1000, func(p []byte) error {
+				request.Content = p
 
-		if err := stream.Send(request); err != nil {
-			return err
-		}
+				if err := stream.Send(request); err != nil {
+					return err
+				}
 
-		// Use a new response so we don't send other fields (Repository, ...) over and over
-		request = &pb.WikiUpdatePageRequest{}
+				// Use a new response so we don't send other fields (Repository, ...) over and over
+				request = &pb.WikiUpdatePageRequest{}
 
-		return nil
-	})
+				return nil
+			})
 
-	require.NoError(t, err)
-	require.True(t, nSends > 1, "should have sent more than one message")
+			require.NoError(t, err)
+			require.True(t, nSends > 1, "should have sent more than one message")
 
-	_, err = stream.CloseAndRecv()
-	require.NoError(t, err)
+			_, err = stream.CloseAndRecv()
+			require.NoError(t, err)
 
-	headID := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "show", "--format=format:%H", "--no-patch", "HEAD")
-	commit, err := gitlog.GetCommit(ctx, wikiRepo, string(headID), "")
-	require.NoError(t, err, "look up git commit before merge is applied")
+			headID := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "show", "--format=format:%H", "--no-patch", "HEAD")
+			commit, err := gitlog.GetCommit(ctx, wikiRepo, string(headID), "")
+			require.NoError(t, err, "look up git commit before merge is applied")
 
-	require.Equal(t, authorName, commit.Author.Name, "author name mismatched")
-	require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
-	require.Equal(t, message, commit.Subject, "message mismatched")
+			require.Equal(t, authorName, commit.Author.Name, "author name mismatched")
+			require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
+			require.Equal(t, message, commit.Subject, "message mismatched")
 
-	pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:Instálling-Gitaly.md")
-	require.Equal(t, content, pageContent, "mismatched content")
+			pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:Instálling-Gitaly.md")
+			require.Equal(t, tc.content, pageContent, "mismatched content")
+		})
+	}
 }
 
 func TestFailedWikiUpdatePageDueToValidations(t *testing.T) {
@@ -204,40 +232,6 @@ func TestFailedWikiUpdatePageDueToValidations(t *testing.T) {
 			},
 			code: codes.InvalidArgument,
 		},
-		{
-			desc: "empty commit details' user id",
-			request: &pb.WikiUpdatePageRequest{
-				Repository: wikiRepo,
-				PagePath:   []byte("//Installing Gitaly.md"),
-				Title:      []byte("Installing Gitaly"),
-				Format:     "markdown",
-				CommitDetails: &pb.WikiCommitDetails{
-					Name:     []byte("A name"),
-					Email:    []byte("a@b.com"),
-					Message:  []byte("A message"),
-					UserName: []byte("username"),
-				},
-				Content: []byte(""),
-			},
-			code: codes.InvalidArgument,
-		},
-		{
-			desc: "empty commit details' username",
-			request: &pb.WikiUpdatePageRequest{
-				Repository: wikiRepo,
-				PagePath:   []byte("//Installing Gitaly.md"),
-				Title:      []byte("Installing Gitaly"),
-				Format:     "markdown",
-				CommitDetails: &pb.WikiCommitDetails{
-					Name:    []byte("A name"),
-					Email:   []byte("a@b.com"),
-					Message: []byte("A message"),
-					UserId:  int32(1),
-				},
-				Content: []byte(""),
-			},
-			code: codes.InvalidArgument,
-		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/wiki/util.go b/internal/service/wiki/util.go
index 5c18ca625..6ab3ee446 100644
--- a/internal/service/wiki/util.go
+++ b/internal/service/wiki/util.go
@@ -16,14 +16,6 @@ func validateRequestCommitDetails(request requestWithCommitDetails) error {
 		return fmt.Errorf("empty CommitDetails")
 	}
 
-	if commitDetails.GetUserId() == 0 {
-		return fmt.Errorf("empty CommitDetails.UserId")
-	}
-
-	if len(commitDetails.GetUserName()) == 0 {
-		return fmt.Errorf("empty CommitDetails.UserName")
-	}
-
 	if len(commitDetails.GetName()) == 0 {
 		return fmt.Errorf("empty CommitDetails.Name")
 	}
diff --git a/internal/service/wiki/write_page_test.go b/internal/service/wiki/write_page_test.go
index 54542ede5..abb471b4e 100644
--- a/internal/service/wiki/write_page_test.go
+++ b/internal/service/wiki/write_page_test.go
@@ -26,59 +26,93 @@ func TestSuccessfulWikiWritePageRequest(t *testing.T) {
 	client, conn := newWikiClient(t, serverSocketPath)
 	defer conn.Close()
 
-	content := bytes.Repeat([]byte("Mock wiki page content"), 10000)
-
 	authorID := int32(1)
 	authorUserName := []byte("ahmad")
 	authorName := []byte("Ahmad Sherif")
 	authorEmail := []byte("ahmad@gitlab.com")
 	message := []byte("Add installation instructions")
 
-	request := &pb.WikiWritePageRequest{
-		Repository: wikiRepo,
-		Name:       []byte("Instálling Gitaly"),
-		Format:     "markdown",
-		CommitDetails: &pb.WikiCommitDetails{
-			Name:     authorName,
-			Email:    authorEmail,
-			Message:  message,
-			UserId:   authorID,
-			UserName: authorUserName,
+	testCases := []struct {
+		desc       string
+		req        *pb.WikiWritePageRequest
+		gollumPath string
+		content    []byte
+	}{
+		{
+			desc: "with user id and username",
+			req: &pb.WikiWritePageRequest{
+				Repository: wikiRepo,
+				Name:       []byte("Instálling Gitaly"),
+				Format:     "markdown",
+				CommitDetails: &pb.WikiCommitDetails{
+					Name:     authorName,
+					Email:    authorEmail,
+					Message:  message,
+					UserId:   authorID,
+					UserName: authorUserName,
+				},
+			},
+			gollumPath: "Instálling-Gitaly.md",
+			content:    bytes.Repeat([]byte("Mock wiki page content"), 10000),
+		},
+		{
+			desc: "without user id and username", // deprecate in gitlab 11.0 https://gitlab.com/gitlab-org/gitaly/issues/1154
+			req: &pb.WikiWritePageRequest{
+				Repository: wikiRepo,
+				Name:       []byte("Instálling Gitaly 2"),
+				Format:     "markdown",
+				CommitDetails: &pb.WikiCommitDetails{
+					Name:     authorName,
+					Email:    authorEmail,
+					Message:  message,
+					UserId:   authorID,
+					UserName: authorUserName,
+				},
+			},
+			gollumPath: "Instálling-Gitaly-2.md",
+			content:    bytes.Repeat([]byte("Mock wiki page content 2"), 10000),
 		},
 	}
 
-	stream, err := client.WikiWritePage(ctx)
-	require.NoError(t, err)
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			stream, err := client.WikiWritePage(ctx)
+			require.NoError(t, err)
 
-	nSends, err := sendBytes(content, 1000, func(p []byte) error {
-		request.Content = p
+			request := tc.req
+			nSends, err := sendBytes(tc.content, 1000, func(p []byte) error {
+				request.Content = p
 
-		if err := stream.Send(request); err != nil {
-			return err
-		}
+				if err := stream.Send(request); err != nil {
+					return err
+				}
 
-		// Use a new response so we don't send other fields (Repository, ...) over and over
-		request = &pb.WikiWritePageRequest{}
+				// Use a new response so we don't send other fields (Repository, ...) over and over
+				request = &pb.WikiWritePageRequest{}
 
-		return nil
-	})
+				return nil
+			})
 
-	require.NoError(t, err)
-	require.True(t, nSends > 1, "should have sent more than one message")
+			require.NoError(t, err)
+			require.True(t, nSends > 1, "should have sent more than one message")
 
-	_, err = stream.CloseAndRecv()
-	require.NoError(t, err)
+			resp, err := stream.CloseAndRecv()
+			require.NoError(t, err)
+
+			require.Empty(t, resp.DuplicateError, "DuplicateError must be empty")
 
-	headID := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "show", "--format=format:%H", "--no-patch", "HEAD")
-	commit, err := gitlog.GetCommit(ctx, wikiRepo, string(headID), "")
-	require.NoError(t, err, "look up git commit after writing a wiki page")
+			headID := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "show", "--format=format:%H", "--no-patch", "HEAD")
+			commit, err := gitlog.GetCommit(ctx, wikiRepo, string(headID), "")
+			require.NoError(t, err, "look up git commit after writing a wiki page")
 
-	require.Equal(t, authorName, commit.Author.Name, "author name mismatched")
-	require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
-	require.Equal(t, message, commit.Subject, "message mismatched")
+			require.Equal(t, authorName, commit.Author.Name, "author name mismatched")
+			require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
+			require.Equal(t, message, commit.Subject, "message mismatched")
 
-	pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:Instálling-Gitaly.md")
-	require.Equal(t, content, pageContent, "mismatched content")
+			pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:"+tc.gollumPath)
+			require.Equal(t, tc.content, pageContent, "mismatched content")
+		})
+	}
 }
 
 func TestFailedWikiWritePageDueToDuplicatePage(t *testing.T) {
@@ -271,38 +305,6 @@ func TestFailedWikiWritePageDueToValidations(t *testing.T) {
 			},
 			code: codes.InvalidArgument,
 		},
-		{
-			desc: "empty commit details' user id",
-			request: &pb.WikiWritePageRequest{
-				Repository: wikiRepo,
-				Name:       []byte("Installing Gitaly"),
-				Format:     "markdown",
-				CommitDetails: &pb.WikiCommitDetails{
-					Name:     []byte("A name"),
-					Email:    []byte("a@b.com"),
-					Message:  []byte("A message"),
-					UserName: []byte("username"),
-				},
-				Content: []byte(""),
-			},
-			code: codes.InvalidArgument,
-		},
-		{
-			desc: "empty commit details' username",
-			request: &pb.WikiWritePageRequest{
-				Repository: wikiRepo,
-				Name:       []byte("Installing Gitaly"),
-				Format:     "markdown",
-				CommitDetails: &pb.WikiCommitDetails{
-					Name:    []byte("A name"),
-					Email:   []byte("a@b.com"),
-					Message: []byte("A message"),
-					UserId:  int32(1),
-				},
-				Content: []byte(""),
-			},
-			code: codes.InvalidArgument,
-		},
 	}
 
 	for _, testCase := range testCases {