Merge branch 'revert-1ef53665' into 'master'

Revert "Add section to approval rule name index"

See merge request gitlab-org/gitlab!34679

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: 33121
-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.
-    #
-    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
-
-    # 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.
-    #
-    merge_request_ids.each_slice(10) do |ids|
-      MergeRequest.where(id: ids).each do |merge_request|
-        MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
-      end
-    end
-
-    remove_concurrent_index_by_name :approval_merge_request_rules, SECTIONAL_INDEX_NAME
-  end
-end

db/structure.sql

@@ -9156,7 +9156,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) AND (section IS NULL));
+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 INDEX ci_builds_gitlab_monitor_metrics ON public.ci_builds USING btree (status, created_at, project_id) WHERE ((type)::text = 'Ci::Build'::text);
 
@@ -9334,9 +9334,7 @@ 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) 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 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 INDEX index_approval_rules_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id) WHERE (rule_type = 2);
 
@@ -13963,7 +13961,6 @@ 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, section: 'section')
+        existing = create(:code_owner_rule, name: '*.rb', merge_request: merge_request)
 
-        new = build(:code_owner_rule, merge_request: existing.merge_request, name: '*.rb', section: 'section')
+        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)

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
-    it "recreates 'legacy' indices and removes duplicate code owner approval rules" do
-      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