Merge branch '34509-improves-markdown-rendering-performance-for-commits-list' into 'master'

Resolve "Projects::CommitsController#show is slow partially due to SQL queries"

Closes #34509

See merge request !13762

4 files changed, 186 insertions, 50 deletions

diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb
index 70eb01c9c44..03d706062b7 100644
--- a/spec/helpers/markup_helper_spec.rb
+++ b/spec/helpers/markup_helper_spec.rb
@@ -52,12 +52,71 @@ describe MarkupHelper do
     end
   end
 
-  describe '#link_to_gfm' do
+  describe '#markdown_field' do
+    let(:attribute) { :title }
+
+    describe 'with already redacted attribute' do
+      it 'returns the redacted attribute' do
+        commit.redacted_title_html = 'commit title'
+
+        expect(Banzai).not_to receive(:render_field)
+
+        expect(helper.markdown_field(commit, attribute)).to eq('commit title')
+      end
+    end
+
+    describe 'without redacted attribute' do
+      it 'renders the markdown value' do
+        expect(Banzai).to receive(:render_field).with(commit, attribute).and_call_original
+
+        helper.markdown_field(commit, attribute)
+      end
+    end
+  end
+
+  describe '#link_to_markdown_field' do
+    let(:link)    { '/commits/0a1b2c3d' }
+    let(:issues)  { create_list(:issue, 2, project: project) }
+
+    it 'handles references nested in links with all the text' do
+      allow(commit).to receive(:title).and_return("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real")
+
+      actual = helper.link_to_markdown_field(commit, :title, link)
+      doc = Nokogiri::HTML.parse(actual)
+
+      # Make sure we didn't create invalid markup
+      expect(doc.errors).to be_empty
+
+      # Leading commit link
+      expect(doc.css('a')[0].attr('href')).to eq link
+      expect(doc.css('a')[0].text).to eq 'This should finally fix '
+
+      # First issue link
+      expect(doc.css('a')[1].attr('href'))
+        .to eq project_issue_path(project, issues[0])
+      expect(doc.css('a')[1].text).to eq issues[0].to_reference
+
+      # Internal commit link
+      expect(doc.css('a')[2].attr('href')).to eq link
+      expect(doc.css('a')[2].text).to eq ' and '
+
+      # Second issue link
+      expect(doc.css('a')[3].attr('href'))
+        .to eq project_issue_path(project, issues[1])
+      expect(doc.css('a')[3].text).to eq issues[1].to_reference
+
+      # Trailing commit link
+      expect(doc.css('a')[4].attr('href')).to eq link
+      expect(doc.css('a')[4].text).to eq ' for real'
+    end
+  end
+
+  describe '#link_to_markdown' do
     let(:link)    { '/commits/0a1b2c3d' }
     let(:issues)  { create_list(:issue, 2, project: project) }
 
     it 'handles references nested in links with all the text' do
-      actual = helper.link_to_gfm("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real", link)
+      actual = helper.link_to_markdown("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real", link)
       doc = Nokogiri::HTML.parse(actual)
 
       # Make sure we didn't create invalid markup
@@ -87,7 +146,7 @@ describe MarkupHelper do
     end
 
     it 'forwards HTML options' do
-      actual = helper.link_to_gfm("Fixed in #{commit.id}", link, class: 'foo')
+      actual = helper.link_to_markdown("Fixed in #{commit.id}", link, class: 'foo')
       doc = Nokogiri::HTML.parse(actual)
 
       expect(doc.css('a')).to satisfy do |v|
@@ -98,23 +157,43 @@ describe MarkupHelper do
 
     it "escapes HTML passed in as the body" do
       actual = "This is a <h1>test</h1> - see #{issues[0].to_reference}"
-      expect(helper.link_to_gfm(actual, link))
+      expect(helper.link_to_markdown(actual, link))
         .to match('&lt;h1&gt;test&lt;/h1&gt;')
     end
 
     it 'ignores reference links when they are the entire body' do
       text = issues[0].to_reference
