Sync code owner approval rules during mergeability check

app/services/merge_requests/reload_merge_head_diff_service.rb

@@ -43,3 +43,5 @@ def recreate_merge_head_diff
     end
   end
 end
+
+MergeRequests::ReloadMergeHeadDiffService.prepend_mod

ee/app/services/ee/merge_requests/reload_merge_head_diff_service.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module ReloadMergeHeadDiffService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute
+        super.tap do |response|
+          next unless response[:status] == :success
+          next unless merge_request.project.licensed_feature_available?(:code_owners)
+          next if merge_request.on_train?
+
+          sync_code_owner_approval_rules
+        end
+      end
+
+      private
+
+      def sync_code_owner_approval_rules
+        ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
+      end
+    end
+  end
+end

ee/lib/gitlab/code_owners.rb

@@ -55,39 +55,11 @@ def self.loader_for_merge_request(merge_request, merge_request_diff)
     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.
-      merge_request_diff ||= merge_head_or_empty_diff(merge_request)
-      if merge_request_diff.overflow?
-        slow_path_lookup(merge_request, merge_request_diff)
-      else
-        fast_path_lookup(merge_request, merge_request_diff)
-      end
+      # NOTE: merge_head_diff is preferred as we want to include latest changes from the target branch
+      merge_request_diff ||= merge_request.merge_head_diff || merge_request.merge_request_diff
+      merge_request_diff.modified_paths(fallback_on_overflow: true)
     end
     private_class_method :paths_for_merge_request
