Create ApprovalMergeRequestRule for code owners

When enabling the `multiple_code_owner_rules` feature, we will create
a separate code owner rule for each pattern mentioned in the
`CODEOWNERS` file.

The code owner rules are updated by the MergeRequests::RefreshService
and MergeRequests::CreateService.

app/models/user.rb

@@ -275,6 +275,7 @@ class User < ApplicationRecord
   scope :confirmed, -> { where.not(confirmed_at: nil) }
   scope :by_username, -> (usernames) { iwhere(username: Array(usernames).map(&:to_s)) }
   scope :for_todos, -> (todos) { where(id: todos.select(:user_id)) }
+  scope :with_emails, -> { preload(:emails) }
 
   # Limits the users to those that have TODOs, optionally in the given state.
   #

ee/app/services/ee/merge_requests/create_service.rb

@@ -9,7 +9,7 @@ module EE
       def after_create(issuable)
         super
 
-        issuable.sync_code_owners_with_approvers
+        ::MergeRequests::SyncCodeOwnerApprovalRules.new(issuable).execute
       end
     end
   end

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -43,7 +43,9 @@ module EE
         if ::Feature.enabled?(:approval_rules, project)
           results = yield
 
-          merge_requests_for_source_branch.each(&:sync_code_owners_with_approvers)
+          merge_requests_for_source_branch.each do |merge_request|
+            ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
+          end
         else
           previous_diffs = fetch_latest_merge_request_diffs
 

ee/app/services/merge_requests/sync_code_owner_approval_rules.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class SyncCodeOwnerApprovalRules
+    def initialize(merge_request, params = {})
+      @merge_request = merge_request
+      @previous_diff = params[:previous_diff]
+    end
+
+    def execute
+      if ::Feature.enabled?(:multiple_code_owner_rules)
+        sync_rules
+      else
+        merge_request.sync_code_owners_with_approvers
+      end
+    end
+
+    private
+
+    attr_reader :merge_request, :previous_diff
+
+    def sync_rules
+      return if merge_request.merged?
+
+      delete_outdated_code_owner_rules
+
+      existing_rules = merge_request.approval_rules.matching_pattern(patterns).to_a
+
+      code_owner_entries.each do |entry|
+        rule = existing_rules.detect { |rule| rule.name == entry.pattern }
+        rule ||= create_rule(entry)
+
+        rule.users = entry.users
+        rule.save
+      end
+    end
+
+    def create_rule(entry)
+      ApprovalMergeRequestRule.find_or_create_code_owner_rule(merge_request, entry.pattern)
+    end
+
+    def delete_outdated_code_owner_rules
+      merge_request.approval_rules.not_matching_pattern(patterns).delete_all
+    end
+
+    def patterns
+      @patterns ||= code_owner_entries.map(&:pattern)
+    end
+
+    def code_owner_entries
+      @code_owner_entries ||= Gitlab::CodeOwners
+                                .entries_for_merge_request(merge_request, merge_request_diff: previous_diff)
+    end
+  end
+end

ee/changelogs/unreleased/bvl-multiple-codeowner-approval-rules.yml

+---
+title: Create merge request approval rule for each code owner entry
+merge_request: 9455
+author:
+type: changed

ee/lib/gitlab/code_owners.rb

@@ -9,23 +9,35 @@ module Gitlab
       if blob.project.feature_available?(:code_owners)
         Loader.new(blob.project, blob.commit_id, blob.path).members
       else
-        User.none
+        []
       end
     end
 
     # @param merge_request [MergeRequest]
     # @param merge_request_diff [MergeRequestDiff]
-    #   Find code owners at a particular MergeRequestDiff.
+    #   Find code owners entries at a particular MergeRequestDiff.
     #   Assumed to be the most recent one if not provided.
+    def self.entries_for_merge_request(merge_request, merge_request_diff: nil)
+      loader_for_merge_request(merge_request, merge_request_diff)&.entries || []
+    end
+
     def self.for_merge_request(merge_request, merge_request_diff: nil)
