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

app/controllers/concerns/renders_commits.rb

+module RendersCommits
+  def prepare_commits_for_rendering(commits)
+    Banzai::CommitRenderer.render(commits, @project, current_user)
+
+    commits
+  end
+end

app/controllers/projects/commits_controller.rb

@@ -2,6 +2,7 @@ require "base64"
 
 class Projects::CommitsController < Projects::ApplicationController
   include ExtractsPath
+  include RendersCommits
 
   before_action :require_non_empty_project
   before_action :assign_ref_vars
@@ -56,5 +57,7 @@ class Projects::CommitsController < Projects::ApplicationController
       else
         @repository.commits(@ref, path: @path, limit: @limit, offset: @offset)
       end
+
+    @commits = prepare_commits_for_rendering(@commits)
   end
 end

app/controllers/projects/compare_controller.rb

@@ -3,6 +3,7 @@ require 'addressable/uri'
 class Projects::CompareController < Projects::ApplicationController
   include DiffForPath
   include DiffHelper
+  include RendersCommits
 
   # Authorize
   before_action :require_non_empty_project
@@ -50,7 +51,7 @@ class Projects::CompareController < Projects::ApplicationController
       .execute(@project, @start_ref)
 
     if @compare
-      @commits = @compare.commits
+      @commits = prepare_commits_for_rendering(@compare.commits)
       @diffs = @compare.diffs(diff_options)
 
       environment_params = @repository.branch_exists?(@head_ref) ? { ref: @head_ref } : { commit: @compare.commit }

app/controllers/projects/merge_requests/creations_controller.rb

 class Projects::MergeRequests::CreationsController < Projects::MergeRequests::ApplicationController
   include DiffForPath
   include DiffHelper
+  include RendersCommits
 
   skip_before_action :merge_request
   skip_before_action :ensure_ref_fetched
@@ -107,7 +108,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
 
     @target_project = @merge_request.target_project
     @source_project = @merge_request.source_project
-    @commits = @merge_request.commits
+    @commits = prepare_commits_for_rendering(@merge_request.commits)
     @commit = @merge_request.diff_head_commit
 
     @note_counts = Note.where(commit_id: @commits.map(&:id))

app/controllers/projects/merge_requests_controller.rb

@@ -2,6 +2,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   include ToggleSubscriptionAction
   include IssuableActions
   include RendersNotes
+  include RendersCommits
   include ToggleAwardEmoji
   include IssuableCollections
 
@@ -94,7 +95,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   def commits
     # Get commits from repository
     # or from cache if already merged
-    @commits = @merge_request.commits
+    @commits = prepare_commits_for_rendering(@merge_request.commits)
     @note_counts = Note.where(commit_id: @commits.map(&:id))
       .group(:commit_id).count
 

app/controllers/search_controller.rb

@@ -2,6 +2,7 @@ class SearchController < ApplicationController
   skip_before_action :authenticate_user!
 
   include SearchHelper
+  include RendersCommits
 
   layout 'search'
 
@@ -20,6 +21,8 @@ class SearchController < ApplicationController
     @search_results = search_service.search_results
     @search_objects = search_service.search_objects
 
+    render_commits if @scope == 'commits'
+
     check_single_commit_result
   end
 
@@ -38,6 +41,10 @@ class SearchController < ApplicationController
 
   private
 
+  def render_commits
+    @search_objects = prepare_commits_for_rendering(@search_objects)
+  end
+
   def check_single_commit_result
     if @search_results.single_commit_result?
       only_commit = @search_results.objects('commits').first

app/helpers/markup_helper.rb

@@ -21,25 +21,28 @@ module MarkupHelper
   end
 
   # Use this in places where you would normally use link_to(gfm(...), ...).
-  #
+  def link_to_markdown(body, url, html_options = {})
+    return '' if body.blank?
+
+    link_to_html(markdown(body, pipeline: :single_line), url, html_options)
+  end
+
+  def link_to_markdown_field(object, field, url, html_options = {})
+    rendered_field = markdown_field(object, field)
+
+    link_to_html(rendered_field, url, html_options)
+  end
+
   # It solves a problem occurring with nested links (i.e.
   # "<a>outer text <a>gfm ref</a> more outer text</a>"). This will not be
   # interpreted as intended. Browsers will parse something like
   # "<a>outer text </a><a>gfm ref</a> more outer text" (notice the last part is
-  # not linked any more). link_to_gfm corrects that. It wraps all parts to
+  # not linked any more). link_to_html corrects that. It wraps all parts to
   # explicitly produce the correct linking behavior (i.e.
   # "<a>outer text </a><a>gfm ref</a><a> more outer text</a>").
