From 881bca7d7cc5f2c2d2d69586a89c9bdfea07983f Mon Sep 17 00:00:00 2001
From: ghinfey <ghinfey@gitlab.com>
Date: Fri, 31 Jan 2025 12:18:09 +0000
Subject: [PATCH 1/4] Add v2 approval rule table and model

Adds new approval rule table and model
for new approval rule architecture.

Changelog: changed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179839
EE: true
---
 db/docs/merge_requests_approval_rules.yml     | 13 +++++
 ...50_create_merge_requests_approval_rules.rb | 31 ++++++++++
 ..._rules_multi_column_not_null_constraint.rb | 15 +++++
 db/schema_migrations/20250123151650           |  1 +
 db/schema_migrations/20250130110721           |  1 +
 db/structure.sql                              | 44 ++++++++++++++
 ee/app/models/ee/group.rb                     |  2 +
 ee/app/models/ee/project.rb                   |  2 +
 ee/app/models/merge_requests/approval_rule.rb | 30 ++++++++++
 .../merge_requests/approval_rules.rb          | 13 +++++
 .../merge_requests/approval_rule_spec.rb      | 57 +++++++++++++++++++
 spec/lib/gitlab/import_export/all_models.yml  |  2 +
 12 files changed, 211 insertions(+)
 create mode 100644 db/docs/merge_requests_approval_rules.yml
 create mode 100644 db/migrate/20250123151650_create_merge_requests_approval_rules.rb
 create mode 100644 db/migrate/20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
 create mode 100644 db/schema_migrations/20250123151650
 create mode 100644 db/schema_migrations/20250130110721
 create mode 100644 ee/app/models/merge_requests/approval_rule.rb
 create mode 100644 ee/spec/factories/merge_requests/approval_rules.rb
 create mode 100644 ee/spec/models/merge_requests/approval_rule_spec.rb

diff --git a/db/docs/merge_requests_approval_rules.yml b/db/docs/merge_requests_approval_rules.yml
new file mode 100644
index 00000000000000..fc3ebd5900c59b
--- /dev/null
+++ b/db/docs/merge_requests_approval_rules.yml
@@ -0,0 +1,13 @@
+---
+table_name: merge_requests_approval_rules
+classes:
+  - MergeRequests::ApprovalRule
+feature_categories:
+  - code_review_workflow
+description: Main table that stores information about approval rules v2.
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179839
+milestone: '17.9'
+gitlab_schema: gitlab_main_cell
+sharding_key:
+  group_id: namespaces
+  project_id: projects
diff --git a/db/migrate/20250123151650_create_merge_requests_approval_rules.rb b/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
new file mode 100644
index 00000000000000..2a099bb854fef6
--- /dev/null
+++ b/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class CreateMergeRequestsApprovalRules < Gitlab::Database::Migration[2.2]
+  milestone '17.9'
+
+  def change
+    create_table :merge_requests_approval_rules do |t|
+      t.integer :approvals_required, null: false, default: 0
+      t.text :name, limit: 255, null: false
+      t.integer :rule_type, null: false, default: 0, limit: 2
+      t.integer :origin, null: false, default: 0, limit: 2
+      t.belongs_to(
+        :project,
+        foreign_key: { to_table: :projects, on_delete: :cascade },
+        null: true
+      )
+      t.belongs_to(
+        :group,
+        foreign_key: { to_table: :namespaces, on_delete: :cascade },
+        null: true
+      )
+      t.belongs_to(
+        :source_rule,
+        foreign_key: { to_table: :merge_requests_approval_rules, on_delete: :nullify },
+        index: { name: 'index_approval_rules_on_source_rule_id' },
+        null: true
+      )
+      t.timestamps_with_timezone null: false
+    end
+  end
+end
diff --git a/db/migrate/20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb b/db/migrate/20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
new file mode 100644
index 00000000000000..24e0e203b532c4
--- /dev/null
+++ b/db/migrate/20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class AddMergeRequestsApprovalRulesMultiColumnNotNullConstraint < Gitlab::Database::Migration[2.2]
+  milestone '17.9'
+
+  disable_ddl_transaction!
+
+  def up
+    add_multi_column_not_null_constraint(:merge_requests_approval_rules, :group_id, :project_id)
+  end
+
+  def down
+    remove_multi_column_not_null_constraint(:merge_requests_approval_rules, :group_id, :project_id)
+  end
+end
diff --git a/db/schema_migrations/20250123151650 b/db/schema_migrations/20250123151650
new file mode 100644
index 00000000000000..6fbb499a26496d
--- /dev/null
+++ b/db/schema_migrations/20250123151650
@@ -0,0 +1 @@
+d149047a5d5fa4fa8242fde7f0d266cebef8d56e0db282d745c6da6e0f9f1c1d
\ No newline at end of file
diff --git a/db/schema_migrations/20250130110721 b/db/schema_migrations/20250130110721
new file mode 100644
index 00000000000000..7a1e2fbe886aeb
--- /dev/null
+++ b/db/schema_migrations/20250130110721
@@ -0,0 +1 @@
+4804fd7719ef467850550e1a488d544ec412f83e51335a367eca119ba550161c
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index aa7db723ee4426..61f7765888872c 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -15885,6 +15885,30 @@ CREATE TABLE merge_requests (
     CONSTRAINT check_970d272570 CHECK ((lock_version IS NOT NULL))
 );
 