-      return [] if merge_request.source_project.nil? || merge_request.source_branch.nil?
-      return [] unless merge_request.target_project.feature_available?(:code_owners)
+      loader = loader_for_merge_request(merge_request, merge_request_diff)
+      return [] unless loader
+
+      loader.members.reject { |owner| owner == merge_request.author }
+    end
+
+    def self.loader_for_merge_request(merge_request, merge_request_diff)
+      return if merge_request.source_project.nil? || merge_request.source_branch.nil?
+      return unless merge_request.target_project.feature_available?(:code_owners)
 
       Loader.new(
         merge_request.target_project,
         merge_request.target_branch,
         merge_request.modified_paths(past_merge_request_diff: merge_request_diff)
-      ).members.where_not_in(merge_request.author).to_a
+      )
     end
+    private_class_method :loader_for_merge_request
   end
 end

ee/lib/gitlab/code_owners/entry.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module CodeOwners
+    class Entry
+      Data = Struct.new(:pattern, :owner_line)
+
+      attr_reader :data
+      protected :data
+
+      delegate :pattern, :hash, :owner_line, to: :data
+
+      def initialize(pattern, owner_line)
+        @data = Data.new(pattern, owner_line)
+      end
+
+      def users
+        raise "CodeOwners for #{owner_line} not loaded" unless defined?(@users)
+
+        @users.to_a
+      end
+
+      def add_matching_users_from(new_users)
+        @users ||= Set.new
+
+        matching_users = new_users.select { |u| matching_user?(u) }
+        @users.merge(matching_users)
+      end
+
+      def ==(other)
+        return false unless other.is_a?(self.class)
+
+        data == other.data
+      end
+      alias_method :eql?, :==
+
+      private
+
+      def extractor
+        @extractor ||= Gitlab::UserExtractor.new(owner_line)
+      end
+
+      def emails
+        @emails ||= extractor.emails.map(&:downcase)
+      end
+
+      def usernames
+        @usernames ||= extractor.usernames.map(&:downcase)
+      end
+
+      def matching_user?(user)
+        usernames.include?(user.username.downcase) || matching_emails?(user)
+      end
+
+      def matching_emails?(user)
+        raise "Emails not loaded for #{user}" unless user.emails.loaded?
+
+        emails.any? { |email| user.any_email?(email) }
+      end
+    end
+  end
+end

ee/lib/gitlab/code_owners/file.rb

@@ -15,14 +15,14 @@ module Gitlab
         parsed_data.empty?
       end
 
-      def owners_for_path(path)
+      def entry_for_path(path)
         path = "/#{path}" unless path.start_with?('/')
 
         matching_pattern = parsed_data.keys.reverse.detect do |pattern|
           path_matches?(pattern, path)
         end
 
-        parsed_data[matching_pattern]
+        parsed_data[matching_pattern].dup if matching_pattern
       end
 
       private
@@ -45,9 +45,9 @@ module Gitlab
 
           pattern, _separator, owners = line.partition(/(?<!\\)\s+/)
 
-          pattern = normalize_pattern(pattern)
+          normalized_pattern = normalize_pattern(pattern)
 
-          parsed[pattern] = owners
+          parsed[normalized_pattern] = Entry.new(pattern, owners)
         end
 
         parsed

ee/lib/gitlab/code_owners/loader.rb

@@ -7,29 +7,39 @@ module Gitlab
         @project, @ref, @paths = project, ref, Array(paths)
       end
 
+      def entries
+        return [] if empty_code_owners?
+
+        @entries ||= load_entries
+      end
+
       def members
-        @_members ||= @project.members_among(raw_users)
+        @members ||= entries.map(&:users).flatten.uniq
       end
 
-      def non_members
-        @_non_members ||= raw_users.where_not_in(@project.authorized_users)
+      def empty_code_owners?
+        code_owners_file.empty?
       end
 
-      def raw_users
-        return User.none if empty_code_owners?
+      private
 
