diff --git a/changelogs/unreleased/215194-add-section-to-index_approval_rule_name_for_code_owners_rule_type.yml b/changelogs/unreleased/215194-add-section-to-index_approval_rule_name_for_code_owners_rule_type.yml new file mode 100644 index 0000000000000000000000000000000000000000..4f5103d2305d6cebffa40df9af75c2f03770ae5f --- /dev/null +++ b/changelogs/unreleased/215194-add-section-to-index_approval_rule_name_for_code_owners_rule_type.yml @@ -0,0 +1,5 @@ +--- +title: Add :section to approval_merge_request_rule unique index +merge_request: 33121 +author: +type: other diff --git a/db/migrate/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type.rb b/db/migrate/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type.rb new file mode 100644 index 0000000000000000000000000000000000000000..7e31c9880cddfae36011ad2e94222e5b341c2643 --- /dev/null +++ b/db/migrate/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type.rb @@ -0,0 +1,112 @@ +# 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 diff --git a/db/structure.sql b/db/structure.sql index 3602106ab39944c9a4ae81d640998c9d1ee1f954..f118a90b3bf84d41803b6a355e332b3a9f0a920f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9145,7 +9145,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); @@ -9325,7 +9325,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); @@ -13941,6 +13943,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200526153844 20200526164946 20200526164947 +20200526231421 20200527092027 20200527094322 20200527095401 diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index 9336baea5fd77866106580b52d5bbc7371e15be6..7c098c060a2babb551ce7c52298cbbefc8a8204a 100644 --- a/ee/spec/models/approval_merge_request_rule_spec.rb +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -45,9 +45,9 @@ 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) diff --git a/spec/migrations/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type_spec.rb b/spec/migrations/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4aa912ef873c12df17066ba4a365358a943e97a6 --- /dev/null +++ b/spec/migrations/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type_spec.rb @@ -0,0 +1,142 @@ +# 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