-      act = helper.link_to_gfm(text, '/foo')
+      act = helper.link_to_markdown(text, '/foo')
       expect(act).to eq %Q(<a href="/foo">#{issues[0].to_reference}</a>)
     end
 
     it 'replaces commit message with emoji to link' do
-      actual = link_to_gfm(':book: Book', '/foo')
+      actual = link_to_markdown(':book: Book', '/foo')
       expect(actual)
         .to eq '<gl-emoji title="open book" data-name="book" data-unicode-version="6.0">📖</gl-emoji><a href="/foo"> Book</a>'
     end
   end
 
+  describe '#link_to_html' do
+    it 'wraps the rendered content in a link' do
+      link = '/commits/0a1b2c3d'
+      issue = create(:issue, project: project)
+
+      rendered = helper.markdown("This should finally fix #{issue.to_reference} for real", pipeline: :single_line)
+      doc = Nokogiri::HTML.parse(rendered)
+
+      expect(doc.css('a')[0].attr('href'))
+        .to eq project_issue_path(project, issue)
+      expect(doc.css('a')[0].text).to eq issue.to_reference
+
+      wrapped = helper.link_to_html(rendered, link)
+      doc = Nokogiri::HTML.parse(wrapped)
+
+      expect(doc.css('a')[0].attr('href')).to eq link
+      expect(doc.css('a')[0].text).to eq 'This should finally fix '
+    end
+  end
+
   describe '#render_wiki_content' do
     before do
       @wiki = double('WikiPage')
diff --git a/spec/lib/banzai/commit_renderer_spec.rb b/spec/lib/banzai/commit_renderer_spec.rb
new file mode 100644
index 00000000000..049d025a5b9
--- /dev/null
+++ b/spec/lib/banzai/commit_renderer_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Banzai::CommitRenderer do
+  describe '.render' do
+    it 'renders a commit description and title' do
+      user = double(:user)
+      project = create(:project, :repository)
+
+      expect(Banzai::ObjectRenderer).to receive(:new).with(project, user).and_call_original
+
+      described_class::ATTRIBUTES.each do |attr|
+        expect_any_instance_of(Banzai::ObjectRenderer).to receive(:render).with([project.commit], attr).once.and_call_original
+        expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr)
+      end
+
+      described_class.render([project.commit], project, user)
+    end
+  end
+end
diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb
index 7f5d481c36c..b172a1b718c 100644
--- a/spec/lib/banzai/object_renderer_spec.rb
+++ b/spec/lib/banzai/object_renderer_spec.rb
@@ -1,53 +1,77 @@
 require 'spec_helper'
 
 describe Banzai::ObjectRenderer do
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :repository) }
   let(:user) { project.owner }
   let(:renderer) { described_class.new(project, user, custom_value: 'value') }
   let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_VERSION) }
 
   describe '#render' do
-    it 'renders and redacts an Array of objects' do
-      renderer.render([object], :note)
+    context 'with cache' do
+      it 'renders and redacts an Array of objects' do
+        renderer.render([object], :note)
 
-      expect(object.redacted_note_html).to eq '<p dir="auto">hello</p>'
-      expect(object.user_visible_reference_count).to eq 0
-    end
+        expect(object.redacted_note_html).to eq '<p dir="auto">hello</p>'
+        expect(object.user_visible_reference_count).to eq 0
+      end
 
-    it 'calls Banzai::Redactor to perform redaction' do
-      expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original
+      it 'calls Banzai::Redactor to perform redaction' do
+        expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original
 
-      renderer.render([object], :note)
-    end
+        renderer.render([object], :note)
+      end
 
-    it 'retrieves field content using Banzai.render_field' do
-      expect(Banzai).to receive(:render_field).with(object, :note).and_call_original
+      it 'retrieves field content using Banzai::Renderer.render_field' do
+        expect(Banzai::Renderer).to receive(:render_field).with(object, :note).and_call_original
 
-      renderer.render([object], :note)
-    end
+        renderer.render([object], :note)
+      end
 
