Merge branch 'fj-fix-wiki-page-operations-content-nil' into 'master'

Fixed bug with wiki operations enumerator when content nil

See merge request gitlab-org/gitaly!962

7 files changed, 132 insertions, 65 deletions

diff --git a/changelogs/unreleased/fj-fix-wiki-page-operations-content-nil.yml b/changelogs/unreleased/fj-fix-wiki-page-operations-content-nil.yml
new file mode 100644
index 000000000..f4d7f8e2e
--- /dev/null
+++ b/changelogs/unreleased/fj-fix-wiki-page-operations-content-nil.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed bug with wiki operations enumerator when content nil
+merge_request: 962
+author:
+type: fixed
diff --git a/internal/service/wiki/find_file_test.go b/internal/service/wiki/find_file_test.go
index 72dd58676..9e750c916 100644
--- a/internal/service/wiki/find_file_test.go
+++ b/internal/service/wiki/find_file_test.go
@@ -40,7 +40,10 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 	content, err := ioutil.ReadFile("testdata/clouds.png")
 	require.NoError(t, err)
 
-	err = ioutil.WriteFile(path.Join(sandboxWikiPath, "cloúds.png"), content, 0777)
+	err = ioutil.WriteFile(path.Join(sandboxWikiPath, "cloúds.png"), content, 0644)
+	require.NoError(t, err)
+
+	err = ioutil.WriteFile(path.Join(sandboxWikiPath, "no_content.png"), nil, 0644)
 	require.NoError(t, err)
 
 	// Sandbox wiki is empty, so we create a commit to be used later
@@ -65,9 +68,10 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 	}
 
 	testCases := []struct {
-		desc     string
-		request  *gitalypb.WikiFindFileRequest
-		response *gitalypb.WikiFindFileResponse
+		desc            string
+		request         *gitalypb.WikiFindFileRequest
+		response        *gitalypb.WikiFindFileResponse
+		expectedContent []byte
 	}{
 		{
 			desc: "name only",
@@ -75,7 +79,8 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 				Repository: sandboxWiki,
 				Name:       []byte("cloúds.png"),
 			},
-			response: response,
+			response:        response,
+			expectedContent: content,
 		},
 		{
 			desc: "name + revision that includes the file",
@@ -84,7 +89,8 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 				Name:       []byte("cloúds.png"),
 				Revision:   newHeadID,
 			},
-			response: response,
+			response:        response,
+			expectedContent: content,
 		},
 		{
 			desc: "name + revision that does not include the file",
@@ -93,7 +99,8 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 				Name:       []byte("cloúds.png"),
 				Revision:   oldHeadID,
 			},
-			response: &gitalypb.WikiFindFileResponse{},
+			response:        &gitalypb.WikiFindFileResponse{},
+			expectedContent: content,
 		},
 		{
 			desc: "non-existent name",
@@ -101,7 +108,21 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 				Repository: sandboxWiki,
 				Name:       []byte("moar-clouds.png"),
 			},
-			response: &gitalypb.WikiFindFileResponse{},
+			response:        &gitalypb.WikiFindFileResponse{},
+			expectedContent: content,
+		},
+		{
+			desc: "file with no content",
+			request: &gitalypb.WikiFindFileRequest{
+				Repository: sandboxWiki,
+				Name:       []byte("no_content.png"),
+			},
+			response: &gitalypb.WikiFindFileResponse{
+				Name:     []byte("no_content.png"),
+				MimeType: "image/png",
+				Path:     []byte("no_content.png"),
+			},
+			expectedContent: nil,
 		},
 	}
 
@@ -125,7 +146,7 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 
 			require.Equal(t, expectedResponse, receivedResponse, "mismatched response")
 			if len(expectedResponse.Name) > 0 {
-				require.Equal(t, content, receivedContent, "mismatched content")
+				require.Equal(t, testCase.expectedContent, receivedContent, "mismatched content")
 			}
 		})
 	}
