Merge branch 'id-load-limited-commits-for-commits-tab' into 'master'

Load recent commits from database to display on Commits tab

See merge request gitlab-org/gitlab!19631

app/controllers/concerns/renders_commits.rb

 # frozen_string_literal: true
 
 module RendersCommits
-  def limited_commits(commits)
-    if commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
+  def limited_commits(commits, commits_count)
+    if commits_count > MergeRequestDiff::COMMITS_SAFE_SIZE
       [
         commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE),
-        commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE
+        commits_count - MergeRequestDiff::COMMITS_SAFE_SIZE
       ]
     else
       [commits, 0]
@@ -14,9 +14,10 @@ module RendersCommits
 
   # This is used as a helper method in a controller.
   # rubocop: disable Gitlab/ModuleWithInstanceVariables
-  def set_commits_for_rendering(commits)
-    @total_commit_count = commits.size
-    limited, @hidden_commit_count = limited_commits(commits)
+  def set_commits_for_rendering(commits, commits_count: nil)
+    @total_commit_count = commits_count || commits.size
+    limited, @hidden_commit_count = limited_commits(commits, @total_commit_count)
+    commits.each(&:lazy_author) # preload authors
     prepare_commits_for_rendering(limited)
   end
   # rubocop: enable Gitlab/ModuleWithInstanceVariables

app/controllers/projects/merge_requests/creations_controller.rb

@@ -109,7 +109,13 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
 
     @target_project = @merge_request.target_project
     @source_project = @merge_request.source_project
-    @commits = set_commits_for_rendering(@merge_request.commits)
+
+    @commits =
+      set_commits_for_rendering(
+        @merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch),
+          commits_count: @merge_request.commits_count
+      )
+
     @commit = @merge_request.diff_head_commit
 
     # FIXME: We have to assign a presenter to another instance variable

app/controllers/projects/merge_requests_controller.rb

@@ -90,7 +90,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     # Get commits from repository
     # or from cache if already merged
     @commits =
-      set_commits_for_rendering(@merge_request.commits.with_latest_pipeline)
+      set_commits_for_rendering(
+        @merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch),
+          commits_count: @merge_request.commits_count
+      )
 
     render json: { html: view_to_html_string('projects/merge_requests/_commits') }
   end

app/models/merge_request.rb

@@ -374,11 +374,12 @@ class MergeRequest < ApplicationRecord
     "#{project.to_reference(from, full: full)}#{reference}"
   end
 
-  def commits
-    return merge_request_diff.commits if persisted?
+  def commits(limit: nil)
+    return merge_request_diff.commits(limit: limit) if persisted?
 
     commits_arr = if compare_commits
-                    compare_commits.reverse
+                    reversed_commits = compare_commits.reverse
+                    limit ? reversed_commits.take(limit) : reversed_commits
                   else
                     []
                   end
@@ -386,6 +387,10 @@ class MergeRequest < ApplicationRecord
     CommitCollection.new(source_project, commits_arr, source_branch)
   end
 
+  def recent_commits
+    commits(limit: MergeRequestDiff::COMMITS_SAFE_SIZE)
+  end
+
   def commits_count
     if persisted?
       merge_request_diff.commits_count

app/models/merge_request_diff.rb

@@ -213,8 +213,10 @@ class MergeRequestDiff < ApplicationRecord
     end
   end
 
-  def commits
-    @commits ||= load_commits
+  def commits(limit: nil)
+    strong_memoize(:"commits_#{limit || 'all'}") do
+      load_commits(limit: limit)
+    end
   end
 
   def last_commit_sha
@@ -529,8 +531,9 @@ class MergeRequestDiff < ApplicationRecord
     end
   end
 
-  def load_commits
-    commits = merge_request_diff_commits.map { |commit| Commit.from_hash(commit.to_hash, project) }
+  def load_commits(limit: nil)
+    commits = merge_request_diff_commits.limit(limit)
+      .map { |commit| Commit.from_hash(commit.to_hash, project) }
 
     CommitCollection
       .new(merge_request.source_project, commits, merge_request.source_branch)

spec/controllers/concerns/renders_commits_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RendersCommits do
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+  let_it_be(:user) { create(:user) }
+
+  controller(ApplicationController) do
+    # `described_class` is not available in this context
+    include RendersCommits # rubocop:disable RSpec/DescribedClass
+
+    def index
+      @merge_request = MergeRequest.find(params[:id])
+      @commits = set_commits_for_rendering(
+        @merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch),
+          commits_count: @merge_request.commits_count
+      )
+
+      render json: { html: view_to_html_string('projects/merge_requests/_commits') }
+    end
+  end
+
+  before do
+    sign_in(user)
+  end
+
+  def go
+    get :index, params: { id: merge_request.id }
+  end
+
+  it 'sets instance variables for counts' do
+    stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 10)
+
+    go
+
+    expect(assigns[:total_commit_count]).to eq(29)
+    expect(assigns[:hidden_commit_count]).to eq(19)
+    expect(assigns[:commits].size).to eq(10)
+  end
+
+  context 'rendering commits' do
+    render_views
+
+    it 'avoids N + 1' do
+      stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 5)
+
+      control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+        go
+      end.count
+
+      stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 15)
+
+      expect do
+        go
+      end.not_to exceed_all_query_limit(control_count)
+    end
+  end
+end

spec/models/merge_request_spec.rb

@@ -3484,4 +3484,67 @@ describe MergeRequest do
   end
 
   it_behaves_like 'versioned description'
+
+  describe '#commits' do
+    context 'persisted merge request' do
+      context 'with a limit' do
+        it 'returns a limited number of commits' do
+          expect(subject.commits(limit: 2).map(&:sha)).to eq(%w[
+            b83d6e391c22777fca1ed3012fce84f633d7fed0
+            498214de67004b1da3d820901307bed2a68a8ef6
+          ])
+          expect(subject.commits(limit: 3).map(&:sha)).to eq(%w[
+            b83d6e391c22777fca1ed3012fce84f633d7fed0
+            498214de67004b1da3d820901307bed2a68a8ef6
+            1b12f15a11fc6e62177bef08f47bc7b5ce50b141
+          ])
+        end
+      end
+
+      context 'without a limit' do
+        it 'returns all commits of the merge request diff' do
+          expect(subject.commits.size).to eq(29)
+        end
+      end
+    end
+
+    context 'new merge request' do
+      subject { build(:merge_request) }
+
+      context 'compare commits' do
+        let(:first_commit) { double }
+        let(:second_commit) { double }
+
+        before do
+          subject.compare_commits = [
+            first_commit, second_commit
+          ]
+        end
+
+        context 'without a limit' do
+          it 'returns all the compare commits' do
+            expect(subject.commits.to_a).to eq([second_commit, first_commit])
+          end
+        end
+
+        context 'with a limit' do
+          it 'returns a limited number of commits' do
+            expect(subject.commits(limit: 1).to_a).to eq([second_commit])
+          end
+        end
+      end
+    end
+  end
+
+  describe '#recent_commits' do
+    before do
+      stub_const("#{MergeRequestDiff}::COMMITS_SAFE_SIZE", 2)
+    end
+
+    it 'returns the safe number of commits' do
+      expect(subject.recent_commits.map(&:sha)).to eq(%w[
+        b83d6e391c22777fca1ed3012fce84f633d7fed0 498214de67004b1da3d820901307bed2a68a8ef6
+      ])
+    end
+  end
 end