-        @_raw_users ||= begin
-          owner_lines = @paths.map { |path| code_owners_file.owners_for_path(path) }
+      def load_entries
+        entries = @paths.map { |path| code_owners_file.entry_for_path(path) }.compact.uniq
+        members = all_members_for_entries(entries)
 
-          Gitlab::UserExtractor.new(owner_lines).users
+        entries.each do |entry|
+          entry.add_matching_users_from(members)
         end
-      end
 
-      def empty_code_owners?
-        code_owners_file.empty?
+        entries
       end
 
-      private
+      def all_members_for_entries(entries)
+        owner_lines = entries.map(&:owner_line)
+        all_users = Gitlab::UserExtractor.new(owner_lines).users.with_emails
+
+        @project.members_among(all_users)
+      end
 
       def code_owners_file
         if RequestStore.active?

ee/spec/lib/gitlab/code_owners/entry_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::CodeOwners::Entry do
+  subject(:entry) { described_class.new('/**/file', '@user jane@gitlab.org') }
+  let(:user) { build(:user, username: 'user') }
+
+  it 'is uniq by the pattern and owner line' do
+    equal_entry = described_class.new('/**/file', '@user jane@gitlab.org')
+    other_entry = described_class.new('/**/other_file', '@user jane@gitlab.org')
+
+    expect(equal_entry).to eq(entry)
+    expect([entry, equal_entry, other_entry].uniq).to contain_exactly(entry, other_entry)
+  end
+
+  describe '#users' do
+    it 'raises an error if no users have been added' do
+      expect { entry.users }.to raise_error(/not loaded/)
+    end
+
+    it 'returns the users in an array' do
+      entry.add_matching_users_from([user])
+
+      expect(entry.users).to eq([user])
+    end
+  end
+
+  describe '#add_matching_users_from' do
+    it 'does not add the same user twice' do
+      2.times { entry.add_matching_users_from([user]) }
+
+      expect(entry.users).to contain_exactly(user)
+    end
+
+    it 'raises an error when adding a user without emails preloaded' do
+      expect { entry.add_matching_users_from([build(:user)]) }.to raise_error(/Emails not loaded/)
+    end
+
+    it 'only adds users mentioned in the owner line' do
+      other_user = create(:user)
+      other_user.emails
+
+      entry.add_matching_users_from([other_user, user])
+
+      expect(entry.users).to contain_exactly(user)
+    end
+
+    it 'adds users by username, case-insensitively' do
+      user = build(:user, username: 'USER')
+
+      entry.add_matching_users_from([user])
+
+      expect(entry.users).to contain_exactly(user)
+    end
+
+    it 'adds users by primary email, case-insensitively' do
+      second_user = create(:user, email: 'JANE@GITLAB.ORG')
+      second_user.emails
+
+      entry.add_matching_users_from([second_user, user])
+
+      expect(entry.users).to contain_exactly(user, second_user)
+    end
+
+    it 'adds users by secondary email, case-insensitively' do
+      second_user = create(:user)
+      second_user.emails.create!(email: 'JaNe@GitLab.org')
+      second_user.emails
+
+      entry.add_matching_users_from([second_user, user])
+
+      expect(entry.users).to contain_exactly(user, second_user)
+    end
+  end
+end

ee/spec/lib/gitlab/code_owners/file_spec.rb

@@ -15,27 +15,22 @@ describe Gitlab::CodeOwners::File do
   subject(:file) { described_class.new(blob) }
 
   describe '#parsed_data' do
+    def owner_line(pattern)
+      file.parsed_data[pattern].owner_line
+    end
+
     it 'parses all the required lines' do
       expected_patterns = [
         '/**/*', '/**/#file_with_pound.rb', '/**/*.rb', '/**/CODEOWNERS', '/**/LICENSE', '/docs/**/*',
         '/docs/*', '/config/**/*', '/**/lib/**/*', '/**/path with spaces/**/*'
       ]
+
       expect(file.parsed_data.keys)
         .to contain_exactly(*expected_patterns)
     end
 
     it 'allows usernames and emails' do
