Merge branch 'fj-53523-add-option-avoid-loading-wiki-page-content' into 'master'

Avoid loading Wiki Pages content when listed

Closes #53523

See merge request gitlab-org/gitlab-ce!22801

Gemfile

@@ -416,7 +416,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 1.19.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 1.22.0', require: 'gitaly'
 
 gem 'grpc', '~> 1.19.0'
 

Gemfile.lock

@@ -280,7 +280,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (1.19.0)
+    gitaly-proto (1.22.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-default_value_for (3.1.1)
@@ -1052,7 +1052,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 1.19.0)
+  gitaly-proto (~> 1.22.0)
   github-markup (~> 1.7.0)
   gitlab-default_value_for (~> 3.1.1)
   gitlab-labkit (~> 0.1.2)

app/controllers/projects/wikis_controller.rb

@@ -17,7 +17,7 @@ class Projects::WikisController < Projects::ApplicationController
 
   def pages
     @wiki_pages = Kaminari.paginate_array(
-      @project_wiki.pages(sort: params[:sort], direction: params[:direction])
+      @project_wiki.list_pages(sort: params[:sort], direction: params[:direction])
     ).page(params[:page])
 
     @wiki_entries = WikiPage.group_by_directory(@wiki_pages)
@@ -118,7 +118,7 @@ class Projects::WikisController < Projects::ApplicationController
     @sidebar_page = @project_wiki.find_sidebar(params[:version_id])
 
     unless @sidebar_page # Fallback to default sidebar
-      @sidebar_wiki_entries = WikiPage.group_by_directory(@project_wiki.pages(limit: 15))
+      @sidebar_wiki_entries = WikiPage.group_by_directory(@project_wiki.list_pages(limit: 15))
     end
   rescue ProjectWiki::CouldNotCreateWikiError
     flash[:notice] = _("Could not create Wiki Repository at this time. Please try again later.")

app/models/project_wiki.rb

@@ -82,17 +82,25 @@ class ProjectWiki
   end
 
   def empty?
-    pages(limit: 1).empty?
+    list_pages(limit: 1).empty?
   end
 
+  # Lists wiki pages of the repository.
+  #
+  # limit - max number of pages returned by the method.
+  # sort - criterion by which the pages are sorted.
+  # direction - order of the sorted pages.
+  # load_content - option, which specifies whether the content inside the page
+  #                will be loaded.
+  #
   # Returns an Array of GitLab WikiPage instances or an
   # empty Array if this Wiki has no pages.
