Merge branch 'sh-lazy-load-wiki-pages' into 'master'

Only load Wiki formatted data upon request

Closes gitlab-ce#45860

See merge request gitlab-org/gitaly!839

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