Merge branch 'fj-replace-wiki-update-page-with-rpc' into 'master'

Replace Wiki update page operation with Gitaly RPC

See merge request gitlab-org/gitlab!83833

app/models/wiki.rb

@@ -16,6 +16,13 @@ class Wiki
     'Org'      => :org
   }.freeze unless defined?(MARKUPS)
 
+  DEFAULT_MARKUP_EXTENSIONS = { # rubocop:disable Style/MultilineIfModifier
+    markdown: 'md',
+    rdoc:     'rdoc',
+    asciidoc: 'asciidoc',
+    org:      'org'
+  }.freeze unless defined?(DEFAULT_MARKUP_EXTENSIONS)
+
   CouldNotCreateWikiError = Class.new(StandardError)
 
   HOMEPAGE = 'home'
@@ -184,12 +191,37 @@ class Wiki
   end
 
   def update_page(page, content:, title: nil, format: :markdown, message: nil)
-    commit = commit_details(:updated, message, page.title)
+    if Feature.enabled?(:gitaly_replace_wiki_update_page, container, default_enabled: :yaml)
+      with_valid_format(format) do |default_extension|
+        title = title.presence || Pathname(page.path).sub_ext('').to_s
 
-    wiki.update_page(page.path, title || page.name, format.to_sym, content, commit)
-    after_wiki_activity
+        # If the format is the same we keep the former extension. This check is for formats
+        # that can have more than one extension like Markdown (.md, .markdown)
+        # If we don't do this we will override the existing extension.
+        extension = page.format != format.to_sym ? default_extension : File.extname(page.path).downcase[1..]
 
-    true
+        capture_git_error(:updated) do
+          repository.update_file(
+            user,
+            sluggified_full_path(title, extension),
+            content,
+            previous_path: page.path,
+            **multi_commit_options(:updated, message, title))
+
+          after_wiki_activity
+
+          true
+        end
+      end
+    else
+      commit = commit_details(:updated, message, page.title)
+
+      wiki.update_page(page.path, title || page.name, format.to_sym, content, commit)
+
+      after_wiki_activity
+
+      true
+    end
   end
 
   def delete_page(page, message = nil)
@@ -296,7 +328,7 @@ class Wiki
     git_user = Gitlab::Git::User.from_gitlab(user)
 
     {
-      branch_name: repository.root_ref,
+      branch_name: repository.root_ref || default_branch,
       message: commit_message,
       author_email: git_user.email,
       author_name: git_user.name
@@ -321,6 +353,24 @@ class Wiki
   def default_message(action, title)
     "#{user.username} #{action} page: #{title}"
   end
+
+  def with_valid_format(format, &block)
+    unless Wiki::MARKUPS.value?(format.to_sym)
+      @error_message = _('Invalid format selected')
+
+      return false
+    end
+
+    yield Wiki::DEFAULT_MARKUP_EXTENSIONS[format.to_sym]
+  end
+
+  def sluggified_full_path(title, extension)
+    sluggified_title(title) + '.' + extension
+  end
+
+  def sluggified_title(title)
+    Gitlab::EncodingHelper.encode_utf8_no_detect(title).tr(' ', '-')
+  end
 end
 
 Wiki.prepend_mod_with('Wiki')

config/feature_flags/development/gitaly_replace_wiki_update_page.yml

+---
+name: gitaly_replace_wiki_update_page
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83833
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/357246
+milestone: '14.10'
+type: development
+group: group::editor
+default_enabled: false

locale/gitlab.pot

@@ -20500,6 +20500,9 @@ msgstr ""
 msgid "Invalid file."
 msgstr ""
 
+msgid "Invalid format selected"
+msgstr ""
+
 msgid "Invalid hash"
 msgstr ""
 

spec/models/wiki_page_spec.rb

@@ -473,6 +473,21 @@ RSpec.describe WikiPage do
       end
     end
 
+    describe 'in subdir' do
+      it 'keeps the page in the same dir when the content is updated' do
+        title = 'foo/Existing Page'
+        page = create_wiki_page(title: title)
+
+        expect(page.slug).to eq 'foo/Existing-Page'
+        expect(page.update(title: title, content: 'new_content')).to be_truthy
+
+        page = wiki.find_page(title)
+
+        expect(page.slug).to eq 'foo/Existing-Page'
+        expect(page.content).to eq 'new_content'
+      end
+    end
+
     context 'when renaming a page' do
       it 'raises an error if the page already exists' do
         existing_page = create_wiki_page

spec/support/shared_examples/models/wiki_shared_examples.rb

@@ -427,45 +427,122 @@ RSpec.shared_examples 'wiki model' do
   end
 
   describe '#update_page' do
-    let(:page) { create(:wiki_page, wiki: subject, title: 'update-page') }
+    shared_examples 'update_page tests' do
+      with_them do
+        let!(:page) { create(:wiki_page, wiki: subject, title: original_title, format: original_format, content: 'original content') }
+
+        let(:message) { 'updated page' }
+        let(:updated_content) { 'updated content' }
+
+        def update_page
+          subject.update_page(
+            page.page,
+            content: updated_content,
+            title: updated_title,
+            format: updated_format,
+            message: message
+          )
+        end
+
+        specify :aggregate_failures do
+          expect(subject).to receive(:after_wiki_activity)
+          expect(update_page).to eq true
 
-    def update_page
-      subject.update_page(
-        page.page,
-        content: 'some other content',
-        format: :markdown,
-        message: 'updated page'
-      )
+          page = subject.find_page(updated_title.presence || original_title)
+
+          expect(page.raw_content).to eq(updated_content)
+          expect(page.path).to eq(expected_path)
+          expect(page.version.message).to eq(message)
+          expect(user.commit_email).not_to eq(user.email)
+          expect(commit.author_email).to eq(user.commit_email)
+          expect(commit.committer_email).to eq(user.commit_email)
+        end
+      end
     end
 
-    it 'updates the content of the page' do
-      update_page
-      page = subject.find_page('update-page')
+    shared_context 'common examples' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:original_title, :original_format, :updated_title, :updated_format, :expected_path) do
+        'test page' | :markdown | 'new test page' | :markdown | 'new-test-page.md'
+        'test page' | :markdown | 'test page'     | :markdown | 'test-page.md'
+        'test page' | :markdown | 'test page'     | :asciidoc | 'test-page.asciidoc'
+
+        'test page' | :markdown | 'new dir/new test page' | :markdown | 'new-dir/new-test-page.md'
+        'test page' | :markdown | 'new dir/test page'     | :markdown | 'new-dir/test-page.md'
+
+        'test dir/test page' | :markdown | 'new dir/new test page'  | :markdown | 'new-dir/new-test-page.md'
+        'test dir/test page' | :markdown | 'test dir/test page'     | :markdown | 'test-dir/test-page.md'
+        'test dir/test page' | :markdown | 'test dir/test page'     | :asciidoc | 'test-dir/test-page.asciidoc'
 
