diff options
author | Stan Hu <stanhu@gmail.com> | 2018-07-30 03:25:22 +0300 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-08-06 18:53:27 +0300 |
commit | a9afcae7423e97cbad76184bd0fce4e43a72f88a (patch) | |
tree | 1dfe681facee085c08aa5cfc7655f86be7821623 | |
parent | bc7b07db791a8a7a7634acb71f9f2915d751b6c8 (diff) |
Only load Wiki formatted data upon request
Previously the formatted data for each Wiki page requested would be generated
unnecessarily. This would be expensive especially if the Wiki rendering
required shelling out to another process, as reStructuredText files does. Now,
we only load the formatted data and rely on the separate RPC to request the
formatted data.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/45860
-rw-r--r-- | changelogs/unreleased/sh-lazy-load-wiki-pages.yml | 5 | ||||
-rw-r--r-- | ruby/lib/gitlab/git/wiki.rb | 12 | ||||
-rw-r--r-- | ruby/lib/gitlab/git/wiki_page.rb | 19 | ||||
-rw-r--r-- | ruby/spec/lib/gitlab/git/wiki_spec.rb | 91 | ||||
-rw-r--r-- | ruby/spec/spec_helper.rb | 2 | ||||
-rw-r--r-- | ruby/spec/test_repo_helper.rb | 12 |
6 files changed, 117 insertions, 24 deletions
diff --git a/changelogs/unreleased/sh-lazy-load-wiki-pages.yml b/changelogs/unreleased/sh-lazy-load-wiki-pages.yml new file mode 100644 index 000000000..c08d68fba --- /dev/null +++ b/changelogs/unreleased/sh-lazy-load-wiki-pages.yml @@ -0,0 +1,5 @@ +--- +title: Only load Wiki formatted data upon request +merge_request: 839 +author: +type: performance diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb index 91a6c1bbe..9830f7858 100644 --- a/ruby/lib/gitlab/git/wiki.rb +++ b/ruby/lib/gitlab/git/wiki.rb @@ -79,14 +79,6 @@ module Gitlab page.url_path end - def page_formatted_data(title:, dir: nil, version: nil) - version = version&.id - - # We don't use #page because if wiki_find_page feature is enabled, we would - # get a page without formatted_data. - gollum_find_page(title: title, dir: dir, version: version)&.formatted_data - end - def gollum_wiki @gollum_wiki ||= Gollum::Wiki.new(@repository.path) end @@ -154,10 +146,6 @@ module Gitlab gollum_wiki.paged(page_name, page_dir) end - def new_page(gollum_page) - Gitlab::Git::WikiPage.new(gollum_page, new_version(gollum_page, gollum_page.version.id)) - end - def gollum_write_page(name, format, content, commit_details) assert_type!(format, Symbol) assert_type!(commit_details, CommitDetails) diff --git a/ruby/lib/gitlab/git/wiki_page.rb b/ruby/lib/gitlab/git/wiki_page.rb index 669ae11a4..35010c616 100644 --- a/ruby/lib/gitlab/git/wiki_page.rb +++ b/ruby/lib/gitlab/git/wiki_page.rb @@ -1,19 +1,11 @@ module Gitlab module Git class WikiPage - attr_reader :url_path, :title, :format, :path, :version, :raw_data, :name, :text_data, :historical, :formatted_data + attr_reader :url_path, :title, :format, :path, :version, :raw_data, :name, :text_data, :historical - # This class is meant to be serializable so that it can be constructed - # by Gitaly and sent over the network to GitLab. - # - # Because Gollum::Page is not serializable we must get all the data from - # 'gollum_page' during initialization, and NOT store it in an instance - # variable. - # - # Note that 'version' is a WikiPageVersion instance which it itself - # serializable. That means it's OK to store 'version' in an instance - # variable. def initialize(gollum_page, version) + @gollum_page = gollum_page + @url_path = gollum_page.url_path @title = gollum_page.title @format = gollum_page.format @@ -21,11 +13,14 @@ module Gitlab @raw_data = gollum_page.raw_data @name = gollum_page.name @historical = gollum_page.historical? - @formatted_data = gollum_page.formatted_data if gollum_page.is_a?(Gollum::Page) @version = version end + def formatted_data + @gollum_page.formatted_data + end + def historical? @historical end diff --git a/ruby/spec/lib/gitlab/git/wiki_spec.rb b/ruby/spec/lib/gitlab/git/wiki_spec.rb new file mode 100644 index 000000000..baf07c016 --- /dev/null +++ b/ruby/spec/lib/gitlab/git/wiki_spec.rb @@ -0,0 +1,91 @@ +require 'spec_helper' + +describe Gitlab::Git::Wiki do + include TestRepo + + let(:repository) { gitlab_git_from_gitaly(new_empty_test_repo) } + let(:user) { project.owner } + + subject { described_class.new(repository) } + + describe '#pages' do + let(:pages) { subject.pages } + + before do + create_page('page1', 'content') + create_page('page2', 'content2') + end + + after do + destroy_page('page1') + destroy_page('page2') + end + + it 'returns all the pages' do + expect(pages.count).to eq(2) + expect(pages.first.title).to eq 'page1' + expect(pages.last.title).to eq 'page2' + end + + it 'returns only one page' do + pages = subject.pages(limit: 1) + + expect(pages.count).to eq(1) + expect(pages.first.title).to eq 'page1' + end + + it 'returns formatted data' do + expect(pages.first.formatted_data).to be_a(String) + end + end + + describe '#page' do + before do + create_page('page1', 'content') + create_page('foo/page1', 'content foo/page1') + end + + after do + destroy_page('page1') + destroy_page('page1', 'foo') + end + + it 'returns the right page' do + expect(subject.page(title: 'page1', dir: '').url_path).to eq 'page1' + expect(subject.page(title: 'page1', dir: 'foo').url_path).to eq 'foo/page1' + end + + it 'returns formatted data' do + expect(subject.page(title: 'page1', dir: '').formatted_data).to be_a(String) + end + end + + describe '#delete_page' do + after do + destroy_page('page1') + end + + it 'only removes the page with the same path' do + create_page('page1', 'content') + create_page('*', 'content') + + subject.delete_page('*', commit_details('whatever')) + + expect(subject.pages.count).to eq 1 + expect(subject.pages.first.title).to eq 'page1' + end + end + + def create_page(name, content) + subject.write_page(name, :markdown, content, commit_details(name)) + end + + def commit_details(name) + Gitlab::Git::Wiki::CommitDetails.new(1, 'test-user', 'Test User', 'test@example.com', "created page #{name}") + end + + def destroy_page(title, dir = '') + page = subject.page(title: title, dir: dir) + subject.delete_page(page.path, commit_details(title)) + end +end diff --git a/ruby/spec/spec_helper.rb b/ruby/spec/spec_helper.rb index 1f7d988e3..75652b01a 100644 --- a/ruby/spec/spec_helper.rb +++ b/ruby/spec/spec_helper.rb @@ -2,3 +2,5 @@ require_relative '../lib/gitaly_server.rb' require_relative '../lib/gitlab/git.rb' require_relative 'support/sentry.rb' require 'test_repo_helper' + +ENV['GITALY_RUBY_GIT_BIN_PATH'] ||= 'git' diff --git a/ruby/spec/test_repo_helper.rb b/ruby/spec/test_repo_helper.rb index afcddf9ca..a2792610d 100644 --- a/ruby/spec/test_repo_helper.rb +++ b/ruby/spec/test_repo_helper.rb @@ -27,6 +27,12 @@ module TestRepo Gitaly::Repository.new(storage_name: DEFAULT_STORAGE_NAME, relative_path: relative_path) end + def new_empty_test_repo + relative_path = "mutable-#{SecureRandom.hex(6)}.git" + TestRepo.init_new_repo!(File.join(DEFAULT_STORAGE_DIR, relative_path)) + Gitaly::Repository.new(storage_name: DEFAULT_STORAGE_NAME, relative_path: relative_path) + end + def rugged_from_gitaly(gitaly_repo) Rugged::Repository.new(repo_path_from_gitaly(gitaly_repo)) end @@ -51,6 +57,12 @@ module TestRepo return if system(*%W[git clone --quiet --bare #{TEST_REPO_ORIGIN} #{destination}]) abort "Failed to clone test repo. Try running 'make prepare-tests' and try again." end + + def self.init_new_repo!(destination) + return if system(*%W[git init --quiet --bare #{destination}]) + + abort "Failed to init test repo." + end end TestRepo.prepare_test_repository |