-  def link_to_gfm(body, url, html_options = {})
-    return '' if body.blank?
+  def link_to_html(redacted, url, html_options = {})
+    fragment = Nokogiri::HTML::DocumentFragment.parse(redacted)
 
-    context = {
-      project: @project,
-      current_user: (current_user if defined?(current_user)),
-      pipeline: :single_line
-    }
-    gfm_body = Banzai.render(body, context)
-
-    fragment = Nokogiri::HTML::DocumentFragment.parse(gfm_body)
     if fragment.children.size == 1 && fragment.children[0].name == 'a'
       # Fragment has only one node, and it's a link generated by `gfm`.
       # Replace it with our requested link.
@@ -82,7 +85,10 @@ module MarkupHelper
 
   def markdown_field(object, field)
     object = object.for_display if object.respond_to?(:for_display)
+    redacted_field_html = object.try(:"redacted_#{field}_html")
+
     return '' unless object.present?
+    return redacted_field_html if redacted_field_html
 
     html = Banzai.render_field(object, field)
     prepare_for_rendering(html, object.banzai_render_context(field))

app/models/commit.rb

@@ -16,6 +16,8 @@ class Commit
   participant :notes_with_associations
 
   attr_accessor :project, :author
+  attr_accessor :redacted_description_html
+  attr_accessor :redacted_title_html
 
   DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines]
 
@@ -26,6 +28,13 @@ class Commit
   # The SHA can be between 7 and 40 hex characters.
   COMMIT_SHA_PATTERN = '\h{7,40}'.freeze
 
+  def banzai_render_context(field)
+    context = { pipeline: :single_line, project: self.project }
+    context[:author] = self.author if self.author
+
+    context
+  end
+
   class << self
     def decorate(commits, project)
       commits.map do |commit|

app/views/projects/blame/show.html.haml

@@ -22,7 +22,7 @@
                   = author_avatar(commit, size: 36)
                   .commit-row-title
                     %span.item-title.str-truncated-100
-                      = link_to_gfm commit.title, project_commit_path(@project, commit.id), class: "cdark", title: commit.title
+                      = link_to_markdown commit.title, project_commit_path(@project, commit.id), class: "cdark", title: commit.title
                     .pull-right
                       = link_to commit.short_id, project_commit_path(@project, commit), class: "commit-sha"
                      

app/views/projects/branches/_commit.html.haml

@@ -4,6 +4,6 @@
   = link_to commit.short_id, project_commit_path(project, commit.id), class: "commit-sha"
   ·
   %span.str-truncated
-    = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message"
+    = link_to_markdown commit.title, project_commit_path(project, commit.id), class: "commit-row-message"
   ·
   #{time_ago_with_tooltip(commit.committed_date)}

app/views/projects/commits/_commit.html.haml

@@ -16,7 +16,7 @@
 
     .commit-detail
       .commit-content
-        = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message item-title"
+        = link_to_markdown_field(commit, :title, project_commit_path(project, commit.id), class: "commit-row-message item-title")
         %span.commit-row-message.visible-xs-inline
           ·
           = commit.short_id
@@ -28,7 +28,8 @@
 
         - if commit.description?
           %pre.commit-row-description.js-toggle-content
-            = preserve(markdown(commit.description, pipeline: :single_line, author: commit.author))
+            = preserve(markdown_field(commit, :description))
+
         .commiter
           - commit_author_link = commit_author_link(commit, avatar: false, size: 24)
           - commit_timeago = time_ago_with_tooltip(commit.committed_date)

app/views/projects/commits/_inline_commit.html.haml

@@ -3,6 +3,6 @@
     = link_to commit.short_id, project_commit_path(project, commit), class: "commit-sha"
      
     %span.str-truncated
-      = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message"
+      = link_to_markdown_field(commit, :title, project_commit_path(project, commit.id), class: "commit-row-message")
     .pull-right
       #{time_ago_with_tooltip(commit.committed_date)}

app/views/projects/deployments/_commit.html.haml

@@ -12,6 +12,6 @@
       %span.flex-truncate-child
         - if commit_title = deployment.commit_title
           = author_avatar(deployment.commit, size: 20)
-          = link_to_gfm commit_title, project_commit_path(@project, deployment.sha), class: "commit-row-message"
+          = link_to_markdown commit_title, project_commit_path(@project, deployment.sha), class: "commit-row-message"
         - else
           Cant find HEAD commit for this branch

app/views/projects/tree/_tree_commit_column.html.haml

 %span.str-truncated
-  = link_to_gfm commit.full_title, project_commit_path(@project, commit.id), class: "tree-commit-link"
+  = link_to_markdown commit.full_title, project_commit_path(@project, commit.id), class: "tree-commit-link"