-      expect(page.raw_content).to eq('some other content')
+        'test dir/test page' | :markdown | 'new test page'  | :markdown | 'new-test-page.md'
+        'test dir/test page' | :markdown | 'test page'      | :markdown | 'test-page.md'
+
+        'test page' | :markdown | nil | :markdown | 'test-page.md'
+        'test.page' | :markdown | nil | :markdown | 'test.page.md'
+      end
     end
 
-    it 'sets the correct commit message' do
-      update_page
-      page = subject.find_page('update-page')
+    # There are two bugs in Gollum. THe first one is when the title and the format are updated
+    # at the same time https://gitlab.com/gitlab-org/gitlab/-/issues/243519.
+    # The second one is when the wiki page is within a dir and the `title` argument
+    # we pass to the update method is `nil`. Gollum will remove the dir and move the page.
+    #
+    # We can include this context into the former once it is fixed
+    # or when Gollum is removed since the Gitaly approach already fixes it.
+    shared_context 'extended examples' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:original_title, :original_format, :updated_title, :updated_format, :expected_path) do
+        'test page'          | :markdown | 'new test page'          | :asciidoc | 'new-test-page.asciidoc'
+        'test page'          | :markdown | 'new dir/new test page'  | :asciidoc | 'new-dir/new-test-page.asciidoc'
+        'test dir/test page' | :markdown | 'new dir/new test page'  | :asciidoc | 'new-dir/new-test-page.asciidoc'
+        'test dir/test page' | :markdown | 'new test page'          | :asciidoc | 'new-test-page.asciidoc'
+        'test page'          | :markdown | nil                      | :asciidoc | 'test-page.asciidoc'
+        'test dir/test page' | :markdown | nil                      | :asciidoc | 'test-dir/test-page.asciidoc'
+        'test dir/test page' | :markdown | nil                      | :markdown | 'test-dir/test-page.md'
+        'test page'          | :markdown | ''                       | :markdown | 'test-page.md'
+        'test.page'          | :markdown | ''                       | :markdown | 'test.page.md'
+      end
+    end
 
-      expect(page.version.message).to eq('updated page')
+    it_behaves_like 'update_page tests' do
+      include_context 'common examples'
+      include_context 'extended examples'
     end
 
-    it 'sets the correct commit email' do
-      update_page
+    context 'when format is invalid' do
+      let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') }
 
-      expect(user.commit_email).not_to eq(user.email)
-      expect(commit.author_email).to eq(user.commit_email)
-      expect(commit.committer_email).to eq(user.commit_email)
+      it 'returns false and sets error message' do
+        expect(subject.update_page(page.page, content: 'new content', format: :foobar)).to eq false
+        expect(subject.error_message).to match(/Invalid format selected/)
+      end
     end
 
-    it 'runs after_wiki_activity callbacks' do
-      page
+    context 'when page path does not have a default extension' do
+      let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') }
 
-      expect(subject).to receive(:after_wiki_activity)
+      context 'when format is not different' do
+        it 'does not change the default extension' do
+          path = 'test-page.markdown'
+          page.page.instance_variable_set(:@path, path)
 
-      update_page
+          expect(subject.repository).to receive(:update_file).with(user, path, anything, anything)
+
+          subject.update_page(page.page, content: 'new content', format: :markdown)
+        end
+      end
+    end
+
+    context 'when feature flag :gitaly_replace_wiki_update_page is disabled' do
+      before do
+        stub_feature_flags(gitaly_replace_wiki_update_page: false)
+      end
+
+      it_behaves_like 'update_page tests' do
+        include_context 'common examples'
+      end
     end
   end