-      expect(file.parsed_data['/**/LICENSE']).to include('legal', 'janedoe@gitlab.com')
-    end
-
-    context 'when there are entries that do not look like user references' do
-      let(:file_content) do
-        "a-path/ this is all random @username and email@gitlab.org"
-      end
-
-      it 'ignores the entries' do
-        expect(file.parsed_data['/**/a-path/**/*']).to include('username', 'email@gitlab.org')
-      end
+      expect(owner_line('/**/LICENSE')).to include('legal', 'janedoe@gitlab.com')
     end
   end
 
@@ -63,7 +58,7 @@ describe Gitlab::CodeOwners::File do
     end
   end
 
-  describe '#owners_for_path' do
+  describe '#entry_for_path' do
     context 'for a path without matches' do
       let(:file_content) do
         <<~CONTENT
@@ -72,80 +67,82 @@ describe Gitlab::CodeOwners::File do
       end
 
       it 'returns an nil for an unmatched path' do
-        owners = file.owners_for_path('no_matches')
+        entry = file.entry_for_path('no_matches')
 
-        expect(owners).to be_nil
+        expect(entry).to be_nil
       end
     end
 
     it 'matches random files to a pattern' do
-      owners = file.owners_for_path('app/assets/something.vue')
+      entry = file.entry_for_path('app/assets/something.vue')
 
-      expect(owners).to include('default-codeowner')
+      expect(entry.pattern).to eq('*')
+      expect(entry.owner_line).to include('default-codeowner')
     end
 
     it 'uses the last pattern if multiple patterns match' do
-      owners = file.owners_for_path('hello.rb')
+      entry = file.entry_for_path('hello.rb')
 
-      expect(owners).to eq('@ruby-owner')
+      expect(entry.pattern).to eq('*.rb')
+      expect(entry.owner_line).to eq('@ruby-owner')
     end
 
     it 'returns the usernames for a file matching a pattern with a glob' do
-      owners = file.owners_for_path('app/models/repository.rb')
+      entry = file.entry_for_path('app/models/repository.rb')
 
-      expect(owners).to eq('@ruby-owner')
+      expect(entry.owner_line).to eq('@ruby-owner')
     end
 
     it 'allows specifying multiple users' do
-      owners = file.owners_for_path('CODEOWNERS')
+      entry = file.entry_for_path('CODEOWNERS')
 
-      expect(owners).to include('multiple', 'owners', 'tab-separated')
+      expect(entry.owner_line).to include('multiple', 'owners', 'tab-separated')
     end
 
     it 'returns emails and usernames for a matched pattern' do
-      owners = file.owners_for_path('LICENSE')
+      entry = file.entry_for_path('LICENSE')
 
-      expect(owners).to include('legal', 'janedoe@gitlab.com')
+      expect(entry.owner_line).to include('legal', 'janedoe@gitlab.com')
     end
 
     it 'allows escaping the pound sign used for comments' do
-      owners = file.owners_for_path('examples/#file_with_pound.rb')
+      entry = file.entry_for_path('examples/#file_with_pound.rb')
 
-      expect(owners).to include('owner-file-with-pound')
+      expect(entry.owner_line).to include('owner-file-with-pound')
     end
 
     it 'returns the usernames for a file nested in a directory' do
-      owners = file.owners_for_path('docs/projects/index.md')
+      entry = file.entry_for_path('docs/projects/index.md')
 
-      expect(owners).to include('all-docs')
+      expect(entry.owner_line).to include('all-docs')
     end
 
     it 'returns the usernames for a pattern matched with a glob in a folder' do
-      owners = file.owners_for_path('docs/index.md')
+      entry = file.entry_for_path('docs/index.md')
 
-      expect(owners).to include('root-docs')
+      expect(entry.owner_line).to include('root-docs')
     end
 
     it 'allows matching files nested anywhere in the repository', :aggregate_failures do
-      lib_owners = file.owners_for_path('lib/gitlab/git/repository.rb')
-      other_lib_owners = file.owners_for_path('ee/lib/gitlab/git/repository.rb')
+      lib_entry = file.entry_for_path('lib/gitlab/git/repository.rb')
+      other_lib_entry = file.entry_for_path('ee/lib/gitlab/git/repository.rb')
 
