From 2d31f347bf835e0951054cf2ee9d0e2cbc5b77fe Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 21 Oct 2021 15:12:54 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- .../batch_cleaner_service_spec.rb | 110 +++++++++++++++ .../loose_foreign_keys/cleaner_service_spec.rb | 147 +++++++++++++++++++++ 2 files changed, 257 insertions(+) create mode 100644 spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb create mode 100644 spec/services/loose_foreign_keys/cleaner_service_spec.rb (limited to 'spec/services/loose_foreign_keys') diff --git a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb new file mode 100644 index 00000000000..dcef419cc9a --- /dev/null +++ b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::BatchCleanerService do + include MigrationsHelpers + + def create_table_structure + migration = ActiveRecord::Migration.new.extend(Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers) + + migration.create_table :loose_fk_parent_table + + migration.create_table :loose_fk_child_table_1 do |t| + t.bigint :parent_id + end + + migration.create_table :loose_fk_child_table_2 do |t| + t.bigint :parent_id_with_different_column + end + + migration.track_record_deletions(:loose_fk_parent_table) + end + + let(:parent_model) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_parent_table' + + include LooseForeignKey + + loose_foreign_key :loose_fk_child_table_1, :parent_id, on_delete: :async_delete + loose_foreign_key :loose_fk_child_table_2, :parent_id_with_different_column, on_delete: :async_nullify + end + end + + let(:child_model_1) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_child_table_1' + end + end + + let(:child_model_2) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_child_table_2' + end + end + + let(:loose_fk_child_table_1) { table(:loose_fk_child_table_1) } + let(:loose_fk_child_table_2) { table(:loose_fk_child_table_2) } + let(:parent_record_1) { parent_model.create! } + let(:other_parent_record) { parent_model.create! } + + before(:all) do + create_table_structure + end + + before do + parent_record_1 + + loose_fk_child_table_1.create!(parent_id: parent_record_1.id) + loose_fk_child_table_1.create!(parent_id: parent_record_1.id) + + # these will not be deleted + loose_fk_child_table_1.create!(parent_id: other_parent_record.id) + loose_fk_child_table_1.create!(parent_id: other_parent_record.id) + + loose_fk_child_table_2.create!(parent_id_with_different_column: parent_record_1.id) + loose_fk_child_table_2.create!(parent_id_with_different_column: parent_record_1.id) + + # these will not be deleted + loose_fk_child_table_2.create!(parent_id_with_different_column: other_parent_record.id) + loose_fk_child_table_2.create!(parent_id_with_different_column: other_parent_record.id) + end + + after(:all) do + migration = ActiveRecord::Migration.new + migration.drop_table :loose_fk_parent_table + migration.drop_table :loose_fk_child_table_1 + migration.drop_table :loose_fk_child_table_2 + end + + context 'when parent records are deleted' do + before do + parent_record_1.delete + + expect(loose_fk_child_table_1.count).to eq(4) + expect(loose_fk_child_table_2.count).to eq(4) + + described_class.new(parent_klass: parent_model, + deleted_parent_records: LooseForeignKeys::DeletedRecord.status_pending.all, + models_by_table_name: { + 'loose_fk_child_table_1' => child_model_1, + 'loose_fk_child_table_2' => child_model_2 + }).execute + end + + it 'cleans up the child records' do + expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty + expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) + end + + it 'cleans up the parent DeletedRecord' do + expect(LooseForeignKeys::DeletedRecord.status_pending.count).to eq(0) + end + + it 'does not delete unrelated records' do + expect(loose_fk_child_table_1.where(parent_id: other_parent_record.id).count).to eq(2) + expect(loose_fk_child_table_2.where(parent_id_with_different_column: other_parent_record.id).count).to eq(2) + end + end +end diff --git a/spec/services/loose_foreign_keys/cleaner_service_spec.rb b/spec/services/loose_foreign_keys/cleaner_service_spec.rb new file mode 100644 index 00000000000..6f37ac49435 --- /dev/null +++ b/spec/services/loose_foreign_keys/cleaner_service_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::CleanerService do + let(:schema) { ApplicationRecord.connection.current_schema } + let(:deleted_records) do + [ + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.projects", primary_key_value: non_existing_record_id), + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.projects", primary_key_value: non_existing_record_id) + ] + end + + let(:loose_fk_definition) do + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + 'projects', + 'issues', + { + column: 'project_id', + on_delete: :async_nullify + } + ) + end + + subject(:cleaner_service) do + described_class.new( + model: Issue, + foreign_key_definition: loose_fk_definition, + deleted_parent_records: deleted_records + ) + end + + context 'when invalid foreign key definition is passed' do + context 'when invalid on_delete argument was given' do + before do + loose_fk_definition.options[:on_delete] = :invalid + end + + it 'raises KeyError' do + expect { cleaner_service.execute }.to raise_error(StandardError, /Invalid on_delete argument/) + end + end + end + + describe 'query generation' do + context 'when single primary key is used' do + let(:issue) { create(:issue) } + + let(:deleted_records) do + [ + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.projects", primary_key_value: issue.project_id) + ] + end + + it 'generates an IN query for nullifying the rows' do + expected_query = %{UPDATE "issues" SET "project_id" = NULL WHERE ("issues"."id") IN (SELECT "issues"."id" FROM "issues" WHERE "issues"."project_id" IN (#{issue.project_id}) LIMIT 500)} + expect(ApplicationRecord.connection).to receive(:execute).with(expected_query).and_call_original + + cleaner_service.execute + + issue.reload + expect(issue.project_id).to be_nil + end + + it 'generates an IN query for deleting the rows' do + loose_fk_definition.options[:on_delete] = :async_delete + + expected_query = %{DELETE FROM "issues" WHERE ("issues"."id") IN (SELECT "issues"."id" FROM "issues" WHERE "issues"."project_id" IN (#{issue.project_id}) LIMIT 1000)} + expect(ApplicationRecord.connection).to receive(:execute).with(expected_query).and_call_original + + cleaner_service.execute + + expect(Issue.exists?(id: issue.id)).to eq(false) + end + end + + context 'when composite primary key is used' do + let!(:user) { create(:user) } + let!(:project) { create(:project) } + + let(:loose_fk_definition) do + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + 'users', + 'project_authorizations', + { + column: 'user_id', + on_delete: :async_delete + } + ) + end + + let(:deleted_records) do + [ + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.users", primary_key_value: user.id) + ] + end + + subject(:cleaner_service) do + described_class.new( + model: ProjectAuthorization, + foreign_key_definition: loose_fk_definition, + deleted_parent_records: deleted_records + ) + end + + before do + project.add_developer(user) + end + + it 'generates an IN query for deleting the rows' do + expected_query = %{DELETE FROM "project_authorizations" WHERE ("project_authorizations"."user_id", "project_authorizations"."project_id", "project_authorizations"."access_level") IN (SELECT "project_authorizations"."user_id", "project_authorizations"."project_id", "project_authorizations"."access_level" FROM "project_authorizations" WHERE "project_authorizations"."user_id" IN (#{user.id}) LIMIT 1000)} + expect(ApplicationRecord.connection).to receive(:execute).with(expected_query).and_call_original + + cleaner_service.execute + + expect(ProjectAuthorization.exists?(user_id: user.id)).to eq(false) + end + + context 'when the query generation is incorrect (paranoid check)' do + it 'raises error if the foreign key condition is missing' do + expect_next_instance_of(LooseForeignKeys::CleanerService) do |instance| + expect(instance).to receive(:delete_query).and_return('wrong query') + end + + expect { cleaner_service.execute }.to raise_error /FATAL: foreign key condition is missing from the generated query/ + end + end + end + + context 'when with_skip_locked parameter is true' do + subject(:cleaner_service) do + described_class.new( + model: Issue, + foreign_key_definition: loose_fk_definition, + deleted_parent_records: deleted_records, + with_skip_locked: true + ) + end + + it 'generates a query with the SKIP LOCKED clause' do + expect(ApplicationRecord.connection).to receive(:execute).with(/FOR UPDATE SKIP LOCKED/).and_call_original + + cleaner_service.execute + end + end + end +end -- cgit v1.2.3