Execute limited request for diff commits instead of preloading

The number of diff commits can be overwhelming sometimes and
preloading them and iterating over them using map
can be very expensive

We'd better make multiple limited requests (even if it's N+1)
then running into a subtle case when thousands of elements
are loaded into memory and iterated

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
+  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
+    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
+
+    context 'new merge request' do
+      subject { build(:merge_request) }
+
+      context 'compare commits' do
         before do
-      allow(subject.merge_request_diff).to receive(:commit_shas)
-        .and_return(['sha1'])
+          subject.compare_commits = [
+            double(sha: 'sha1'), double(sha: 'sha2')
+          ]
         end
 
-    it 'delegates to merge request diff' do
-      expect(subject.commit_shas).to eq ['sha1']
+        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