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

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