Merge branch...

Merge branch '215194-add-section-to-index_approval_rule_name_for_code_owners_rule_type' into 'master'

[RUN AS-IF-FOSS] Update indices on ApprovalMergeRequestRule

See merge request gitlab-org/gitlab!34680

changelogs/unreleased/215194-add-section-to-index_approval_rule_name_for_code_owners_rule_type.yml

+---
+title: Add :section to approval_merge_request_rule unique index
+merge_request: 34680
+author:
+type: other

db/migrate/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type.rb

+# frozen_string_literal: true
+
+class UpdateIndexApprovalRuleNameForCodeOwnersRuleType < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  LEGACY_INDEX_NAME_RULE_TYPE   = "index_approval_rule_name_for_code_owners_rule_type"
+  LEGACY_INDEX_NAME_CODE_OWNERS = "approval_rule_name_index_for_code_owners"
+
+  SECTIONAL_INDEX_NAME = "index_approval_rule_name_for_sectional_code_owners_rule_type"
+
+  CODE_OWNER_RULE_TYPE = 2
+
+  def up
+    unless index_exists_by_name?(:approval_merge_request_rules, SECTIONAL_INDEX_NAME)
+      # Ensure only 1 code_owner rule with the same name and section per merge_request
+      #
+      add_concurrent_index(
+        :approval_merge_request_rules,
+        [:merge_request_id, :name, :section],
+        unique: true,
+        where: "rule_type = #{CODE_OWNER_RULE_TYPE}",
+        name: SECTIONAL_INDEX_NAME
+      )
+    end
+
+    remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME_RULE_TYPE
+    remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME_CODE_OWNERS
+
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :name],
+      unique: true,
+      where: "rule_type = #{CODE_OWNER_RULE_TYPE} AND section IS NULL",
+      name: LEGACY_INDEX_NAME_RULE_TYPE
+    )
+
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :code_owner, :name],
+      unique: true,
+      where: "code_owner = true AND section IS NULL",
+      name: LEGACY_INDEX_NAME_CODE_OWNERS
+    )
+  end
+
+  def down
+    # In a rollback situation, we can't guarantee that there will not be
+    #   records that were allowed under the more specific SECTIONAL_INDEX_NAME
+    #   index but would cause uniqueness violations under both the
+    #   LEGACY_INDEX_NAME_RULE_TYPE and LEGACY_INDEX_NAME_CODE_OWNERS indices.
+    #   Therefore, we need to first find all the MergeRequests with
+    #   ApprovalMergeRequestRules that would violate these "new" indices and
+    #   delete those approval rules, then create the new index, then finally
+    #   recreate the approval rules for those merge requests.
+    #
+
+    # First, find all MergeRequests with ApprovalMergeRequestRules that will
+    #   violate the new index.
+    #
+    if Gitlab.ee?
+      merge_request_ids = ApprovalMergeRequestRule
+        .select(:merge_request_id)
+        .where(rule_type: CODE_OWNER_RULE_TYPE)
+        .group(:merge_request_id, :rule_type, :name)
+        .includes(:merge_request)
+        .having("count(*) > 1")
+        .collect(&:merge_request_id)
+
+      # Delete ALL their code_owner approval rules
+      #
+      merge_request_ids.each_slice(10) do |ids|
+        ApprovalMergeRequestRule.where(merge_request_id: ids).code_owner.delete_all
+      end
+    end
+
+    # Remove legacy partial indices that only apply to `section IS NULL` records
+    #
+    remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME_RULE_TYPE
+    remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME_CODE_OWNERS
+
+    # Reconstruct original "legacy" indices
+    #
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :name],
+      unique: true,
+      where: "rule_type = #{CODE_OWNER_RULE_TYPE}",
+      name: LEGACY_INDEX_NAME_RULE_TYPE
+    )
+
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :code_owner, :name],
+      unique: true,
+      where: "code_owner = true",
+      name: LEGACY_INDEX_NAME_CODE_OWNERS
+    )
+
+    # MergeRequest::SyncCodeOwnerApprovalRules recreates the code_owner rules
+    #   from scratch, adding them to the index. Duplicates will be rejected.
+    #
+    if Gitlab.ee?
+      merge_request_ids.each_slice(10) do |ids|
+        MergeRequest.where(id: ids).each do |merge_request|
+          MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
+        end
+      end
+    end
+
+    remove_concurrent_index_by_name :approval_merge_request_rules, SECTIONAL_INDEX_NAME
+  end
+end