-      expect(lib_owners).to include('lib-owner')
-      expect(other_lib_owners).to include('lib-owner')
+      expect(lib_entry.owner_line).to include('lib-owner')
+      expect(other_lib_entry.owner_line).to include('lib-owner')
     end
 
     it 'allows allows limiting the matching files to the root of the repository', :aggregate_failures do
-      config_owners = file.owners_for_path('config/database.yml')
-      other_config_owners = file.owners_for_path('other/config/database.yml')
+      config_entry = file.entry_for_path('config/database.yml')
+      other_config_entry = file.entry_for_path('other/config/database.yml')
 
-      expect(config_owners).to include('config-owner')
-      expect(other_config_owners).to eq('@default-codeowner')
+      expect(config_entry.owner_line).to include('config-owner')
+      expect(other_config_entry.owner_line).to eq('@default-codeowner')
     end
 
     it 'correctly matches paths with spaces' do
-      owners = file.owners_for_path('path with spaces/README.md')
+      entry = file.entry_for_path('path with spaces/README.md')
 
-      expect(owners).to eq('@space-owner')
+      expect(entry.owner_line).to eq('@space-owner')
     end
 
     context 'paths with whitespaces and username lookalikes' do
@@ -154,10 +151,10 @@ describe Gitlab::CodeOwners::File do
       end
 
       it 'parses correctly' do
-        owners = file.owners_for_path('a/weird path with/ @username / and-email@lookalikes.com /test.rb')
+        entry = file.entry_for_path('a/weird path with/ @username / and-email@lookalikes.com /test.rb')
 
-        expect(owners).to include('user-1', 'user-2', 'email@gitlab.org')
-        expect(owners).not_to include('username', 'and-email@lookalikes.com')
+        expect(entry.owner_line).to include('user-1', 'user-2', 'email@gitlab.org')
+        expect(entry.owner_line).not_to include('username', 'and-email@lookalikes.com')
       end
     end
 
@@ -167,15 +164,15 @@ describe Gitlab::CodeOwners::File do
       end
 
       it 'matches files in the root directory' do
-        owners = file.owners_for_path('README.md')
+        entry = file.entry_for_path('README.md')
 
-        expect(owners).to include('user-1', 'user-2')
+        expect(entry.owner_line).to include('user-1', 'user-2')
       end
 
       it 'does not match nested files' do
-        owners = file.owners_for_path('nested/path/README.md')
+        entry = file.entry_for_path('nested/path/README.md')
 
-        expect(owners).to be_nil
+        expect(entry).to be_nil
       end
     end
 
@@ -185,14 +182,14 @@ describe Gitlab::CodeOwners::File do
       end
 
       it 'does not match a file in a folder that looks the same' do
-        owners = file.owners_for_path('fufoo/bar')
+        entry = file.entry_for_path('fufoo/bar')
 
-        expect(owners).to be_nil
+        expect(entry).to be_nil
       end
 
       it 'matches the file in any folder' do
-        expect(file.owners_for_path('baz/foo/bar')).to include('user-1', 'user-2')
-        expect(file.owners_for_path('/foo/bar')).to include('user-1', 'user-2')
+        expect(file.entry_for_path('baz/foo/bar').owner_line).to include('user-1', 'user-2')
+        expect(file.entry_for_path('/foo/bar').owner_line).to include('user-1', 'user-2')
       end
     end
   end

ee/spec/lib/gitlab/code_owners/loader_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::CodeOwners::Loader do
   include FakeBlobHelpers
   set(:group) { create(:group) }
   set(:project) { create(:project, namespace: group) }
-  subject(:loader) { described_class.new(project, 'with-codeowners', path) }
+  subject(:loader) { described_class.new(project, 'with-codeowners', paths) }
 
   let!(:owner_1) { create(:user, username: 'owner-1') }
   let!(:email_owner) { create(:user, username: 'owner-2') }
@@ -21,89 +21,54 @@ describe Gitlab::CodeOwners::Loader do
     CODEOWNERS
   end
   let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
