Merge branch '34118-add-fallback-method-of-generating-paths-for-codeowner-entries' into 'master'

Add fallback path lookup for large MRs

Closes #34118

See merge request gitlab-org/gitlab!28842

ee/lib/gitlab/code_owners.rb

@@ -30,9 +30,44 @@ module Gitlab
       Loader.new(
         merge_request.target_project,
         merge_request.target_branch,
-        merge_request.modified_paths(past_merge_request_diff: merge_request_diff)
+        paths_for_merge_request(merge_request, merge_request_diff)
       )
     end
     private_class_method :loader_for_merge_request
+
+    def self.paths_for_merge_request(merge_request, merge_request_diff)
+      # Because MergeRequest#modified_paths is limited to only returning 1_000
+      #   records, if the diff_size is more than 1_000, we need to fall back to
+      #   the MUCH slower method of using Repository#diff_stats, which isn't
+      #   subject to the same limit.
+      #
+      if oversized_merge_request?(merge_request, merge_request_diff)
+        slow_path_lookup(merge_request, merge_request_diff)
+      else
+        fast_path_lookup(merge_request, merge_request_diff)
+      end
+    end
+    private_class_method :paths_for_merge_request
+
+    def self.oversized_merge_request?(merge_request, merge_request_diff)
+      mrd_to_check_for_overflow = merge_request_diff || merge_request.merge_request_diff
+
+      mrd_to_check_for_overflow.overflow?
+    end
+
+    def self.slow_path_lookup(merge_request, merge_request_diff)
+      merge_request_diff ||= merge_request.merge_request_diff
+
+      merge_request.project.repository.diff_stats(
+        merge_request_diff.base_commit_sha,
+        merge_request_diff.head_commit_sha
+      ).paths
+    end
+    private_class_method :slow_path_lookup
+
+    def self.fast_path_lookup(merge_request, merge_request_diff)
+      merge_request.modified_paths(past_merge_request_diff: merge_request_diff)
+    end
+    private_class_method :fast_path_lookup
   end
 end

ee/spec/lib/gitlab/code_owners_spec.rb

@@ -44,10 +44,40 @@ describe Gitlab::CodeOwners do
     end
   end
 
+  describe ".fast_path_lookup and .slow_path_lookup" do
+    let(:codeowner_lookup_ref) { merge_request.target_branch }
+    let(:codeowner_content) { 'files/ruby/feature.rb @owner-1' }
+    let(:merge_request) do
+      create(
+        :merge_request,
+        source_project: project,
+        source_branch: 'feature',
+        target_project: project,
+        target_branch: 'with-codeowners'
+      )
+    end
+
+    before do
+      stub_licensed_features(code_owners: true)
+    end
+
+    it "return equivalent results" do
+      fast_results = described_class.entries_for_merge_request(merge_request).first
+
+      expect(merge_request.merge_request_diff).to receive(:overflow?).and_return(true)
+
+      slow_results = described_class.entries_for_merge_request(merge_request).first
+
+      expect(slow_results.users).to eq(fast_results.users)
+      expect(slow_results.groups).to eq(fast_results.groups)
+      expect(slow_results.pattern).to eq(fast_results.pattern)
+    end
+  end
+
   describe '.entries_for_merge_request' do
     let(:codeowner_lookup_ref) { merge_request.target_branch }
     let(:merge_request) do
-      build(
+      create(
         :merge_request,
         source_project: project,
         source_branch: 'feature',
@@ -71,16 +101,28 @@ describe Gitlab::CodeOwners do
       end
 
       context 'when merge_request_diff is specified' do
-        let(:merge_request_diff) { double(:merge_request_diff) }
-
         it 'returns owners at the specified ref' do
-          expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: merge_request_diff).and_return(['docs/CODEOWNERS'])
+          expect(described_class).to receive(:fast_path_lookup).and_call_original
+          expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: merge_request.merge_request_diff).and_return(['docs/CODEOWNERS'])
 
-          entry = described_class.entries_for_merge_request(merge_request, merge_request_diff: merge_request_diff).first
+          entry = described_class.entries_for_merge_request(merge_request, merge_request_diff: merge_request.merge_request_diff).first
 
           expect(entry.users).to eq([code_owner])
         end
       end
+
+      context 'when the merge request is large (>1_000 files)' do
+        before do
+          expect(merge_request.merge_request_diff).to receive(:overflow?).and_return(true)
+        end
+
+        it 'generates paths via .slow_path_lookup' do
+          expect(described_class).not_to receive(:fast_path_lookup)
+          expect(described_class).to receive(:slow_path_lookup).and_call_original
+
+          described_class.entries_for_merge_request(merge_request)
+        end
+      end
     end
 
     context 'when the feature is not available' do
@@ -89,7 +131,7 @@ describe Gitlab::CodeOwners do
       end
 
       it 'skips reading codeowners and returns an empty array' do
-        expect(merge_request).not_to receive(:modified_paths)
+        expect(described_class).not_to receive(:loader_for_merge_request)
 
         expect(described_class.entries_for_merge_request(merge_request)).to eq([])
       end