diff options
Diffstat (limited to 'spec/lib/gitlab/database')
6 files changed, 552 insertions, 150 deletions
diff --git a/spec/lib/gitlab/database/batch_count_spec.rb b/spec/lib/gitlab/database/batch_count_spec.rb index 7be84b8f980..e7cb53f2dbd 100644 --- a/spec/lib/gitlab/database/batch_count_spec.rb +++ b/spec/lib/gitlab/database/batch_count_spec.rb @@ -35,6 +35,10 @@ describe Gitlab::Database::BatchCount do expect(described_class.batch_count(model, "#{model.table_name}.id")).to eq(5) end + it 'counts with Arel column' do + expect(described_class.batch_count(model, model.arel_table[:id])).to eq(5) + end + it 'counts table with batch_size 50K' do expect(described_class.batch_count(model, batch_size: 50_000)).to eq(5) end @@ -98,6 +102,10 @@ describe Gitlab::Database::BatchCount do expect(described_class.batch_distinct_count(model, "#{model.table_name}.#{column}")).to eq(2) end + it 'counts with Arel column' do + expect(described_class.batch_distinct_count(model, model.arel_table[column])).to eq(2) + end + it 'counts with :column field with batch_size of 50K' do expect(described_class.batch_distinct_count(model, column, batch_size: 50_000)).to eq(2) end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 3a0148615b9..203d39be22b 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -217,9 +217,10 @@ describe Gitlab::Database::MigrationHelpers do it 'appends ON DELETE SET NULL statement' do expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) - expect(model).to receive(:execute).with(/RESET ALL/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) expect(model).to receive(:execute).with(/ON DELETE SET NULL/) @@ -233,9 +234,10 @@ describe Gitlab::Database::MigrationHelpers do it 'appends ON DELETE CASCADE statement' do expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) - expect(model).to receive(:execute).with(/RESET ALL/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) expect(model).to receive(:execute).with(/ON DELETE CASCADE/) @@ -249,9 +251,10 @@ describe Gitlab::Database::MigrationHelpers do it 'appends no ON DELETE statement' do expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) - expect(model).to receive(:execute).with(/RESET ALL/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) expect(model).not_to receive(:execute).with(/ON DELETE/) @@ -266,10 +269,11 @@ describe Gitlab::Database::MigrationHelpers do it 'creates a concurrent foreign key and validates it' do expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/NOT VALID/) expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) - expect(model).to receive(:execute).with(/RESET ALL/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) model.add_concurrent_foreign_key(:projects, :users, column: :user_id) end @@ -293,10 +297,11 @@ describe Gitlab::Database::MigrationHelpers do it 'creates a new foreign key' do expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/NOT VALID/) expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT.+foo/) - expect(model).to receive(:execute).with(/RESET ALL/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :foo) end @@ -321,10 +326,11 @@ describe Gitlab::Database::MigrationHelpers do it 'creates a new foreign key' do expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/NOT VALID/) expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT.+bar/) - expect(model).to receive(:execute).with(/RESET ALL/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :bar) end @@ -361,6 +367,7 @@ describe Gitlab::Database::MigrationHelpers do aggregate_failures do expect(model).not_to receive(:concurrent_foreign_key_name) expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/ALTER TABLE projects VALIDATE CONSTRAINT/) expect(model).to receive(:execute).ordered.with(/RESET ALL/) @@ -377,6 +384,7 @@ describe Gitlab::Database::MigrationHelpers do aggregate_failures do expect(model).to receive(:concurrent_foreign_key_name) expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(/ALTER TABLE projects VALIDATE CONSTRAINT/) expect(model).to receive(:execute).ordered.with(/RESET ALL/) @@ -527,6 +535,26 @@ describe Gitlab::Database::MigrationHelpers do end end end + + # This spec runs without an enclosing transaction (:delete truncation method for db_cleaner) + context 'when the statement_timeout is already disabled', :delete do + before do + ActiveRecord::Base.connection.execute('SET statement_timeout TO 0') + end + + after do + # Use ActiveRecord::Base.connection instead of model.execute + # so that this call is not counted below + ActiveRecord::Base.connection.execute('RESET ALL') + end + + it 'yields control without disabling the timeout or resetting' do + expect(model).not_to receive(:execute).with('SET statement_timeout TO 0') + expect(model).not_to receive(:execute).with('RESET ALL') + + expect { |block| model.disable_statement_timeout(&block) }.to yield_control + end + end end describe '#true_value' do @@ -596,140 +624,12 @@ describe Gitlab::Database::MigrationHelpers do describe '#add_column_with_default' do let(:column) { Project.columns.find { |c| c.name == "id" } } - context 'outside of a transaction' do - context 'when a column limit is not set' do - before do - expect(model).to receive(:transaction_open?) - .and_return(false) - .at_least(:once) - - expect(model).to receive(:transaction).and_yield - - expect(model).to receive(:add_column) - .with(:projects, :foo, :integer, default: nil) - - expect(model).to receive(:change_column_default) - .with(:projects, :foo, 10) - - expect(model).to receive(:column_for) - .with(:projects, :foo).and_return(column) - end - - it 'adds the column while allowing NULL values' do - expect(model).to receive(:update_column_in_batches) - .with(:projects, :foo, 10) - - expect(model).not_to receive(:change_column_null) - - model.add_column_with_default(:projects, :foo, :integer, - default: 10, - allow_null: true) - end - - it 'adds the column while not allowing NULL values' do - expect(model).to receive(:update_column_in_batches) - .with(:projects, :foo, 10) - - expect(model).to receive(:change_column_null) - .with(:projects, :foo, false) - - model.add_column_with_default(:projects, :foo, :integer, default: 10) - end - - it 'removes the added column whenever updating the rows fails' do - expect(model).to receive(:update_column_in_batches) - .with(:projects, :foo, 10) - .and_raise(RuntimeError) - - expect(model).to receive(:remove_column) - .with(:projects, :foo) - - expect do - model.add_column_with_default(:projects, :foo, :integer, default: 10) - end.to raise_error(RuntimeError) - end - - it 'removes the added column whenever changing a column NULL constraint fails' do - expect(model).to receive(:change_column_null) - .with(:projects, :foo, false) - .and_raise(RuntimeError) - - expect(model).to receive(:remove_column) - .with(:projects, :foo) - - expect do - model.add_column_with_default(:projects, :foo, :integer, default: 10) - end.to raise_error(RuntimeError) - end - end - - context 'when `update_column_in_batches_args` is given' do - let(:column) { UserDetail.columns.find { |c| c.name == "user_id" } } - - it 'uses `user_id` for `update_column_in_batches`' do - allow(model).to receive(:transaction_open?).and_return(false) - allow(model).to receive(:transaction).and_yield - allow(model).to receive(:column_for).with(:user_details, :foo).and_return(column) - allow(model).to receive(:update_column_in_batches).with(:user_details, :foo, 10, batch_column_name: :user_id) - allow(model).to receive(:change_column_null).with(:user_details, :foo, false) - allow(model).to receive(:change_column_default).with(:user_details, :foo, 10) + it 'delegates to #add_column' do + expect(model).to receive(:add_column).with(:projects, :foo, :integer, default: 10, limit: nil, null: true) - expect(model).to receive(:add_column) - .with(:user_details, :foo, :integer, default: nil) - - model.add_column_with_default( - :user_details, - :foo, - :integer, - default: 10, - update_column_in_batches_args: { batch_column_name: :user_id } - ) - end - end - - context 'when a column limit is set' do - it 'adds the column with a limit' do - allow(model).to receive(:transaction_open?).and_return(false) - allow(model).to receive(:transaction).and_yield - allow(model).to receive(:column_for).with(:projects, :foo).and_return(column) - allow(model).to receive(:update_column_in_batches).with(:projects, :foo, 10) - allow(model).to receive(:change_column_null).with(:projects, :foo, false) - allow(model).to receive(:change_column_default).with(:projects, :foo, 10) - - expect(model).to receive(:add_column) - .with(:projects, :foo, :integer, default: nil, limit: 8) - - model.add_column_with_default(:projects, :foo, :integer, default: 10, limit: 8) - end - end - - it 'adds a column with an array default value for a jsonb type' do - create(:project) - allow(model).to receive(:transaction_open?).and_return(false) - allow(model).to receive(:transaction).and_yield - expect(model).to receive(:update_column_in_batches).with(:projects, :foo, '[{"foo":"json"}]').and_call_original - - model.add_column_with_default(:projects, :foo, :jsonb, default: [{ foo: "json" }]) - end - - it 'adds a column with an object default value for a jsonb type' do - create(:project) - allow(model).to receive(:transaction_open?).and_return(false) - allow(model).to receive(:transaction).and_yield - expect(model).to receive(:update_column_in_batches).with(:projects, :foo, '{"foo":"json"}').and_call_original - - model.add_column_with_default(:projects, :foo, :jsonb, default: { foo: "json" }) - end - end - - context 'inside a transaction' do - it 'raises RuntimeError' do - expect(model).to receive(:transaction_open?).and_return(true) - - expect do - model.add_column_with_default(:projects, :foo, :integer, default: 10) - end.to raise_error(RuntimeError) - end + model.add_column_with_default(:projects, :foo, :integer, + default: 10, + allow_null: true) end end @@ -782,7 +682,7 @@ describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:update_column_in_batches) - expect(model).to receive(:change_column_null).with(:users, :new, false) + expect(model).to receive(:add_not_null_constraint).with(:users, :new) expect(model).to receive(:copy_indexes).with(:users, :old, :new) expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new) @@ -790,6 +690,25 @@ describe Gitlab::Database::MigrationHelpers do model.rename_column_concurrently(:users, :old, :new) end + it 'passes the batch_column_name' do + expect(model).to receive(:column_exists?).with(:users, :other_batch_column).and_return(true) + expect(model).to receive(:check_trigger_permissions!).and_return(true) + + expect(model).to receive(:create_column_from).with( + :users, :old, :new, type: nil, batch_column_name: :other_batch_column + ).and_return(true) + + expect(model).to receive(:install_rename_triggers).and_return(true) + + model.rename_column_concurrently(:users, :old, :new, batch_column_name: :other_batch_column) + end + + it 'raises an error with invalid batch_column_name' do + expect do + model.rename_column_concurrently(:users, :old, :new, batch_column_name: :invalid) + end.to raise_error(RuntimeError, /Column invalid does not exist on users/) + end + context 'when default is false' do let(:old_column) do double(:column, @@ -896,7 +815,7 @@ describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:update_column_in_batches) - expect(model).to receive(:change_column_null).with(:users, :old, false) + expect(model).to receive(:add_not_null_constraint).with(:users, :old) expect(model).to receive(:copy_indexes).with(:users, :new, :old) expect(model).to receive(:copy_foreign_keys).with(:users, :new, :old) @@ -904,6 +823,25 @@ describe Gitlab::Database::MigrationHelpers do model.undo_cleanup_concurrent_column_rename(:users, :old, :new) end + it 'passes the batch_column_name' do + expect(model).to receive(:column_exists?).with(:users, :other_batch_column).and_return(true) + expect(model).to receive(:check_trigger_permissions!).and_return(true) + + expect(model).to receive(:create_column_from).with( + :users, :new, :old, type: nil, batch_column_name: :other_batch_column + ).and_return(true) + + expect(model).to receive(:install_rename_triggers).and_return(true) + + model.undo_cleanup_concurrent_column_rename(:users, :old, :new, batch_column_name: :other_batch_column) + end + + it 'raises an error with invalid batch_column_name' do + expect do + model.undo_cleanup_concurrent_column_rename(:users, :old, :new, batch_column_name: :invalid) + end.to raise_error(RuntimeError, /Column invalid does not exist on users/) + end + context 'when default is false' do let(:new_column) do double(:column, @@ -1365,6 +1303,22 @@ describe Gitlab::Database::MigrationHelpers do end end + it 'returns the final expected delay' do + Sidekiq::Testing.fake! do + final_delay = model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, batch_size: 2) + + expect(final_delay.to_f).to eq(20.minutes.to_f) + end + end + + it 'returns zero when nothing gets queued' do + Sidekiq::Testing.fake! do + final_delay = model.queue_background_migration_jobs_by_range_at_intervals(User.none, 'FooJob', 10.minutes) + + expect(final_delay).to eq(0) + end + end + context 'with batch_size option' do it 'queues jobs correctly' do Sidekiq::Testing.fake! do @@ -1389,12 +1343,25 @@ describe Gitlab::Database::MigrationHelpers do end end - context 'with other_arguments option' do + context 'with other_job_arguments option' do + it 'queues jobs correctly' do + Sidekiq::Testing.fake! do + model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, other_job_arguments: [1, 2]) + + expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3, 1, 2]]) + expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.minutes.from_now.to_f) + end + end + end + + context 'with initial_delay option' do it 'queues jobs correctly' do - model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, other_arguments: [1, 2]) + Sidekiq::Testing.fake! do + model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, other_job_arguments: [1, 2], initial_delay: 10.minutes) - expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3, 1, 2]]) - expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.minutes.from_now.to_f) + expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3, 1, 2]]) + expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(20.minutes.from_now.to_f) + end end end end @@ -2158,6 +2125,7 @@ describe Gitlab::Database::MigrationHelpers do .and_return(false).exactly(1) expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) @@ -2201,6 +2169,7 @@ describe Gitlab::Database::MigrationHelpers do .and_return(false).exactly(1) expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:with_lock_retries).and_call_original expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) @@ -2242,6 +2211,7 @@ describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:check_constraint_exists?).and_return(true) expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) expect(model).to receive(:execute).with(/statement_timeout/) expect(model).to receive(:execute).ordered.with(validate_sql) expect(model).to receive(:execute).ordered.with(/RESET ALL/) @@ -2381,4 +2351,135 @@ describe Gitlab::Database::MigrationHelpers do end end end + + describe '#add_not_null_constraint' do + context 'when it is called with the default options' do + it 'calls add_check_constraint with an infered constraint name and validate: true' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + check = "name IS NOT NULL" + + expect(model).to receive(:column_is_nullable?).and_return(true) + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:add_check_constraint) + .with(:test_table, check, constraint_name, validate: true) + + model.add_not_null_constraint(:test_table, :name) + end + end + + context 'when all parameters are provided' do + it 'calls add_check_constraint with the correct parameters' do + constraint_name = 'check_name_not_null' + check = "name IS NOT NULL" + + expect(model).to receive(:column_is_nullable?).and_return(true) + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:add_check_constraint) + .with(:test_table, check, constraint_name, validate: false) + + model.add_not_null_constraint( + :test_table, + :name, + constraint_name: constraint_name, + validate: false + ) + end + end + + context 'when the column is defined as NOT NULL' do + it 'does not add a check constraint' do + expect(model).to receive(:column_is_nullable?).and_return(false) + expect(model).not_to receive(:check_constraint_name) + expect(model).not_to receive(:add_check_constraint) + + model.add_not_null_constraint(:test_table, :name) + end + end + end + + describe '#validate_not_null_constraint' do + context 'when constraint_name is not provided' do + it 'calls validate_check_constraint with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:validate_check_constraint) + .with(:test_table, constraint_name) + + model.validate_not_null_constraint(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls validate_check_constraint with the correct parameters' do + constraint_name = 'check_name_not_null' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:validate_check_constraint) + .with(:test_table, constraint_name) + + model.validate_not_null_constraint(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#remove_not_null_constraint' do + context 'when constraint_name is not provided' do + it 'calls remove_check_constraint with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:remove_check_constraint) + .with(:test_table, constraint_name) + + model.remove_not_null_constraint(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls remove_check_constraint with the correct parameters' do + constraint_name = 'check_name_not_null' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:remove_check_constraint) + .with(:test_table, constraint_name) + + model.remove_not_null_constraint(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#check_not_null_constraint_exists?' do + context 'when constraint_name is not provided' do + it 'calls check_constraint_exists? with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, constraint_name) + + model.check_not_null_constraint_exists?(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls check_constraint_exists? with the correct parameters' do + constraint_name = 'check_name_not_null' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, constraint_name) + + model.check_not_null_constraint_exists?(:test_table, :name, constraint_name: constraint_name) + end + end + end end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/partitioned_foreign_key_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/partitioned_foreign_key_spec.rb new file mode 100644 index 00000000000..77f71676252 --- /dev/null +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/partitioned_foreign_key_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Database::PartitioningMigrationHelpers::PartitionedForeignKey do + let(:foreign_key) do + described_class.new( + to_table: 'issues', + from_table: 'issue_assignees', + from_column: 'issue_id', + to_column: 'id', + cascade_delete: true) + end + + describe 'validations' do + it 'allows keys that reference valid tables and columns' do + expect(foreign_key).to be_valid + end + + it 'does not allow keys without a valid to_table' do + foreign_key.to_table = 'this_is_not_a_real_table' + + expect(foreign_key).not_to be_valid + expect(foreign_key.errors[:to_table].first).to eq('must be a valid table') + end + + it 'does not allow keys without a valid from_table' do + foreign_key.from_table = 'this_is_not_a_real_table' + + expect(foreign_key).not_to be_valid + expect(foreign_key.errors[:from_table].first).to eq('must be a valid table') + end + + it 'does not allow keys without a valid to_column' do + foreign_key.to_column = 'this_is_not_a_real_fk' + + expect(foreign_key).not_to be_valid + expect(foreign_key.errors[:to_column].first).to eq('must be a valid column') + end + + it 'does not allow keys without a valid from_column' do + foreign_key.from_column = 'this_is_not_a_real_pk' + + expect(foreign_key).not_to be_valid + expect(foreign_key.errors[:from_column].first).to eq('must be a valid column') + end + end +end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers_spec.rb new file mode 100644 index 00000000000..0e2fb047469 --- /dev/null +++ b/spec/lib/gitlab/database/partitioning_migration_helpers_spec.rb @@ -0,0 +1,230 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Database::PartitioningMigrationHelpers do + let(:model) do + ActiveRecord::Migration.new.extend(described_class) + end + let_it_be(:connection) { ActiveRecord::Base.connection } + let(:referenced_table) { :issues } + let(:function_name) { model.fk_function_name(referenced_table) } + let(:trigger_name) { model.fk_trigger_name(referenced_table) } + + before do + allow(model).to receive(:puts) + end + + describe 'adding a foreign key' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + + context 'when the table has no foreign keys' do + it 'creates a trigger function to handle the single cascade' do + model.add_partitioned_foreign_key :issue_assignees, referenced_table + + expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when the table already has foreign keys' do + context 'when the foreign key is from a different table' do + before do + model.add_partitioned_foreign_key :issue_assignees, referenced_table + end + + it 'creates a trigger function to handle the multiple cascades' do + model.add_partitioned_foreign_key :epic_issues, referenced_table + + expect_function_to_contain(function_name, + 'delete from issue_assignees where issue_id = old.id', + 'delete from epic_issues where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when the foreign key is from the same table' do + before do + model.add_partitioned_foreign_key :issues, referenced_table, column: :moved_to_id + end + + context 'when the foreign key is from a different column' do + it 'creates a trigger function to handle the multiple cascades' do + model.add_partitioned_foreign_key :issues, referenced_table, column: :duplicated_to_id + + expect_function_to_contain(function_name, + 'delete from issues where moved_to_id = old.id', + 'delete from issues where duplicated_to_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when the foreign key is from the same column' do + it 'ignores the duplicate and properly recreates the trigger function' do + model.add_partitioned_foreign_key :issues, referenced_table, column: :moved_to_id + + expect_function_to_contain(function_name, 'delete from issues where moved_to_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + end + end + + context 'when the foreign key is set to nullify' do + it 'creates a trigger function that nullifies the foreign key' do + model.add_partitioned_foreign_key :issue_assignees, referenced_table, on_delete: :nullify + + expect_function_to_contain(function_name, 'update issue_assignees set issue_id = null where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when the referencing column is a custom value' do + it 'creates a trigger function with the correct column name' do + model.add_partitioned_foreign_key :issues, referenced_table, column: :duplicated_to_id + + expect_function_to_contain(function_name, 'delete from issues where duplicated_to_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when the referenced column is a custom value' do + let(:referenced_table) { :user_details } + + it 'creates a trigger function with the correct column name' do + model.add_partitioned_foreign_key :user_preferences, referenced_table, column: :user_id, primary_key: :user_id + + expect_function_to_contain(function_name, 'delete from user_preferences where user_id = old.user_id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when the given key definition is invalid' do + it 'raises an error with the appropriate message' do + expect do + model.add_partitioned_foreign_key :issue_assignees, referenced_table, column: :not_a_real_issue_id + end.to raise_error(/From column must be a valid column/) + end + end + + context 'when run inside a transaction' do + it 'raises an error' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect do + model.add_partitioned_foreign_key :issue_assignees, referenced_table + end.to raise_error(/can not be run inside a transaction/) + end + end + end + + context 'removing a foreign key' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + + context 'when the table has multiple foreign keys' do + before do + model.add_partitioned_foreign_key :issue_assignees, referenced_table + model.add_partitioned_foreign_key :epic_issues, referenced_table + end + + it 'creates a trigger function without the removed cascade' do + expect_function_to_contain(function_name, + 'delete from issue_assignees where issue_id = old.id', + 'delete from epic_issues where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + + model.remove_partitioned_foreign_key :issue_assignees, referenced_table + + expect_function_to_contain(function_name, 'delete from epic_issues where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when the table has only one remaining foreign key' do + before do + model.add_partitioned_foreign_key :issue_assignees, referenced_table + end + + it 'removes the trigger function altogether' do + expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + + model.remove_partitioned_foreign_key :issue_assignees, referenced_table + + expect(find_function_def(function_name)).to be_nil + expect(find_trigger_def(trigger_name)).to be_nil + end + end + + context 'when the foreign key does not exist' do + before do + model.add_partitioned_foreign_key :issue_assignees, referenced_table + end + + it 'ignores the invalid key and properly recreates the trigger function' do + expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + + model.remove_partitioned_foreign_key :issues, referenced_table, column: :moved_to_id + + expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') + expect_valid_function_trigger(trigger_name, function_name) + end + end + + context 'when run outside a transaction' do + it 'raises an error' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect do + model.remove_partitioned_foreign_key :issue_assignees, referenced_table + end.to raise_error(/can not be run inside a transaction/) + end + end + end + + def expect_function_to_contain(name, *statements) + return_stmt, *body_stmts = parsed_function_statements(name).reverse + + expect(return_stmt).to eq('return old') + expect(body_stmts).to contain_exactly(*statements) + end + + def expect_valid_function_trigger(name, fn_name) + event, activation, definition = cleaned_trigger_def(name) + + expect(event).to eq('delete') + expect(activation).to eq('after') + expect(definition).to eq("execute procedure #{fn_name}()") + end + + def parsed_function_statements(name) + cleaned_definition = find_function_def(name)['fn_body'].downcase.gsub(/\s+/, ' ') + statements = cleaned_definition.sub(/\A\s*begin\s*(.*)\s*end\s*\Z/, "\\1") + statements.split(';').map! { |stmt| stmt.strip.presence }.compact! + end + + def find_function_def(name) + connection.execute("select prosrc as fn_body from pg_proc where proname = '#{name}';").first + end + + def cleaned_trigger_def(name) + find_trigger_def(name).values_at('event', 'activation', 'definition').map!(&:downcase) + end + + def find_trigger_def(name) + connection.execute(<<~SQL).first + select + string_agg(event_manipulation, ',') as event, + action_timing as activation, + action_statement as definition + from information_schema.triggers + where trigger_name = '#{name}' + group by 2, 3 + SQL + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb index 7b8437e4874..fae57996fb6 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb @@ -242,7 +242,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :delete old_path, new_path = [nil, nil] Gitlab::Redis::SharedState.with do |redis| rename_info = redis.lpop(key) - old_path, new_path = JSON.parse(rename_info) + old_path, new_path = Gitlab::Json.parse(rename_info) end expect(old_path).to eq('path/to/namespace') @@ -278,7 +278,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :delete end expect(rename_count).to eq(1) - expect(JSON.parse(stored_renames.first)).to eq(%w(old_path new_path)) + expect(Gitlab::Json.parse(stored_renames.first)).to eq(%w(old_path new_path)) end end end diff --git a/spec/lib/gitlab/database/with_lock_retries_spec.rb b/spec/lib/gitlab/database/with_lock_retries_spec.rb index b6321f2eab1..9c8c9749125 100644 --- a/spec/lib/gitlab/database/with_lock_retries_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_spec.rb @@ -84,7 +84,7 @@ describe Gitlab::Database::WithLockRetries do subject.run do lock_attempts += 1 - if lock_attempts == retry_count # we reached the last retry iteration, if we kill the thread, the last try (no lock_timeout) will succeed) + if lock_attempts == retry_count # we reached the last retry iteration, if we kill the thread, the last try (no lock_timeout) will succeed lock_fiber.resume end @@ -106,9 +106,13 @@ describe Gitlab::Database::WithLockRetries do end context 'after the retries, without setting lock_timeout' do - let(:retry_count) { timing_configuration.size } + let(:retry_count) { timing_configuration.size + 1 } - it_behaves_like 'retriable exclusive lock on `projects`' + it_behaves_like 'retriable exclusive lock on `projects`' do + before do + expect(subject).to receive(:run_block_without_lock_timeout).and_call_original + end + end end context 'when statement timeout is reached' do @@ -129,11 +133,22 @@ describe Gitlab::Database::WithLockRetries do end end + context 'restore local database variables' do + it do + expect { subject.run {} }.not_to change { ActiveRecord::Base.connection.execute("SHOW lock_timeout").to_a } + end + + it do + expect { subject.run {} }.not_to change { ActiveRecord::Base.connection.execute("SHOW idle_in_transaction_session_timeout").to_a } + end + end + context 'casting durations correctly' do let(:timing_configuration) { [[0.015.seconds, 0.025.seconds], [0.015.seconds, 0.025.seconds]] } # 15ms, 25ms it 'executes `SET LOCAL lock_timeout` using the configured timeout value in milliseconds' do expect(ActiveRecord::Base.connection).to receive(:execute).with("SAVEPOINT active_record_1").and_call_original + expect(ActiveRecord::Base.connection).to receive(:execute).with('RESET idle_in_transaction_session_timeout; RESET lock_timeout').and_call_original expect(ActiveRecord::Base.connection).to receive(:execute).with("SET LOCAL lock_timeout TO '15ms'").and_call_original expect(ActiveRecord::Base.connection).to receive(:execute).with("RELEASE SAVEPOINT active_record_1").and_call_original |