-    it 'passes context to PostProcessPipeline' do
-      another_user = create(:user)
-      another_project = create(:project)
-      object = Note.new(
-        note: 'hello',
-        note_html: 'hello',
-        author: another_user,
-        project: another_project
-      )
-
-      expect(Banzai::Pipeline::PostProcessPipeline).to receive(:to_document).with(
-        anything,
-        hash_including(
-          skip_redaction: true,
-          current_user: user,
-          project: another_project,
+      it 'passes context to PostProcessPipeline' do
+        another_user = create(:user)
+        another_project = create(:project)
+        object = Note.new(
+          note: 'hello',
+          note_html: 'hello',
           author: another_user,
-          custom_value: 'value'
+          project: another_project
         )
-      ).and_call_original
 
-      renderer.render([object], :note)
+        expect(Banzai::Pipeline::PostProcessPipeline).to receive(:to_document).with(
+          anything,
+          hash_including(
+            skip_redaction: true,
+            current_user: user,
+            project: another_project,
+            author: another_user,
+            custom_value: 'value'
+          )
+        ).and_call_original
+
+        renderer.render([object], :note)
+      end
+    end
+
+    context 'without cache' do
+      let(:commit) { project.commit }
+
+      it 'renders and redacts an Array of objects' do
+        renderer.render([commit], :title)
+
+        expect(commit.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'")
+      end
+
+      it 'calls Banzai::Redactor to perform redaction' do
+        expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original
+
+        renderer.render([commit], :title)
+      end
+
+      it 'retrieves field content using Banzai::Renderer.cacheless_render_field' do
+        expect(Banzai::Renderer).to receive(:cacheless_render_field).with(commit, :title).and_call_original
+
+        renderer.render([commit], :title)
+      end
     end
   end
 end
diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb
index 0e094405e33..da42272bbef 100644
--- a/spec/lib/banzai/renderer_spec.rb
+++ b/spec/lib/banzai/renderer_spec.rb
@@ -4,6 +4,7 @@ describe Banzai::Renderer do
   def fake_object(fresh:)
     object = double('object')
 
+    allow(object).to receive(:respond_to?).with(:cached_markdown_fields).and_return(true)
     allow(object).to receive(:cached_html_up_to_date?).with(:field).and_return(fresh)
     allow(object).to receive(:cached_html_for).with(:field).and_return('field_html')
 
@@ -12,25 +13,38 @@ describe Banzai::Renderer do
 
   describe '#render_field' do
     let(:renderer) { described_class }
-    subject { renderer.render_field(object, :field) }
 
-    context 'with a stale cache' do
-      let(:object) { fake_object(fresh: false) }
+    context 'without cache' do
+      let(:commit) { create(:project, :repository).commit }
 
-      it 'caches and returns the result' do
-        expect(object).to receive(:refresh_markdown_cache!).with(do_update: true)
+      it 'returns cacheless render field' do
+        expect(renderer).to receive(:cacheless_render_field).with(commit, :title)
 
-        is_expected.to eq('field_html')
+        renderer.render_field(commit, :title)
       end
     end
 
-    context 'with an up-to-date cache' do
-      let(:object) { fake_object(fresh: true) }
+    context 'with cache' do
+      subject { renderer.render_field(object, :field) }
 
-      it 'uses the cache' do
-        expect(object).to receive(:refresh_markdown_cache!).never
+      context 'with a stale cache' do
+        let(:object) { fake_object(fresh: false) }
 
-        is_expected.to eq('field_html')
+        it 'caches and returns the result' do
+          expect(object).to receive(:refresh_markdown_cache!).with(do_update: true)
+
+          is_expected.to eq('field_html')
+        end
+      end
+
+      context 'with an up-to-date cache' do
+        let(:object) { fake_object(fresh: true) }
+
+        it 'uses the cache' do
+          expect(object).to receive(:refresh_markdown_cache!).never
+
+          is_expected.to eq('field_html')
+        end
       end
     end
   end