-  def pages(limit: 0, sort: nil, direction: DIRECTION_ASC)
-    sort ||= TITLE_ORDER
-    direction_desc = direction == DIRECTION_DESC
-
-    wiki.pages(
-      limit: limit, sort: sort, direction_desc: direction_desc
+  def list_pages(limit: 0, sort: nil, direction: DIRECTION_ASC, load_content: false)
+    wiki.list_pages(
+      limit: limit,
+      sort: sort,
+      direction_desc: direction == DIRECTION_DESC,
+      load_content: load_content
     ).map do |page|
       WikiPage.new(self, page, true)
     end

app/services/test_hooks/project_service.rb

@@ -56,7 +56,7 @@ module TestHooks
     end
 
     def wiki_page_events_data
-      page = project.wiki.pages.first
+      page = project.wiki.list_pages(limit: 1).first
       if !project.wiki_enabled? || page.blank?
         throw(:validation_error, s_('TestHooks|Ensure the wiki is enabled and has pages.'))
       end

changelogs/unreleased/fj-53523-add-option-avoid-loading-wiki-page-content.yml

+---
+title: Added list_pages method to avoid loading all wiki pages content
+merge_request: 22801
+author:
+type: performance

lib/api/wikis.rb

@@ -33,7 +33,8 @@ module API
         authorize! :read_wiki, user_project
 
         entity = params[:with_content] ? Entities::WikiPage : Entities::WikiPageBasic
-        present user_project.wiki.pages, with: entity
+
+        present user_project.wiki.list_pages(load_content: params[:with_content]), with: entity
       end
 
       desc 'Get a wiki page' do

lib/gitlab/git/wiki.rb

@@ -86,9 +86,14 @@ module Gitlab
         end
       end
 
-      def pages(limit: 0, sort: nil, direction_desc: false)
+      def list_pages(limit: 0, sort: nil, direction_desc: false, load_content: false)
         wrapped_gitaly_errors do
-          gitaly_get_all_pages(limit: limit, sort: sort, direction_desc: direction_desc)
+          gitaly_list_pages(
+            limit: limit,
+            sort: sort,
+            direction_desc: direction_desc,
+            load_content: load_content
+          )
         end
       end
 
@@ -168,10 +173,17 @@ module Gitlab
         Gitlab::Git::WikiFile.new(wiki_file)
       end
 
-      def gitaly_get_all_pages(limit: 0, sort: nil, direction_desc: false)
-        gitaly_wiki_client.get_all_pages(
-          limit: limit, sort: sort, direction_desc: direction_desc
-        ).map do |wiki_page, version|
+      def gitaly_list_pages(limit: 0, sort: nil, direction_desc: false, load_content: false)
+        params = { limit: limit, sort: sort, direction_desc: direction_desc }
+
+        gitaly_pages =
+          if load_content
+            gitaly_wiki_client.load_all_pages(params)
+          else
+            gitaly_wiki_client.list_all_pages(params)
+          end
+
+        gitaly_pages.map do |wiki_page, version|
           Gitlab::Git::WikiPage.new(wiki_page, version)
         end
       end

lib/gitlab/gitaly_client/wiki_service.rb

@@ -87,7 +87,27 @@ module Gitlab
         wiki_page_from_iterator(response)
       end
 
-      def get_all_pages(limit: 0, sort: nil, direction_desc: false)
+      def list_all_pages(limit: 0, sort: nil, direction_desc: false)
+        sort_value = Gitaly::WikiListPagesRequest::SortBy.resolve(sort.to_s.upcase.to_sym)
+
+        params = { repository: @gitaly_repo, limit: limit, direction_desc: direction_desc }
+        params[:sort] = sort_value if sort_value
+
+        request = Gitaly::WikiListPagesRequest.new(params)
+        stream = GitalyClient.call(@repository.storage, :wiki_service, :wiki_list_pages, request, timeout: GitalyClient.medium_timeout)
+        stream.each_with_object([]) do |message, pages|
+          page = message.page
+
+          next unless page
+
+          wiki_page = GitalyClient::WikiPage.new(page.to_h)
+          version = new_wiki_page_version(page.version)
+
+          pages << [wiki_page, version]
+        end
+      end
+
+      def load_all_pages(limit: 0, sort: nil, direction_desc: false)
         sort_value = Gitaly::WikiGetAllPagesRequest::SortBy.resolve(sort.to_s.upcase.to_sym)
 
         params = { repository: @gitaly_repo, limit: limit, direction_desc: direction_desc }
@@ -95,6 +115,7 @@ module Gitlab
 
         request = Gitaly::WikiGetAllPagesRequest.new(params)
         response = GitalyClient.call(@repository.storage, :wiki_service, :wiki_get_all_pages, request, timeout: GitalyClient.medium_timeout)
+
         pages = []
 
         loop do

spec/controllers/projects/wikis_controller_spec.rb

@@ -19,6 +19,18 @@ describe Projects::WikisController do
     destroy_page(wiki_title)
   end
 
+  describe 'GET #pages' do
+    subject { get :pages, params: { namespace_id: project.namespace, project_id: project, id: wiki_title } }
+
+    it 'does not load the pages content' do
+      expect(controller).to receive(:load_wiki).and_return(project_wiki)
+
+      expect(project_wiki).to receive(:list_pages).twice.and_call_original
+
+      subject
+    end
+  end
+
   describe 'GET #show' do
     render_views
 
@@ -28,9 +40,9 @@ describe Projects::WikisController do
       expect(controller).to receive(:load_wiki).and_return(project_wiki)
 
       # empty? call
-      expect(project_wiki).to receive(:pages).with(limit: 1).and_call_original
+      expect(project_wiki).to receive(:list_pages).with(limit: 1).and_call_original
       # Sidebar entries
-      expect(project_wiki).to receive(:pages).with(limit: 15).and_call_original
+      expect(project_wiki).to receive(:list_pages).with(limit: 15).and_call_original
 
       subject
 
@@ -104,7 +116,7 @@ describe Projects::WikisController do
 
         subject
 
-        expect(response).to redirect_to(project_wiki_path(project, project_wiki.pages.first))
+        expect(response).to redirect_to(project_wiki_path(project, project_wiki.list_pages.first))
       end
     end
 
@@ -138,7 +150,7 @@ describe Projects::WikisController do
         allow(controller).to receive(:valid_encoding?).and_return(false)
 
         subject
-        expect(response).to redirect_to(project_wiki_path(project, project_wiki.pages.first))
+        expect(response).to redirect_to(project_wiki_path(project, project_wiki.list_pages.first))
       end
     end
 
@@ -148,7 +160,7 @@ describe Projects::WikisController do
       it 'updates the page' do
         subject
 
-        wiki_page = project_wiki.pages.first
+        wiki_page = project_wiki.list_pages(load_content: true).first
 
         expect(wiki_page.title).to eq new_title
         expect(wiki_page.content).to eq new_content

spec/lib/gitlab/git/wiki_spec.rb

@@ -21,13 +21,13 @@ describe Gitlab::Git::Wiki do
     end
 
     it 'returns all the pages' do
-      expect(subject.pages.count).to eq(2)
-      expect(subject.pages.first.title).to eq 'page1'
-      expect(subject.pages.last.title).to eq 'page2'
+      expect(subject.list_pages.count).to eq(2)
+      expect(subject.list_pages.first.title).to eq 'page1'
+      expect(subject.list_pages.last.title).to eq 'page2'
     end
 
     it 'returns only one page' do
-      pages = subject.pages(limit: 1)
+      pages = subject.list_pages(limit: 1)
 
       expect(pages.count).to eq(1)
       expect(pages.first.title).to eq 'page1'
@@ -62,8 +62,8 @@ describe Gitlab::Git::Wiki do
 
       subject.delete_page('*', commit_details('whatever'))
 
-      expect(subject.pages.count).to eq 1
-      expect(subject.pages.first.title).to eq 'page1'
+      expect(subject.list_pages.count).to eq 1
+      expect(subject.list_pages.first.title).to eq 'page1'
     end
   end
 
@@ -87,7 +87,7 @@ describe Gitlab::Git::Wiki do
     with_them do
       subject { wiki.preview_slug(title, format) }
 
-      let(:gitaly_slug) { wiki.pages.first }
+      let(:gitaly_slug) { wiki.list_pages.first }
 
       it { is_expected.to eq(expected_slug) }
 
@@ -96,7 +96,7 @@ describe Gitlab::Git::Wiki do
 
         create_page(title, 'content', format: format)
 
-        gitaly_slug = wiki.pages.first.url_path
+        gitaly_slug = wiki.list_pages.first.url_path
 
         is_expected.to eq(gitaly_slug)
       end

spec/lib/gitlab/gitaly_client/wiki_service_spec.rb

@@ -46,7 +46,7 @@ describe Gitlab::GitalyClient::WikiService do
     end
   end
 
-  describe '#get_all_pages' do
+  describe '#load_all_pages' do
     let(:page_2_info) { { title: 'My Page 2', raw_data: 'c', version: page_version } }
     let(:response) do
       [
@@ -63,7 +63,7 @@ describe Gitlab::GitalyClient::WikiService do
     let(:wiki_page_2) { subject[1].first }
     let(:wiki_page_2_version) { subject[1].last }
 
-    subject { client.get_all_pages }
+    subject { client.load_all_pages }
 
     it 'sends a wiki_get_all_pages message' do
       expect_any_instance_of(Gitaly::WikiService::Stub)
@@ -99,7 +99,7 @@ describe Gitlab::GitalyClient::WikiService do
     end
 
     context 'with limits' do
-      subject { client.get_all_pages(limit: 1) }
+      subject { client.load_all_pages(limit: 1) }
 
       it 'sends a request with the limit' do
         expect_any_instance_of(Gitaly::WikiService::Stub)

spec/models/project_wiki_spec.rb

@@ -109,8 +109,7 @@ describe ProjectWiki do
         subject { super().empty? }
         it { is_expected.to be_falsey }
 
-        # Re-enable this when https://gitlab.com/gitlab-org/gitaly/issues/1204 is fixed
-        xit 'only instantiates a Wiki page once' do
+        it 'only instantiates a Wiki page once' do
           expect(WikiPage).to receive(:new).once.and_call_original
 
           subject
@@ -119,22 +118,65 @@ describe ProjectWiki do
     end
   end
 
-  describe "#pages" do
+  describe "#list_pages" do
+    let(:wiki_pages) { subject.list_pages }
+
     before do
-      create_page("index", "This is an awesome new Gollum Wiki")
-      @pages = subject.pages
+      create_page("index", "This is an index")
+      create_page("index2", "This is an index2")
+      create_page("an index3", "This is an index3")
     end
 
     after do
-      destroy_page(@pages.first.page)
+      wiki_pages.each do |wiki_page|
+        destroy_page(wiki_page.page)
+      end
     end
 
     it "returns an array of WikiPage instances" do
-      expect(@pages.first).to be_a WikiPage
+      expect(wiki_pages.first).to be_a WikiPage
+    end
+
+    it 'does not load WikiPage content by default' do
+      wiki_pages.each do |page|
+        expect(page.content).to be_empty
+      end
+    end
+
+    it 'returns all pages by default' do
+      expect(wiki_pages.count).to eq(3)
+    end
+
+    context "with limit option" do
+      it 'returns limited set of pages' do
+        expect(subject.list_pages(limit: 1).count).to eq(1)
+      end
     end
 
-    it "returns the correct number of pages" do
-      expect(@pages.count).to eq(1)
+    context "with sorting options" do
+      it 'returns pages sorted by title by default' do
+        pages = ['an index3', 'index', 'index2']
+
+        expect(subject.list_pages.map(&:title)).to eq(pages)
+        expect(subject.list_pages(direction: "desc").map(&:title)).to eq(pages.reverse)
+      end
+
+      it 'returns pages sorted by created_at' do
+        pages = ['index', 'index2', 'an index3']
+
+        expect(subject.list_pages(sort: 'created_at').map(&:title)).to eq(pages)
+        expect(subject.list_pages(sort: 'created_at', direction: "desc").map(&:title)).to eq(pages.reverse)
+      end
+    end
+
+    context "with load_content option" do
+      let(:pages) { subject.list_pages(load_content: true) }
+
+      it 'loads WikiPage content' do
+        expect(pages.first.content).to eq("This is an index3")
+        expect(pages.second.content).to eq("This is an index")
+        expect(pages.third.content).to eq("This is an index2")
+      end
     end
   end
 
@@ -144,7 +186,7 @@ describe ProjectWiki do
     end
 
     after do
-      subject.pages.each { |page| destroy_page(page.page) }
+      subject.list_pages.each { |page| destroy_page(page.page) }
     end
 
     it "returns the latest version of the page if it exists" do
@@ -195,7 +237,7 @@ describe ProjectWiki do
     end
 
     after do
-      subject.pages.each { |page| destroy_page(page.page) }
+      subject.list_pages.each { |page| destroy_page(page.page) }
     end
 
     it 'finds the page defined as _sidebar' do
@@ -242,12 +284,12 @@ describe ProjectWiki do
 
   describe "#create_page" do
     after do
-      destroy_page(subject.pages.first.page)
+      destroy_page(subject.list_pages.first.page)
     end
 
     it "creates a new wiki page" do
       expect(subject.create_page("test page", "this is content")).not_to eq(false)
-      expect(subject.pages.count).to eq(1)
+      expect(subject.list_pages.count).to eq(1)
     end
 
     it "returns false when a duplicate page exists" do
@@ -262,7 +304,7 @@ describe ProjectWiki do
 
     it "sets the correct commit message" do
       subject.create_page("test page", "some content", :markdown, "commit message")
-      expect(subject.pages.first.page.version.message).to eq("commit message")
+      expect(subject.list_pages.first.page.version.message).to eq("commit message")
     end
 
     it 'sets the correct commit email' do
@@ -293,7 +335,7 @@ describe ProjectWiki do
         format: :markdown,
         message: "updated page"
       )
-      @page = subject.pages.first.page
+      @page = subject.list_pages(load_content: true).first.page
     end
 
     after do
@@ -337,7 +379,7 @@ describe ProjectWiki do
 
     it "deletes the page" do
       subject.delete_page(@page)
-      expect(subject.pages.count).to eq(0)
+      expect(subject.list_pages.count).to eq(0)
     end
 
     it 'sets the correct commit email' do

spec/models/wiki_page_spec.rb

@@ -44,47 +44,49 @@ describe WikiPage do
         WikiDirectory.new('dir_2', pages)
       end
 
-      context 'sort by title' do
-        let(:grouped_entries) { described_class.group_by_directory(wiki.pages) }
-        let(:expected_grouped_entries) { [dir_1_1, dir_1, page_dir_2, dir_2, page_1, page_6] }
-
-        it 'returns an array with pages and directories' do
-          grouped_entries.each_with_index do |page_or_dir, i|
-            expected_page_or_dir = expected_grouped_entries[i]
-            expected_slugs = get_slugs(expected_page_or_dir)
-            slugs = get_slugs(page_or_dir)
-
-            expect(slugs).to match_array(expected_slugs)
+      context "#list_pages" do
+        context 'sort by title' do
+          let(:grouped_entries) { described_class.group_by_directory(wiki.list_pages) }
+          let(:expected_grouped_entries) { [dir_1_1, dir_1, page_dir_2, dir_2, page_1, page_6] }
+
+          it 'returns an array with pages and directories' do
+            grouped_entries.each_with_index do |page_or_dir, i|
+              expected_page_or_dir = expected_grouped_entries[i]
+              expected_slugs = get_slugs(expected_page_or_dir)
+              slugs = get_slugs(page_or_dir)
+
+              expect(slugs).to match_array(expected_slugs)
+            end
           end
         end
-      end
 
-      context 'sort by created_at' do
-        let(:grouped_entries) { described_class.group_by_directory(wiki.pages(sort: 'created_at')) }
-        let(:expected_grouped_entries) { [dir_1_1, page_1, dir_1, page_dir_2, dir_2, page_6] }
+        context 'sort by created_at' do
+          let(:grouped_entries) { described_class.group_by_directory(wiki.list_pages(sort: 'created_at')) }
+          let(:expected_grouped_entries) { [dir_1_1, page_1, dir_1, page_dir_2, dir_2, page_6] }
 
-        it 'returns an array with pages and directories' do
-          grouped_entries.each_with_index do |page_or_dir, i|
-            expected_page_or_dir = expected_grouped_entries[i]
-            expected_slugs = get_slugs(expected_page_or_dir)
-            slugs = get_slugs(page_or_dir)
+          it 'returns an array with pages and directories' do
+            grouped_entries.each_with_index do |page_or_dir, i|
+              expected_page_or_dir = expected_grouped_entries[i]
+              expected_slugs = get_slugs(expected_page_or_dir)
+              slugs = get_slugs(page_or_dir)
 
-            expect(slugs).to match_array(expected_slugs)
+              expect(slugs).to match_array(expected_slugs)
+            end
           end
         end
-      end
 
-      it 'returns an array with retained order with directories at the top' do
-        expected_order = ['dir_1/dir_1_1/page_3', 'dir_1/page_2', 'dir_2', 'dir_2/page_4', 'dir_2/page_5', 'page_1', 'page_6']
+        it 'returns an array with retained order with directories at the top' do
+          expected_order = ['dir_1/dir_1_1/page_3', 'dir_1/page_2', 'dir_2', 'dir_2/page_4', 'dir_2/page_5', 'page_1', 'page_6']
 
-        grouped_entries = described_class.group_by_directory(wiki.pages)
+          grouped_entries = described_class.group_by_directory(wiki.list_pages)
 
-        actual_order =
-          grouped_entries.map do |page_or_dir|
-            get_slugs(page_or_dir)
-          end
-          .flatten
-        expect(actual_order).to eq(expected_order)
+          actual_order =
+            grouped_entries.map do |page_or_dir|
+              get_slugs(page_or_dir)
+            end
+            .flatten
+          expect(actual_order).to eq(expected_order)
+        end
       end
     end
   end
@@ -386,7 +388,7 @@ describe WikiPage do
 
     it "deletes the page" do
       @page.delete
-      expect(wiki.pages).to be_empty
+      expect(wiki.list_pages).to be_empty
     end
 
     it "returns true" do