db/structure.sql

@@ -9163,7 +9163,7 @@ CREATE UNIQUE INDEX any_approver_merge_request_rule_type_unique_index ON public.
 
 CREATE UNIQUE INDEX any_approver_project_rule_type_unique_index ON public.approval_project_rules USING btree (project_id) WHERE (rule_type = 3);
 
-CREATE UNIQUE INDEX approval_rule_name_index_for_code_owners ON public.approval_merge_request_rules USING btree (merge_request_id, code_owner, name) WHERE (code_owner = true);
+CREATE UNIQUE INDEX approval_rule_name_index_for_code_owners ON public.approval_merge_request_rules USING btree (merge_request_id, code_owner, name) WHERE ((code_owner = true) AND (section IS NULL));
 
 CREATE INDEX ci_builds_gitlab_monitor_metrics ON public.ci_builds USING btree (status, created_at, project_id) WHERE ((type)::text = 'Ci::Build'::text);
 
@@ -9341,7 +9341,9 @@ CREATE UNIQUE INDEX index_approval_project_rules_users_1 ON public.approval_proj
 
 CREATE INDEX index_approval_project_rules_users_2 ON public.approval_project_rules_users USING btree (user_id);
 
-CREATE UNIQUE INDEX index_approval_rule_name_for_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id, name) WHERE (rule_type = 2);
+CREATE UNIQUE INDEX index_approval_rule_name_for_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id, name) WHERE ((rule_type = 2) AND (section IS NULL));
+
+CREATE UNIQUE INDEX index_approval_rule_name_for_sectional_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id, name, section) WHERE (rule_type = 2);
 
 CREATE INDEX index_approval_rules_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id) WHERE (rule_type = 2);
 
@@ -13956,6 +13958,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200526153844
 20200526164946
 20200526164947
+20200526231421
 20200527092027
 20200527094322
 20200527095401

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -45,9 +45,9 @@ RSpec.describe ApprovalMergeRequestRule do
       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)
+        existing = create(:code_owner_rule, name: '*.rb', merge_request: merge_request, section: 'section')
 
-        new = build(:code_owner_rule, merge_request: existing.merge_request, name: '*.rb')
+        new = build(:code_owner_rule, merge_request: existing.merge_request, name: '*.rb', section: 'section')
 
         expect(new).not_to be_valid
         expect { new.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)

