Merge branch 'id-avoid-preloading-merge-request-commits' into 'master'

Execute limited request for diff commits instead of preloading

See merge request gitlab-org/gitlab!19485

app/controllers/concerns/issuable_collections.rb

@@ -148,7 +148,7 @@ module IssuableCollections
                                 when 'Issue'
                                   common_attributes + [:project, project: :namespace]
                                 when 'MergeRequest'
-                                  common_attributes + [:target_project, source_project: :route, head_pipeline: :project, target_project: :namespace, latest_merge_request_diff: :merge_request_diff_commits]
+                                  common_attributes + [:target_project, :latest_merge_request_diff, source_project: :route, head_pipeline: :project, target_project: :namespace]
                                 end
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables

app/models/merge_request.rb

@@ -212,8 +212,8 @@ class MergeRequest < ApplicationRecord
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
   scope :with_api_entity_associations, -> {
-    preload(:assignees, :author, :unresolved_notes, :labels, :milestone, :timelogs,
-            latest_merge_request_diff: [:merge_request_diff_commits],
+    preload(:assignees, :author, :unresolved_notes, :labels, :milestone,
+            :timelogs, :latest_merge_request_diff,
             metrics: [:latest_closed_by, :merged_by],
             target_project: [:route, { namespace: :route }],
             source_project: [:route, { namespace: :route }])
@@ -396,14 +396,17 @@ class MergeRequest < ApplicationRecord
     end
   end
 
-  def commit_shas
-    if persisted?
-      merge_request_diff.commit_shas
-    elsif compare_commits
-      compare_commits.to_a.reverse.map(&:sha)
-    else
-      Array(diff_head_sha)
-    end
+  def commit_shas(limit: nil)
+    return merge_request_diff.commit_shas(limit: limit) if persisted?
+
+    shas =
+      if compare_commits
+        compare_commits.to_a.reverse.map(&:sha)
+      else
+        Array(diff_head_sha)
+      end
+
+    limit ? shas.take(limit) : shas
   end
 
   # Returns true if there are commits that match at least one commit SHA.
@@ -913,7 +916,7 @@ class MergeRequest < ApplicationRecord
 
   def commit_notes
     # Fetch comments only from last 100 commits
-    commit_ids = commit_shas.take(100)
+    commit_ids = commit_shas(limit: 100)
 
     Note
       .user

app/models/merge_request_diff.rb

@@ -218,7 +218,7 @@ class MergeRequestDiff < ApplicationRecord
   end
 
   def last_commit_sha
-    commit_shas.first
+    commit_shas(limit: 1).first
   end
 
   def first_commit
@@ -247,8 +247,8 @@ class MergeRequestDiff < ApplicationRecord
     project.commit_by(oid: head_commit_sha)
   end
 
-  def commit_shas
-    merge_request_diff_commits.map(&:sha)
+  def commit_shas(limit: nil)
+    merge_request_diff_commits.limit(limit).pluck(:sha)
   end
 
   def commits_by_shas(shas)

changelogs/unreleased/id-avoid-preloading-merge-request-commits.yml

+---
+title: Execute limited request for diff commits instead of preloading
+merge_request: 19485
+author:
+type: performance

spec/models/merge_request_diff_spec.rb

@@ -378,6 +378,14 @@ describe MergeRequestDiff do
       expect(diff_with_commits.commit_shas).not_to be_empty
       expect(diff_with_commits.commit_shas).to all(match(/\h{40}/))
     end
+
+    context 'with limit attribute' do
+      it 'returns limited number of shas' do
+        expect(diff_with_commits.commit_shas(limit: 2).size).to eq(2)
+        expect(diff_with_commits.commit_shas(limit: 100).size).to eq(29)
+        expect(diff_with_commits.commit_shas.size).to eq(29)
+      end
+    end
   end
 
   describe '#compare_with' do

spec/models/merge_request_spec.rb

@@ -1261,13 +1261,49 @@ describe MergeRequest do
   end
 
   describe '#commit_shas' do
-    before do
-      allow(subject.merge_request_diff).to receive(:commit_shas)
-        .and_return(['sha1'])
+    context 'persisted merge request' do
+      context 'with a limit' do
+        it 'returns a limited number of commit shas' do
+          expect(subject.commit_shas(limit: 2)).to eq(%w[
+            b83d6e391c22777fca1ed3012fce84f633d7fed0 498214de67004b1da3d820901307bed2a68a8ef6
+          ])
+        end
+      end
+
+      context 'without a limit' do
+        it 'returns all commit shas of the merge request diff' do
+          expect(subject.commit_shas.size).to eq(29)
+        end
+      end
     end
 
-    it 'delegates to merge request diff' do
-      expect(subject.commit_shas).to eq ['sha1']
+    context 'new merge request' do
+      subject { build(:merge_request) }
+
+      context 'compare commits' do
+        before do
+          subject.compare_commits = [
+            double(sha: 'sha1'), double(sha: 'sha2')
+          ]
+        end
+
+        context 'without a limit' do
+          it 'returns all shas of compare commits' do
+            expect(subject.commit_shas).to eq(%w[sha2 sha1])
+          end
+        end
+
+        context 'with a limit' do
+          it 'returns a limited number of shas' do
+            expect(subject.commit_shas(limit: 1)).to eq(['sha2'])
+          end
+        end
+      end
+
+      it 'returns diff_head_sha as an array' do
+        expect(subject.commit_shas).to eq([subject.diff_head_sha])
+        expect(subject.commit_shas(limit: 2)).to eq([subject.diff_head_sha])
+      end
     end
   end