diff options
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/controllers/projects/wikis_controller.rb | 4 | ||||
| -rw-r--r-- | app/models/project_wiki.rb | 22 | ||||
| -rw-r--r-- | app/services/test_hooks/project_service.rb | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/fj-53523-add-option-avoid-loading-wiki-page-content.yml | 5 | ||||
| -rw-r--r-- | lib/api/wikis.rb | 3 | ||||
| -rw-r--r-- | lib/gitlab/git/wiki.rb | 24 | ||||
| -rw-r--r-- | lib/gitlab/gitaly_client/wiki_service.rb | 23 | ||||
| -rw-r--r-- | spec/controllers/projects/wikis_controller_spec.rb | 22 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/wiki_spec.rb | 16 | ||||
| -rw-r--r-- | spec/lib/gitlab/gitaly_client/wiki_service_spec.rb | 6 | ||||
| -rw-r--r-- | spec/models/project_wiki_spec.rb | 74 | ||||
| -rw-r--r-- | spec/models/wiki_page_spec.rb | 66 |
14 files changed, 188 insertions, 85 deletions
@@ -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' diff --git a/Gemfile.lock b/Gemfile.lock index da8f8db9528..64f2f78a4f8 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index 88910c91763..fa5bdbc7d49 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/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.") diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index 23ddd708396..c91add6439f 100644 --- a/app/models/project_wiki.rb +++ b/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 diff --git a/app/services/test_hooks/project_service.rb b/app/services/test_hooks/project_service.rb index 6607f5b2418..a71278e8b8b 100644 --- a/app/services/test_hooks/project_service.rb +++ b/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 diff --git a/changelogs/unreleased/fj-53523-add-option-avoid-loading-wiki-page-content.yml b/changelogs/unreleased/fj-53523-add-option-avoid-loading-wiki-page-content.yml new file mode 100644 index 00000000000..49eaff52e5a --- /dev/null +++ b/changelogs/unreleased/fj-53523-add-option-avoid-loading-wiki-page-content.yml @@ -0,0 +1,5 @@ +--- +title: Added list_pages method to avoid loading all wiki pages content +merge_request: 22801 +author: +type: performance diff --git a/lib/api/wikis.rb b/lib/api/wikis.rb index 994074ddc67..5724adb2c40 100644 --- a/lib/api/wikis.rb +++ b/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 diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb index a0dd4a24363..c1bcd8e934a 100644 --- a/lib/gitlab/git/wiki.rb +++ b/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 diff --git a/lib/gitlab/gitaly_client/wiki_service.rb b/lib/gitlab/gitaly_client/wiki_service.rb index e036cdcd800..ce9faad825c 100644 --- a/lib/gitlab/gitaly_client/wiki_service.rb +++ b/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 diff --git a/spec/controllers/projects/wikis_controller_spec.rb b/spec/controllers/projects/wikis_controller_spec.rb index e0a6fc52ee9..f2e0b5e5c1d 100644 --- a/spec/controllers/projects/wikis_controller_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/git/wiki_spec.rb b/spec/lib/gitlab/git/wiki_spec.rb index ded5d7576df..1e577392949 100644 --- a/spec/lib/gitlab/git/wiki_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/gitaly_client/wiki_service_spec.rb b/spec/lib/gitlab/gitaly_client/wiki_service_spec.rb index d82c9c28da0..4fa8e97aca0 100644 --- a/spec/lib/gitlab/gitaly_client/wiki_service_spec.rb +++ b/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) diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb index 2525a6aebe0..d12dd97bb9e 100644 --- a/spec/models/project_wiki_spec.rb +++ b/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 diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index d5c85c11195..520a06e138e 100644 --- a/spec/models/wiki_page_spec.rb +++ b/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 |