-  let(:path) { 'docs/CODEOWNERS' }
+  let(:paths) { 'docs/CODEOWNERS' }
 
   before do
+    project.add_developer(owner_1)
+    project.add_developer(email_owner)
+    project.add_developer(documentation_owner)
+    project.add_developer(test_owner)
+
     create(:email, user: email_owner, email: 'owner2@gitlab.org')
 
     allow(project.repository).to receive(:code_owners_blob).and_return(codeowner_blob)
   end
 
-  describe '#non_members' do
-    before do
-      project.add_developer(owner_1)
-      project.add_developer(email_owner)
-      project.add_developer(test_owner)
-    end
+  describe '#entries' do
+    let(:expected_entry) { Gitlab::CodeOwners::Entry.new('docs/CODEOWNERS', '@owner-1 owner2@gitlab.org @owner-3 @documentation-owner') }
+    let(:first_entry) { loader.entries.first }
 
-    it 'returns all existing users that are not members of the project' do
-      expect(loader.non_members).to contain_exactly(owner_3, documentation_owner)
+    it 'returns entries for the matched line' do
+      expect(loader.entries).to contain_exactly(expected_entry)
     end
 
-    it 'does not return users that are members of the project' do
-      expect(loader.non_members).not_to include(owner_1, email_owner)
+    it 'loads all users that are members of the project into the entry' do
+      expect(first_entry.users).to contain_exactly(owner_1, email_owner, documentation_owner)
     end
 
-    it 'excludes group members of the project' do
-      group.add_developer(documentation_owner)
-
-      expect(loader.non_members).to include(owner_3)
-    end
-  end
-
-  describe '#members' do
-    before do
-      project.add_developer(owner_1)
-      project.add_developer(email_owner)
-      project.add_developer(documentation_owner)
-      project.add_developer(test_owner)
+    it 'does not load non members of the project into the entry' do
+      expect(first_entry.users).not_to include(owner_3)
     end
 
-    it 'returns all existing users that are members of the project' do
-      expect(loader.members).to contain_exactly(owner_1, email_owner, documentation_owner)
-    end
-
-    it 'does not return users that are not members of the project' do
-      expect(loader.members).not_to include(owner_3)
-    end
-
-    it 'includes group members of the project' do
+    it 'loads group members of the project into the entry' do
       group.add_developer(owner_3)
 
-      expect(loader.members).to include(owner_3)
+      expect(first_entry.users).to include(owner_3)
     end
-  end
 
-  describe '#raw_users' do
-    context 'with a CODEOWNERS file' do
-      context 'for a path with code owners' do
-        it 'returns all owners' do
-          expect(loader.raw_users).to contain_exactly(owner_1, owner_3, email_owner, documentation_owner)
-        end
-      end
-
-      context 'for multiple paths with code owners' do
-        let(:path) { ['docs/test.rb', 'spec/spec_helper.rb', 'docs/foo.rb'] }
-
-        it 'returns all owners for all paths' do
-          expect(loader.raw_users).to contain_exactly(documentation_owner, test_owner)
-        end
-      end
+    context 'for multiple paths' do
+      let(:paths) { ['docs/CODEOWNERS', 'spec/loader_spec.rb', 'spec/entry_spec.rb'] }
 
-      context 'for another path' do
-        let(:path) { 'no-codeowner' }
+      it 'loads 2 entries' do
+        other_entry = Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner')
 
-        it 'returns no users' do
-          expect(loader.raw_users).to be_empty
-        end
+        expect(loader.entries).to contain_exactly(expected_entry, other_entry)
       end
-    end
-
-    context 'when there is no codeowners file' do
-      let(:codeowner_blob) { nil }
 
-      it 'returns no users without failing' do
-        expect(loader.raw_users).to be_empty
+      it 'only performs 2 query for users' do
+        # One query for users, one for the emails to later divide them across the
+        # entries
+        expect { loader.entries }.not_to exceed_query_limit(2)
       end
     end
 
