From f25fe2eb5d4efcaa6aee7f44d2f559a20242592f Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Tue, 26 May 2020 16:28:29 -0700 Subject: [PATCH 01/15] Add section to approval rule name index --- ...val_rule_name_for_code_owners_rule_type.rb | 56 +++++++++++++++++++ db/structure.sql | 3 +- 2 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type.rb 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 00000000000000..1c56e773b86853 --- /dev/null +++ b/db/migrate/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +class UpdateIndexApprovalRuleNameForCodeOwnersRuleType < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + LEGACY_INDEX_NAME = "index_approval_rule_name_for_code_owners_rule_type" + SECTIONAL_INDEX_NAME = "index_approval_rule_name_for_sectional_code_owners_rule_type" + + class ApprovalMergeRequestRule < ActiveRecord::Base + enum rule_types: { + code_owner: 2 + } + end + + 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, :rule_type, :name, :section], + unique: true, + where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}", + name: SECTIONAL_INDEX_NAME + ) + end + + if index_exists_by_name?(:approval_merge_request_rules, LEGACY_INDEX_NAME) + remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME + end + end + + def down + unless index_exists_by_name?(:approval_merge_request_rules, LEGACY_INDEX_NAME) + # Reconstruct original "legacy" index, but without the unique constraint; + # in a rollback situation, we can't guarantee that there will not be + # records that were allowed under the more specific SECTIONAL_INDEX_NAME + # but would cause uniqueness violations under this one. + # + add_concurrent_index( + :approval_merge_request_rules, + [:merge_request_id, :rule_type, :name], + where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}", + name: LEGACY_INDEX_NAME + ) + end + + if index_exists_by_name?(:approval_merge_request_rules, SECTIONAL_INDEX_NAME) + remove_concurrent_index_by_name :approval_merge_request_rules, SECTIONAL_INDEX_NAME + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 3602106ab39944..539acb3e525e31 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9325,7 +9325,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); +CREATE UNIQUE INDEX index_approval_rule_name_for_sectional_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id, rule_type, 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 +13941,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200526153844 20200526164946 20200526164947 +20200526231421 20200527092027 20200527094322 20200527095401 -- GitLab From c44d728c8b05ea22e134605e2f9e6b0d06cb271a Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Fri, 29 May 2020 15:53:55 -0700 Subject: [PATCH 02/15] On a rollback, delete any duplicate approval rules --- ...val_rule_name_for_code_owners_rule_type.rb | 40 +++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) 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 index 1c56e773b86853..7005017bc1e3d8 100644 --- 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 @@ -36,17 +36,51 @@ def up def down unless index_exists_by_name?(:approval_merge_request_rules, LEGACY_INDEX_NAME) - # Reconstruct original "legacy" index, but without the unique constraint; - # in a rollback situation, we can't guarantee that there will not be + # In a rollback situation, we can't guarantee that there will not be # records that were allowed under the more specific SECTIONAL_INDEX_NAME - # but would cause uniqueness violations under this one. + # index but would cause uniqueness violations under the LEGACY_INDEX_NAME + # index. Therefore, we need to first find all the MergeRequests with + # ApprovalMergeRequestRules that would violate the new index 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) + .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| + MergeRequest.where(id: ids).each do |merge_request| + merge_request.approval_rules.code_owner.delete_all + end + end + + # Reconstruct original "legacy" index # add_concurrent_index( :approval_merge_request_rules, [:merge_request_id, :rule_type, :name], + unique: true, where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}", name: LEGACY_INDEX_NAME ) + + # 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| + MergeRequest::SyncCodeOwnerApprovalRules.new(merge_request).execute + end + end end if index_exists_by_name?(:approval_merge_request_rules, SECTIONAL_INDEX_NAME) -- GitLab From 98d5b72cc99d538484f58bf31b4093a0a53369f4 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Fri, 29 May 2020 15:57:24 -0700 Subject: [PATCH 03/15] Add changle for migration --- ...to-index_approval_rule_name_for_code_owners_rule_type.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/215194-add-section-to-index_approval_rule_name_for_code_owners_rule_type.yml 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 00000000000000..4f5103d2305d6c --- /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 -- GitLab From 02109738cbb3f100648e9f4f55f42a8fc58eafc6 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Fri, 29 May 2020 16:00:01 -0700 Subject: [PATCH 04/15] Remove unrequired existence check --- ..._index_approval_rule_name_for_code_owners_rule_type.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) 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 index 7005017bc1e3d8..6c4e9f08f99b4b 100644 --- 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 @@ -29,9 +29,7 @@ def up ) end - if index_exists_by_name?(:approval_merge_request_rules, LEGACY_INDEX_NAME) - remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME - end + remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME end def down @@ -83,8 +81,6 @@ def down end end - if index_exists_by_name?(:approval_merge_request_rules, SECTIONAL_INDEX_NAME) - remove_concurrent_index_by_name :approval_merge_request_rules, SECTIONAL_INDEX_NAME - end + remove_concurrent_index_by_name :approval_merge_request_rules, SECTIONAL_INDEX_NAME end end -- GitLab From f8fc6b231c126ae88a3eac5f39b3329d775533d9 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Fri, 29 May 2020 16:01:33 -0700 Subject: [PATCH 05/15] Remove unneeded definition of model --- ...ex_approval_rule_name_for_code_owners_rule_type.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) 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 index 6c4e9f08f99b4b..53fc59c6bd8e40 100644 --- 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 @@ -9,12 +9,7 @@ class UpdateIndexApprovalRuleNameForCodeOwnersRuleType < ActiveRecord::Migration LEGACY_INDEX_NAME = "index_approval_rule_name_for_code_owners_rule_type" SECTIONAL_INDEX_NAME = "index_approval_rule_name_for_sectional_code_owners_rule_type" - - class ApprovalMergeRequestRule < ActiveRecord::Base - enum rule_types: { - code_owner: 2 - } - end + CODE_OWNER_RULE_TYPE = 2 def up unless index_exists_by_name?(:approval_merge_request_rules, SECTIONAL_INDEX_NAME) @@ -24,7 +19,7 @@ def up :approval_merge_request_rules, [:merge_request_id, :rule_type, :name, :section], unique: true, - where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}", + where: "rule_type = #{CODE_OWNER_RULE_TYPE}", name: SECTIONAL_INDEX_NAME ) end @@ -67,7 +62,7 @@ def down :approval_merge_request_rules, [:merge_request_id, :rule_type, :name], unique: true, - where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}", + where: "rule_type = #{CODE_OWNER_RULE_TYPE}", name: LEGACY_INDEX_NAME ) -- GitLab From a1bcc7d3256f4d5db27562dcb7867a4b8ce6d38b Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 1 Jun 2020 00:18:23 -0700 Subject: [PATCH 06/15] Use a more optimized approach for deleting --- ...date_index_approval_rule_name_for_code_owners_rule_type.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 index 53fc59c6bd8e40..930b2d5c4bd981 100644 --- 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 @@ -51,9 +51,7 @@ def down # Delete ALL their code_owner approval rules # merge_request_ids.each_slice(10) do |ids| - MergeRequest.where(id: ids).each do |merge_request| - merge_request.approval_rules.code_owner.delete_all - end + ApprovalMergeRequestRule.where(merge_request_id: ids).code_owner.delete_all end # Reconstruct original "legacy" index -- GitLab From f364ab07b17b2e82d98e6b96c6a36585f57468fc Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 1 Jun 2020 00:22:03 -0700 Subject: [PATCH 07/15] Add where clause to check for rule_type --- ..._update_index_approval_rule_name_for_code_owners_rule_type.rb | 1 + 1 file changed, 1 insertion(+) 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 index 930b2d5c4bd981..040ece6ef80cc7 100644 --- 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 @@ -43,6 +43,7 @@ def down # 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") -- GitLab From 8deb3314a7ad7be568f946fd74b277a5774c2d76 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 1 Jun 2020 17:42:39 -0700 Subject: [PATCH 08/15] Add addition legacy index to migration --- ...val_rule_name_for_code_owners_rule_type.rb | 33 +++++++++++++------ db/structure.sql | 2 -- 2 files changed, 23 insertions(+), 12 deletions(-) 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 index 040ece6ef80cc7..880dc10de70f3a 100644 --- 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 @@ -7,8 +7,11 @@ class UpdateIndexApprovalRuleNameForCodeOwnersRuleType < ActiveRecord::Migration disable_ddl_transaction! - LEGACY_INDEX_NAME = "index_approval_rule_name_for_code_owners_rule_type" + 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 @@ -24,18 +27,20 @@ def up ) end - remove_concurrent_index_by_name :approval_merge_request_rules, LEGACY_INDEX_NAME + 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 end def down - unless index_exists_by_name?(:approval_merge_request_rules, LEGACY_INDEX_NAME) + unless index_exists_by_name?(:approval_merge_request_rules, LEGACY_INDEX_NAME_RULE_TYPE) # 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 the LEGACY_INDEX_NAME - # index. Therefore, we need to first find all the MergeRequests with - # ApprovalMergeRequestRules that would violate the new index and delete - # those approval rules, then create the new index, then finally recreate - # the approval rules for those merge requests. + # 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 @@ -55,14 +60,22 @@ def down ApprovalMergeRequestRule.where(merge_request_id: ids).code_owner.delete_all end - # Reconstruct original "legacy" index + # Reconstruct original "legacy" indices # add_concurrent_index( :approval_merge_request_rules, [:merge_request_id, :rule_type, :name], unique: true, where: "rule_type = #{CODE_OWNER_RULE_TYPE}", - name: LEGACY_INDEX_NAME + 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 diff --git a/db/structure.sql b/db/structure.sql index 539acb3e525e31..1035d5a1ddb097 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9145,8 +9145,6 @@ 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 INDEX ci_builds_gitlab_monitor_metrics ON public.ci_builds USING btree (status, created_at, project_id) WHERE ((type)::text = 'Ci::Build'::text); CREATE INDEX code_owner_approval_required ON public.protected_branches USING btree (project_id, code_owner_approval_required) WHERE (code_owner_approval_required = true); -- GitLab From 1298efe18de02ec049b0ce25c21caec1aadcd579 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 1 Jun 2020 17:43:13 -0700 Subject: [PATCH 09/15] Fix typo in class name --- ...update_index_approval_rule_name_for_code_owners_rule_type.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 880dc10de70f3a..2e3735848e3acf 100644 --- 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 @@ -83,7 +83,7 @@ def down # merge_request_ids.each_slice(10) do |ids| MergeRequest.where(id: ids).each do |merge_request| - MergeRequest::SyncCodeOwnerApprovalRules.new(merge_request).execute + MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute end end end -- GitLab From 86f6aeeba1bb58ad44bc4f77dae81a65bbbc0b87 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 1 Jun 2020 17:43:34 -0700 Subject: [PATCH 10/15] Add spec for index migration --- ...ule_name_for_code_owners_rule_type_spec.rb | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 spec/migrations/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type_spec.rb 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 00000000000000..8eed8d7802ae42 --- /dev/null +++ b/spec/migrations/20200526231421_update_index_approval_rule_name_for_code_owners_rule_type_spec.rb @@ -0,0 +1,112 @@ +# 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 + + before do + approval_rules.delete_all + end + + describe "#up" do + it "creates the new index and removes the 'legacy' indices" do + # Confirm that existing indicies prevent duplicate entries + # + expect { create_sectional_approval_rules } + .to raise_exception(ActiveRecord::RecordNotUnique) + + approval_rules.delete_all + + disable_migrations_output { migrate! } + + expect { create_sectional_approval_rules } + .to change { approval_rules.count }.by(2) + + expect(index_names).to include(described_class::SECTIONAL_INDEX_NAME) + expect(index_names).not_to include(described_class::LEGACY_INDEX_NAME_RULE_TYPE) + expect(index_names).not_to include(described_class::LEGACY_INDEX_NAME_CODE_OWNERS) + end + + # it "allows duplicate name/rule_type records with different sections" do + 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) + + # 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 + # 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(-2) + + # 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(index_names).not_to include(described_class::SECTIONAL_INDEX_NAME) + expect(index_names).to include(described_class::LEGACY_INDEX_NAME_RULE_TYPE) + expect(index_names).to include(described_class::LEGACY_INDEX_NAME_CODE_OWNERS) + end + end +end -- GitLab From 2c04d9933725b713a7a7b095c3dadf7781e8606e Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 1 Jun 2020 22:23:47 -0700 Subject: [PATCH 11/15] Add section to test object creation --- ee/spec/models/approval_merge_request_rule_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index 9336baea5fd778..7c098c060a2bab 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) -- GitLab From f0686732f80746567a9e42602a9d5c3dc8d635dd Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 11 Jun 2020 12:02:30 -0700 Subject: [PATCH 12/15] Drop rule_type restriction on new index --- ...update_index_approval_rule_name_for_code_owners_rule_type.rb | 2 +- db/structure.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 index 2e3735848e3acf..bf5979a34ccb1a 100644 --- 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 @@ -20,7 +20,7 @@ def up # add_concurrent_index( :approval_merge_request_rules, - [:merge_request_id, :rule_type, :name, :section], + [:merge_request_id, :name, :section], unique: true, where: "rule_type = #{CODE_OWNER_RULE_TYPE}", name: SECTIONAL_INDEX_NAME diff --git a/db/structure.sql b/db/structure.sql index 1035d5a1ddb097..91fb0a05a7bfcc 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9323,7 +9323,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_sectional_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id, rule_type, name, section) WHERE (rule_type = 2); +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); -- GitLab From df561d1a722df1fd7159fb692e4817004969d557 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 11 Jun 2020 14:47:44 -0700 Subject: [PATCH 13/15] Add restriction to legacy indices --- ...val_rule_name_for_code_owners_rule_type.rb | 117 ++++++++++-------- db/structure.sql | 4 +- ...ule_name_for_code_owners_rule_type_spec.rb | 52 ++++++-- 3 files changed, 113 insertions(+), 60 deletions(-) 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 index bf5979a34ccb1a..1b469d37676999 100644 --- 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 @@ -29,62 +29,81 @@ def up 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, :rule_type, :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 - unless index_exists_by_name?(:approval_merge_request_rules, LEGACY_INDEX_NAME_RULE_TYPE) - # 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. - # + # 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 + # 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) - # Reconstruct original "legacy" indices - # - add_concurrent_index( - :approval_merge_request_rules, - [:merge_request_id, :rule_type, :name], - unique: true, - where: "rule_type = #{CODE_OWNER_RULE_TYPE}", - name: LEGACY_INDEX_NAME_RULE_TYPE - ) + # 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 - 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 - ) + # 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 - # 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 + # Reconstruct original "legacy" indices + # + add_concurrent_index( + :approval_merge_request_rules, + [:merge_request_id, :rule_type, :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 diff --git a/db/structure.sql b/db/structure.sql index 91fb0a05a7bfcc..87cf83e64d8d97 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9145,6 +9145,8 @@ 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 INDEX ci_builds_gitlab_monitor_metrics ON public.ci_builds USING btree (status, created_at, project_id) WHERE ((type)::text = 'Ci::Build'::text); CREATE INDEX code_owner_approval_required ON public.protected_branches USING btree (project_id, code_owner_approval_required) WHERE (code_owner_approval_required = true); @@ -9323,7 +9325,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_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, rule_type, name) WHERE ((rule_type = 2) AND (section IS NULL)); CREATE INDEX index_approval_rules_code_owners_rule_type ON public.approval_merge_request_rules USING btree (merge_request_id) WHERE (rule_type = 2); 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 index 8eed8d7802ae42..8dd14d2d99255b 100644 --- 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 @@ -50,30 +50,56 @@ def create_sectional_approval_rules ) 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 indicies prevent duplicate entries + # 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) - expect(index_names).to include(described_class::SECTIONAL_INDEX_NAME) - expect(index_names).not_to include(described_class::LEGACY_INDEX_NAME_RULE_TYPE) - expect(index_names).not_to include(described_class::LEGACY_INDEX_NAME_CODE_OWNERS) - end + # 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) - # it "allows duplicate name/rule_type records with different sections" do + expect(index_names).to include( + described_class::SECTIONAL_INDEX_NAME, + described_class::LEGACY_INDEX_NAME_RULE_TYPE, + described_class::LEGACY_INDEX_NAME_CODE_OWNERS + ) + expect(index_names).to include(described_class::LEGACY_INDEX_NAME_RULE_TYPE) + expect(index_names).to include(described_class::LEGACY_INDEX_NAME_CODE_OWNERS) + end end describe "#down" do @@ -82,6 +108,8 @@ def create_sectional_approval_rules 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 @@ -91,22 +119,26 @@ def create_sectional_approval_rules .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 - # rules created above, and MergeRequests::SyncCodeOwnerApprovalRules + # 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(-2) + .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) - expect(index_names).to include(described_class::LEGACY_INDEX_NAME_CODE_OWNERS) + expect(index_names).to include( + described_class::LEGACY_INDEX_NAME_RULE_TYPE, + described_class::LEGACY_INDEX_NAME_CODE_OWNERS + ) end end end -- GitLab From eb73debd67c9edb7ff1507aaace81f7e779a42c6 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 11 Jun 2020 14:55:37 -0700 Subject: [PATCH 14/15] Drop rule_type from legacy index --- ...date_index_approval_rule_name_for_code_owners_rule_type.rb | 4 ++-- db/structure.sql | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) 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 index 1b469d37676999..7e31c9880cddfa 100644 --- 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 @@ -32,7 +32,7 @@ def up add_concurrent_index( :approval_merge_request_rules, - [:merge_request_id, :rule_type, :name], + [:merge_request_id, :name], unique: true, where: "rule_type = #{CODE_OWNER_RULE_TYPE} AND section IS NULL", name: LEGACY_INDEX_NAME_RULE_TYPE @@ -84,7 +84,7 @@ def down # add_concurrent_index( :approval_merge_request_rules, - [:merge_request_id, :rule_type, :name], + [:merge_request_id, :name], unique: true, where: "rule_type = #{CODE_OWNER_RULE_TYPE}", name: LEGACY_INDEX_NAME_RULE_TYPE diff --git a/db/structure.sql b/db/structure.sql index 87cf83e64d8d97..f118a90b3bf84d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -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, rule_type, name) WHERE ((rule_type = 2) AND (section IS NULL)); +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); -- GitLab From b0b04f2d40c408b18740c631b178e1919d8b10af Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 15 Jun 2020 16:36:08 -0700 Subject: [PATCH 15/15] remove extraneous index checks --- ...e_index_approval_rule_name_for_code_owners_rule_type_spec.rb | 2 -- 1 file changed, 2 deletions(-) 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 index 8dd14d2d99255b..4aa912ef873c12 100644 --- 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 @@ -97,8 +97,6 @@ def create_two_matching_nil_section_approval_rules described_class::LEGACY_INDEX_NAME_RULE_TYPE, described_class::LEGACY_INDEX_NAME_CODE_OWNERS ) - expect(index_names).to include(described_class::LEGACY_INDEX_NAME_RULE_TYPE) - expect(index_names).to include(described_class::LEGACY_INDEX_NAME_CODE_OWNERS) end end -- GitLab