Model changes for code owner approval rules

- Adds the validations to match the uniqueness index.
- Adds scopes for finding code owner rules based on a pattern
- Adds a safe way to create code owner rules avoiding race conditions

ee/app/models/approval_merge_request_rule.rb

@@ -7,6 +7,10 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   scope :regular, -> { where(code_owner: false) }
   scope :code_owner, -> { where(code_owner: true) } # special code owner rules, updated internally when code changes
+  scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
+  scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
+
+  validates :name, uniqueness: { scope: [:merge_request, :code_owner] }
 
   belongs_to :merge_request
 
@@ -18,6 +22,13 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   validate :validate_approvals_required
 
+  def self.find_or_create_code_owner_rule(merge_request, pattern)
+    merge_request.approval_rules.safe_find_or_create_by(
+      code_owner: true,
+      name: pattern
+    )
+  end
+
   def project
     merge_request.target_project
   end

ee/spec/factories/approval_rules.rb

@@ -3,7 +3,13 @@
 FactoryBot.define do
   factory :approval_merge_request_rule do
     merge_request
-    name ApprovalRuleLike::DEFAULT_NAME
+    sequence(:name) { |n| "#{ApprovalRuleLike::DEFAULT_NAME}-#{n}" }
+  end
+
+  factory :code_owner_rule, parent: :approval_merge_request_rule do
+    merge_request
+    code_owner true
+    sequence(:name) { |n| "*-#{n}.js" }
   end
 
   factory :approval_project_rule do

ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb

@@ -210,7 +210,7 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
         end
 
         context 'when code owner rule exists' do
-          let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: target, code_owner: true, users: [create(:user)]) }
+          let!(:code_owner_rule) { create(:code_owner_rule, merge_request: target, users: [create(:user)]) }
 
           it 'reuses and updates existing rule' do
             expect do

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -7,6 +7,83 @@ describe ApprovalMergeRequestRule do
 
   subject { create(:approval_merge_request_rule, merge_request: merge_request) }
 
+  describe 'validations' do
+    it 'is valid' do
+      expect(build(:approval_merge_request_rule)).to be_valid
+    end
+
+    it 'is invalid when the name is missing' do
+      expect(build(:approval_merge_request_rule, name: nil)).not_to be_valid
+    end
+
+    context 'code owner rules' do
+      it 'is valid' do
+        expect(build(:code_owner_rule)).to be_valid
+      end
+
+      it 'is invalid when reusing the same name within the same merge request' do
+        existing = create(:code_owner_rule, name: '*.rb', merge_request: merge_request)
+
+        new = build(:code_owner_rule, merge_request: existing.merge_request, name: '*.rb')
+
+        expect(new).not_to be_valid
+        expect { new.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
+      end
+
+      it 'allows a regular rule with the same name as the codeowner rule' do
+        create(:code_owner_rule, name: '*.rb', merge_request: merge_request)
+
+        new = build(:approval_merge_request_rule, name: '*.rb', merge_request: merge_request)
+
+        expect(new).to be_valid
+        expect { new.save! }.not_to raise_error
+      end
+    end
+  end
+
+  context 'scopes'  do
+    set(:rb_rule) { create(:code_owner_rule, name: '*.rb') }
+    set(:js_rule) { create(:code_owner_rule, name: '*.js') }
+    set(:css_rule) { create(:code_owner_rule, name: '*.css') }
+    set(:approval_rule) { create(:approval_merge_request_rule) }
+
+    describe '.not_matching_pattern' do
+      it 'returns the correct rules' do
+        expect(described_class.not_matching_pattern(['*.rb', '*.js']))
+          .to contain_exactly(css_rule)
+      end
+    end
+
+    describe '.matching_pattern' do
+      it 'returns the correct rules' do
+        expect(described_class.matching_pattern(['*.rb', '*.js']))
+          .to contain_exactly(rb_rule, js_rule)
+      end
+    end
+  end
+
+  describe '.find_or_create_code_owner_rule' do
+    set(:merge_request) { create(:merge_request) }
+    set(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) }
+
+    it 'finds an existing rule' do
+      expect(described_class.find_or_create_code_owner_rule(merge_request, '*.rb'))
+        .to eq(existing_code_owner_rule)
+    end
+
+    it 'creates a new rule if it does not exist' do
+      expect { described_class.find_or_create_code_owner_rule(merge_request, '*.js') }
+        .to change { merge_request.approval_rules.count }.by(1)
+    end
+
+    it 'retries when a record was created between the find and the create' do
+      expect(described_class).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique)
+      allow(described_class).to receive(:find_or_create_by).and_call_original
+
+      expect(described_class.find_or_create_code_owner_rule(merge_request, '*.js')).not_to be_nil
+    end
+  end
+
   describe '#project' do
     it 'returns project of MergeRequest' do
       expect(subject.project).to be_present

ee/spec/models/approval_state_spec.rb

@@ -4,10 +4,10 @@ require 'spec_helper'
 
 describe ApprovalState do
   def create_rule(additional_params = {})
-    create(
-      :approval_merge_request_rule,
-      additional_params.merge(merge_request: merge_request)
-    )
+    params = additional_params.merge(merge_request: merge_request)
+    factory = params.delete(:code_owner) ? :code_owner_rule : :approval_merge_request_rule
+
+    create(factory, params)
   end
 
   let(:merge_request) { create(:merge_request) }

ee/spec/models/merge_request_spec.rb

@@ -348,7 +348,7 @@ describe MergeRequest do
     let(:code_owners) { create_list(:user, 2) }
 
     let!(:regular_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: approvers) }
-    let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: code_owners, code_owner: true) }
+    let!(:code_owner_rule) { create(:code_owner_rule, merge_request: merge_request, users: code_owners) }
 
     let!(:approval) { create(:approval, merge_request: merge_request, user: approvers.last) }