@@ -111,7 +76,7 @@ describe Gitlab::CodeOwners::Loader do
       it 'only calls out to the repository once' do
         expect(project.repository).to receive(:code_owners_blob).once
 
-        2.times { loader.raw_users }
+        2.times { loader.entries }
       end
 
       it 'only processes the file once' do
@@ -119,11 +84,17 @@ describe Gitlab::CodeOwners::Loader do
 
         expect(code_owners_file).to receive(:get_parsed_data).once.and_call_original
 
-        2.times { loader.raw_users }
+        2.times { loader.entries }
       end
     end
   end
 
+  describe '#members' do
+    it 'returns users mentioned for the passed path' do
+      expect(loader.members).to contain_exactly(owner_1, email_owner, documentation_owner)
+    end
+  end
+
   describe '#empty_code_owners?' do
     context 'when file does not exist' do
       let(:codeowner_blob) { nil }

ee/spec/lib/gitlab/code_owners_spec.rb

@@ -44,6 +44,46 @@ describe Gitlab::CodeOwners do
     end
   end
 
+  describe '.entries_for_merge_request' do
+    let(:codeowner_lookup_ref) { merge_request.target_branch }
+    let(:merge_request) do
+      build(
+        :merge_request,
+        source_project: project,
+        source_branch: 'feature',
+        target_project: project,
+        target_branch: 'with-codeowners'
+      )
+    end
+
+    context 'when the feature is available' do
+      before do
+        stub_licensed_features(code_owners: true)
+      end
+
+      it 'returns owners for merge request' do
+        expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: nil).and_return(['docs/CODEOWNERS'])
+
+        entry = described_class.entries_for_merge_request(merge_request).first
+
+        expect(entry.pattern).to eq('docs/CODEOWNERS')
+        expect(entry.users).to eq([code_owner])
+      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'])
+
+          entry = described_class.entries_for_merge_request(merge_request, merge_request_diff: merge_request_diff).first
+
+          expect(entry.users).to eq([code_owner])
+        end
+      end
+    end
+  end
+
   describe '.for_merge_request' do
     let(:codeowner_lookup_ref) { merge_request.target_branch }
     let(:merge_request) do

ee/spec/services/ee/merge_requests/create_service_spec.rb

@@ -25,18 +25,31 @@ describe MergeRequests::CreateService do
 
   describe '#execute' do
     context 'code owners' do
-      let!(:owners) { create_list(:user, 2) }
+      it 'refreshes code owners for the merge request' do
+        fake_refresh_service = instance_double(::MergeRequests::SyncCodeOwnerApprovalRules)
 
-      before do
-        allow(::Gitlab::CodeOwners).to receive(:for_merge_request).and_return(owners)
+        expect(::MergeRequests::SyncCodeOwnerApprovalRules)
+          .to receive(:new).with(kind_of(MergeRequest)).and_return(fake_refresh_service)
+        expect(fake_refresh_service).to receive(:execute)
+
+        service.execute
       end
 
-      it 'syncs code owner to approval rule' do
-        merge_request = service.execute
+      context 'when multiple code owner rules is disabled' do
+        let!(:owners) { create_list(:user, 2) }
+
+        before do
+          stub_feature_flags(multiple_code_owner_rules: false)
+          allow(::Gitlab::CodeOwners).to receive(:for_merge_request).and_return(owners)
+        end
+
+        it 'syncs code owner to approval rule' do
+          merge_request = service.execute
 
-        rule = merge_request.approval_rules.code_owner.first
+          rule = merge_request.approval_rules.code_owner.first
 
-        expect(rule.users).to contain_exactly(*owners)
+          expect(rule.users).to contain_exactly(*owners)
+        end
       end
     end
   end

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -140,28 +140,44 @@ describe MergeRequests::RefreshService do
 
       context 'when code owners enabled, with approval_rule enabled' do
         let(:relevant_merge_requests) { [merge_request, another_merge_request] }
-        let(:new_owners) { [owner] }
 