diff --git a/internal/service/wiki/find_page_test.go b/internal/service/wiki/find_page_test.go
index e0c23cbb4..2db25e290 100644
--- a/internal/service/wiki/find_page_test.go
+++ b/internal/service/wiki/find_page_test.go
@@ -24,11 +24,13 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) {
 	page2Name := "Instálling/Step 133-b"
 	page3Name := "Installing/Step 133-c"
 	page4Name := "Encoding is fun"
+	page5Name := "Empty file"
 	page1Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page1Name})
 	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page2Name})
 	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page3Name})
-	page4Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page4Name, content: []byte("f\xFCr")})
-	latestCommit := page4Commit
+	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page4Name, content: []byte("f\xFCr")})
+	page5Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page5Name, forceContentEmpty: true})
+	latestCommit := page5Commit
 
 	testCases := []struct {
 		desc            string
@@ -136,6 +138,26 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) {
 			},
 			expectedContent: []byte("fr"),
 		},
+		{
+			desc: "title for file with empty content",
+			request: &gitalypb.WikiFindPageRequest{
+				Repository: wikiRepo,
+				Title:      []byte("Empty file"),
+			},
+			expectedPage: &gitalypb.WikiPage{
+				Version: &gitalypb.WikiPageVersion{
+					Commit: latestCommit,
+					Format: "markdown",
+				},
+				Title:      []byte(page5Name),
+				Format:     "markdown",
+				UrlPath:    "Empty-file",
+				Path:       []byte("Empty-file.md"),
+				Name:       []byte(page5Name),
+				Historical: false,
+			},
+			expectedContent: nil,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/wiki/formatted_data_test.go b/internal/service/wiki/formatted_data_test.go
index 2047fe261..90263dcbb 100644
--- a/internal/service/wiki/formatted_data_test.go
+++ b/internal/service/wiki/formatted_data_test.go
@@ -27,13 +27,16 @@ func TestSuccessfulWikiGetFormattedDataRequest(t *testing.T) {
 	page1Name := "Home Pagé"
 	page2Name := "Instálling/Step 133-b"
 	page3Name := "Installing/Step 133-c"
+	page4Name := "Empty file"
 	page1Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page1Name, format: format, content: content})
 	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page2Name, format: format, content: content})
 	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page3Name, format: format, content: content})
+	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page4Name, format: format, forceContentEmpty: true})
 
 	testCases := []struct {
-		desc    string
-		request *gitalypb.WikiGetFormattedDataRequest
+		desc            string
+		request         *gitalypb.WikiGetFormattedDataRequest
+		expectedContent []byte
 	}{
 		{
 			desc: "title only",
@@ -41,6 +44,7 @@ func TestSuccessfulWikiGetFormattedDataRequest(t *testing.T) {
 				Repository: wikiRepo,
 				Title:      []byte(page1Name),
 			},
+			expectedContent: expectedContent,
 		},
 		{
 			desc: "title + revision that includes the page",
@@ -49,6 +53,7 @@ func TestSuccessfulWikiGetFormattedDataRequest(t *testing.T) {
 				Title:      []byte(page1Name),
 				Revision:   []byte(page1Commit.Id),
 			},
+			expectedContent: expectedContent,
 		},
 		{
 			desc: "title + directory that includes the page",
@@ -57,6 +62,15 @@ func TestSuccessfulWikiGetFormattedDataRequest(t *testing.T) {
 				Title:      []byte("Step 133-b"),
 				Directory:  []byte("Instálling"),
 			},
+			expectedContent: expectedContent,
+		},
+		{
+			desc: "title for file with empty content",
+			request: &gitalypb.WikiGetFormattedDataRequest{
+				Repository: wikiRepo,
+				Title:      []byte("Empty file"),
+			},
+			expectedContent: nil,
 		},
 	}
 
@@ -69,7 +83,7 @@ func TestSuccessfulWikiGetFormattedDataRequest(t *testing.T) {
 			require.NoError(t, err)
 
 			receivedData := readFullDataFromWikiGetFormattedDataClient(t, c)
-			require.Equal(t, expectedContent, receivedData)
+			require.Equal(t, testCase.expectedContent, receivedData)
 		})
 	}
 }
