Merge branch 'id-limit-commits-for-refresh-mr-service' into 'master'

Load commits in batches for MR RefreshService

See merge request gitlab-org/gitlab!20358

app/models/merge_request.rb

@@ -420,15 +420,6 @@ class MergeRequest < ApplicationRecord
     limit ? shas.take(limit) : shas
   end
 
-  # Returns true if there are commits that match at least one commit SHA.
-  def includes_any_commits?(shas)
-    if persisted?
-      merge_request_diff.commits_by_shas(shas).exists?
-    else
-      (commit_shas & shas).present?
-    end
-  end
-
   def supports_suggestion?
     true
   end

app/models/merge_request_diff.rb

@@ -10,6 +10,7 @@ class MergeRequestDiff < ApplicationRecord
 
   # Don't display more than 100 commits at once
   COMMITS_SAFE_SIZE = 100
+  BATCH_SIZE = 1000
 
   # Applies to closed or merged MRs when determining whether to migrate their
   # diffs to external storage
@@ -254,10 +255,14 @@ class MergeRequestDiff < ApplicationRecord
     merge_request_diff_commits.limit(limit).pluck(:sha)
   end
 
-  def commits_by_shas(shas)
-    return MergeRequestDiffCommit.none unless shas.present?
+  def includes_any_commits?(shas)
+    return false if shas.blank?
 
-    merge_request_diff_commits.where(sha: shas)
+    # when the number of shas is huge (1000+) we don't want
+    # to pass them all as an SQL param, let's pass them in batches
+    shas.each_slice(BATCH_SIZE).any? do |batched_shas|
+      merge_request_diff_commits.where(sha: batched_shas).exists?
+    end
   end
 
   def diff_refs=(new_diff_refs)

app/services/merge_requests/refresh_service.rb

@@ -106,7 +106,7 @@ module MergeRequests
       filter_merge_requests(merge_requests).each do |merge_request|
         if branch_and_project_match?(merge_request) || @push.force_push?
           merge_request.reload_diff(current_user)
-        elsif merge_request.includes_any_commits?(push_commit_ids)
+        elsif merge_request.merge_request_diff.includes_any_commits?(push_commit_ids)
           merge_request.reload_diff(current_user)
         end
 

spec/models/merge_request_diff_spec.rb

@@ -426,24 +426,38 @@ describe MergeRequestDiff do
     end
   end
 
-  describe '#commits_by_shas' do
-    let(:commit_shas) { diff_with_commits.commit_shas }
-
-    it 'returns empty if no SHAs were provided' do
-      expect(diff_with_commits.commits_by_shas([])).to be_empty
+  describe '#includes_any_commits?' do
+    let(:non_existent_shas) do
+      Array.new(30) { Digest::SHA1.hexdigest(SecureRandom.hex) }
     end
 
-    it 'returns one SHA' do
-      commits = diff_with_commits.commits_by_shas([commit_shas.first, Gitlab::Git::BLANK_SHA])
+    subject { diff_with_commits }
+
+    context 'processes the passed shas in batches' do
+      context 'number of existing commits is greater than batch size' do
+        it 'performs a separate request for each batch' do
+          stub_const('MergeRequestDiff::BATCH_SIZE', 5)
+
+          commit_shas = subject.commit_shas
+
+          query_count = ActiveRecord::QueryRecorder.new do
+            subject.includes_any_commits?(non_existent_shas + commit_shas)
+          end.count
+
+          expect(query_count).to eq(7)
+        end
+      end
+    end
 
-      expect(commits.count).to eq(1)
+    it 'returns false if passed commits do not exist' do
+      expect(subject.includes_any_commits?([])).to eq(false)
+      expect(subject.includes_any_commits?([Gitlab::Git::BLANK_SHA])).to eq(false)
     end
 
-    it 'returns all matching SHAs' do
-      commits = diff_with_commits.commits_by_shas(commit_shas)
+    it 'returns true if passed commits exists' do
+      args_with_existing_commits = non_existent_shas << subject.head_commit_sha
 
-      expect(commits.count).to eq(commit_shas.count)
-      expect(commits.map(&:sha)).to match_array(commit_shas)
+      expect(subject.includes_any_commits?(args_with_existing_commits)).to eq(true)
     end
   end
 

spec/models/merge_request_spec.rb

@@ -3134,36 +3134,6 @@ describe MergeRequest do
     end
   end
 
-  describe '#includes_any_commits?' do
-    it 'returns false' do
-      expect(subject.includes_any_commits?([])).to be_falsey
-    end
-
-    it 'returns false' do
-      expect(subject.includes_any_commits?([Gitlab::Git::BLANK_SHA])).to be_falsey
-    end
-
-    it 'returns true' do
-      expect(subject.includes_any_commits?([subject.merge_request_diff.head_commit_sha])).to be_truthy
-    end
-
-    it 'returns true even when there is a non-existent comit' do
-      expect(subject.includes_any_commits?([Gitlab::Git::BLANK_SHA, subject.merge_request_diff.head_commit_sha])).to be_truthy
-    end
-
-    context 'unpersisted merge request' do
-      let(:new_mr) { build(:merge_request) }
-
-      it 'returns false' do
-        expect(new_mr.includes_any_commits?([Gitlab::Git::BLANK_SHA])).to be_falsey
-      end
-
-      it 'returns true' do
-        expect(new_mr.includes_any_commits?([subject.merge_request_diff.head_commit_sha])).to be_truthy
-      end
-    end
-  end
-
   describe '#can_allow_collaboration?' do
     let(:target_project) { create(:project, :public) }
     let(:source_project) { fork_project(target_project) }