-        before do
+        it 'refreshes the code owner rules for all relevant merge requests' do
+          fake_refresh_service = instance_double(::MergeRequests::SyncCodeOwnerApprovalRules)
+
           relevant_merge_requests.each do |merge_request|
-            expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
+            expect(::MergeRequests::SyncCodeOwnerApprovalRules)
+              .to receive(:new).with(merge_request).and_return(fake_refresh_service)
+            expect(fake_refresh_service).to receive(:execute)
           end
 
-          [forked_merge_request].each do |merge_request|
-            expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request)
-          end
+          subject
         end
 
-        it 'triggers syncing of code owners' do
-          relevant_merge_requests.each do |merge_request|
-            expect(merge_request.approval_rules.code_owner.exists?).to eq(false)
+        context 'when multiple code owner rules are disabled' do
+          let(:new_owners) { [owner] }
+
+          before do
+            stub_feature_flags(multiple_code_owner_rules: false)
+            relevant_merge_requests.each do |merge_request|
+              expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
+            end
+
+            [forked_merge_request].each do |merge_request|
+              expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request)
+            end
           end
 
-          subject
+          it 'triggers syncing of code owners' do
+            relevant_merge_requests.each do |merge_request|
+              expect(merge_request.approval_rules.code_owner.exists?).to eq(false)
+            end
 
-          relevant_merge_requests.each do |merge_request|
-            code_owner_rule = merge_request.approval_rules.code_owner.first
-            expect(code_owner_rule.users).to eq(new_owners)
+            subject
+
+            relevant_merge_requests.each do |merge_request|
+              code_owner_rule = merge_request.approval_rules.code_owner.first
+              expect(code_owner_rule.users).to eq(new_owners)
+            end
           end
         end
       end

ee/spec/services/merge_requests/sync_code_owner_approval_rules_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::SyncCodeOwnerApprovalRules do
+  let(:merge_request) { create(:merge_request) }
+  let(:rb_owners) { create_list(:user, 2) }
+  let(:doc_owners) { create_list(:user, 2) }
+  let(:rb_entry) { build_entry('*.rb', rb_owners) }
+  let(:doc_entry) { build_entry('doc/*', doc_owners) }
+  let(:entries) { [rb_entry, doc_entry] }
+
+  def build_entry(pattern, users)
+    entry = Gitlab::CodeOwners::Entry.new(pattern, users.map(&:to_reference).join(' '))
+    entry.add_matching_users_from(users)
+
+    entry
+  end
+
+  subject(:service) { described_class.new(merge_request) }
+
+  describe '#execute' do
+    before do
+      allow(Gitlab::CodeOwners)
+        .to receive(:entries_for_merge_request).with(merge_request, merge_request_diff: nil)
+              .and_return(entries)
+    end
+
+    it "creates rules for code owner entries that don't have a rule" do
+      expect { service.execute }.to change { merge_request.approval_rules.count }.by(2)
+
+      rb_rule = merge_request.approval_rules.code_owner.find_by(name: rb_entry.pattern)
+      doc_rule = merge_request.approval_rules.code_owner.find_by(name: doc_entry.pattern)
+
+      expect(rb_rule.users).to eq(rb_owners)
+      expect(doc_rule.users).to eq(doc_owners)
+    end
+
+    it 'deletes rules that are not relevant anymore' do
+      other_rule = create(:code_owner_rule, merge_request: merge_request)
+
+      service.execute
+
+      expect(merge_request.approval_rules).not_to include(other_rule)
+      expect { other_rule.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it 'updates rules for which the users changed' do
+      other_rule = create(:code_owner_rule, merge_request: merge_request, name: '*.rb')
+      other_rule.users += doc_owners
+      other_rule.save!
+
+      service.execute
+
+      expect(other_rule.reload.users).to eq(rb_owners)
+    end
+
+    context 'when multiple code owner rules are disabled' do
+      before do
+        stub_feature_flags(multiple_code_owner_rules: false)
+      end
+
+      it 'calls the old sync method' do
+        expect(merge_request).to receive(:sync_code_owners_with_approvers)
+
+        service.execute
+      end
+    end
+  end
+end