diff options
Diffstat (limited to 'spec/lib/gitlab/database')
18 files changed, 667 insertions, 158 deletions
diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb index 7a433be0e2f..a1c979bba50 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -99,6 +99,15 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end end + describe '.created_after' do + let!(:migration_old) { create :batched_background_migration, created_at: 2.days.ago } + let!(:migration_new) { create :batched_background_migration, created_at: 0.days.ago } + + it 'only returns migrations created after the specified time' do + expect(described_class.created_after(1.day.ago)).to contain_exactly(migration_new) + end + end + describe '.queued' do let!(:migration1) { create(:batched_background_migration, :finished) } let!(:migration2) { create(:batched_background_migration, :paused) } diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb index 6a4ac317cad..83c0275a870 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb @@ -3,23 +3,20 @@ require 'spec_helper' RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, '#perform' do - subject { described_class.new(connection: connection, metrics: metrics_tracker).perform(job_record) } + subject(:perform) { described_class.new(connection: connection, metrics: metrics_tracker).perform(job_record) } let(:connection) { Gitlab::Database.database_base_models[:main].connection } let(:metrics_tracker) { instance_double('::Gitlab::Database::BackgroundMigration::PrometheusMetrics', track: nil) } - let(:job_class) { Gitlab::BackgroundMigration::CopyColumnUsingBackgroundMigrationJob } + let(:job_class) { Class.new(Gitlab::BackgroundMigration::BatchedMigrationJob) } let_it_be(:pause_ms) { 250 } let_it_be(:active_migration) { create(:batched_background_migration, :active, job_arguments: [:id, :other_id]) } let!(:job_record) do - create(:batched_background_migration_job, - batched_migration: active_migration, - pause_ms: pause_ms - ) + create(:batched_background_migration_job, batched_migration: active_migration, pause_ms: pause_ms) end - let(:job_instance) { double('job instance', batch_metrics: {}) } + let(:job_instance) { instance_double('Gitlab::BackgroundMigration::BatchedMigrationJob') } around do |example| Gitlab::Database::SharedModel.using_connection(connection) do @@ -28,23 +25,35 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end before do + allow(active_migration).to receive(:job_class).and_return(job_class) + allow(job_class).to receive(:new).and_return(job_instance) end it 'runs the migration job' do - expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') - - subject + expect(job_class).to receive(:new) + .with(start_id: 1, + end_id: 10, + batch_table: 'events', + batch_column: 'id', + sub_batch_size: 1, + pause_ms: pause_ms, + connection: connection) + .and_return(job_instance) + + expect(job_instance).to receive(:perform).with('id', 'other_id') + + perform end it 'updates the tracking record in the database' do - test_metrics = { 'my_metris' => 'some value' } + test_metrics = { 'my_metrics' => 'some value' } - expect(job_instance).to receive(:perform) + expect(job_instance).to receive(:perform).with('id', 'other_id') expect(job_instance).to receive(:batch_metrics).and_return(test_metrics) freeze_time do - subject + perform reloaded_job_record = job_record.reload @@ -69,11 +78,11 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' it 'increments attempts and updates other fields' do updated_metrics = { 'updated_metrics' => 'some_value' } - expect(job_instance).to receive(:perform) + expect(job_instance).to receive(:perform).with('id', 'other_id') expect(job_instance).to receive(:batch_metrics).and_return(updated_metrics) freeze_time do - subject + perform job_record.reload @@ -88,10 +97,10 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' context 'when the migration job does not raise an error' do it 'marks the tracking record as succeeded' do - expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') + expect(job_instance).to receive(:perform).with('id', 'other_id') freeze_time do - subject + perform reloaded_job_record = job_record.reload @@ -101,22 +110,20 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end it 'tracks metrics of the execution' do - expect(job_instance).to receive(:perform) + expect(job_instance).to receive(:perform).with('id', 'other_id') expect(metrics_tracker).to receive(:track).with(job_record) - subject + perform end end context 'when the migration job raises an error' do shared_examples 'an error is raised' do |error_class| it 'marks the tracking record as failed' do - expect(job_instance).to receive(:perform) - .with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') - .and_raise(error_class) + expect(job_instance).to receive(:perform).with('id', 'other_id').and_raise(error_class) freeze_time do - expect { subject }.to raise_error(error_class) + expect { perform }.to raise_error(error_class) reloaded_job_record = job_record.reload @@ -126,10 +133,10 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end it 'tracks metrics of the execution' do - expect(job_instance).to receive(:perform).and_raise(error_class) + expect(job_instance).to receive(:perform).with('id', 'other_id').and_raise(error_class) expect(metrics_tracker).to receive(:track).with(job_record) - expect { subject }.to raise_error(error_class) + expect { perform }.to raise_error(error_class) end end @@ -138,41 +145,14 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' it_behaves_like 'an error is raised', ActiveRecord::StatementTimeout.new('Timeout!') end - context 'when the batched background migration does not inherit from BaseJob' do - let(:migration_class) { Class.new } - - before do - stub_const('Gitlab::BackgroundMigration::Foo', migration_class) - end + context 'when the batched background migration does not inherit from BatchedMigrationJob' do + let(:job_class) { Class.new } - let(:active_migration) { create(:batched_background_migration, :active, job_class_name: 'Foo') } - let!(:job_record) { create(:batched_background_migration_job, batched_migration: active_migration) } - - it 'does not pass any argument' do - expect(Gitlab::BackgroundMigration::Foo).to receive(:new).with(no_args).and_return(job_instance) - - expect(job_instance).to receive(:perform) - - subject - end - end - - context 'when the batched background migration inherits from BaseJob' do - let(:active_migration) { create(:batched_background_migration, :active, job_class_name: 'Foo') } - let!(:job_record) { create(:batched_background_migration_job, batched_migration: active_migration) } - - let(:migration_class) { Class.new(::Gitlab::BackgroundMigration::BaseJob) } - - before do - stub_const('Gitlab::BackgroundMigration::Foo', migration_class) - end - - it 'passes the correct connection' do - expect(Gitlab::BackgroundMigration::Foo).to receive(:new).with(connection: connection).and_return(job_instance) - - expect(job_instance).to receive(:perform) + it 'runs the job with the correct arguments' do + expect(job_class).to receive(:new).with(no_args).and_return(job_instance) + expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') - subject + perform end end end diff --git a/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb b/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb index e7b5bad8626..1009ec354c3 100644 --- a/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb @@ -401,8 +401,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a ci: :dml_not_allowed }, gitlab_schema_gitlab_shared: { - main: :dml_access_denied, - ci: :dml_access_denied + main: :runtime_error, + ci: :runtime_error }, gitlab_schema_gitlab_main: { main: :success, @@ -465,7 +465,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a "does raise exception when accessing feature flags" => { migration: ->(klass) do def up - Feature.enabled?(:redis_hll_tracking, type: :ops, default_enabled: :yaml) + Feature.enabled?(:redis_hll_tracking, type: :ops) end def down @@ -486,6 +486,37 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a ci: :skipped } } + }, + "does raise exception about cross schema access when suppressing restriction to ensure" => { + migration: ->(klass) do + # The purpose of this test is to ensure that we use ApplicationRecord + # a correct connection will be used: + # - this is a case for finalizing background migrations + def up + Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas.with_suppressed do + ::ApplicationRecord.connection.execute("SELECT 1 FROM ci_builds") + end + end + + def down + end + end, + query_matcher: /FROM ci_builds/, + setup: -> (_) { skip_if_multiple_databases_not_setup }, + expected: { + no_gitlab_schema: { + main: :cross_schema_error, + ci: :success + }, + gitlab_schema_gitlab_shared: { + main: :cross_schema_error, + ci: :success + }, + gitlab_schema_gitlab_main: { + main: :cross_schema_error, + ci: :skipped + } + } } } end @@ -517,6 +548,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a %i[no_gitlab_schema gitlab_schema_gitlab_main gitlab_schema_gitlab_shared].each do |restrict_gitlab_migration| context "while restrict_gitlab_migration=#{restrict_gitlab_migration}" do it "does run migrate :up and :down" do + instance_eval(&setup) if setup + expected_result = expected.fetch(restrict_gitlab_migration)[db_config_name.to_sym] skip "not configured" unless expected_result @@ -543,10 +576,18 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a expect { migration_class.migrate(:up) }.to raise_error(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas::DMLAccessDeniedError) expect { ignore_error(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas::DMLAccessDeniedError) { migration_class.migrate(:down) } }.not_to raise_error + when :runtime_error + expect { migration_class.migrate(:up) }.to raise_error(RuntimeError) + expect { ignore_error(RuntimeError) { migration_class.migrate(:down) } }.not_to raise_error + when :ddl_not_allowed expect { migration_class.migrate(:up) }.to raise_error(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas::DDLNotAllowedError) expect { ignore_error(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas::DDLNotAllowedError) { migration_class.migrate(:down) } }.not_to raise_error + when :cross_schema_error + expect { migration_class.migrate(:up) }.to raise_error(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection::CrossSchemaAccessError) + expect { ignore_error(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection::CrossSchemaAccessError) { migration_class.migrate(:down) } }.not_to raise_error + when :skipped expect_next_instance_of(migration_class) do |migration_object| expect(migration_object).to receive(:migration_skipped).and_call_original diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 798eee0de3e..04fe1fad10e 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -1537,10 +1537,12 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:add_concurrent_index) .with(:issues, %w(gl_project_id), + { unique: false, name: 'index_on_issues_gl_project_id', length: [], - order: []) + order: [] + }) model.copy_indexes(:issues, :project_id, :gl_project_id) end @@ -1564,10 +1566,12 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:add_concurrent_index) .with(:issues, %w(gl_project_id foobar), + { unique: false, name: 'index_on_issues_gl_project_id_foobar', length: [], - order: []) + order: [] + }) model.copy_indexes(:issues, :project_id, :gl_project_id) end @@ -1591,11 +1595,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:add_concurrent_index) .with(:issues, %w(gl_project_id), + { unique: false, name: 'index_on_issues_gl_project_id', length: [], order: [], - where: 'foo') + where: 'foo' + }) model.copy_indexes(:issues, :project_id, :gl_project_id) end @@ -1619,11 +1625,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:add_concurrent_index) .with(:issues, %w(gl_project_id), + { unique: false, name: 'index_on_issues_gl_project_id', length: [], order: [], - using: 'foo') + using: 'foo' + }) model.copy_indexes(:issues, :project_id, :gl_project_id) end @@ -1647,11 +1655,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:add_concurrent_index) .with(:issues, %w(gl_project_id), + { unique: false, name: 'index_on_issues_gl_project_id', length: [], order: [], - opclass: { 'gl_project_id' => 'bar' }) + opclass: { 'gl_project_id' => 'bar' } + }) model.copy_indexes(:issues, :project_id, :gl_project_id) end @@ -1660,14 +1670,16 @@ RSpec.describe Gitlab::Database::MigrationHelpers do context 'using an index with multiple columns and custom operator classes' do it 'copies the index' do index = double(:index, - columns: %w(project_id foobar), - name: 'index_on_issues_project_id_foobar', - using: :gin, - where: nil, - opclasses: { 'project_id' => 'bar', 'foobar' => :gin_trgm_ops }, - unique: false, - lengths: [], - orders: []) + { + columns: %w(project_id foobar), + name: 'index_on_issues_project_id_foobar', + using: :gin, + where: nil, + opclasses: { 'project_id' => 'bar', 'foobar' => :gin_trgm_ops }, + unique: false, + lengths: [], + orders: [] + }) allow(model).to receive(:indexes_for).with(:issues, 'project_id') .and_return([index]) @@ -1675,12 +1687,14 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:add_concurrent_index) .with(:issues, %w(gl_project_id foobar), + { unique: false, name: 'index_on_issues_gl_project_id_foobar', length: [], order: [], opclass: { 'gl_project_id' => 'bar', 'foobar' => :gin_trgm_ops }, - using: :gin) + using: :gin + }) model.copy_indexes(:issues, :project_id, :gl_project_id) end @@ -1689,14 +1703,16 @@ RSpec.describe Gitlab::Database::MigrationHelpers do context 'using an index with multiple columns and a custom operator class on the non affected column' do it 'copies the index' do index = double(:index, - columns: %w(project_id foobar), - name: 'index_on_issues_project_id_foobar', - using: :gin, - where: nil, - opclasses: { 'foobar' => :gin_trgm_ops }, - unique: false, - lengths: [], - orders: []) + { + columns: %w(project_id foobar), + name: 'index_on_issues_project_id_foobar', + using: :gin, + where: nil, + opclasses: { 'foobar' => :gin_trgm_ops }, + unique: false, + lengths: [], + orders: [] + }) allow(model).to receive(:indexes_for).with(:issues, 'project_id') .and_return([index]) @@ -1704,12 +1720,14 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:add_concurrent_index) .with(:issues, %w(gl_project_id foobar), + { unique: false, name: 'index_on_issues_gl_project_id_foobar', length: [], order: [], opclass: { 'foobar' => :gin_trgm_ops }, - using: :gin) + using: :gin + }) model.copy_indexes(:issues, :project_id, :gl_project_id) end @@ -2210,12 +2228,17 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end describe '#ensure_batched_background_migration_is_finished' do + let(:job_class_name) { 'CopyColumnUsingBackgroundMigrationJob' } + let(:table) { :events } + let(:column_name) { :id } + let(:job_arguments) { [["id"], ["id_convert_to_bigint"], nil] } + let(:configuration) do { - job_class_name: 'CopyColumnUsingBackgroundMigrationJob', - table_name: :events, - column_name: :id, - job_arguments: [["id"], ["id_convert_to_bigint"], nil] + job_class_name: job_class_name, + table_name: table, + column_name: column_name, + job_arguments: job_arguments } end @@ -2224,11 +2247,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers do it 'raises an error when migration exists and is not marked as finished' do create(:batched_background_migration, :active, configuration) + allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| + allow(runner).to receive(:finalize).with(job_class_name, table, column_name, job_arguments).and_return(false) + end + expect { ensure_batched_background_migration_is_finished } .to raise_error "Expected batched background migration for the given configuration to be marked as 'finished', but it is 'active':" \ "\t#{configuration}" \ "\n\n" \ - "Finalize it manualy by running" \ + "Finalize it manually by running" \ "\n\n" \ "\tsudo gitlab-rake gitlab:background_migrations:finalize[CopyColumnUsingBackgroundMigrationJob,events,id,'[[\"id\"]\\,[\"id_convert_to_bigint\"]\\,null]']" \ "\n\n" \ @@ -2251,6 +2278,28 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect { ensure_batched_background_migration_is_finished } .not_to raise_error end + + it 'finalizes the migration' do + migration = create(:batched_background_migration, :active, configuration) + + allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| + expect(runner).to receive(:finalize).with(job_class_name, table, column_name, job_arguments).and_return(migration.finish!) + end + + ensure_batched_background_migration_is_finished + end + + context 'when the flag finalize is false' do + it 'does not finalize the migration' do + create(:batched_background_migration, :active, configuration) + + allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| + expect(runner).not_to receive(:finalize).with(job_class_name, table, column_name, job_arguments) + end + + expect { model.ensure_batched_background_migration_is_finished(**configuration.merge(finalize: false)) }.to raise_error(RuntimeError) + end + end end describe '#index_exists_by_name?' do @@ -3162,15 +3211,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers do context 'without proper permissions' do before do - allow(model).to receive(:execute).with(/CREATE EXTENSION IF NOT EXISTS #{extension}/).and_raise(ActiveRecord::StatementInvalid, 'InsufficientPrivilege: permission denied') + allow(model).to receive(:execute) + .with(/CREATE EXTENSION IF NOT EXISTS #{extension}/) + .and_raise(ActiveRecord::StatementInvalid, 'InsufficientPrivilege: permission denied') end - it 'raises the exception' do - expect { subject }.to raise_error(ActiveRecord::StatementInvalid, /InsufficientPrivilege/) - end - - it 'prints an error message' do - expect { subject }.to output(/user is not allowed/).to_stderr.and raise_error + it 'raises an exception and prints an error message' do + expect { subject } + .to output(/user is not allowed/).to_stderr + .and raise_error(ActiveRecord::StatementInvalid, /InsufficientPrivilege/) end end end @@ -3188,15 +3237,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers do context 'without proper permissions' do before do - allow(model).to receive(:execute).with(/DROP EXTENSION IF EXISTS #{extension}/).and_raise(ActiveRecord::StatementInvalid, 'InsufficientPrivilege: permission denied') - end - - it 'raises the exception' do - expect { subject }.to raise_error(ActiveRecord::StatementInvalid, /InsufficientPrivilege/) + allow(model).to receive(:execute) + .with(/DROP EXTENSION IF EXISTS #{extension}/) + .and_raise(ActiveRecord::StatementInvalid, 'InsufficientPrivilege: permission denied') end - it 'prints an error message' do - expect { subject }.to output(/user is not allowed/).to_stderr.and raise_error + it 'raises an exception and prints an error message' do + expect { subject } + .to output(/user is not allowed/).to_stderr + .and raise_error(ActiveRecord::StatementInvalid, /InsufficientPrivilege/) end end end diff --git a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb index e64f5807385..b0caa21e01a 100644 --- a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb @@ -3,16 +3,31 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do + let(:base_class) { ActiveRecord::Migration } + let(:model) do - ActiveRecord::Migration.new.extend(described_class) + base_class.new + .extend(described_class) + .extend(Gitlab::Database::Migrations::ReestablishedConnectionStack) end - shared_examples_for 'helpers that enqueue background migrations' do |worker_class, tracking_database| + shared_examples_for 'helpers that enqueue background migrations' do |worker_class, connection_class, tracking_database| before do allow(model).to receive(:tracking_database).and_return(tracking_database) + + # Due to lib/gitlab/database/load_balancing/configuration.rb:92 requiring RequestStore + # we cannot use stub_feature_flags(force_no_sharing_primary_model: true) + allow(connection_class.connection.load_balancer.configuration) + .to receive(:use_dedicated_connection?).and_return(true) + + allow(model).to receive(:connection).and_return(connection_class.connection) end describe '#queue_background_migration_jobs_by_range_at_intervals' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + context 'when the model has an ID column' do let!(:id1) { create(:user).id } let!(:id2) { create(:user).id } @@ -196,6 +211,34 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do end.to raise_error(StandardError, /does not have an ID/) end end + + context 'when using Migration[2.0]' do + let(:base_class) { Class.new(Gitlab::Database::Migration[2.0]) } + + context 'when restriction is set to gitlab_shared' do + before do + base_class.restrict_gitlab_migration gitlab_schema: :gitlab_shared + end + + it 'does raise an exception' do + expect do + model.queue_background_migration_jobs_by_range_at_intervals(ProjectAuthorization, 'FooJob', 10.seconds) + end.to raise_error /use `restrict_gitlab_migration:` " with `:gitlab_shared`/ + end + end + end + + context 'when within transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(true) + end + + it 'does raise an exception' do + expect do + model.queue_background_migration_jobs_by_range_at_intervals(ProjectAuthorization, 'FooJob', 10.seconds) + end.to raise_error /The `#queue_background_migration_jobs_by_range_at_intervals` can not be run inside a transaction./ + end + end end describe '#requeue_background_migration_jobs_by_range_at_intervals' do @@ -205,6 +248,10 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do let!(:successful_job_1) { create(:background_migration_job, class_name: job_class_name, status: :succeeded, arguments: [5, 6]) } let!(:successful_job_2) { create(:background_migration_job, class_name: job_class_name, status: :succeeded, arguments: [7, 8]) } + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + around do |example| freeze_time do Sidekiq::Testing.fake! do @@ -219,6 +266,38 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do expect(subject).to eq(20.minutes) end + context 'when using Migration[2.0]' do + let(:base_class) { Class.new(Gitlab::Database::Migration[2.0]) } + + it 'does re-enqueue pending jobs' do + subject + + expect(worker_class.jobs).not_to be_empty + end + + context 'when restriction is set' do + before do + base_class.restrict_gitlab_migration gitlab_schema: :gitlab_main + end + + it 'does raise an exception' do + expect { subject } + .to raise_error /The `#requeue_background_migration_jobs_by_range_at_intervals` cannot use `restrict_gitlab_migration:`./ + end + end + end + + context 'when within transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(true) + end + + it 'does raise an exception' do + expect { subject } + .to raise_error /The `#requeue_background_migration_jobs_by_range_at_intervals` can not be run inside a transaction./ + end + end + context 'when nothing is queued' do subject { model.requeue_background_migration_jobs_by_range_at_intervals('FakeJob', 10.minutes) } @@ -290,7 +369,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do end end - describe '#finalized_background_migration' do + describe '#finalize_background_migration' do let(:coordinator) { Gitlab::BackgroundMigration::JobCoordinator.new(worker_class) } let!(:tracked_pending_job) { create(:background_migration_job, class_name: job_class_name, status: :pending, arguments: [1]) } @@ -309,8 +388,8 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do allow(Gitlab::BackgroundMigration).to receive(:coordinator_for_database) .with(tracking_database).and_return(coordinator) - expect(coordinator).to receive(:migration_class_for) - .with(job_class_name).at_least(:once) { job_class } + allow(coordinator).to receive(:migration_class_for) + .with(job_class_name) { job_class } Sidekiq::Testing.disable! do worker_class.perform_async(job_class_name, [1, 2]) @@ -318,6 +397,8 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do worker_class.perform_in(10, job_class_name, [5, 6]) worker_class.perform_in(20, job_class_name, [7, 8]) end + + allow(model).to receive(:transaction_open?).and_return(false) end it_behaves_like 'finalized tracked background migration', worker_class do @@ -326,6 +407,52 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do end end + context 'when within transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(true) + end + + it 'does raise an exception' do + expect { model.finalize_background_migration(job_class_name, delete_tracking_jobs: %w[pending succeeded]) } + .to raise_error /The `#finalize_background_migration` can not be run inside a transaction./ + end + end + + context 'when using Migration[2.0]' do + let(:base_class) { Class.new(Gitlab::Database::Migration[2.0]) } + + it_behaves_like 'finalized tracked background migration', worker_class do + before do + model.finalize_background_migration(job_class_name) + end + end + + context 'when restriction is set' do + before do + base_class.restrict_gitlab_migration gitlab_schema: :gitlab_main + end + + it 'does raise an exception' do + expect { model.finalize_background_migration(job_class_name, delete_tracking_jobs: %w[pending succeeded]) } + .to raise_error /The `#finalize_background_migration` cannot use `restrict_gitlab_migration:`./ + end + end + end + + context 'when running migration in reconfigured ActiveRecord::Base context' do + it_behaves_like 'reconfigures connection stack', tracking_database do + it 'does restore connection hierarchy' do + expect_next_instances_of(job_class, 1..) do |job| + expect(job).to receive(:perform) do + validate_connections! + end + end + + model.finalize_background_migration(job_class_name, delete_tracking_jobs: %w[pending succeeded]) + end + end + end + context 'when removing all tracked job records' do let!(:job_class) do Class.new do @@ -443,7 +570,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do end context 'when the migration is running against the main database' do - it_behaves_like 'helpers that enqueue background migrations', BackgroundMigrationWorker, 'main' + it_behaves_like 'helpers that enqueue background migrations', BackgroundMigrationWorker, ActiveRecord::Base, 'main' end context 'when the migration is running against the ci database', if: Gitlab::Database.has_config?(:ci) do @@ -453,7 +580,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do end end - it_behaves_like 'helpers that enqueue background migrations', BackgroundMigration::CiDatabaseWorker, 'ci' + it_behaves_like 'helpers that enqueue background migrations', BackgroundMigration::CiDatabaseWorker, Ci::ApplicationRecord, 'ci' end describe '#delete_job_tracking' do diff --git a/spec/lib/gitlab/database/migrations/base_background_runner_spec.rb b/spec/lib/gitlab/database/migrations/base_background_runner_spec.rb new file mode 100644 index 00000000000..34c83c42056 --- /dev/null +++ b/spec/lib/gitlab/database/migrations/base_background_runner_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::BaseBackgroundRunner, :freeze_time do + let(:result_dir) { Dir.mktmpdir } + + after do + FileUtils.rm_rf(result_dir) + end + + context 'subclassing' do + subject { described_class.new(result_dir: result_dir) } + + it 'requires that jobs_by_migration_name be implemented' do + expect { subject.jobs_by_migration_name }.to raise_error(NotImplementedError) + end + + it 'requires that run_job be implemented' do + expect { subject.run_job(nil) }.to raise_error(NotImplementedError) + end + end +end diff --git a/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb b/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb index f9347a174c4..d1a66036149 100644 --- a/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb @@ -163,4 +163,45 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d end end end + + describe '#finalize_batched_background_migration' do + let!(:batched_migration) { create(:batched_background_migration, job_class_name: 'MyClass', table_name: :projects, column_name: :id, job_arguments: []) } + + it 'finalizes the migration' do + allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| + expect(runner).to receive(:finalize).with('MyClass', :projects, :id, []) + end + + migration.finalize_batched_background_migration(job_class_name: 'MyClass', table_name: :projects, column_name: :id, job_arguments: []) + end + + context 'when the migration does not exist' do + it 'raises an exception' do + expect do + migration.finalize_batched_background_migration(job_class_name: 'MyJobClass', table_name: :projects, column_name: :id, job_arguments: []) + end.to raise_error(RuntimeError, 'Could not find batched background migration') + end + end + + context 'when uses a CI connection', :reestablished_active_record_base do + before do + skip_if_multiple_databases_not_setup + + ActiveRecord::Base.establish_connection(:ci) # rubocop:disable Database/EstablishConnection + end + + it 'raises an exception' do + ci_migration = create(:batched_background_migration, :active) + + expect do + migration.finalize_batched_background_migration( + job_class_name: ci_migration.job_class_name, + table_name: ci_migration.table_name, + column_name: ci_migration.column_name, + job_arguments: ci_migration.job_arguments + ) + end.to raise_error /is currently not supported when running in decomposed/ + end + end + end end diff --git a/spec/lib/gitlab/database/migrations/observers/query_statistics_spec.rb b/spec/lib/gitlab/database/migrations/observers/query_statistics_spec.rb index 2515f0d4a06..66de25d65bb 100644 --- a/spec/lib/gitlab/database/migrations/observers/query_statistics_spec.rb +++ b/spec/lib/gitlab/database/migrations/observers/query_statistics_spec.rb @@ -43,6 +43,7 @@ RSpec.describe Gitlab::Database::Migrations::Observers::QueryStatistics do <<~SQL SELECT query, calls, total_time, max_time, mean_time, rows FROM pg_stat_statements + WHERE pg_get_userbyid(userid) = current_user ORDER BY total_time DESC SQL end diff --git a/spec/lib/gitlab/database/migrations/reestablished_connection_stack_spec.rb b/spec/lib/gitlab/database/migrations/reestablished_connection_stack_spec.rb new file mode 100644 index 00000000000..cfb308c63e4 --- /dev/null +++ b/spec/lib/gitlab/database/migrations/reestablished_connection_stack_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::ReestablishedConnectionStack do + let(:base_class) { ActiveRecord::Migration } + + let(:model) do + base_class.new + .extend(described_class) + end + + describe '#with_restored_connection_stack' do + Gitlab::Database.database_base_models.each do |db_config_name, _| + context db_config_name do + it_behaves_like "reconfigures connection stack", db_config_name do + it 'does restore connection hierarchy' do + model.with_restored_connection_stack do + validate_connections! + end + end + + primary_db_config = ActiveRecord::Base.configurations.primary?(db_config_name) + + it 'does reconfigure connection handler', unless: primary_db_config do + original_handler = ActiveRecord::Base.connection_handler + new_handler = nil + + model.with_restored_connection_stack do + new_handler = ActiveRecord::Base.connection_handler + + # establish connection + ApplicationRecord.connection.select_one("SELECT 1 FROM projects LIMIT 1") + Ci::ApplicationRecord.connection.select_one("SELECT 1 FROM ci_builds LIMIT 1") + end + + expect(new_handler).not_to eq(original_handler), "is reconnected" + expect(new_handler).not_to be_active_connections + expect(ActiveRecord::Base.connection_handler).to eq(original_handler), "is restored" + end + + it 'does keep original connection handler', if: primary_db_config do + original_handler = ActiveRecord::Base.connection_handler + new_handler = nil + + model.with_restored_connection_stack do + new_handler = ActiveRecord::Base.connection_handler + end + + expect(new_handler).to eq(original_handler) + expect(ActiveRecord::Base.connection_handler).to eq(original_handler) + end + end + end + end + end +end diff --git a/spec/lib/gitlab/database/migrations/runner_spec.rb b/spec/lib/gitlab/database/migrations/runner_spec.rb index 8b1ccf05eb1..e7f68e3e4a8 100644 --- a/spec/lib/gitlab/database/migrations/runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/runner_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Migrations::Runner do + include Database::MultipleDatabases + let(:result_dir) { Pathname.new(Dir.mktmpdir) } let(:migration_runs) { [] } # This list gets populated as the runner tries to run migrations @@ -136,4 +138,35 @@ RSpec.describe Gitlab::Database::Migrations::Runner do expect(runner.result_dir).to eq(described_class::BASE_RESULT_DIR.join( 'background_migrations')) end end + + describe '.batched_background_migrations' do + it 'is a TestBatchedBackgroundRunner' do + expect(described_class.batched_background_migrations(for_database: 'main')).to be_a(Gitlab::Database::Migrations::TestBatchedBackgroundRunner) + end + + context 'choosing the database to test against' do + it 'chooses the main database' do + runner = described_class.batched_background_migrations(for_database: 'main') + + chosen_connection_name = Gitlab::Database.db_config_name(runner.connection) + + expect(chosen_connection_name).to eq('main') + end + + it 'chooses the ci database' do + skip_if_multiple_databases_not_setup + + runner = described_class.batched_background_migrations(for_database: 'ci') + + chosen_connection_name = Gitlab::Database.db_config_name(runner.connection) + + expect(chosen_connection_name).to eq('ci') + end + + it 'throws an error with an invalid name' do + expect { described_class.batched_background_migrations(for_database: 'not_a_database') } + .to raise_error(/not a valid database name/) + end + end + end end diff --git a/spec/lib/gitlab/database/migrations/test_background_runner_spec.rb b/spec/lib/gitlab/database/migrations/test_background_runner_spec.rb index 9407efad91f..a2fe91712c7 100644 --- a/spec/lib/gitlab/database/migrations/test_background_runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/test_background_runner_spec.rb @@ -3,7 +3,9 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Migrations::TestBackgroundRunner, :redis do + include Gitlab::Database::Migrations::ReestablishedConnectionStack include Gitlab::Database::Migrations::BackgroundMigrationHelpers + include Database::MigrationTestingHelpers # In order to test the interaction between queueing sidekiq jobs and seeing those jobs in queues, # we need to disable sidekiq's testing mode and actually send our jobs to redis @@ -12,6 +14,7 @@ RSpec.describe Gitlab::Database::Migrations::TestBackgroundRunner, :redis do end let(:result_dir) { Dir.mktmpdir } + let(:connection) { ApplicationRecord.connection } after do FileUtils.rm_rf(result_dir) @@ -41,40 +44,6 @@ RSpec.describe Gitlab::Database::Migrations::TestBackgroundRunner, :redis do end context 'running migrations', :freeze_time do - def define_background_migration(name) - klass = Class.new do - # Can't simply def perform here as we won't have access to the block, - # similarly can't define_method(:perform, &block) here as it would change the block receiver - define_method(:perform) { |*args| yield(*args) } - end - stub_const("Gitlab::BackgroundMigration::#{name}", klass) - klass - end - - def expect_migration_call_counts(migrations_to_calls) - migrations_to_calls.each do |migration, calls| - expect_next_instances_of(migration, calls) do |m| - expect(m).to receive(:perform).and_call_original - end - end - end - - def expect_recorded_migration_runs(migrations_to_runs) - migrations_to_runs.each do |migration, runs| - path = File.join(result_dir, migration.name.demodulize) - num_subdirs = Pathname(path).children.count(&:directory?) - expect(num_subdirs).to eq(runs) - end - end - - def expect_migration_runs(migrations_to_run_counts) - expect_migration_call_counts(migrations_to_run_counts) - - yield - - expect_recorded_migration_runs(migrations_to_run_counts) - end - it 'runs the migration class correctly' do calls = [] define_background_migration(migration_name) do |i| diff --git a/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb b/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb new file mode 100644 index 00000000000..fbfff1268cc --- /dev/null +++ b/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freeze_time do + include Gitlab::Database::MigrationHelpers + include Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers + include Database::MigrationTestingHelpers + + let(:result_dir) { Dir.mktmpdir } + + after do + FileUtils.rm_rf(result_dir) + end + + let(:connection) { ApplicationRecord.connection } + + let(:table_name) { "_test_column_copying"} + + before do + connection.execute(<<~SQL) + CREATE TABLE #{table_name} ( + id bigint primary key not null, + data bigint + ); + + insert into #{table_name} (id) select i from generate_series(1, 1000) g(i); + SQL + end + + context 'running a real background migration' do + it 'runs sampled jobs from the batched background migration' do + queue_batched_background_migration('CopyColumnUsingBackgroundMigrationJob', + table_name, :id, + :id, :data, + batch_size: 100, + job_interval: 5.minutes) # job_interval is skipped when testing + described_class.new(result_dir: result_dir, connection: connection).run_jobs(for_duration: 1.minute) + unmigrated_row_count = define_batchable_model(table_name).where('id != data').count + + expect(unmigrated_row_count).to eq(0) + end + end + + context 'with jobs to run' do + let(:migration_name) { 'TestBackgroundMigration' } + + before do + queue_batched_background_migration(migration_name, table_name, :id, job_interval: 5.minutes, batch_size: 100) + end + + it 'samples jobs' do + calls = [] + define_background_migration(migration_name) do |*args| + calls << args + end + + described_class.new(result_dir: result_dir, connection: connection).run_jobs(for_duration: 3.minutes) + + expect(calls.count).to eq(10) # 1000 rows / batch size 100 = 10 + end + + context 'with multiple jobs to run' do + it 'runs all jobs created within the last 48 hours' do + old_migration = define_background_migration(migration_name) + + travel 3.days + + new_migration = define_background_migration('NewMigration') { travel 1.second } + queue_batched_background_migration('NewMigration', table_name, :id, + job_interval: 5.minutes, + batch_size: 10, + sub_batch_size: 5) + + other_new_migration = define_background_migration('NewMigration2') { travel 2.seconds } + queue_batched_background_migration('NewMigration2', table_name, :id, + job_interval: 5.minutes, + batch_size: 10, + sub_batch_size: 5) + + expect_migration_runs(new_migration => 3, other_new_migration => 2, old_migration => 0) do + described_class.new(result_dir: result_dir, connection: connection).run_jobs(for_duration: 5.seconds) + end + end + end + end +end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb index 8ab3816529b..edb8ae36c45 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb @@ -54,7 +54,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do expect_add_concurrent_index_and_call_original(partition2_identifier, column_name, partition2_index) expect(migration).to receive(:with_lock_retries).ordered.and_yield - expect(migration).to receive(:add_index).with(table_name, column_name, name: index_name).ordered.and_call_original + expect(migration).to receive(:add_index).with(table_name, column_name, { name: index_name }).ordered.and_call_original migration.add_concurrent_partitioned_index(table_name, column_name, name: index_name) @@ -64,7 +64,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do end def expect_add_concurrent_index_and_call_original(table, column, index) - expect(migration).to receive(:add_concurrent_index).ordered.with(table, column, name: index) + expect(migration).to receive(:add_concurrent_index).ordered.with(table, column, { name: index }) .and_wrap_original { |_, table, column, options| connection.add_index(table, column, **options) } end end @@ -90,13 +90,13 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do it 'forwards them to the index helper methods', :aggregate_failures do expect(migration).to receive(:add_concurrent_index) - .with(partition1_identifier, column_name, name: partition1_index, where: 'x > 0', unique: true) + .with(partition1_identifier, column_name, { name: partition1_index, where: 'x > 0', unique: true }) expect(migration).to receive(:add_index) - .with(table_name, column_name, name: index_name, where: 'x > 0', unique: true) + .with(table_name, column_name, { name: index_name, where: 'x > 0', unique: true }) migration.add_concurrent_partitioned_index(table_name, column_name, - name: index_name, where: 'x > 0', unique: true) + { name: index_name, where: 'x > 0', unique: true }) end end diff --git a/spec/lib/gitlab/database/query_analyzer_spec.rb b/spec/lib/gitlab/database/query_analyzer_spec.rb index 3b4cbc79de2..0b849063562 100644 --- a/spec/lib/gitlab/database/query_analyzer_spec.rb +++ b/spec/lib/gitlab/database/query_analyzer_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do let(:analyzer) { double(:query_analyzer) } - let(:user_analyzer) { double(:query_analyzer) } + let(:user_analyzer) { double(:user_query_analyzer) } let(:disabled_analyzer) { double(:disabled_query_analyzer) } before do @@ -49,14 +49,36 @@ RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do end end - it 'does not evaluate enabled? again do yield block' do - expect(analyzer).not_to receive(:enabled?) + it 'does initialize analyzer only once' do + expect(analyzer).to receive(:enabled?).once + expect(analyzer).to receive(:begin!).once + expect(analyzer).to receive(:end!).once expect { |b| described_class.instance.within(&b) }.to yield_control end - it 'raises exception when trying to re-define analyzers' do - expect { |b| described_class.instance.within([user_analyzer], &b) }.to raise_error /Query analyzers are already defined, cannot re-define them/ + it 'does initialize user analyzer when enabled' do + expect(user_analyzer).to receive(:enabled?).and_return(true) + expect(user_analyzer).to receive(:begin!) + expect(user_analyzer).to receive(:end!) + + expect { |b| described_class.instance.within([user_analyzer], &b) }.to yield_control + end + + it 'does initialize user analyzer only once' do + expect(user_analyzer).to receive(:enabled?).and_return(false, true) + expect(user_analyzer).to receive(:begin!).once + expect(user_analyzer).to receive(:end!).once + + expect { |b| described_class.instance.within([user_analyzer, user_analyzer, user_analyzer], &b) }.to yield_control + end + + it 'does not initializer user analyzer when disabled' do + expect(user_analyzer).to receive(:enabled?).and_return(false) + expect(user_analyzer).not_to receive(:begin!) + expect(user_analyzer).not_to receive(:end!) + + expect { |b| described_class.instance.within([user_analyzer], &b) }.to yield_control end end @@ -162,7 +184,7 @@ RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do def process_sql(sql) described_class.instance.within do ApplicationRecord.load_balancer.read_write do |connection| - described_class.instance.process_sql(sql, connection) + described_class.instance.send(:process_sql, sql, connection) end end end diff --git a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb index b8c1ecd9089..0d687db0f96 100644 --- a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb @@ -140,7 +140,7 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasMetrics, query_ana def process_sql(model, sql) Gitlab::Database::QueryAnalyzer.instance.within do # Skip load balancer and retrieve connection assigned to model - Gitlab::Database::QueryAnalyzer.instance.process_sql(sql, model.retrieve_connection) + Gitlab::Database::QueryAnalyzer.instance.send(:process_sql, sql, model.retrieve_connection) end end end diff --git a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb new file mode 100644 index 00000000000..5e8afc0102e --- /dev/null +++ b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection, query_analyzers: false do + let(:analyzer) { described_class } + + context 'properly observes all queries', :request_store do + using RSpec::Parameterized::TableSyntax + + where do + { + "for simple query observes schema correctly" => { + model: ApplicationRecord, + sql: "SELECT 1 FROM projects", + expect_error: nil, + setup: nil + }, + "for query accessing gitlab_ci and gitlab_main" => { + model: ApplicationRecord, + sql: "SELECT 1 FROM projects LEFT JOIN ci_builds ON ci_builds.project_id=projects.id", + expect_error: /The query tried to access \["projects", "ci_builds"\]/, + setup: -> (_) { skip_if_multiple_databases_not_setup } + }, + "for query accessing gitlab_ci and gitlab_main the gitlab_schemas is always ordered" => { + model: ApplicationRecord, + sql: "SELECT 1 FROM ci_builds LEFT JOIN projects ON ci_builds.project_id=projects.id", + expect_error: /The query tried to access \["ci_builds", "projects"\]/, + setup: -> (_) { skip_if_multiple_databases_not_setup } + }, + "for query accessing main table from CI database" => { + model: Ci::ApplicationRecord, + sql: "SELECT 1 FROM projects", + expect_error: /The query tried to access \["projects"\]/, + setup: -> (_) { skip_if_multiple_databases_not_setup } + }, + "for query accessing CI database" => { + model: Ci::ApplicationRecord, + sql: "SELECT 1 FROM ci_builds", + expect_error: nil + }, + "for query accessing CI table from main database" => { + model: ::ApplicationRecord, + sql: "SELECT 1 FROM ci_builds", + expect_error: /The query tried to access \["ci_builds"\]/, + setup: -> (_) { skip_if_multiple_databases_not_setup } + } + } + end + + with_them do + it do + instance_eval(&setup) if setup + + if expect_error + expect { process_sql(model, sql) }.to raise_error(expect_error) + else + expect { process_sql(model, sql) }.not_to raise_error + end + end + end + end + + def process_sql(model, sql) + Gitlab::Database::QueryAnalyzer.instance.within([analyzer]) do + # Skip load balancer and retrieve connection assigned to model + Gitlab::Database::QueryAnalyzer.instance.send(:process_sql, sql, model.retrieve_connection) + end + end +end diff --git a/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb b/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb index a2c7916fa01..261bef58bb6 100644 --- a/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb @@ -155,7 +155,7 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas, query_a yield if block_given? # Skip load balancer and retrieve connection assigned to model - Gitlab::Database::QueryAnalyzer.instance.process_sql(sql, model.retrieve_connection) + Gitlab::Database::QueryAnalyzer.instance.send(:process_sql, sql, model.retrieve_connection) end end end diff --git a/spec/lib/gitlab/database/shared_model_spec.rb b/spec/lib/gitlab/database/shared_model_spec.rb index 54af4a0c4dc..574111f4c01 100644 --- a/spec/lib/gitlab/database/shared_model_spec.rb +++ b/spec/lib/gitlab/database/shared_model_spec.rb @@ -51,7 +51,7 @@ RSpec.describe Gitlab::Database::SharedModel do expect do described_class.using_connection(second_connection) {} - end.to raise_error(/cannot nest connection overrides/) + end.to raise_error(/Cannot change connection for Gitlab::Database::SharedModel/) expect(described_class.connection).to be(new_connection) end |