Add latest changes from gitlab-org/gitlab@master

2 files changed, 257 insertions, 0 deletions

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