-
-    def self.merge_head_or_empty_diff(merge_request)
-      # NOTE: We need to make sure merge_head_diff gets created first.
-      ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
-
-      merge_request.merge_head_diff || ::MergeRequestDiff.new(merge_request_id: merge_request.id)
-    end
-    private_class_method :merge_head_or_empty_diff
-
-    def self.slow_path_lookup(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

@@ -13,12 +13,9 @@
   before do
     project.add_developer(code_owner)
-
-    allow_next_instance_of(Repository) do |repo|
-      allow(repo).to receive(:code_owners_blob)
+    allow(project.repository).to receive(:code_owners_blob)
       .with(ref: codeowner_lookup_ref)
       .and_return(codeowner_blob)
-    end
   end
   describe '.for_blob' do
@@ -105,43 +102,11 @@
     end
   end
-  describe '.fast_path_lookup and .slow_path_lookup' do
-    let(:codeowner_lookup_ref) { 'with-codeowners' }
-    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 'returns equivalent results' do
-      fast_results = described_class.entries_for_merge_request(merge_request).first
-
-      allow_next_instance_of(MergeRequestDiff) do |mrd|
-        expect(mrd).to receive(:overflow?) { true }
-      end
-
-      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
     subject(:entries) { described_class.entries_for_merge_request(merge_request, merge_request_diff: merge_request_diff) }
     let(:merge_request_diff) { nil }
-    let(:codeowner_lookup_ref) { 'with-codeowners' }
+    let(:codeowner_lookup_ref) { merge_request.target_branch }
     let(:merge_request) do
       create(
         :merge_request,
@@ -157,22 +122,12 @@
         stub_licensed_features(code_owners: true)
       end
-      it 'runs mergeability check to reload merge_head_diff' do
-        mergeability_service = instance_double(MergeRequests::MergeabilityCheckService)
-        expect(mergeability_service).to receive(:execute).with(recheck: true)
-        expect(MergeRequests::MergeabilityCheckService).to receive(:new).with(merge_request) { mergeability_service }
-
-        expect(merge_request).to receive(:merge_head_diff).and_call_original
-
-        entries
-      end
-
       context 'when merge_head_diff exists' do
         before do
-          expect(described_class).to receive(:fast_path_lookup) do |_, mrd|
-            expect(mrd.state).to eq 'collected'
-            expect(mrd.diff_type).to eq 'merge_head'
-          end.and_return(modified_paths)
+          merge_head_diff = instance_double(MergeRequestDiff)
+          expect(merge_head_diff).to receive(:modified_paths).with(fallback_on_overflow: true).and_return(modified_paths)
+          expect(merge_request).to receive(:merge_head_diff).and_return(merge_head_diff)
+          expect(merge_request).not_to receive(:merge_request_diff).and_call_original
         end
         context 'when the changed file paths have matching code owners' do
@@ -193,51 +148,30 @@
       end
       context 'when merge_head_diff does not exist' do
-        it 'falls back to an empty merge_request_diff' do
-          mergeability_service = instance_double(MergeRequests::MergeabilityCheckService)
-          expect(mergeability_service).to receive(:execute).with(recheck: true)
-          expect(MergeRequests::MergeabilityCheckService).to receive(:new).with(merge_request).and_return(mergeability_service)
+        before do
           expect(merge_request).to receive(:merge_head_diff).and_return(nil)
-          expect(described_class).to receive(:fast_path_lookup) do |_, mrd|
-            expect(mrd.state).to eq 'empty'
-          end.and_return([])
-          expect(entries).to be_empty
+        end
+        it 'falls back to merge_request_diff' do
+          expect(merge_request.merge_request_diff).to receive(:modified_paths).with(fallback_on_overflow: true).and_call_original
+          entries
         end
       end
       context 'when merge_request_diff is specified' do
         let(:merge_request_diff) { merge_request.merge_request_diff }
+        let(:modified_paths) { ['docs/CODEOWNERS'] }
         before do
-          expect(described_class).to receive(:fast_path_lookup) do |_, mrd|
-            expect(mrd.state).to eq 'collected'
-            expect(mrd.diff_type).to eq 'regular'
-          end.and_return(['docs/CODEOWNERS'])
+          expect(merge_request_diff).to receive(:modified_paths).with(fallback_on_overflow: true).and_return(modified_paths)
         end
         it 'returns owners at the specified ref' do
-          expect(MergeRequests::MergeabilityCheckService).not_to receive(:new)
-
+          expect(merge_request).not_to receive(:merge_head_diff)
           expect(entries.first).to have_attributes(pattern: 'docs/CODEOWNERS', users: [code_owner])
         end
       end
-
-      context 'when the merge request is large (>1_000 files)' do
-        before do
-          allow_next_instance_of(MergeRequestDiff) do |mrd|
-            allow(mrd).to receive(:overflow?) { true }
-          end
-        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
-
-          entries
-        end
-      end
     end
     context 'when the feature is not available' do

ee/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::ReloadMergeHeadDiffService do
+  let(:merge_request) { create(:merge_request) }
+
+  subject { described_class.new(merge_request).execute }
+
+  describe '#execute' do
+    before do
+      MergeRequests::MergeToRefService
+        .new(project: merge_request.project, current_user: merge_request.author)
+        .execute(merge_request)
+    end
+
+    context 'code_owners feature is available' do
+      before do
+        stub_licensed_features(code_owners: true)
+      end
+
+      context 'when reloading was successful' do
+        context 'when merge request is not on a merge train' do
+          it 'syncs code owner approval rules' do
+            sync_service = instance_double(MergeRequests::SyncCodeOwnerApprovalRules)
+            expect(sync_service).to receive(:execute)
+            expect(MergeRequests::SyncCodeOwnerApprovalRules).to receive(:new)
+              .with(merge_request)
+              .and_return(sync_service)
+            expect(subject[:status]).to eq(:success)
+          end
+        end
+
+        context 'when merge request is on a merge train' do
+          let(:merge_request) { create(:merge_request, :on_train) }
+
+          it 'does not sync code owner approval rules' do
+            expect(MergeRequests::SyncCodeOwnerApprovalRules).not_to receive(:new)
+            expect(subject[:status]).to eq(:success)
+          end
+        end
+      end
+
+      context 'when reloading failed' do
+        before do
+          allow(merge_request).to receive(:create_merge_head_diff!).and_raise('fail')
+        end
+
+        it 'does not sync code owner approval rules' do
+          expect(MergeRequests::SyncCodeOwnerApprovalRules).not_to receive(:new)
+          expect(subject[:status]).to eq(:error)
+        end
+      end
+    end
+
+    context 'code_owners feature is not available' do
+      before do
+        stub_licensed_features(code_owners: false)
+      end
+
+      context 'when reloading was successful' do
+        it 'syncs code owner approval rules' do
+          expect(MergeRequests::SyncCodeOwnerApprovalRules).not_to receive(:new)
+          expect(subject[:status]).to eq(:success)
+        end
+      end
+    end
+  end
+end