app/views/shared/notes/_note.html.haml

@@ -28,7 +28,7 @@
                 commented
               - if note.system
                 %span.system-note-message
-                  = note.redacted_note_html
+                  = markdown_field(note, :note)
               %a{ href: "##{dom_id(note)}" }
                 = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago')
         - unless note.system?
@@ -39,7 +39,7 @@
               = render 'projects/notes/actions', note: note, note_editable: note_editable
       .note-body{ class: note_editable ? 'js-task-list-container' : '' }
         .note-text.md
-          = note.redacted_note_html
+          = markdown_field(note, :note)
         = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago')
         .original-note-content.hidden{ data: { post_url: note_url(note), target_id: note.noteable.id, target_type: note.noteable.class.name.underscore } }
           #{note.note}

app/views/shared/projects/_project.html.haml

@@ -31,8 +31,7 @@
 
       - if show_last_commit_as_description
         .description.prepend-top-5
-          = link_to_gfm project.commit.title, project_commit_path(project, project.commit),
-            class: "commit-row-message"
+          = link_to_markdown(project.commit.title, project_commit_path(project, project.commit), class: "commit-row-message")
       - elsif project.description.present?
         .description.prepend-top-5
           = markdown_field(project, :description)

changelogs/unreleased/34509-improves-markdown-rendering-performance-for-commits-list.yml

+---
+title: Improves markdown rendering performance for commit lists.
+merge_request:
+author:
+type: other

config/initializers/8_metrics.rb

@@ -114,9 +114,6 @@ def instrument_classes(instrumentation)
   # This is a Rails scope so we have to instrument it manually.
   instrumentation.instrument_method(Project, :visible_to_user)
 
-  # Needed for https://gitlab.com/gitlab-org/gitlab-ce/issues/34509
-  instrumentation.instrument_method(MarkupHelper, :link_to_gfm)
-
   # Needed for https://gitlab.com/gitlab-org/gitlab-ce/issues/30224#note_32306159
   instrumentation.instrument_instance_method(MergeRequestDiff, :load_commits)
 

lib/banzai/commit_renderer.rb

+module Banzai
+  module CommitRenderer
+    ATTRIBUTES = [:description, :title].freeze
+
+    def self.render(commits, project, user = nil)
+      obj_renderer = ObjectRenderer.new(project, user)
+
+      ATTRIBUTES.each { |attr| obj_renderer.render(commits, attr) }
+    end
+  end
+end

lib/banzai/object_renderer.rb

@@ -38,7 +38,7 @@ module Banzai
       objects.each_with_index do |object, index|
         redacted_data = redacted[index]
         object.__send__("redacted_#{attribute}_html=", redacted_data[:document].to_html.html_safe) # rubocop:disable GitlabSecurity/PublicSend
-        object.user_visible_reference_count = redacted_data[:visible_reference_count]
+        object.user_visible_reference_count = redacted_data[:visible_reference_count] if object.respond_to?(:user_visible_reference_count)
       end
     end
 

lib/banzai/renderer.rb

@@ -36,6 +36,10 @@ module Banzai
     # The context to use is managed by the object and cannot be changed.
     # Use #render, passing it the field text, if a custom rendering is needed.
     def self.render_field(object, field)
+      unless object.respond_to?(:cached_markdown_fields)
+        return cacheless_render_field(object, field)
+      end
+
       object.refresh_markdown_cache!(do_update: update_object?(object)) unless object.cached_html_up_to_date?(field)
 
       object.cached_html_for(field)

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')

spec/lib/banzai/commit_renderer_spec.rb

+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

spec/lib/banzai/object_renderer_spec.rb

 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
+    context 'with cache' do
       it 'renders and redacts an Array of objects' do
         renderer.render([object], :note)
 
@@ -20,8 +21,8 @@ describe Banzai::ObjectRenderer do
         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
@@ -50,4 +51,27 @@ describe Banzai::ObjectRenderer do
         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

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,6 +13,18 @@ describe Banzai::Renderer do
 
   describe '#render_field' do
     let(:renderer) { described_class }
+
+    context 'without cache' do
+      let(:commit) { create(:project, :repository).commit }
+
+      it 'returns cacheless render field' do
+        expect(renderer).to receive(:cacheless_render_field).with(commit, :title)
+
+        renderer.render_field(commit, :title)
+      end
+    end
+
+    context 'with cache' do
       subject { renderer.render_field(object, :field) }
 
       context 'with a stale cache' do
@@ -34,4 +47,5 @@ describe Banzai::Renderer do
         end
       end
     end
+  end
 end