+CREATE TABLE merge_requests_approval_rules (
+    id bigint NOT NULL,
+    approvals_required integer DEFAULT 0 NOT NULL,
+    name text NOT NULL,
+    rule_type smallint DEFAULT 0 NOT NULL,
+    origin smallint DEFAULT 0 NOT NULL,
+    project_id bigint,
+    group_id bigint,
+    source_rule_id bigint,
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL,
+    CONSTRAINT check_ba7b03c61a CHECK ((num_nonnulls(group_id, project_id) = 1)),
+    CONSTRAINT check_c7c36145b7 CHECK ((char_length(name) <= 255))
+);
+
+CREATE SEQUENCE merge_requests_approval_rules_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE merge_requests_approval_rules_id_seq OWNED BY merge_requests_approval_rules.id;
+
 CREATE TABLE merge_requests_closing_issues (
     id bigint NOT NULL,
     merge_request_id bigint NOT NULL,
@@ -25102,6 +25126,8 @@ ALTER TABLE ONLY merge_request_user_mentions ALTER COLUMN id SET DEFAULT nextval
 
 ALTER TABLE ONLY merge_requests ALTER COLUMN id SET DEFAULT nextval('merge_requests_id_seq'::regclass);
 
+ALTER TABLE ONLY merge_requests_approval_rules ALTER COLUMN id SET DEFAULT nextval('merge_requests_approval_rules_id_seq'::regclass);
+
 ALTER TABLE ONLY merge_requests_closing_issues ALTER COLUMN id SET DEFAULT nextval('merge_requests_closing_issues_id_seq'::regclass);
 
 ALTER TABLE ONLY merge_requests_compliance_violations ALTER COLUMN id SET DEFAULT nextval('merge_requests_compliance_violations_id_seq'::regclass);
@@ -27651,6 +27677,9 @@ ALTER TABLE ONLY merge_request_reviewers
 ALTER TABLE ONLY merge_request_user_mentions
     ADD CONSTRAINT merge_request_user_mentions_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY merge_requests_approval_rules
+    ADD CONSTRAINT merge_requests_approval_rules_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY merge_requests_closing_issues
     ADD CONSTRAINT merge_requests_closing_issues_pkey PRIMARY KEY (id);
 
@@ -31020,6 +31049,8 @@ CREATE INDEX index_approval_rule_on_protected_environment_id ON protected_enviro
 
 CREATE INDEX index_approval_rules_code_owners_rule_type ON approval_merge_request_rules USING btree (merge_request_id) WHERE (rule_type = 2);
 
+CREATE INDEX index_approval_rules_on_source_rule_id ON merge_requests_approval_rules USING btree (source_rule_id);
+
 CREATE INDEX index_approvals_on_merge_request_id_and_created_at ON approvals USING btree (merge_request_id, created_at);
 
 CREATE UNIQUE INDEX index_approvals_on_user_id_and_merge_request_id ON approvals USING btree (user_id, merge_request_id);
@@ -32990,6 +33021,10 @@ CREATE INDEX index_merge_request_reviewers_on_user_id ON merge_request_reviewers
 
 CREATE UNIQUE INDEX index_merge_request_user_mentions_on_note_id ON merge_request_user_mentions USING btree (note_id) WHERE (note_id IS NOT NULL);
 
+CREATE INDEX index_merge_requests_approval_rules_on_group_id ON merge_requests_approval_rules USING btree (group_id);
+
+CREATE INDEX index_merge_requests_approval_rules_on_project_id ON merge_requests_approval_rules USING btree (project_id);
+
 CREATE INDEX index_merge_requests_closing_issues_on_issue_id ON merge_requests_closing_issues USING btree (issue_id);
 
 CREATE INDEX index_merge_requests_closing_issues_on_merge_request_id ON merge_requests_closing_issues USING btree (merge_request_id);
@@ -39789,6 +39824,9 @@ ALTER TABLE ONLY automation_rules
 ALTER TABLE ONLY incident_management_oncall_participants
     ADD CONSTRAINT fk_rails_032b12996a FOREIGN KEY (oncall_rotation_id) REFERENCES incident_management_oncall_rotations(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY merge_requests_approval_rules
+    ADD CONSTRAINT fk_rails_03983bf729 FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY events
     ADD CONSTRAINT fk_rails_0434b48643 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
@@ -40713,6 +40751,9 @@ ALTER TABLE ONLY operations_scopes
 ALTER TABLE ONLY milestone_releases
     ADD CONSTRAINT fk_rails_7ae0756a2d FOREIGN KEY (milestone_id) REFERENCES milestones(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY merge_requests_approval_rules
+    ADD CONSTRAINT fk_rails_7af76dbd21 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY scan_execution_policy_rules
     ADD CONSTRAINT fk_rails_7be2571ecf FOREIGN KEY (security_policy_id) REFERENCES security_policies(id) ON DELETE CASCADE;
 
@@ -41679,6 +41720,9 @@ ALTER TABLE ONLY merge_requests_closing_issues
 ALTER TABLE p_ci_job_artifact_reports
     ADD CONSTRAINT fk_rails_f9b8550174 FOREIGN KEY (partition_id, job_artifact_id) REFERENCES p_ci_job_artifacts(partition_id, id) ON UPDATE CASCADE ON DELETE CASCADE;
 
+ALTER TABLE ONLY merge_requests_approval_rules
+    ADD CONSTRAINT fk_rails_fa5b38e373 FOREIGN KEY (source_rule_id) REFERENCES merge_requests_approval_rules(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY banned_users
     ADD CONSTRAINT fk_rails_fa5bb598e5 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
 
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 4c45dd2953a031..00efe0015a914c 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -99,6 +99,8 @@ module Group
 
       has_many :security_exclusions, class_name: 'Security::GroupSecurityExclusion'
 
+      has_many :v2_approval_rules_from_group_origin, class_name: 'MergeRequests::ApprovalRule', inverse_of: :source_group
+
       delegate :deleting_user, :marked_for_deletion_on, to: :deletion_schedule, allow_nil: true
 
       delegate :repository_read_only,
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index d3448eca50ff28..96007512310c5e 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -217,6 +217,8 @@ def lock_for_confirmation!(id)
       has_many :project_control_compliance_statuses,
         class_name: 'ComplianceManagement::ComplianceFramework::ProjectControlComplianceStatus'
 
+      has_many :v2_approval_rules_from_project_origin, class_name: 'MergeRequests::ApprovalRule', inverse_of: :source_project
+
       elastic_index_dependant_association :issues, on_change: :visibility_level
       elastic_index_dependant_association :issues, on_change: :archived
       elastic_index_dependant_association :work_items, on_change: :visibility_level
diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb
new file mode 100644
index 00000000000000..90a37cd6417d30
--- /dev/null
+++ b/ee/app/models/merge_requests/approval_rule.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module MergeRequests
+  class ApprovalRule < ApplicationRecord
+    self.table_name = 'merge_requests_approval_rules'
+
+    # source group or project. Only 1 should be supplied for sharding purposes.
+    belongs_to :source_group, foreign_key: 'group_id', class_name: 'Group',
+      inverse_of: :v2_approval_rules_from_group_origin
+    belongs_to :source_project, foreign_key: 'project_id', class_name: 'Project',
+      inverse_of: :v2_approval_rules_from_project_origin
+
+    validate :source_group_or_project_present
+
+    with_options validate: true do
+      enum :rule_type, { regular: 0, code_owner: 1, report_approver: 2, any_approver: 3 }, default: :regular
+      enum :origin, { group: 0, project: 1, merge_request: 2 }, prefix: :originates_from
+    end
+
+    private
+
+    def source_group_or_project_present
+      if source_group.blank? && source_project.blank?
+        errors.add(:base, "Must have either a source group or source project")
+      elsif source_group.present? && source_project.present?
+        errors.add(:base, "Cannot have both a source group and source project")
+      end
+    end
+  end
+end
diff --git a/ee/spec/factories/merge_requests/approval_rules.rb b/ee/spec/factories/merge_requests/approval_rules.rb
new file mode 100644
index 00000000000000..f6d150880cf2c8
--- /dev/null
+++ b/ee/spec/factories/merge_requests/approval_rules.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :merge_requests_approval_rule, class: 'MergeRequests::ApprovalRule' do
+    trait :with_source_group do
+      association :source_group, factory: :group
+    end
+
+    trait :with_source_project do
+      association :source_project, factory: :project
+    end
+  end
+end
diff --git a/ee/spec/models/merge_requests/approval_rule_spec.rb b/ee/spec/models/merge_requests/approval_rule_spec.rb
new file mode 100644
index 00000000000000..73721d2dc4fdf8
--- /dev/null
+++ b/ee/spec/models/merge_requests/approval_rule_spec.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::ApprovalRule, type: :model, feature_category: :code_review_workflow do
+  subject(:rule) { build(:merge_requests_approval_rule, *traits) }
+
+  let(:traits) { [] }
+
+  describe 'associations' do
+    context 'when associated with a source_group' do
+      let(:traits) { [:with_source_group] }
+
+      it { is_expected.to belong_to(:source_group) }
+    end
+
+    context 'when associated with a source_project' do
+      let(:traits) { [:with_source_project] }
+
+      it { is_expected.to belong_to(:source_project) }
+    end
+  end
+
+  describe 'validations' do
+    context 'when associated with a source_group' do
+      let(:traits) { [:with_source_group] }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when associated with a source_project' do
+      let(:traits) { [:with_source_project] }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when not associated with either source_group or source_project' do
+      it { is_expected.not_to be_valid }
+
+      it 'has the correct error message' do
+        rule.valid?
+        expect(rule.errors[:base]).to include("Must have either a source group or source project")
+      end
+    end
+
+    context 'when associated with both source_group and source_project' do
+      let(:traits) { [:with_source_group, :with_source_project] }
+
+      it { is_expected.not_to be_valid }
+
+      it 'has the correct error message' do
+        rule.valid?
+        expect(rule.errors[:base]).to include("Cannot have both a source group and source project")
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index b2bff3b96eda89..45e952cb1d85f7 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -255,6 +255,7 @@ merge_requests:
 - approver_users
 - approver_groups
 - approved_by_users
+- v2_approval_rules_from_project_origin
 - draft_notes
 - merge_train_car
 - blocks_as_blocker
@@ -769,6 +770,7 @@ project:
 - approval_merge_request_rules
 - approval_merge_request_rule_sources
 - approval_project_rules
+- v2_approval_rules_from_project_origin
 - approvers
 - approver_users
 - audit_events
-- 
GitLab


From fc23f4bcc329da61b56894aa49925f4ce1faa4cc Mon Sep 17 00:00:00 2001
From: ghinfey <ghinfey@gitlab.com>
Date: Tue, 4 Feb 2025 11:26:20 +0000
Subject: [PATCH 2/4] Remove association for sharding

---
 ...50_create_merge_requests_approval_rules.rb |  4 +-
 db/structure.sql                              |  2 +-
 ee/app/models/ee/group.rb                     |  2 -
 ee/app/models/ee/project.rb                   |  2 -
 ee/app/models/merge_requests/approval_rule.rb | 18 +++------
 .../merge_requests/approval_rules.rb          | 11 +++---
 .../merge_requests/approval_rule_spec.rb      | 38 +++++++------------
 spec/lib/gitlab/import_export/all_models.yml  |  2 -
 8 files changed, 28 insertions(+), 51 deletions(-)

diff --git a/db/migrate/20250123151650_create_merge_requests_approval_rules.rb b/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
index 2a099bb854fef6..5d4cf0432fbd4a 100644
--- a/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
+++ b/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
@@ -4,9 +4,9 @@ class CreateMergeRequestsApprovalRules < Gitlab::Database::Migration[2.2]
   milestone '17.9'
 
   def change
-    create_table :merge_requests_approval_rules do |t|
-      t.integer :approvals_required, null: false, default: 0
+    create_table :merge_requests_approval_rules do |t| # -- Migration/EnsureFactoryForTable false positive
       t.text :name, limit: 255, null: false
+      t.integer :approvals_required, null: false, default: 0
       t.integer :rule_type, null: false, default: 0, limit: 2
       t.integer :origin, null: false, default: 0, limit: 2
       t.belongs_to(
diff --git a/db/structure.sql b/db/structure.sql
index 61f7765888872c..2ad08d4d907b0e 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -15887,8 +15887,8 @@ CREATE TABLE merge_requests (
 
 CREATE TABLE merge_requests_approval_rules (
     id bigint NOT NULL,
-    approvals_required integer DEFAULT 0 NOT NULL,
     name text NOT NULL,
+    approvals_required integer DEFAULT 0 NOT NULL,
     rule_type smallint DEFAULT 0 NOT NULL,
     origin smallint DEFAULT 0 NOT NULL,
     project_id bigint,
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 00efe0015a914c..4c45dd2953a031 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -99,8 +99,6 @@ module Group
 
       has_many :security_exclusions, class_name: 'Security::GroupSecurityExclusion'
 
-      has_many :v2_approval_rules_from_group_origin, class_name: 'MergeRequests::ApprovalRule', inverse_of: :source_group
-
       delegate :deleting_user, :marked_for_deletion_on, to: :deletion_schedule, allow_nil: true
 
       delegate :repository_read_only,
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index 96007512310c5e..d3448eca50ff28 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -217,8 +217,6 @@ def lock_for_confirmation!(id)
       has_many :project_control_compliance_statuses,
         class_name: 'ComplianceManagement::ComplianceFramework::ProjectControlComplianceStatus'
 
-      has_many :v2_approval_rules_from_project_origin, class_name: 'MergeRequests::ApprovalRule', inverse_of: :source_project
-
       elastic_index_dependant_association :issues, on_change: :visibility_level
       elastic_index_dependant_association :issues, on_change: :archived
       elastic_index_dependant_association :work_items, on_change: :visibility_level
diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb
index 90a37cd6417d30..49c7bf5c079526 100644
--- a/ee/app/models/merge_requests/approval_rule.rb
+++ b/ee/app/models/merge_requests/approval_rule.rb
@@ -4,13 +4,7 @@ module MergeRequests
   class ApprovalRule < ApplicationRecord
     self.table_name = 'merge_requests_approval_rules'
 
-    # source group or project. Only 1 should be supplied for sharding purposes.
-    belongs_to :source_group, foreign_key: 'group_id', class_name: 'Group',
-      inverse_of: :v2_approval_rules_from_group_origin
-    belongs_to :source_project, foreign_key: 'project_id', class_name: 'Project',
-      inverse_of: :v2_approval_rules_from_project_origin
-
-    validate :source_group_or_project_present
+    validate :group_id_or_project_id_present
 
     with_options validate: true do
       enum :rule_type, { regular: 0, code_owner: 1, report_approver: 2, any_approver: 3 }, default: :regular
@@ -19,11 +13,11 @@ class ApprovalRule < ApplicationRecord
 
     private
 
-    def source_group_or_project_present
-      if source_group.blank? && source_project.blank?
-        errors.add(:base, "Must have either a source group or source project")
-      elsif source_group.present? && source_project.present?
-        errors.add(:base, "Cannot have both a source group and source project")
+    def group_id_or_project_id_present
+      if group_id.blank? && project_id.blank?
+        errors.add(:base, "Must have either a group id or project id")
+      elsif group_id.present? && project_id.present?
+        errors.add(:base, "Cannot have both a group id and project id")
       end
     end
   end
diff --git a/ee/spec/factories/merge_requests/approval_rules.rb b/ee/spec/factories/merge_requests/approval_rules.rb
index f6d150880cf2c8..a56d74503f402a 100644
--- a/ee/spec/factories/merge_requests/approval_rules.rb
+++ b/ee/spec/factories/merge_requests/approval_rules.rb
@@ -2,12 +2,13 @@
 
 FactoryBot.define do
   factory :merge_requests_approval_rule, class: 'MergeRequests::ApprovalRule' do
-    trait :with_source_group do
-      association :source_group, factory: :group
-    end
+    sequence(:name) { |n| "Approval Rule #{n}" }
+    approvals_required { 2 }
+    rule_type { 0 }
+    origin { 0 }
 
-    trait :with_source_project do
-      association :source_project, factory: :project
+    trait :with_source_rule do
+      association :source_rule, factory: :merge_requests_approval_rule
     end
   end
 end
diff --git a/ee/spec/models/merge_requests/approval_rule_spec.rb b/ee/spec/models/merge_requests/approval_rule_spec.rb
index 73721d2dc4fdf8..5a2f49f03a2096 100644
--- a/ee/spec/models/merge_requests/approval_rule_spec.rb
+++ b/ee/spec/models/merge_requests/approval_rule_spec.rb
@@ -3,54 +3,42 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequests::ApprovalRule, type: :model, feature_category: :code_review_workflow do
-  subject(:rule) { build(:merge_requests_approval_rule, *traits) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:group) { create(:group) }
+  let(:attributes) { {} }
 
-  let(:traits) { [] }
-
-  describe 'associations' do
-    context 'when associated with a source_group' do
-      let(:traits) { [:with_source_group] }
-
-      it { is_expected.to belong_to(:source_group) }
-    end
-
-    context 'when associated with a source_project' do
-      let(:traits) { [:with_source_project] }
-
-      it { is_expected.to belong_to(:source_project) }
-    end
-  end
+  subject(:rule) { build(:merge_requests_approval_rule, attributes) }
 
   describe 'validations' do
-    context 'when associated with a source_group' do
-      let(:traits) { [:with_source_group] }
+    context 'with group_id' do
+      let(:attributes) { { group_id: group.id } }
 
       it { is_expected.to be_valid }
     end
 
-    context 'when associated with a source_project' do
-      let(:traits) { [:with_source_project] }
+    context 'with project_id' do
+      let(:attributes) { { project_id: project.id } }
 
       it { is_expected.to be_valid }
     end
 
-    context 'when not associated with either source_group or source_project' do
+    context 'without project_id or group_id' do
       it { is_expected.not_to be_valid }
 
       it 'has the correct error message' do
         rule.valid?
-        expect(rule.errors[:base]).to include("Must have either a source group or source project")
+        expect(rule.errors[:base]).to contain_exactly("Must have either a group id or project id")
       end
     end
 
-    context 'when associated with both source_group and source_project' do
-      let(:traits) { [:with_source_group, :with_source_project] }
+    context 'with both project_id and group_id' do
+      let(:attributes) { { project_id: project.id, group_id: group.id } }
 
       it { is_expected.not_to be_valid }
 
       it 'has the correct error message' do
         rule.valid?
-        expect(rule.errors[:base]).to include("Cannot have both a source group and source project")
+        expect(rule.errors[:base]).to contain_exactly("Cannot have both a group id and project id")
       end
     end
   end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 45e952cb1d85f7..b2bff3b96eda89 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -255,7 +255,6 @@ merge_requests:
 - approver_users
 - approver_groups
 - approved_by_users
-- v2_approval_rules_from_project_origin
 - draft_notes
 - merge_train_car
 - blocks_as_blocker
@@ -770,7 +769,6 @@ project:
 - approval_merge_request_rules
 - approval_merge_request_rule_sources
 - approval_project_rules
-- v2_approval_rules_from_project_origin
 - approvers
 - approver_users
 - audit_events
-- 
GitLab


From f6d5a6a36de1da3a61a8fa5ed106d2e80b17d14b Mon Sep 17 00:00:00 2001
From: ghinfey <ghinfey@gitlab.com>
Date: Wed, 5 Feb 2025 10:44:54 +0000
Subject: [PATCH 3/4] Add foreign keys in seperate migrations

---
 ...50_create_merge_requests_approval_rules.rb | 22 +++++--------------
 ..._requests_approval_rules_source_rule_fk.rb | 17 ++++++++++++++
 ...erge_requests_approval_rules_project_fk.rb | 16 ++++++++++++++
 ..._merge_requests_approval_rules_group_fk.rb | 16 ++++++++++++++
 ...rules_multi_column_not_null_constraint.rb} |  1 -
 db/schema_migrations/20250130110721           |  1 -
 db/schema_migrations/20250205094214           |  1 +
 db/schema_migrations/20250205094243           |  1 +
 db/schema_migrations/20250205094302           |  1 +
 db/schema_migrations/20250205094331           |  1 +
 db/structure.sql                              | 22 +++++++++----------
 11 files changed, 70 insertions(+), 29 deletions(-)
 create mode 100644 db/migrate/20250205094214_add_merge_requests_approval_rules_source_rule_fk.rb
 create mode 100644 db/migrate/20250205094243_add_merge_requests_approval_rules_project_fk.rb
 create mode 100644 db/migrate/20250205094302_add_merge_requests_approval_rules_group_fk.rb
 rename db/migrate/{20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb => 20250205094331_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb} (99%)
 delete mode 100644 db/schema_migrations/20250130110721
 create mode 100644 db/schema_migrations/20250205094214
 create mode 100644 db/schema_migrations/20250205094243
 create mode 100644 db/schema_migrations/20250205094302
 create mode 100644 db/schema_migrations/20250205094331

diff --git a/db/migrate/20250123151650_create_merge_requests_approval_rules.rb b/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
index 5d4cf0432fbd4a..700e18d4dedbd2 100644
--- a/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
+++ b/db/migrate/20250123151650_create_merge_requests_approval_rules.rb
@@ -9,22 +9,12 @@ def change
       t.integer :approvals_required, null: false, default: 0
       t.integer :rule_type, null: false, default: 0, limit: 2
       t.integer :origin, null: false, default: 0, limit: 2
-      t.belongs_to(
-        :project,
-        foreign_key: { to_table: :projects, on_delete: :cascade },
-        null: true
-      )
-      t.belongs_to(
-        :group,
-        foreign_key: { to_table: :namespaces, on_delete: :cascade },
-        null: true
-      )
-      t.belongs_to(
-        :source_rule,
-        foreign_key: { to_table: :merge_requests_approval_rules, on_delete: :nullify },
-        index: { name: 'index_approval_rules_on_source_rule_id' },
-        null: true
-      )
+      t.bigint :project_id, null: true
+      t.bigint :group_id, null: true
+      t.bigint :source_rule_id, null: true
+      t.index :project_id
+      t.index :group_id
+      t.index :source_rule_id
       t.timestamps_with_timezone null: false
     end
   end
diff --git a/db/migrate/20250205094214_add_merge_requests_approval_rules_source_rule_fk.rb b/db/migrate/20250205094214_add_merge_requests_approval_rules_source_rule_fk.rb
new file mode 100644
index 00000000000000..03a6a9ba8a790b
--- /dev/null
+++ b/db/migrate/20250205094214_add_merge_requests_approval_rules_source_rule_fk.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AddMergeRequestsApprovalRulesSourceRuleFk < Gitlab::Database::Migration[2.2]
+  milestone '17.9'
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :merge_requests_approval_rules, :merge_requests_approval_rules, column: :source_rule_id,
+      on_delete: :nullify
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_requests_approval_rules, column: :source_rule_id
+    end
+  end
+end
diff --git a/db/migrate/20250205094243_add_merge_requests_approval_rules_project_fk.rb b/db/migrate/20250205094243_add_merge_requests_approval_rules_project_fk.rb
new file mode 100644
index 00000000000000..40a1ee3859b19e
--- /dev/null
+++ b/db/migrate/20250205094243_add_merge_requests_approval_rules_project_fk.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class AddMergeRequestsApprovalRulesProjectFk < Gitlab::Database::Migration[2.2]
+  milestone '17.9'
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :merge_requests_approval_rules, :projects, column: :project_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_requests_approval_rules, column: :project_id
+    end
+  end
+end
diff --git a/db/migrate/20250205094302_add_merge_requests_approval_rules_group_fk.rb b/db/migrate/20250205094302_add_merge_requests_approval_rules_group_fk.rb
new file mode 100644
index 00000000000000..b558885fda6873
--- /dev/null
+++ b/db/migrate/20250205094302_add_merge_requests_approval_rules_group_fk.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class AddMergeRequestsApprovalRulesGroupFk < Gitlab::Database::Migration[2.2]
+  milestone '17.9'
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :merge_requests_approval_rules, :namespaces, column: :group_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_requests_approval_rules, column: :group_id
+    end
+  end
+end
diff --git a/db/migrate/20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb b/db/migrate/20250205094331_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
similarity index 99%
rename from db/migrate/20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
rename to db/migrate/20250205094331_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
index 24e0e203b532c4..b34c7ea12a4260 100644
--- a/db/migrate/20250130110721_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
+++ b/db/migrate/20250205094331_add_merge_requests_approval_rules_multi_column_not_null_constraint.rb
@@ -2,7 +2,6 @@
 
 class AddMergeRequestsApprovalRulesMultiColumnNotNullConstraint < Gitlab::Database::Migration[2.2]
   milestone '17.9'
-
   disable_ddl_transaction!
 
   def up
diff --git a/db/schema_migrations/20250130110721 b/db/schema_migrations/20250130110721
deleted file mode 100644
index 7a1e2fbe886aeb..00000000000000
--- a/db/schema_migrations/20250130110721
+++ /dev/null
@@ -1 +0,0 @@
-4804fd7719ef467850550e1a488d544ec412f83e51335a367eca119ba550161c
\ No newline at end of file
diff --git a/db/schema_migrations/20250205094214 b/db/schema_migrations/20250205094214
new file mode 100644
index 00000000000000..d8a44e1c1b5c2e
--- /dev/null
+++ b/db/schema_migrations/20250205094214
@@ -0,0 +1 @@
+b22251b9e81427177253b4121f677e6a690378b55cc2836e3da0019b71822629
\ No newline at end of file
diff --git a/db/schema_migrations/20250205094243 b/db/schema_migrations/20250205094243
new file mode 100644
index 00000000000000..45f04e8079f5f0
--- /dev/null
+++ b/db/schema_migrations/20250205094243
@@ -0,0 +1 @@
+6d8e72cc35740ab5214c24b9d2094bce9289edd5c999ef89d4c2aa9b4d949a07
\ No newline at end of file
diff --git a/db/schema_migrations/20250205094302 b/db/schema_migrations/20250205094302
new file mode 100644
index 00000000000000..b54934da8ad24e
--- /dev/null
+++ b/db/schema_migrations/20250205094302
@@ -0,0 +1 @@
+89018d03700f67c3d24a34d03529697d7e75dcffad09c6234239a7c661746071
\ No newline at end of file
diff --git a/db/schema_migrations/20250205094331 b/db/schema_migrations/20250205094331
new file mode 100644
index 00000000000000..c5eb507d3afba1
--- /dev/null
+++ b/db/schema_migrations/20250205094331
@@ -0,0 +1 @@
+660df89c5562b36729e4f93d57ad28b761a275105a806eb91258fb01f3b3703f
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 2ad08d4d907b0e..5aa852900c5cd0 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -31049,8 +31049,6 @@ CREATE INDEX index_approval_rule_on_protected_environment_id ON protected_enviro
 
 CREATE INDEX index_approval_rules_code_owners_rule_type ON approval_merge_request_rules USING btree (merge_request_id) WHERE (rule_type = 2);
 
-CREATE INDEX index_approval_rules_on_source_rule_id ON merge_requests_approval_rules USING btree (source_rule_id);
-
 CREATE INDEX index_approvals_on_merge_request_id_and_created_at ON approvals USING btree (merge_request_id, created_at);
 
 CREATE UNIQUE INDEX index_approvals_on_user_id_and_merge_request_id ON approvals USING btree (user_id, merge_request_id);
@@ -33025,6 +33023,8 @@ CREATE INDEX index_merge_requests_approval_rules_on_group_id ON merge_requests_a
 
 CREATE INDEX index_merge_requests_approval_rules_on_project_id ON merge_requests_approval_rules USING btree (project_id);
 
+CREATE INDEX index_merge_requests_approval_rules_on_source_rule_id ON merge_requests_approval_rules USING btree (source_rule_id);
+
 CREATE INDEX index_merge_requests_closing_issues_on_issue_id ON merge_requests_closing_issues USING btree (issue_id);
 
 CREATE INDEX index_merge_requests_closing_issues_on_merge_request_id ON merge_requests_closing_issues USING btree (merge_request_id);
@@ -37910,6 +37910,9 @@ ALTER TABLE ONLY cluster_agent_url_configurations
 ALTER TABLE ONLY incident_management_escalation_rules
     ADD CONSTRAINT fk_0314ee86eb FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY merge_requests_approval_rules
+    ADD CONSTRAINT fk_03983bf729 FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY audit_events_instance_google_cloud_logging_configurations
     ADD CONSTRAINT fk_03a15ca4fa FOREIGN KEY (stream_destination_id) REFERENCES audit_events_instance_external_streaming_destinations(id) ON DELETE SET NULL;
 
@@ -38780,6 +38783,9 @@ ALTER TABLE ONLY scan_result_policies
 ALTER TABLE ONLY catalog_resource_versions
     ADD CONSTRAINT fk_7ad8849db4 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY merge_requests_approval_rules
+    ADD CONSTRAINT fk_7af76dbd21 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY issue_customer_relations_contacts
     ADD CONSTRAINT fk_7b92f835bb FOREIGN KEY (contact_id) REFERENCES customer_relations_contacts(id) ON DELETE CASCADE;
 
@@ -39725,6 +39731,9 @@ ALTER TABLE ONLY application_settings
 ALTER TABLE ONLY issuable_severities
     ADD CONSTRAINT fk_f9df19ecb6 FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY merge_requests_approval_rules
+    ADD CONSTRAINT fk_fa5b38e373 FOREIGN KEY (source_rule_id) REFERENCES merge_requests_approval_rules(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY clusters_managed_resources
     ADD CONSTRAINT fk_fad3c3b2e2 FOREIGN KEY (environment_id) REFERENCES environments(id) ON DELETE CASCADE;
 
@@ -39824,9 +39833,6 @@ ALTER TABLE ONLY automation_rules
 ALTER TABLE ONLY incident_management_oncall_participants
     ADD CONSTRAINT fk_rails_032b12996a FOREIGN KEY (oncall_rotation_id) REFERENCES incident_management_oncall_rotations(id) ON DELETE CASCADE;
 
-ALTER TABLE ONLY merge_requests_approval_rules
-    ADD CONSTRAINT fk_rails_03983bf729 FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE;
-
 ALTER TABLE ONLY events
     ADD CONSTRAINT fk_rails_0434b48643 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
@@ -40751,9 +40757,6 @@ ALTER TABLE ONLY operations_scopes
 ALTER TABLE ONLY milestone_releases
     ADD CONSTRAINT fk_rails_7ae0756a2d FOREIGN KEY (milestone_id) REFERENCES milestones(id) ON DELETE CASCADE;
 
-ALTER TABLE ONLY merge_requests_approval_rules
-    ADD CONSTRAINT fk_rails_7af76dbd21 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
-
 ALTER TABLE ONLY scan_execution_policy_rules
     ADD CONSTRAINT fk_rails_7be2571ecf FOREIGN KEY (security_policy_id) REFERENCES security_policies(id) ON DELETE CASCADE;
 
@@ -41720,9 +41723,6 @@ ALTER TABLE ONLY merge_requests_closing_issues
 ALTER TABLE p_ci_job_artifact_reports
     ADD CONSTRAINT fk_rails_f9b8550174 FOREIGN KEY (partition_id, job_artifact_id) REFERENCES p_ci_job_artifacts(partition_id, id) ON UPDATE CASCADE ON DELETE CASCADE;
 
-ALTER TABLE ONLY merge_requests_approval_rules
-    ADD CONSTRAINT fk_rails_fa5b38e373 FOREIGN KEY (source_rule_id) REFERENCES merge_requests_approval_rules(id) ON DELETE SET NULL;
-
 ALTER TABLE ONLY banned_users
     ADD CONSTRAINT fk_rails_fa5bb598e5 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
 
-- 
GitLab


From 3192832f86519fc8e8f158fde317a317b015f6cf Mon Sep 17 00:00:00 2001
From: ghinfey <ghinfey@gitlab.com>
Date: Thu, 6 Feb 2025 13:17:49 +0000
Subject: [PATCH 4/4] Review chages refactor validations

---
 ee/app/models/merge_requests/approval_rule.rb | 24 +++++++----
 .../merge_requests/approval_rule_spec.rb      | 42 ++++++++++---------
 2 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb
index 49c7bf5c079526..46538828aea773 100644
--- a/ee/app/models/merge_requests/approval_rule.rb
+++ b/ee/app/models/merge_requests/approval_rule.rb
@@ -4,7 +4,7 @@ module MergeRequests
   class ApprovalRule < ApplicationRecord
     self.table_name = 'merge_requests_approval_rules'
 
-    validate :group_id_or_project_id_present
+    validate :ensure_single_sharding_key
 
     with_options validate: true do
       enum :rule_type, { regular: 0, code_owner: 1, report_approver: 2, any_approver: 3 }, default: :regular
@@ -13,12 +13,22 @@ class ApprovalRule < ApplicationRecord
 
     private
 
-    def group_id_or_project_id_present
-      if group_id.blank? && project_id.blank?
-        errors.add(:base, "Must have either a group id or project id")
-      elsif group_id.present? && project_id.present?
-        errors.add(:base, "Cannot have both a group id and project id")
-      end
+    def ensure_single_sharding_key
+      return errors.add(:base, "Must have either `group_id` or `project_id`") if no_sharding_key?
+
+      errors.add(:base, "Cannot have both `group_id` and `project_id`") if multiple_sharding_keys?
+    end
+
+    def sharding_keys
+      [group_id, project_id]
+    end
+
+    def no_sharding_key?
+      sharding_keys.all?(&:blank?)
+    end
+
+    def multiple_sharding_keys?
+      sharding_keys.all?(&:present?)
     end
   end
 end
diff --git a/ee/spec/models/merge_requests/approval_rule_spec.rb b/ee/spec/models/merge_requests/approval_rule_spec.rb
index 5a2f49f03a2096..f038a559295d65 100644
--- a/ee/spec/models/merge_requests/approval_rule_spec.rb
+++ b/ee/spec/models/merge_requests/approval_rule_spec.rb
@@ -10,35 +10,37 @@
   subject(:rule) { build(:merge_requests_approval_rule, attributes) }
 
   describe 'validations' do
-    context 'with group_id' do
-      let(:attributes) { { group_id: group.id } }
+    describe 'sharding key validation' do
+      context 'with group_id' do
+        let(:attributes) { { group_id: group.id } }
 
-      it { is_expected.to be_valid }
-    end
+        it { is_expected.to be_valid }
+      end
 
-    context 'with project_id' do
-      let(:attributes) { { project_id: project.id } }
+      context 'with project_id' do
+        let(:attributes) { { project_id: project.id } }
 
-      it { is_expected.to be_valid }
-    end
+        it { is_expected.to be_valid }
+      end
 
-    context 'without project_id or group_id' do
-      it { is_expected.not_to be_valid }
+      context 'without project_id or group_id' do
+        it { is_expected.not_to be_valid }
 
-      it 'has the correct error message' do
-        rule.valid?
-        expect(rule.errors[:base]).to contain_exactly("Must have either a group id or project id")
+        it 'has the correct error message' do
+          rule.valid?
+          expect(rule.errors[:base]).to contain_exactly("Must have either `group_id` or `project_id`")
+        end
       end
-    end
 
-    context 'with both project_id and group_id' do
-      let(:attributes) { { project_id: project.id, group_id: group.id } }
+      context 'with both project_id and group_id' do
+        let(:attributes) { { project_id: project.id, group_id: group.id } }
 
-      it { is_expected.not_to be_valid }
+        it { is_expected.not_to be_valid }
 
-      it 'has the correct error message' do
-        rule.valid?
-        expect(rule.errors[:base]).to contain_exactly("Cannot have both a group id and project id")
+        it 'has the correct error message' do
+          rule.valid?
+          expect(rule.errors[:base]).to contain_exactly("Cannot have both `group_id` and `project_id`")
+        end
       end
     end
   end
-- 
GitLab