diff options
Diffstat (limited to 'spec/lib/gitlab/database/migration_helpers_spec.rb')
| -rw-r--r-- | spec/lib/gitlab/database/migration_helpers_spec.rb | 391 |
1 files changed, 246 insertions, 145 deletions
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 |