diff --git a/internal/service/wiki/get_all_pages_test.go b/internal/service/wiki/get_all_pages_test.go
index 5162b530c..c8529a7bd 100644
--- a/internal/service/wiki/get_all_pages_test.go
+++ b/internal/service/wiki/get_all_pages_test.go
@@ -25,10 +25,12 @@ func TestSuccessfulWikiGetAllPagesRequest(t *testing.T) {
 
 	page1Name := "Page 1"
 	page2Name := "Page 2"
+	page3Name := "Page 3"
 	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page1Name})
-	page2Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page2Name})
+	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page2Name, forceContentEmpty: true})
+	page3Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page3Name})
 	expectedPage1 := &gitalypb.WikiPage{
-		Version:    &gitalypb.WikiPageVersion{Commit: page2Commit, Format: "markdown"},
+		Version:    &gitalypb.WikiPageVersion{Commit: page3Commit, Format: "markdown"},
 		Title:      []byte(page1Name),
 		Format:     "markdown",
 		UrlPath:    "Page-1",
@@ -38,12 +40,22 @@ func TestSuccessfulWikiGetAllPagesRequest(t *testing.T) {
 		Historical: false,
 	}
 	expectedPage2 := &gitalypb.WikiPage{
-		Version:    &gitalypb.WikiPageVersion{Commit: page2Commit, Format: "markdown"},
+		Version:    &gitalypb.WikiPageVersion{Commit: page3Commit, Format: "markdown"},
 		Title:      []byte(page2Name),
 		Format:     "markdown",
 		UrlPath:    "Page-2",
 		Path:       []byte("Page-2.md"),
 		Name:       []byte(page2Name),
+		RawData:    nil,
+		Historical: false,
+	}
+	expectedPage3 := &gitalypb.WikiPage{
+		Version:    &gitalypb.WikiPageVersion{Commit: page3Commit, Format: "markdown"},
+		Title:      []byte(page3Name),
+		Format:     "markdown",
+		UrlPath:    "Page-3",
+		Path:       []byte("Page-3.md"),
+		Name:       []byte(page3Name),
 		RawData:    mockPageContent,
 		Historical: false,
 	}
@@ -56,7 +68,7 @@ func TestSuccessfulWikiGetAllPagesRequest(t *testing.T) {
 		{
 			desc:          "No limit",
 			limit:         0,
-			expectedCount: 2,
+			expectedCount: 3,
 		},
 		{
 			desc:          "Limit of 1",
@@ -64,13 +76,13 @@ func TestSuccessfulWikiGetAllPagesRequest(t *testing.T) {
 			expectedCount: 1,
 		},
 		{
-			desc:          "Limit of 2",
-			limit:         1,
-			expectedCount: 1,
+			desc:          "Limit of 3",
+			limit:         3,
+			expectedCount: 3,
 		},
 	}
 
-	expectedPages := []*gitalypb.WikiPage{expectedPage1, expectedPage2}
+	expectedPages := []*gitalypb.WikiPage{expectedPage1, expectedPage2, expectedPage3}
 
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
diff --git a/internal/service/wiki/testhelper_test.go b/internal/service/wiki/testhelper_test.go
index c67da5609..7d6b2e503 100644
--- a/internal/service/wiki/testhelper_test.go
+++ b/internal/service/wiki/testhelper_test.go
@@ -21,9 +21,10 @@ import (
 )
 
 type createWikiPageOpts struct {
-	title   string
-	content []byte
-	format  string
+	title             string
+	content           []byte
+	format            string
+	forceContentEmpty bool
 }
 
 var (
@@ -84,7 +85,7 @@ func newWikiClient(t *testing.T, serverSocketPath string) (gitalypb.WikiServiceC
 
 func writeWikiPage(t *testing.T, client gitalypb.WikiServiceClient, wikiRepo *gitalypb.Repository, opts createWikiPageOpts) {
 	var content []byte
-	if len(opts.content) == 0 {
+	if len(opts.content) == 0 && !opts.forceContentEmpty {
 		content = mockPageContent
 	} else {
 		content = opts.content
@@ -192,7 +193,6 @@ func createTestWikiPage(t *testing.T, client gitalypb.WikiServiceClient, wikiRep
 
 	wikiRepoPath, err := helper.GetRepoPath(wikiRepo)
 	require.NoError(t, err)
-
 	writeWikiPage(t, client, wikiRepo, opts)
 	head1ID := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "show", "--format=format:%H", "--no-patch", "HEAD")
 	pageCommit, err := gitlog.GetCommit(ctx, wikiRepo, string(head1ID))
diff --git a/ruby/lib/gitaly_server/wiki_service.rb b/ruby/lib/gitaly_server/wiki_service.rb
index 877e619eb..c68993cc4 100644
--- a/ruby/lib/gitaly_server/wiki_service.rb
+++ b/ruby/lib/gitaly_server/wiki_service.rb
@@ -61,28 +61,14 @@ module GitalyServer
           end
         end
 
-        version = Gitaly::WikiPageVersion.new(
-          commit: gitaly_commit_from_rugged(page.version.commit.raw_commit),
-          format: page.version.format.to_s
-        )
-        gitaly_wiki_page = Gitaly::WikiPage.new(
-          version: version,
-          format: page.format.to_s,
-          title: page.title.b,
-          url_path: page.url_path.to_s,
-          path: page.path.b,
-          name: page.name.b,
-          historical: page.historical?
-        )
-
         Enumerator.new do |y|
+          y.yield Gitaly::WikiFindPageResponse.new(page: build_gitaly_wiki_page(page))
+
           io = StringIO.new(page.text_data)
           while chunk = io.read(Gitlab.config.git.write_buffer_size)
-            gitaly_wiki_page.raw_data = chunk
+            gitaly_wiki_page = Gitaly::WikiPage.new(raw_data: chunk)
 
             y.yield Gitaly::WikiFindPageResponse.new(page: gitaly_wiki_page)
-
-            gitaly_wiki_page = Gitaly::WikiPage.new
           end
         end
       end
@@ -96,27 +82,13 @@ module GitalyServer
 
         Enumerator.new do |y|
           wiki.pages(limit: pages_limit).each do |page|
-            version = Gitaly::WikiPageVersion.new(
-              commit: gitaly_commit_from_rugged(page.version.commit.raw_commit),
-              format: page.version.format.to_s
-            )
-            gitaly_wiki_page = Gitaly::WikiPage.new(
-              version: version,
-              format: page.format.to_s,
-              title: page.title.b,
-              url_path: page.url_path.to_s,
-              path: page.path.b,
-              name: page.name.b,
-              historical: page.historical?
-            )
+            y.yield Gitaly::WikiGetAllPagesResponse.new(page: build_gitaly_wiki_page(page))
 
             io = StringIO.new(page.text_data)
             while chunk = io.read(Gitlab.config.git.write_buffer_size)
-              gitaly_wiki_page.raw_data = chunk
+              gitaly_wiki_page = Gitaly::WikiPage.new(raw_data: chunk)
 
               y.yield Gitaly::WikiGetAllPagesResponse.new(page: gitaly_wiki_page)
-
-              gitaly_wiki_page = Gitaly::WikiPage.new
             end
 
             y.yield Gitaly::WikiGetAllPagesResponse.new(end_of_page: true)
@@ -145,13 +117,11 @@ module GitalyServer
         )
 
         Enumerator.new do |y|
+          y.yield response
+
           io = StringIO.new(file.raw_data)
           while chunk = io.read(Gitlab.config.git.write_buffer_size)
-            response.raw_data = chunk
-
-            y.yield response
-
-            response = Gitaly::WikiFindFileResponse.new
+            y.yield Gitaly::WikiFindFileResponse.new(raw_data: chunk)
           end
         end
       end
@@ -236,5 +206,28 @@ module GitalyServer
         end
       end
     end
+
+    private
+
+    def build_gitaly_wiki_page(page = nil)
+      return Gitaly::WikiPage.new unless page
+
+      Gitaly::WikiPage.new(
+        version: build_gitaly_page_version(page),
+        format: page.format.to_s,
+        title: page.title.b,
+        url_path: page.url_path.to_s,
+        path: page.path.b,
+        name: page.name.b,
+        historical: page.historical?
+      )
+    end
+
+    def build_gitaly_page_version(page)
+      Gitaly::WikiPageVersion.new(
+        commit: gitaly_commit_from_rugged(page.version.commit.raw_commit),
+        format: page.version.format.to_s
+      )
+    end
   end
 end