spec/migrations/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200526231421_update_index_approval_rule_name_for_code_owners_rule_type.rb')
+
+describe UpdateIndexApprovalRuleNameForCodeOwnersRuleType do
+  let(:migration) { described_class.new }
+
+  let(:approval_rules) { table(:approval_merge_request_rules) }
+  let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab') }
+
+  let(:project) do
+    table(:projects).create!(
+      namespace_id: namespace.id,
+      name: 'gitlab',
+      path: 'gitlab'
+    )
+  end
+
+  let(:merge_request) do
+    table(:merge_requests).create!(
+      target_project_id: project.id,
+      source_project_id: project.id,
+      target_branch: 'feature',
+      source_branch: 'master'
+    )
+  end
+
+  let(:index_names) do
+    ActiveRecord::Base.connection
+      .indexes(:approval_merge_request_rules)
+      .collect(&:name)
+  end
+
+  def create_sectional_approval_rules
+    approval_rules.create!(
+      merge_request_id: merge_request.id,
+      name: "*.rb",
+      code_owner: true,
+      rule_type: 2,
+      section: "First Section"
+    )
+
+    approval_rules.create!(
+      merge_request_id: merge_request.id,
+      name: "*.rb",
+      code_owner: true,
+      rule_type: 2,
+      section: "Second Section"
+    )
+  end
+
+  def create_two_matching_nil_section_approval_rules
+    2.times do
+      approval_rules.create!(
+        merge_request_id: merge_request.id,
+        name: "nil_section",
+        code_owner: true,
+        rule_type: 2
+      )
+    end
+  end
+
+  before do
+    approval_rules.delete_all
+  end
+
+  describe "#up" do
+    it "creates the new index and removes the 'legacy' indices" do
+      # Confirm that existing legacy indices prevent duplicate entries
+      #
+      expect { create_sectional_approval_rules }
+        .to raise_exception(ActiveRecord::RecordNotUnique)
+      expect { create_two_matching_nil_section_approval_rules }
+        .to raise_exception(ActiveRecord::RecordNotUnique)
+
+      approval_rules.delete_all
+
+      disable_migrations_output { migrate! }
+
+      # After running the migration, expect `section == nil` rules to still
+      #   be blocked by the legacy indices, but sectional rules are allowed.
+      #
+      expect { create_sectional_approval_rules }
+        .to change { approval_rules.count }.by(2)
+      expect { create_two_matching_nil_section_approval_rules }
+        .to raise_exception(ActiveRecord::RecordNotUnique)
+
+      # Attempt to rerun the creation of sectional rules, and see that sectional
+      #   rules are unique by section
+      #
+      expect { create_sectional_approval_rules }
+        .to raise_exception(ActiveRecord::RecordNotUnique)
+
+      expect(index_names).to include(
+        described_class::SECTIONAL_INDEX_NAME,
+        described_class::LEGACY_INDEX_NAME_RULE_TYPE,
+        described_class::LEGACY_INDEX_NAME_CODE_OWNERS
+      )
+    end
+  end
+
+  describe "#down" do
+    context "run as FOSS" do
+      before do
+        expect(Gitlab).to receive(:ee?).twice.and_return(false)
+      end
+
+      it "recreates legacy indices, but does not invoke EE-specific code" do
+        disable_migrations_output { migrate! }
+
+        expect(index_names).to include(
+          described_class::SECTIONAL_INDEX_NAME,
+          described_class::LEGACY_INDEX_NAME_RULE_TYPE,
+          described_class::LEGACY_INDEX_NAME_CODE_OWNERS
+        )
+
+        # Since ApprovalMergeRequestRules are EE-specific, we expect none to
+        #   be deleted during the migration.
+        #
+        expect { disable_migrations_output { migration.down } }
+          .not_to change { approval_rules.count }
+
+        index_names = ActiveRecord::Base.connection
+          .indexes(:approval_merge_request_rules)
+          .collect(&:name)
+
+        expect(index_names).not_to include(described_class::SECTIONAL_INDEX_NAME)
+        expect(index_names).to include(
+          described_class::LEGACY_INDEX_NAME_RULE_TYPE,
+          described_class::LEGACY_INDEX_NAME_CODE_OWNERS
+        )
+      end
+    end
+
+    context "EE" do
+      it "recreates 'legacy' indices and removes duplicate code owner approval rules" do
+        skip("This test is skipped under FOSS") unless Gitlab.ee?
+
+        disable_migrations_output { migrate! }
+
+        expect { create_sectional_approval_rules }
+          .to change { approval_rules.count }.by(2)
+        expect { create_two_matching_nil_section_approval_rules }
+          .to raise_exception(ActiveRecord::RecordNotUnique)
+
+        # Run the down migration. This will remove the 2 approval rules we create
+        #   above, and call MergeRequests::SyncCodeOwnerApprovalRules to recreate
+        #   new ones.
+        #
+        expect(MergeRequests::SyncCodeOwnerApprovalRules)
+          .to receive(:new).with(MergeRequest.find(merge_request.id)).once.and_call_original
+
+        # We expect approval_rules.count to be changed by -2 as we're deleting the
+        #   3 rules created above, and MergeRequests::SyncCodeOwnerApprovalRules
+        #   will not be able to create new one with an empty (or missing)
+        #   CODEOWNERS file.
+        #
+        expect { disable_migrations_output { migration.down } }
+          .to change { approval_rules.count }.by(-3)
+
+        # Test that the index does not allow us to create the same rules as the
+        #   previous sectional index.
+        #
+        expect { create_sectional_approval_rules }
+          .to raise_exception(ActiveRecord::RecordNotUnique)
+        expect { create_two_matching_nil_section_approval_rules }
+          .to raise_exception(ActiveRecord::RecordNotUnique)
+
+        expect(index_names).not_to include(described_class::SECTIONAL_INDEX_NAME)
+        expect(index_names).to include(
+          described_class::LEGACY_INDEX_NAME_RULE_TYPE,
+          described_class::LEGACY_INDEX_NAME_CODE_OWNERS
+        )
+      end
+    end
+  end
+end