diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-04-20 14:43:17 +0300 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-04-20 14:43:17 +0300 |
| commit | dfc94207fec2d84314b1a5410cface22e8b369bd (patch) | |
| tree | c54022f61ced104305889a64de080998a0dc773b /spec/lib/gitlab/database | |
| parent | b874efeff674f6bf0355d5d242ecf81c6f7155df (diff) | |
Add latest changes from gitlab-org/gitlab@15-11-stable-eev15.11.0-rc42
Diffstat (limited to 'spec/lib/gitlab/database')
48 files changed, 1313 insertions, 486 deletions
diff --git a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb index 7c5c368fcb5..b2ba1a60fbb 100644 --- a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb @@ -143,6 +143,92 @@ RSpec.describe Gitlab::Database::AsyncIndexes::MigrationHelpers, feature_categor end end + describe '#prepare_async_index_from_sql' do + let(:index_definition) { "CREATE INDEX CONCURRENTLY #{index_name} ON #{table_name} USING btree(id)" } + + subject(:prepare_async_index_from_sql) do + migration.prepare_async_index_from_sql(index_definition) + end + + before do + connection.create_table(table_name) + + allow(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas).to receive(:require_ddl_mode!).and_call_original + end + + it 'requires ddl mode' do + prepare_async_index_from_sql + + expect(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas).to have_received(:require_ddl_mode!) + end + + context 'when the given index is invalid' do + let(:index_definition) { "SELECT FROM users" } + + it 'raises a RuntimeError' do + expect { prepare_async_index_from_sql }.to raise_error(RuntimeError, 'Index statement not found!') + end + end + + context 'when the given index is valid' do + context 'when the index algorithm is not concurrent' do + let(:index_definition) { "CREATE INDEX #{index_name} ON #{table_name} USING btree(id)" } + + it 'raises a RuntimeError' do + expect { prepare_async_index_from_sql }.to raise_error(RuntimeError, 'Index must be created concurrently!') + end + end + + context 'when the index algorithm is concurrent' do + context 'when the statement tries to create an index for non-existing table' do + let(:index_definition) { "CREATE INDEX CONCURRENTLY #{index_name} ON foo_table USING btree(id)" } + + it 'raises a RuntimeError' do + expect { prepare_async_index_from_sql }.to raise_error(RuntimeError, 'Table does not exist!') + end + end + + context 'when the statement tries to create an index for an existing table' do + context 'when the async index creation is not available' do + before do + connection.drop_table(:postgres_async_indexes) + end + + it 'does not raise an error' do + expect { prepare_async_index_from_sql }.not_to raise_error + end + end + + context 'when the async index creation is available' do + context 'when there is already an index with the given name' do + before do + connection.add_index(table_name, 'id', name: index_name) + end + + it 'does not create the async index record' do + expect { prepare_async_index_from_sql }.not_to change { index_model.where(name: index_name).count } + end + end + + context 'when there is no index with the given name' do + let(:async_index) { index_model.find_by(name: index_name) } + + it 'creates the async index record' do + expect { prepare_async_index_from_sql }.to change { index_model.where(name: index_name).count }.by(1) + end + + it 'sets the async index attributes correctly' do + prepare_async_index_from_sql + + expect(async_index).to have_attributes(table_name: table_name, definition: index_definition) + end + end + end + end + end + end + end + describe '#prepare_async_index_removal' do before do connection.create_table(table_name) diff --git a/spec/lib/gitlab/database/background_migration/batched_job_spec.rb b/spec/lib/gitlab/database/background_migration/batched_job_spec.rb index 073a30e7839..d9b81a2be30 100644 --- a/spec/lib/gitlab/database/background_migration/batched_job_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_job_spec.rb @@ -378,41 +378,27 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedJob, type: :model d let(:attempts) { 0 } let(:batch_size) { 10 } let(:sub_batch_size) { 6 } - let(:feature_flag) { :reduce_sub_batch_size_on_timeouts } let(:job) do create(:batched_background_migration_job, attempts: attempts, batch_size: batch_size, sub_batch_size: sub_batch_size) end - where(:feature_flag_state, :within_boundaries, :outside_boundaries, :limit_reached) do - [ - [true, true, false, false], - [false, false, false, false] - ] - end - - with_them do - before do - stub_feature_flags(feature_flag => feature_flag_state) - end + context 'when the number of attempts is lower than the limit and batch size are within boundaries' do + let(:attempts) { 1 } - context 'when the number of attempts is lower than the limit and batch size are within boundaries' do - let(:attempts) { 1 } - - it { expect(job.can_reduce_sub_batch_size?).to be(within_boundaries) } - end + it { expect(job.can_reduce_sub_batch_size?).to be(true) } + end - context 'when the number of attempts is lower than the limit and batch size are outside boundaries' do - let(:batch_size) { 1 } + context 'when the number of attempts is lower than the limit and batch size are outside boundaries' do + let(:batch_size) { 1 } - it { expect(job.can_reduce_sub_batch_size?).to be(outside_boundaries) } - end + it { expect(job.can_reduce_sub_batch_size?).to be(false) } + end - context 'when the number of attempts is greater than the limit and batch size are within boundaries' do - let(:attempts) { 3 } + context 'when the number of attempts is greater than the limit and batch size are within boundaries' do + let(:attempts) { 3 } - it { expect(job.can_reduce_sub_batch_size?).to be(limit_reached) } - end + it { expect(job.can_reduce_sub_batch_size?).to be(false) } end end 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 d132559acea..546f9353808 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :model do +RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :model, feature_category: :database do it_behaves_like 'having unique enum values' it { is_expected.to be_a Gitlab::Database::SharedModel } @@ -328,6 +328,17 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end end + describe '.finalizing' do + let!(:migration1) { create(:batched_background_migration, :active) } + let!(:migration2) { create(:batched_background_migration, :paused) } + let!(:migration3) { create(:batched_background_migration, :finalizing) } + let!(:migration4) { create(:batched_background_migration, :finished) } + + it 'returns only finalizing migrations' do + expect(described_class.finalizing).to contain_exactly(migration3) + end + end + describe '.successful_rows_counts' do let!(:migration1) { create(:batched_background_migration) } let!(:migration2) { create(:batched_background_migration) } diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/patroni_apdex_spec.rb b/spec/lib/gitlab/database/background_migration/health_status/indicators/patroni_apdex_spec.rb new file mode 100644 index 00000000000..d3102a105ea --- /dev/null +++ b/spec/lib/gitlab/database/background_migration/health_status/indicators/patroni_apdex_spec.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::PatroniApdex, :aggregate_failures, feature_category: :database do # rubocop:disable Layout/LineLength + let(:schema) { :main } + let(:connection) { Gitlab::Database.database_base_models[schema].connection } + + around do |example| + Gitlab::Database::SharedModel.using_connection(connection) do + example.run + end + end + + describe '#evaluate' do + let(:prometheus_url) { 'http://thanos:9090' } + let(:prometheus_config) { [prometheus_url, { allow_local_requests: true, verify: true }] } + + let(:prometheus_client) { instance_double(Gitlab::PrometheusClient) } + + let(:context) do + Gitlab::Database::BackgroundMigration::HealthStatus::Context + .new(connection, ['users'], gitlab_schema) + end + + let(:gitlab_schema) { "gitlab_#{schema}" } + let(:client_ready) { true } + let(:database_apdex_sli_query_main) { 'Apdex query for main' } + let(:database_apdex_sli_query_ci) { 'Apdex query for ci' } + let(:database_apdex_slo_main) { 0.99 } + let(:database_apdex_slo_ci) { 0.95 } + let(:database_apdex_settings) do + { + prometheus_api_url: prometheus_url, + apdex_sli_query: { + main: database_apdex_sli_query_main, + ci: database_apdex_sli_query_ci + }, + apdex_slo: { + main: database_apdex_slo_main, + ci: database_apdex_slo_ci + } + } + end + + subject(:evaluate) { described_class.new(context).evaluate } + + before do + stub_application_setting(database_apdex_settings: database_apdex_settings) + + allow(Gitlab::PrometheusClient).to receive(:new).with(*prometheus_config).and_return(prometheus_client) + allow(prometheus_client).to receive(:ready?).and_return(client_ready) + end + + shared_examples 'Patroni Apdex Evaluator' do |schema| + context "with #{schema} schema" do + let(:schema) { schema } + let(:apdex_slo_above_sli) { { main: 0.991, ci: 0.951 } } + let(:apdex_slo_below_sli) { { main: 0.989, ci: 0.949 } } + + it 'returns NoSignal signal in case the feature flag is disabled' do + stub_feature_flags(batched_migrations_health_status_patroni_apdex: false) + + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::NotAvailable) + expect(evaluate.reason).to include('indicator disabled') + end + + context 'without database_apdex_settings' do + let(:database_apdex_settings) { nil } + + it 'returns Unknown signal' do + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate.reason).to include('Patroni Apdex Settings not configured') + end + end + + context 'when Prometheus client is not ready' do + let(:client_ready) { false } + + it 'returns Unknown signal' do + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate.reason).to include('Prometheus client is not ready') + end + end + + context 'when apdex SLI query is not configured' do + let(:"database_apdex_sli_query_#{schema}") { nil } + + it 'returns Unknown signal' do + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate.reason).to include('Apdex SLI query is not configured') + end + end + + context 'when slo is not configured' do + let(:"database_apdex_slo_#{schema}") { nil } + + it 'returns Unknown signal' do + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate.reason).to include('Apdex SLO is not configured') + end + end + + it 'returns Normal signal when Patroni apdex SLI is above SLO' do + expect(prometheus_client).to receive(:query) + .with(send("database_apdex_sli_query_#{schema}")) + .and_return([{ "value" => [1662423310.878, apdex_slo_above_sli[schema]] }]) + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Normal) + expect(evaluate.reason).to include('Patroni service apdex is above SLO') + end + + it 'returns Stop signal when Patroni apdex is below SLO' do + expect(prometheus_client).to receive(:query) + .with(send("database_apdex_sli_query_#{schema}")) + .and_return([{ "value" => [1662423310.878, apdex_slo_below_sli[schema]] }]) + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Stop) + expect(evaluate.reason).to include('Patroni service apdex is below SLO') + end + + context 'when Patroni apdex can not be calculated' do + where(:result) do + [ + nil, + [], + [{}], + [{ 'value' => 1 }], + [{ 'value' => [1] }] + ] + end + + with_them do + it 'returns Unknown signal' do + expect(prometheus_client).to receive(:query).and_return(result) + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate.reason).to include('Patroni service apdex can not be calculated') + end + end + end + end + end + + Gitlab::Database.database_base_models.each do |database_base_model, connection| + next unless connection.present? + + it_behaves_like 'Patroni Apdex Evaluator', database_base_model.to_sym + end + end +end diff --git a/spec/lib/gitlab/database/background_migration/health_status_spec.rb b/spec/lib/gitlab/database/background_migration/health_status_spec.rb index 8bc04d80fa1..e14440f1fb4 100644 --- a/spec/lib/gitlab/database/background_migration/health_status_spec.rb +++ b/spec/lib/gitlab/database/background_migration/health_status_spec.rb @@ -19,8 +19,10 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus do let(:health_status) { Gitlab::Database::BackgroundMigration::HealthStatus } let(:autovacuum_indicator_class) { health_status::Indicators::AutovacuumActiveOnTable } let(:wal_indicator_class) { health_status::Indicators::WriteAheadLog } + let(:patroni_apdex_indicator_class) { health_status::Indicators::PatroniApdex } let(:autovacuum_indicator) { instance_double(autovacuum_indicator_class) } let(:wal_indicator) { instance_double(wal_indicator_class) } + let(:patroni_apdex_indicator) { instance_double(patroni_apdex_indicator_class) } before do allow(autovacuum_indicator_class).to receive(:new).with(migration.health_context).and_return(autovacuum_indicator) @@ -36,8 +38,11 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus do expect(autovacuum_indicator).to receive(:evaluate).and_return(normal_signal) expect(wal_indicator_class).to receive(:new).with(migration.health_context).and_return(wal_indicator) expect(wal_indicator).to receive(:evaluate).and_return(not_available_signal) + expect(patroni_apdex_indicator_class).to receive(:new).with(migration.health_context) + .and_return(patroni_apdex_indicator) + expect(patroni_apdex_indicator).to receive(:evaluate).and_return(not_available_signal) - expect(evaluate).to contain_exactly(normal_signal, not_available_signal) + expect(evaluate).to contain_exactly(normal_signal, not_available_signal, not_available_signal) end end diff --git a/spec/lib/gitlab/database/consistency_checker_spec.rb b/spec/lib/gitlab/database/consistency_checker_spec.rb index c0f0c349ddd..be03bd00619 100644 --- a/spec/lib/gitlab/database/consistency_checker_spec.rb +++ b/spec/lib/gitlab/database/consistency_checker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::ConsistencyChecker, feature_category: :pods do +RSpec.describe Gitlab::Database::ConsistencyChecker, feature_category: :cell do let(:batch_size) { 10 } let(:max_batches) { 4 } let(:max_runtime) { described_class::MAX_RUNTIME } diff --git a/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb b/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb index 768855464c1..a57f02b22df 100644 --- a/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb @@ -2,18 +2,13 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing::ActionCableCallbacks, :request_store do +RSpec.describe Gitlab::Database::LoadBalancing::ActionCableCallbacks, :request_store, feature_category: :shared do describe '.wrapper' do - it 'uses primary and then releases the connection and clears the session' do + it 'releases the connection and clears the session' do expect(Gitlab::Database::LoadBalancing).to receive(:release_hosts) expect(Gitlab::Database::LoadBalancing::Session).to receive(:clear_session) - described_class.wrapper.call( - nil, - lambda do - expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).to eq(true) - end - ) + described_class.wrapper.call(nil, lambda {}) end context 'with an exception' do diff --git a/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb index 7eb20f77417..83fc614bde3 100644 --- a/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb @@ -67,16 +67,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware, feature let(:location) { '0/D525E3A8' } - context 'when feature flag is disabled' do - let(:expected_consistency) { :always } - - before do - stub_feature_flags(load_balancing_for_test_data_consistency_worker: false) - end - - include_examples 'does not pass database locations' - end - context 'when write was not performed' do before do allow(Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary?).and_return(false) @@ -106,7 +96,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware, feature expected_location = {} Gitlab::Database::LoadBalancing.each_load_balancer do |lb| - expect(lb).to receive(:host).and_return(nil) + expect(lb).to receive(:host).at_least(:once).and_return(nil) expect(lb).to receive(:primary_write_location).and_return(location) expected_location[lb.name] = location diff --git a/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb index abf10456d0a..7ad0ddbca8e 100644 --- a/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_gitlab_redis_queues do +RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_gitlab_redis_queues, feature_category: :scalability do let(:middleware) { described_class.new } let(:worker) { worker_class.new } let(:location) { '0/D525E3A8' } @@ -15,6 +15,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ replication_lag!(false) Gitlab::Database::LoadBalancing::Session.clear_session + + stub_const("#{described_class.name}::REPLICA_WAIT_SLEEP_SECONDS", 0.0) end after do @@ -76,14 +78,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ end shared_examples_for 'sticks based on data consistency' do - context 'when load_balancing_for_test_data_consistency_worker is disabled' do - before do - stub_feature_flags(load_balancing_for_test_data_consistency_worker: false) - end - - include_examples 'stick to the primary', 'primary' - end - context 'when database wal location is set' do let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'wal_locations' => wal_locations } } @@ -119,9 +113,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ end end - shared_examples_for 'sleeps when necessary' do + shared_examples_for 'essential sleep' do context 'when WAL locations are blank', :freeze_time do - let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", "wal_locations" => {}, "created_at" => Time.current.to_f - (described_class::MINIMUM_DELAY_INTERVAL_SECONDS - 0.3) } } + let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", "wal_locations" => {}, "created_at" => Time.current.to_f - (described_class::REPLICA_WAIT_SLEEP_SECONDS + 0.2) } } it 'does not sleep' do expect(middleware).not_to receive(:sleep) @@ -134,7 +128,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'wal_locations' => wal_locations, "created_at" => Time.current.to_f - elapsed_time } } context 'when delay interval has not elapsed' do - let(:elapsed_time) { described_class::MINIMUM_DELAY_INTERVAL_SECONDS - 0.3 } + let(:elapsed_time) { described_class::REPLICA_WAIT_SLEEP_SECONDS + 0.2 } context 'when replica is up to date' do before do @@ -158,30 +152,24 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ end it 'sleeps until the minimum delay is reached' do - expect(middleware).to receive(:sleep).with(be_within(0.01).of(described_class::MINIMUM_DELAY_INTERVAL_SECONDS - elapsed_time)) + expect(middleware).to receive(:sleep).with(described_class::REPLICA_WAIT_SLEEP_SECONDS) run_middleware end end - end - - context 'when delay interval has elapsed' do - let(:elapsed_time) { described_class::MINIMUM_DELAY_INTERVAL_SECONDS + 0.3 } - - it 'does not sleep' do - expect(middleware).not_to receive(:sleep) - run_middleware - end - end - - context 'when created_at is in the future' do - let(:elapsed_time) { -5 } + context 'when replica is never not up to date' do + before do + Gitlab::Database::LoadBalancing.each_load_balancer do |lb| + allow(lb).to receive(:select_up_to_date_host).and_return(false, false) + end + end - it 'does not sleep' do - expect(middleware).not_to receive(:sleep) + it 'sleeps until the maximum delay is reached' do + expect(middleware).to receive(:sleep).exactly(3).times.with(described_class::REPLICA_WAIT_SLEEP_SECONDS) - run_middleware + run_middleware + end end end end @@ -200,7 +188,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ context 'when delay interval has not elapsed', :freeze_time do let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'wal_locations' => wal_locations, "created_at" => Time.current.to_f - elapsed_time } } - let(:elapsed_time) { described_class::MINIMUM_DELAY_INTERVAL_SECONDS - 0.3 } + let(:elapsed_time) { described_class::REPLICA_WAIT_SLEEP_SECONDS + 0.2 } it 'does not sleep' do expect(middleware).not_to receive(:sleep) @@ -214,7 +202,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ include_context 'data consistency worker class', :delayed, :load_balancing_for_test_data_consistency_worker include_examples 'sticks based on data consistency' - include_examples 'sleeps when necessary' + include_examples 'essential sleep' context 'when replica is not up to date' do before do @@ -263,7 +251,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ include_context 'data consistency worker class', :sticky, :load_balancing_for_test_data_consistency_worker include_examples 'sticks based on data consistency' - include_examples 'sleeps when necessary' + include_examples 'essential sleep' context 'when replica is not up to date' do before do diff --git a/spec/lib/gitlab/database/load_balancing_spec.rb b/spec/lib/gitlab/database/load_balancing_spec.rb index 59e16e6ca8b..7196b4bc337 100644 --- a/spec/lib/gitlab/database/load_balancing_spec.rb +++ b/spec/lib/gitlab/database/load_balancing_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing, :suppress_gitlab_schemas_validate_connection, feature_category: :pods do +RSpec.describe Gitlab::Database::LoadBalancing, :suppress_gitlab_schemas_validate_connection, feature_category: :cell do describe '.base_models' do it 'returns the models to apply load balancing to' do models = described_class.base_models @@ -497,14 +497,14 @@ RSpec.describe Gitlab::Database::LoadBalancing, :suppress_gitlab_schemas_validat where(:queries, :expected_role) do [ # Reload cache. The schema loading queries should be handled by - # primary. + # replica. [ -> { model.connection.clear_cache! model.connection.schema_cache.add('users') model.connection.pool.release_connection }, - :primary + :replica ], # Call model's connection method diff --git a/spec/lib/gitlab/database/lock_writes_manager_spec.rb b/spec/lib/gitlab/database/lock_writes_manager_spec.rb index c06c463d918..2aa95372338 100644 --- a/spec/lib/gitlab/database/lock_writes_manager_spec.rb +++ b/spec/lib/gitlab/database/lock_writes_manager_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: :pods do +RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: :cell do let(:connection) { ApplicationRecord.connection } let(:test_table) { '_test_table' } let(:logger) { instance_double(Logger) } @@ -122,6 +122,13 @@ RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: : } end + it 'returns result hash with action skipped' do + subject.lock_writes + + expect(subject.lock_writes).to eq({ action: "skipped", database: "main", dry_run: false, +table: test_table }) + end + context 'when running in dry_run mode' do let(:dry_run) { true } @@ -146,6 +153,11 @@ RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: : connection.execute("truncate #{test_table}") end.not_to raise_error end + + it 'returns result hash with action locked' do + expect(subject.lock_writes).to eq({ action: "locked", database: "main", dry_run: dry_run, +table: test_table }) + end end end @@ -186,6 +198,11 @@ RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: : subject.unlock_writes end + it 'returns result hash with action unlocked' do + expect(subject.unlock_writes).to eq({ action: "unlocked", database: "main", dry_run: dry_run, +table: test_table }) + end + context 'when running in dry_run mode' do let(:dry_run) { true } @@ -206,6 +223,11 @@ RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: : connection.execute("delete from #{test_table}") end.to raise_error(ActiveRecord::StatementInvalid, /Table: "#{test_table}" is write protected/) end + + it 'returns result hash with dry_run true' do + expect(subject.unlock_writes).to eq({ action: "unlocked", database: "main", dry_run: dry_run, +table: test_table }) + end end end diff --git a/spec/lib/gitlab/database/loose_foreign_keys_spec.rb b/spec/lib/gitlab/database/loose_foreign_keys_spec.rb index 3c2d9ca82f2..552df64096a 100644 --- a/spec/lib/gitlab/database/loose_foreign_keys_spec.rb +++ b/spec/lib/gitlab/database/loose_foreign_keys_spec.rb @@ -85,31 +85,40 @@ RSpec.describe Gitlab::Database::LooseForeignKeys do end end - describe '.definitions' do - subject(:definitions) { described_class.definitions } - - it 'contains at least all parent tables that have triggers' do - all_definition_parent_tables = definitions.map { |d| d.to_table }.to_set + context 'all tables have correct triggers installed' do + let(:all_tables_from_yaml) { described_class.definitions.pluck(:to_table).uniq } + let(:all_tables_with_triggers) do triggers_query = <<~SQL - SELECT event_object_table, trigger_name - FROM information_schema.triggers + SELECT event_object_table FROM information_schema.triggers WHERE trigger_name LIKE '%_loose_fk_trigger' - GROUP BY event_object_table, trigger_name SQL - all_triggers = ApplicationRecord.connection.execute(triggers_query) - - all_triggers.each do |trigger| - table = trigger['event_object_table'] - trigger_name = trigger['trigger_name'] - error_message = <<~END - Missing a loose foreign key definition for parent table: #{table} with trigger: #{trigger_name}. - Loose foreign key definitions must be added before triggers are added and triggers must be removed before removing the loose foreign key definition. - Read more at https://docs.gitlab.com/ee/development/database/loose_foreign_keys.html ." - END - expect(all_definition_parent_tables).to include(table), error_message - end + ApplicationRecord.connection.execute(triggers_query) + .pluck('event_object_table').uniq + end + + it 'all YAML tables do have `track_record_deletions` installed' do + missing_trigger_tables = all_tables_from_yaml - all_tables_with_triggers + + expect(missing_trigger_tables).to be_empty, <<~END + The loose foreign keys definitions require using `track_record_deletions` + for the following tables: #{missing_trigger_tables}. + Read more at https://docs.gitlab.com/ee/development/database/loose_foreign_keys.html." + END + end + + it 'no extra tables have `track_record_deletions` installed' do + extra_trigger_tables = all_tables_with_triggers - all_tables_from_yaml + + pending 'This result of this test is informatory, and not critical' if extra_trigger_tables.any? + + expect(extra_trigger_tables).to be_empty, <<~END + The following tables have unused `track_record_deletions` triggers installed, + but they are not referenced by any of the loose foreign key definitions: #{extra_trigger_tables}. + You can remove them in one of the future releases as part of `db/post_migrate`. + Read more at https://docs.gitlab.com/ee/development/database/loose_foreign_keys.html." + END end end diff --git a/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb b/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb index be9346e3829..090a9f53523 100644 --- a/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, - :reestablished_active_record_base, :delete, query_analyzers: false, feature_category: :pods do + :reestablished_active_record_base, :delete, query_analyzers: false, feature_category: :cell do using RSpec::Parameterized::TableSyntax let(:schema_class) { Class.new(Gitlab::Database::Migration[2.1]) } @@ -86,7 +86,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, let(:create_gitlab_shared_table_migration_class) { create_table_migration(gitlab_shared_table_name) } before do - skip_if_multiple_databases_are_setup(:ci) + skip_if_database_exists(:ci) end it 'does not lock any newly created tables' do @@ -106,7 +106,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when multiple databases' do before do - skip_if_multiple_databases_not_setup(:ci) + skip_if_shared_database(:ci) end let(:migration_class) { create_table_migration(table_name, skip_automatic_lock_on_writes) } @@ -238,7 +238,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when renaming a table' do before do - skip_if_multiple_databases_not_setup(:ci) + skip_if_shared_database(:ci) create_table_migration(old_table_name).migrate(:up) # create the table first before renaming it end @@ -277,7 +277,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, let(:config_model) { Gitlab::Database.database_base_models[:main] } before do - skip_if_multiple_databases_are_setup(:ci) + skip_if_database_exists(:ci) end it 'does not lock any newly created tables' do @@ -305,7 +305,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when multiple databases' do before do - skip_if_multiple_databases_not_setup(:ci) + skip_if_shared_database(:ci) migration_class.connection.execute("CREATE TABLE #{table_name}()") migration_class.migrate(:up) end diff --git a/spec/lib/gitlab/database/migration_helpers/convert_to_bigint_spec.rb b/spec/lib/gitlab/database/migration_helpers/convert_to_bigint_spec.rb index b1971977e7c..cee5f54bd6a 100644 --- a/spec/lib/gitlab/database/migration_helpers/convert_to_bigint_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/convert_to_bigint_spec.rb @@ -7,9 +7,9 @@ RSpec.describe Gitlab::Database::MigrationHelpers::ConvertToBigint, feature_cate using RSpec::Parameterized::TableSyntax where(:dot_com, :dev_or_test, :jh, :expectation) do - true | true | true | false + true | true | true | true true | false | true | false - false | true | true | false + false | true | true | true false | false | true | false true | true | false | true true | false | false | true diff --git a/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb index 25fc676d09e..2b58cdff931 100644 --- a/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb @@ -7,20 +7,22 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do ActiveRecord::Migration.new.extend(described_class) end + let_it_be(:table_name) { :_test_loose_fk_test_table } + let(:model) do Class.new(ApplicationRecord) do - self.table_name = '_test_loose_fk_test_table' + self.table_name = :_test_loose_fk_test_table end end before(:all) do - migration.create_table :_test_loose_fk_test_table do |t| + migration.create_table table_name do |t| t.timestamps end end after(:all) do - migration.drop_table :_test_loose_fk_test_table + migration.drop_table table_name end before do @@ -33,11 +35,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do expect(LooseForeignKeys::DeletedRecord.count).to eq(0) end + + it { expect(migration.has_loose_foreign_key?(table_name)).to be_falsy } end context 'when the record deletion tracker trigger is installed' do before do - migration.track_record_deletions(:_test_loose_fk_test_table) + migration.track_record_deletions(table_name) end it 'stores the record deletion' do @@ -55,7 +59,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do .first expect(deleted_record.primary_key_value).to eq(record_to_be_deleted.id) - expect(deleted_record.fully_qualified_table_name).to eq('public._test_loose_fk_test_table') + expect(deleted_record.fully_qualified_table_name).to eq("public.#{table_name}") expect(deleted_record.partition_number).to eq(1) end @@ -64,5 +68,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do expect(LooseForeignKeys::DeletedRecord.count).to eq(3) end + + it { expect(migration.has_loose_foreign_key?(table_name)).to be_truthy } 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 714fbab5aff..faf0447c054 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 @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_analyzers: false, - stub_feature_flags: false, feature_category: :pods do + stub_feature_flags: false, feature_category: :cell do let(:schema_class) { Class.new(Gitlab::Database::Migration[1.0]).include(described_class) } # We keep only the GitlabSchemasValidateConnection analyzer running @@ -506,7 +506,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a def down; end end, query_matcher: /FROM ci_builds/, - setup: -> (_) { skip_if_multiple_databases_not_setup }, + setup: -> (_) { skip_if_shared_database(:ci) }, expected: { no_gitlab_schema: { main: :cross_schema_error, diff --git a/spec/lib/gitlab/database/migration_helpers/wraparound_vacuum_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers/wraparound_vacuum_helpers_spec.rb new file mode 100644 index 00000000000..eb67e81f677 --- /dev/null +++ b/spec/lib/gitlab/database/migration_helpers/wraparound_vacuum_helpers_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::MigrationHelpers::WraparoundVacuumHelpers, feature_category: :database do + include Database::DatabaseHelpers + + let(:table_name) { 'ci_builds' } + + describe '#check_if_wraparound_in_progress' do + let(:migration) do + ActiveRecord::Migration.new.extend(described_class) + end + + subject { migration.check_if_wraparound_in_progress(table_name) } + + it 'delegates to the wraparound class' do + expect(described_class::WraparoundCheck) + .to receive(:new) + .with(table_name, migration: migration) + .and_call_original + + expect { subject }.not_to raise_error + end + end + + describe described_class::WraparoundCheck do + let(:migration) do + ActiveRecord::Migration.new.extend(Gitlab::Database::MigrationHelpers::WraparoundVacuumHelpers) + end + + describe '#execute' do + subject do + described_class.new(table_name, migration: migration).execute + end + + context 'with wraparound vacuuum running' do + before do + swapout_view_for_table(:pg_stat_activity, connection: migration.connection) + + migration.connection.execute(<<~SQL.squish) + INSERT INTO pg_stat_activity ( + datid, datname, pid, backend_start, xact_start, query_start, + state_change, wait_event_type, wait_event, state, backend_xmin, + query, backend_type) + VALUES ( + 16401, 'gitlabhq_dblab', 178, '2023-03-30 08:10:50.851322+00', + '2023-03-30 08:10:50.890485+00', now() - '150 minutes'::interval, + '2023-03-30 08:10:50.890485+00', 'IO', 'DataFileRead', 'active','3214790381'::xid, + 'autovacuum: VACUUM public.ci_builds (to prevent wraparound)', 'autovacuum worker') + SQL + end + + it 'outputs a message related to autovacuum' do + expect { subject } + .to output(/Autovacuum with wraparound prevention mode is running on `ci_builds`/).to_stdout + end + + it { expect { subject }.to output(/autovacuum: VACUUM public.ci_builds \(to prevent wraparound\)/).to_stdout } + it { expect { subject }.to output(/Current duration: 2 hours, 30 minutes/).to_stdout } + it { expect { subject }.to output(/Process id: 178/).to_stdout } + it { expect { subject }.to output(/`select pg_cancel_backend\(178\);`/).to_stdout } + + context 'when GITLAB_MIGRATIONS_DISABLE_WRAPAROUND_CHECK is set' do + before do + stub_env('GITLAB_MIGRATIONS_DISABLE_WRAPAROUND_CHECK' => 'true') + end + + it { expect { subject }.not_to output(/autovacuum/i).to_stdout } + + it 'is disabled on .com' do + expect(Gitlab).to receive(:com?).and_return(true) + + expect { subject }.not_to raise_error + end + end + + context 'when executed by self-managed' do + before do + allow(Gitlab).to receive(:com?).and_return(false) + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + end + + it { expect { subject }.not_to output(/autovacuum/i).to_stdout } + end + end + + context 'with wraparound vacuuum not running' do + it { expect { subject }.not_to output(/autovacuum/i).to_stdout } + end + + context 'when the table does not exist' do + let(:table_name) { :no_table } + + it { expect { subject }.to raise_error described_class::WraparoundError, /no_table/ } + end + end + end +end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 3f6528558b1..a3eab560c67 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -14,6 +14,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers, feature_category: :database d allow(model).to receive(:puts) end + it { expect(model.singleton_class.ancestors).to include(described_class::WraparoundVacuumHelpers) } + describe 'overridden dynamic model helpers' do let(:test_table) { '_test_batching_table' } @@ -120,157 +122,6 @@ RSpec.describe Gitlab::Database::MigrationHelpers, feature_category: :database d end end - describe '#create_table_with_constraints' do - let(:table_name) { :test_table } - let(:column_attributes) do - [ - { name: 'id', sql_type: 'bigint', null: false, default: nil }, - { name: 'created_at', sql_type: 'timestamp with time zone', null: false, default: nil }, - { name: 'updated_at', sql_type: 'timestamp with time zone', null: false, default: nil }, - { name: 'some_id', sql_type: 'integer', null: false, default: nil }, - { name: 'active', sql_type: 'boolean', null: false, default: 'true' }, - { name: 'name', sql_type: 'text', null: true, default: nil } - ] - end - - before do - allow(model).to receive(:transaction_open?).and_return(true) - end - - context 'when no check constraints are defined' do - it 'creates the table as expected' do - model.create_table_with_constraints table_name do |t| - t.timestamps_with_timezone - t.integer :some_id, null: false - t.boolean :active, null: false, default: true - t.text :name - end - - expect_table_columns_to_match(column_attributes, table_name) - end - end - - context 'when check constraints are defined' do - context 'when the text_limit is explicity named' do - it 'creates the table as expected' do - model.create_table_with_constraints table_name do |t| - t.timestamps_with_timezone - t.integer :some_id, null: false - t.boolean :active, null: false, default: true - t.text :name - - t.text_limit :name, 255, name: 'check_name_length' - t.check_constraint :some_id_is_positive, 'some_id > 0' - end - - expect_table_columns_to_match(column_attributes, table_name) - - expect_check_constraint(table_name, 'check_name_length', 'char_length(name) <= 255') - expect_check_constraint(table_name, 'some_id_is_positive', 'some_id > 0') - end - end - - context 'when the text_limit is not named' do - it 'creates the table as expected, naming the text limit' do - model.create_table_with_constraints table_name do |t| - t.timestamps_with_timezone - t.integer :some_id, null: false - t.boolean :active, null: false, default: true - t.text :name - - t.text_limit :name, 255 - t.check_constraint :some_id_is_positive, 'some_id > 0' - end - - expect_table_columns_to_match(column_attributes, table_name) - - expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 255') - expect_check_constraint(table_name, 'some_id_is_positive', 'some_id > 0') - end - end - - it 'runs the change within a with_lock_retries' do - expect(model).to receive(:with_lock_retries).ordered.and_yield - expect(model).to receive(:create_table).ordered.and_call_original - expect(model).to receive(:execute).with(<<~SQL).ordered - ALTER TABLE "#{table_name}"\nADD CONSTRAINT "check_cda6f69506" CHECK (char_length("name") <= 255) - SQL - - model.create_table_with_constraints table_name do |t| - t.text :name - t.text_limit :name, 255 - end - end - - context 'when with_lock_retries re-runs the block' do - it 'only creates constraint for unique definitions' do - expected_sql = <<~SQL - ALTER TABLE "#{table_name}"\nADD CONSTRAINT "check_cda6f69506" CHECK (char_length("name") <= 255) - SQL - - expect(model).to receive(:create_table).twice.and_call_original - - expect(model).to receive(:execute).with(expected_sql).and_raise(ActiveRecord::LockWaitTimeout) - expect(model).to receive(:execute).with(expected_sql).and_call_original - - model.create_table_with_constraints table_name do |t| - t.timestamps_with_timezone - t.integer :some_id, null: false - t.boolean :active, null: false, default: true - t.text :name - - t.text_limit :name, 255 - end - - expect_table_columns_to_match(column_attributes, table_name) - - expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 255') - end - end - - context 'when constraints are given invalid names' do - let(:expected_max_length) { described_class::MAX_IDENTIFIER_NAME_LENGTH } - let(:expected_error_message) { "The maximum allowed constraint name is #{expected_max_length} characters" } - - context 'when the explicit text limit name is not valid' do - it 'raises an error' do - too_long_length = expected_max_length + 1 - - expect do - model.create_table_with_constraints table_name do |t| - t.timestamps_with_timezone - t.integer :some_id, null: false - t.boolean :active, null: false, default: true - t.text :name - - t.text_limit :name, 255, name: ('a' * too_long_length) - t.check_constraint :some_id_is_positive, 'some_id > 0' - end - end.to raise_error(expected_error_message) - end - end - - context 'when a check constraint name is not valid' do - it 'raises an error' do - too_long_length = expected_max_length + 1 - - expect do - model.create_table_with_constraints table_name do |t| - t.timestamps_with_timezone - t.integer :some_id, null: false - t.boolean :active, null: false, default: true - t.text :name - - t.text_limit :name, 255 - t.check_constraint ('a' * too_long_length), 'some_id > 0' - end - end.to raise_error(expected_error_message) - end - end - end - end - end - describe '#add_concurrent_index' do context 'outside a transaction' do before do @@ -1199,6 +1050,38 @@ RSpec.describe Gitlab::Database::MigrationHelpers, feature_category: :database d it_behaves_like 'foreign key checks' end + context 'if the schema cache does not include the constrained_columns column' do + let(:target_table) { nil } + + around do |ex| + model.transaction do + require_migration!('add_columns_to_postgres_foreign_keys') + AddColumnsToPostgresForeignKeys.new.down + Gitlab::Database::PostgresForeignKey.reset_column_information + Gitlab::Database::PostgresForeignKey.columns_hash # Force populate the column hash in the old schema + AddColumnsToPostgresForeignKeys.new.up + + # Rolling back reverts the schema cache information, so we need to run the example here before the rollback. + ex.run + + raise ActiveRecord::Rollback + end + + # make sure that we're resetting the schema cache here so that we don't leak the change to other tests. + Gitlab::Database::PostgresForeignKey.reset_column_information + # Double-check that the column information is back to normal + expect(Gitlab::Database::PostgresForeignKey.columns_hash.keys).to include('constrained_columns') + end + + # This test verifies that the situation we're trying to set up for the shared examples is actually being + # set up correctly + it 'correctly sets up the test without the column in the columns_hash' do + expect(Gitlab::Database::PostgresForeignKey.columns_hash.keys).not_to include('constrained_columns') + end + + it_behaves_like 'foreign key checks' + end + it 'compares by target table if no column given' do expect(model.foreign_key_exists?(:projects, :other_table)).to be_falsey end diff --git a/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb b/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb new file mode 100644 index 00000000000..515f59345ee --- /dev/null +++ b/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::PgBackendPid, feature_category: :database do + describe Gitlab::Database::Migrations::PgBackendPid::MigratorPgBackendPid do + let(:klass) do + Class.new do + def with_advisory_lock_connection + yield :conn + end + end + end + + it 're-yields with same arguments and wraps it with calls to .say' do + patched_instance = klass.prepend(described_class).new + expect(Gitlab::Database::Migrations::PgBackendPid).to receive(:say).twice + + expect { |b| patched_instance.with_advisory_lock_connection(&b) }.to yield_with_args(:conn) + end + end + + describe '.patch!' do + it 'patches ActiveRecord::Migrator' do + expect(ActiveRecord::Migrator).to receive(:prepend).with(described_class::MigratorPgBackendPid) + + described_class.patch! + end + end + + describe '.say' do + it 'outputs the connection information' do + conn = ActiveRecord::Base.connection + + expect(conn).to receive(:object_id).and_return(9876) + expect(conn).to receive(:select_value).with('SELECT pg_backend_pid()').and_return(12345) + expect(Gitlab::Database).to receive(:db_config_name).with(conn).and_return('main') + + expected_output = "main: == [advisory_lock_connection] object_id: 9876, pg_backend_pid: 12345\n" + + expect { described_class.say(conn) }.to output(expected_output).to_stdout + end + end +end diff --git a/spec/lib/gitlab/database/migrations/runner_spec.rb b/spec/lib/gitlab/database/migrations/runner_spec.rb index 66eb5a5de51..7c71076e8f3 100644 --- a/spec/lib/gitlab/database/migrations/runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/runner_spec.rb @@ -65,7 +65,7 @@ RSpec.describe Gitlab::Database::Migrations::Runner, :reestablished_active_recor end before do - skip_if_multiple_databases_not_setup unless database == :main + skip_if_shared_database(database) stub_const('Gitlab::Database::Migrations::Runner::BASE_RESULT_DIR', base_result_dir) allow(ActiveRecord::Migrator).to receive(:new) do |dir, _all_migrations, _schema_migration_class, version_to_migrate| diff --git a/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb b/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb index cd3a94f5737..f4b13033270 100644 --- a/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb +++ b/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb @@ -2,11 +2,15 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition do +RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition, feature_category: :database do include Gitlab::Database::DynamicModelHelpers include Database::TableSchemaHelpers - let(:migration_context) { Gitlab::Database::Migration[2.0].new } + let(:migration_context) do + Gitlab::Database::Migration[2.0].new.tap do |migration| + migration.extend Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers + end + end let(:connection) { migration_context.connection } let(:table_name) { '_test_table_to_partition' } @@ -73,7 +77,9 @@ RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition end describe "#prepare_for_partitioning" do - subject(:prepare) { converter.prepare_for_partitioning } + subject(:prepare) { converter.prepare_for_partitioning(async: async) } + + let(:async) { false } it 'adds a check constraint' do expect { prepare }.to change { @@ -83,9 +89,100 @@ RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition .count }.from(0).to(1) end + + context 'when it fails to add constraint' do + before do + allow(migration_context).to receive(:add_check_constraint) + end + + it 'raises UnableToPartition error' do + expect { prepare } + .to raise_error(described_class::UnableToPartition) + .and change { + Gitlab::Database::PostgresConstraint + .check_constraints + .by_table_identifier(table_identifier) + .count + }.by(0) + end + end + + context 'when async' do + let(:async) { true } + + it 'adds a NOT VALID check constraint' do + expect { prepare }.to change { + Gitlab::Database::PostgresConstraint + .check_constraints + .by_table_identifier(table_identifier) + .count + }.from(0).to(1) + + constraint = + Gitlab::Database::PostgresConstraint + .check_constraints + .by_table_identifier(table_identifier) + .last + + expect(constraint.definition).to end_with('NOT VALID') + end + + it 'adds a PostgresAsyncConstraintValidation record' do + expect { prepare }.to change { + Gitlab::Database::AsyncConstraints::PostgresAsyncConstraintValidation.count + }.from(0).to(1) + + record = Gitlab::Database::AsyncConstraints::PostgresAsyncConstraintValidation.last + expect(record.name).to eq described_class::PARTITIONING_CONSTRAINT_NAME + expect(record).to be_check_constraint + end + + context 'when constraint exists but is not valid' do + before do + converter.prepare_for_partitioning(async: true) + end + + it 'validates the check constraint' do + expect { prepare }.to change { + Gitlab::Database::PostgresConstraint + .check_constraints + .by_table_identifier(table_identifier).first.constraint_valid? + }.from(false).to(true) + end + + context 'when it fails to validate constraint' do + before do + allow(migration_context).to receive(:validate_check_constraint) + end + + it 'raises UnableToPartition error' do + expect { prepare } + .to raise_error(described_class::UnableToPartition, + starting_with('Error validating partitioning constraint')) + .and change { + Gitlab::Database::PostgresConstraint + .check_constraints + .by_table_identifier(table_identifier) + .count + }.by(0) + end + end + end + + context 'when constraint exists and is valid' do + before do + converter.prepare_for_partitioning(async: false) + end + + it 'raises UnableToPartition error' do + expect(Gitlab::AppLogger).to receive(:info).with(starting_with('Nothing to do')) + prepare + end + end + end end - describe '#revert_prepare_for_partitioning' do + describe '#revert_preparation_for_partitioning' do before do converter.prepare_for_partitioning end @@ -102,11 +199,13 @@ RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition end end - describe "#convert_to_zero_partition" do + describe "#partition" do subject(:partition) { converter.partition } + let(:async) { false } + before do - converter.prepare_for_partitioning + converter.prepare_for_partitioning(async: async) end context 'when the primary key is incorrect' do @@ -130,7 +229,15 @@ RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition end it 'throws a reasonable error message' do - expect { partition }.to raise_error(described_class::UnableToPartition, /constraint /) + expect { partition }.to raise_error(described_class::UnableToPartition, /is not ready for partitioning./) + end + end + + context 'when supporting check constraint is not valid' do + let(:async) { true } + + it 'throws a reasonable error message' do + expect { partition }.to raise_error(described_class::UnableToPartition, /is not ready for partitioning./) end end @@ -203,7 +310,7 @@ RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition proc do allow(migration_context.connection).to receive(:add_foreign_key).and_call_original expect(migration_context.connection).to receive(:add_foreign_key).with(from_table, to_table, any_args) - .and_wrap_original(&fail_first_time) + .and_wrap_original(&fail_first_time) end end @@ -231,9 +338,24 @@ RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition end end end + + context 'when table has LFK triggers' do + before do + migration_context.track_record_deletions(table_name) + end + + it 'moves the trigger on the parent table', :aggregate_failures do + expect(migration_context.has_loose_foreign_key?(table_name)).to be_truthy + + expect { partition }.not_to raise_error + + expect(migration_context.has_loose_foreign_key?(table_name)).to be_truthy + expect(migration_context.has_loose_foreign_key?(parent_table_name)).to be_truthy + end + end end - describe '#revert_conversion_to_zero_partition' do + describe '#revert_partitioning' do before do converter.prepare_for_partitioning converter.partition @@ -269,5 +391,21 @@ RSpec.describe Gitlab::Database::Partitioning::ConvertTableToFirstListPartition expect { revert_conversion }.to change { converter.send(:sequences_owned_by, table_name).count }.from(0) .and change { converter.send(:sequences_owned_by, parent_table_name).count }.to(0) end + + context 'when table has LFK triggers' do + before do + migration_context.track_record_deletions(parent_table_name) + migration_context.track_record_deletions(table_name) + end + + it 'restores the trigger on the partition', :aggregate_failures do + expect(migration_context.has_loose_foreign_key?(table_name)).to be_truthy + expect(migration_context.has_loose_foreign_key?(parent_table_name)).to be_truthy + + expect { revert_conversion }.not_to raise_error + + expect(migration_context.has_loose_foreign_key?(table_name)).to be_truthy + end + end end end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb index 1885e84ac4c..fc279051800 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb @@ -54,6 +54,11 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::BackfillPartition allow(backfill_job).to receive(:sleep) end + after do + connection.drop_table source_table + connection.drop_table destination_table + end + let(:source_model) { Class.new(ActiveRecord::Base) } let(:destination_model) { Class.new(ActiveRecord::Base) } let(:timestamp) { Time.utc(2020, 1, 2).round } @@ -82,7 +87,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::BackfillPartition end it 'breaks the assigned batch into smaller batches' do - expect_next_instance_of(described_class::BulkCopy) do |bulk_copy| + expect_next_instance_of(Gitlab::Database::PartitioningMigrationHelpers::BulkCopy) do |bulk_copy| expect(bulk_copy).to receive(:copy_between).with(source1.id, source2.id) expect(bulk_copy).to receive(:copy_between).with(source3.id, source3.id) end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb index e76b1da3834..d87ef7a0953 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb @@ -2,10 +2,11 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers do +RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers, feature_category: :database do include Database::PartitioningHelpers include Database::TriggerHelpers include Database::TableSchemaHelpers + include MigrationsHelpers let(:migration) do ActiveRecord::Migration.new.extend(described_class) @@ -98,7 +99,8 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe migration.prepare_constraint_for_list_partitioning(table_name: source_table, partitioning_column: partition_column, parent_table_name: partitioned_table, - initial_partitioning_value: min_date) + initial_partitioning_value: min_date, + async: false) end end end @@ -484,17 +486,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end context 'when records exist in the source table' do - let(:migration_class) { '::Gitlab::Database::PartitioningMigrationHelpers::BackfillPartitionedTable' } + let(:migration_class) { described_class::MIGRATION } let(:sub_batch_size) { described_class::SUB_BATCH_SIZE } - let(:pause_seconds) { described_class::PAUSE_SECONDS } let!(:first_id) { source_model.create!(name: 'Bob', age: 20).id } let!(:second_id) { source_model.create!(name: 'Alice', age: 30).id } let!(:third_id) { source_model.create!(name: 'Sam', age: 40).id } before do stub_const("#{described_class.name}::BATCH_SIZE", 2) - - expect(migration).to receive(:queue_background_migration_jobs_by_range_at_intervals).and_call_original + stub_const("#{described_class.name}::SUB_BATCH_SIZE", 1) end it 'enqueues jobs to copy each batch of data' do @@ -503,13 +503,13 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe Sidekiq::Testing.fake! do migration.enqueue_partitioning_data_migration source_table - expect(BackgroundMigrationWorker.jobs.size).to eq(2) - - first_job_arguments = [first_id, second_id, source_table.to_s, partitioned_table, 'id'] - expect(BackgroundMigrationWorker.jobs[0]['args']).to eq([migration_class, first_job_arguments]) - - second_job_arguments = [third_id, third_id, source_table.to_s, partitioned_table, 'id'] - expect(BackgroundMigrationWorker.jobs[1]['args']).to eq([migration_class, second_job_arguments]) + expect(migration_class).to have_scheduled_batched_migration( + table_name: source_table, + column_name: :id, + job_arguments: [partitioned_table], + batch_size: described_class::BATCH_SIZE, + sub_batch_size: described_class::SUB_BATCH_SIZE + ) end end end diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb index 4c0fde46b2f..4aa9d5f6df0 100644 --- a/spec/lib/gitlab/database/partitioning_spec.rb +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -2,11 +2,11 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Partitioning do +RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do include Database::PartitioningHelpers include Database::TableSchemaHelpers - let(:connection) { ApplicationRecord.connection } + let(:main_connection) { ApplicationRecord.connection } around do |example| previously_registered_models = described_class.registered_models.dup @@ -84,7 +84,7 @@ RSpec.describe Gitlab::Database::Partitioning do before do table_names.each do |table_name| - connection.execute(<<~SQL) + execute_on_each_database(<<~SQL) CREATE TABLE #{table_name} ( id serial not null, created_at timestamptz not null, @@ -101,32 +101,12 @@ RSpec.describe Gitlab::Database::Partitioning do end context 'with multiple databases' do - before do - table_names.each do |table_name| - ci_connection.execute("DROP TABLE IF EXISTS #{table_name}") - - ci_connection.execute(<<~SQL) - CREATE TABLE #{table_name} ( - id serial not null, - created_at timestamptz not null, - PRIMARY KEY (id, created_at)) - PARTITION BY RANGE (created_at); - SQL - end - end - - after do - table_names.each do |table_name| - ci_connection.execute("DROP TABLE IF EXISTS #{table_name}") - end - end - it 'creates partitions in each database' do - skip_if_multiple_databases_not_setup(:ci) + skip_if_shared_database(:ci) expect { described_class.sync_partitions(models) } - .to change { find_partitions(table_names.first, conn: connection).size }.from(0) - .and change { find_partitions(table_names.last, conn: connection).size }.from(0) + .to change { find_partitions(table_names.first, conn: main_connection).size }.from(0) + .and change { find_partitions(table_names.last, conn: main_connection).size }.from(0) .and change { find_partitions(table_names.first, conn: ci_connection).size }.from(0) .and change { find_partitions(table_names.last, conn: ci_connection).size }.from(0) end @@ -161,10 +141,12 @@ RSpec.describe Gitlab::Database::Partitioning do end before do + skip_if_shared_database(:ci) + (table_names + ['partitioning_test3']).each do |table_name| - ci_connection.execute("DROP TABLE IF EXISTS #{table_name}") + execute_on_each_database("DROP TABLE IF EXISTS #{table_name}") - ci_connection.execute(<<~SQL) + execute_on_each_database(<<~SQL) CREATE TABLE #{table_name} ( id serial not null, created_at timestamptz not null, @@ -181,14 +163,12 @@ RSpec.describe Gitlab::Database::Partitioning do end it 'manages partitions for models for the given database', :aggregate_failures do - skip_if_multiple_databases_not_setup(:ci) - expect { described_class.sync_partitions([models.first, ci_model], only_on: 'ci') } .to change { find_partitions(ci_model.table_name, conn: ci_connection).size }.from(0) - expect(find_partitions(models.first.table_name).size).to eq(0) + expect(find_partitions(models.first.table_name, conn: main_connection).size).to eq(0) expect(find_partitions(models.first.table_name, conn: ci_connection).size).to eq(0) - expect(find_partitions(ci_model.table_name).size).to eq(0) + expect(find_partitions(ci_model.table_name, conn: main_connection).size).to eq(0) end end end diff --git a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb index c128c56c708..03343c134ae 100644 --- a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb +++ b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb @@ -203,7 +203,7 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ end end - context 'when supporting foreign keys to inherited tables' do + context 'when supporting foreign keys on partitioned tables' do before do ApplicationRecord.connection.execute(<<~SQL) create table #{schema_table_name('parent')} ( @@ -246,6 +246,40 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ end end + context 'with two tables both partitioned' do + before do + ApplicationRecord.connection.execute(<<~SQL) + create table #{table_name('parent')} ( + id bigserial primary key not null + ) partition by hash(id); + + create table #{table_name('child')} + partition of #{table_name('parent')} for values with (remainder 1, modulus 2); + + create table #{table_name('ref_parent')} ( + id bigserial primary key not null + ) partition by hash(id); + + create table #{table_name('ref_child_1')} + partition of #{table_name('ref_parent')} for values with (remainder 1, modulus 3); + + create table #{table_name('ref_child_2')} + partition of #{table_name('ref_parent')} for values with (remainder 2, modulus 3); + + alter table #{table_name('parent')} add constraint fk foreign key (id) references #{table_name('ref_parent')} (id); + SQL + end + + describe '#child_foreign_keys' do + it 'is the child foreign keys of the partitioned parent fk' do + fk = described_class.by_constrained_table_name(table_name('parent')).first + children = fk.child_foreign_keys + expect(children.count).to eq(1) + expect(children.first.constrained_table_name).to eq(table_name('child')) + end + end + end + def schema_table_name(name) "public.#{table_name(name)}" 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 index d31be6cb883..ed05d1ce169 100644 --- 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 @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection, query_analyzers: false, - feature_category: :pods do + feature_category: :cell do let(:analyzer) { described_class } # We keep only the GitlabSchemasValidateConnection analyzer running @@ -28,19 +28,19 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection 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(:ci) } + setup: -> (_) { skip_if_shared_database(:ci) } }, "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(:ci) } + setup: -> (_) { skip_if_shared_database(:ci) } }, "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(:ci) } + setup: -> (_) { skip_if_shared_database(:ci) } }, "for query accessing CI database" => { model: Ci::ApplicationRecord, @@ -51,13 +51,13 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection model: ::ApplicationRecord, sql: "SELECT 1 FROM ci_builds", expect_error: /The query tried to access \["ci_builds"\]/, - setup: -> (_) { skip_if_multiple_databases_not_setup(:ci) } + setup: -> (_) { skip_if_shared_database(:ci) } }, "for query accessing unknown gitlab_schema" => { model: ::ApplicationRecord, sql: "SELECT 1 FROM new_table", expect_error: /The query tried to access \["new_table"\] \(of undefined_new_table\)/, - setup: -> (_) { skip_if_multiple_databases_not_setup(:ci) } + setup: -> (_) { skip_if_shared_database(:ci) } } } end @@ -77,7 +77,7 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection context "when analyzer is enabled for tests", :query_analyzers do before do - skip_if_multiple_databases_not_setup(:ci) + skip_if_shared_database(:ci) end it "throws an error when trying to access a table that belongs to the gitlab_main schema from the ci database" do diff --git a/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb b/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb index a4322689bf9..887dd7c9838 100644 --- a/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification, query_analyzers: false, - feature_category: :pods do + feature_category: :cell do let_it_be(:pipeline, refind: true) { create(:ci_pipeline) } let_it_be(:project, refind: true) { create(:project) } @@ -118,6 +118,18 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio end end + context 'when ci_pipelines are ignored for cross modification' do + it 'does not raise error' do + Project.transaction do + expect do + described_class.temporary_ignore_tables_in_transaction(%w[ci_pipelines], url: 'TODO') do + run_queries + end + end.not_to raise_error + end + end + end + context 'when data modification happens in nested transactions' do it 'raises error' do Project.transaction(requires_new: true) do diff --git a/spec/lib/gitlab/database/schema_validation/adapters/column_database_adapter_spec.rb b/spec/lib/gitlab/database/schema_validation/adapters/column_database_adapter_spec.rb new file mode 100644 index 00000000000..13c4bc0b054 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/adapters/column_database_adapter_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Adapters::ColumnDatabaseAdapter, feature_category: :database do + subject(:adapter) { described_class.new(db_result) } + + let(:column_name) { 'email' } + let(:column_default) { "'no-reply@gitlab.com'::character varying" } + let(:not_null) { true } + let(:db_result) do + { + 'table_name' => 'projects', + 'column_name' => column_name, + 'data_type' => 'character varying', + 'column_default' => column_default, + 'not_null' => not_null + } + end + + describe '#name' do + it { expect(adapter.name).to eq('email') } + end + + describe '#table_name' do + it { expect(adapter.table_name).to eq('projects') } + end + + describe '#data_type' do + it { expect(adapter.data_type).to eq('character varying') } + end + + describe '#default' do + context "when there's no default value in the column" do + let(:column_default) { nil } + + it { expect(adapter.default).to be_nil } + end + + context 'when the column name is id' do + let(:column_name) { 'id' } + + it { expect(adapter.default).to be_nil } + end + + context 'when the column default includes nextval' do + let(:column_default) { "nextval('my_seq'::regclass)" } + + it { expect(adapter.default).to be_nil } + end + + it { expect(adapter.default).to eq("DEFAULT 'no-reply@gitlab.com'::character varying") } + end + + describe '#nullable' do + context 'when column is not null' do + it { expect(adapter.nullable).to eq('NOT NULL') } + end + + context 'when column is nullable' do + let(:not_null) { false } + + it { expect(adapter.nullable).to be_nil } + end + end +end diff --git a/spec/lib/gitlab/database/schema_validation/adapters/column_structure_sql_adapter_spec.rb b/spec/lib/gitlab/database/schema_validation/adapters/column_structure_sql_adapter_spec.rb new file mode 100644 index 00000000000..d7e5c6e896e --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/adapters/column_structure_sql_adapter_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Adapters::ColumnStructureSqlAdapter, feature_category: :database do + subject(:adapter) { described_class.new(table_name, column_def) } + + let(:table_name) { 'my_table' } + let(:file_path) { Rails.root.join('spec/fixtures/structure.sql') } + let(:table_stmts) { PgQuery.parse(File.read(file_path)).tree.stmts.filter_map { |s| s.stmt.create_stmt } } + let(:column_stmts) { table_stmts.find { |table| table.relation.relname == 'test_table' }.table_elts } + let(:column_def) { column_stmts.find { |col| col.column_def.colname == column_name }.column_def } + + where(:column_name, :data_type, :default_value, :nullable) do + [ + ['id', 'bigint', nil, 'NOT NULL'], + ['integer_column', 'integer', nil, nil], + ['integer_with_default_column', 'integer', 'DEFAULT 1', nil], + ['smallint_with_default_column', 'smallint', 'DEFAULT 0', 'NOT NULL'], + ['double_precision_with_default_column', 'double precision', 'DEFAULT 1.0', nil], + ['numeric_with_default_column', 'numeric', 'DEFAULT 1.0', 'NOT NULL'], + ['boolean_with_default_colum', 'boolean', 'DEFAULT true', 'NOT NULL'], + ['varying_with_default_column', 'character varying', "DEFAULT 'DEFAULT'::character varying", 'NOT NULL'], + ['varying_with_limit_and_default_column', 'character varying(255)', "DEFAULT 'DEFAULT'::character varying", nil], + ['text_with_default_column', 'text', "DEFAULT ''::text", 'NOT NULL'], + ['array_with_default_column', 'character varying(255)[]', "DEFAULT '{one,two}'::character varying[]", 'NOT NULL'], + ['jsonb_with_default_column', 'jsonb', "DEFAULT '[]'::jsonb", 'NOT NULL'], + ['timestamptz_with_default_column', 'timestamp(6) with time zone', "DEFAULT now()", nil], + ['timestamp_with_default_column', 'timestamp(6) without time zone', + "DEFAULT '2022-01-23 00:00:00+00'::timestamp without time zone", 'NOT NULL'], + ['date_with_default_column', 'date', 'DEFAULT 2023-04-05', nil], + ['inet_with_default_column', 'inet', "DEFAULT '0.0.0.0'::inet", 'NOT NULL'], + ['macaddr_with_default_column', 'macaddr', "DEFAULT '00-00-00-00-00-000'::macaddr", 'NOT NULL'], + ['uuid_with_default_column', 'uuid', "DEFAULT '00000000-0000-0000-0000-000000000000'::uuid", 'NOT NULL'], + ['bytea_with_default_column', 'bytea', "DEFAULT '\\xDEADBEEF'::bytea", nil] + ] + end + + with_them do + describe '#name' do + it { expect(adapter.name).to eq(column_name) } + end + + describe '#table_name' do + it { expect(adapter.table_name).to eq(table_name) } + end + + describe '#data_type' do + it { expect(adapter.data_type).to eq(data_type) } + end + + describe '#nullable' do + it { expect(adapter.nullable).to eq(nullable) } + end + + describe '#default' do + it { expect(adapter.default).to eq(default_value) } + end + end + + context 'when the data type is not mapped' do + let(:column_name) { 'unmapped_column_type' } + let(:error_class) { Gitlab::Database::SchemaValidation::Adapters::UndefinedPGType } + + describe '#data_type' do + it { expect { adapter.data_type }.to raise_error(error_class) } + end + end +end diff --git a/spec/lib/gitlab/database/schema_validation/database_spec.rb b/spec/lib/gitlab/database/schema_validation/database_spec.rb index eadaf683a29..8fd98382625 100644 --- a/spec/lib/gitlab/database/schema_validation/database_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/database_spec.rb @@ -2,109 +2,90 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::SchemaValidation::Database, feature_category: :database do +RSpec.shared_examples 'database schema assertions for' do |fetch_by_name_method, exists_method, all_objects_method| subject(:database) { described_class.new(connection) } let(:database_model) { Gitlab::Database.database_base_models['main'] } let(:connection) { database_model.connection } - context 'when having indexes' do - let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index } - let(:results) do - [['index', 'CREATE UNIQUE INDEX "index" ON public.achievements USING btree (namespace_id, lower(name))']] - end + before do + allow(connection).to receive(:select_rows).and_return(results) + allow(connection).to receive(:exec_query).and_return(results) + end - before do - allow(connection).to receive(:select_rows).and_return(results) + describe "##{fetch_by_name_method}" do + it 'returns nil when schema object does not exists' do + expect(database.public_send(fetch_by_name_method, 'invalid-object-name')).to be_nil end - describe '#fetch_index_by_name' do - context 'when index does not exist' do - it 'returns nil' do - index = database.fetch_index_by_name('non_existing_index') - - expect(index).to be_nil - end - end - - it 'returns index by name' do - index = database.fetch_index_by_name('index') - - expect(index.name).to eq('index') - end + it 'returns the schema object by name' do + expect(database.public_send(fetch_by_name_method, valid_schema_object_name).name).to eq(valid_schema_object_name) end + end - describe '#index_exists?' do - context 'when index exists' do - it 'returns true' do - index_exists = database.index_exists?('index') + describe "##{exists_method}" do + it 'returns true when schema object exists' do + expect(database.public_send(exists_method, valid_schema_object_name)).to be_truthy + end - expect(index_exists).to be_truthy - end - end + it 'returns false when schema object does not exists' do + expect(database.public_send(exists_method, 'invalid-object')).to be_falsey + end + end - context 'when index does not exist' do - it 'returns false' do - index_exists = database.index_exists?('non_existing_index') + describe "##{all_objects_method}" do + it 'returns all the schema objects' do + schema_objects = database.public_send(all_objects_method) - expect(index_exists).to be_falsey - end - end + expect(schema_objects).to all(be_a(schema_object)) + expect(schema_objects.map(&:name)).to eq([valid_schema_object_name]) end + end +end - describe '#indexes' do - it 'returns indexes' do - indexes = database.indexes - - expect(indexes).to all(be_a(schema_object)) - expect(indexes.map(&:name)).to eq(['index']) - end +RSpec.describe Gitlab::Database::SchemaValidation::Database, feature_category: :database do + context 'when having indexes' do + let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index } + let(:valid_schema_object_name) { 'index' } + let(:results) do + [['index', 'CREATE UNIQUE INDEX "index" ON public.achievements USING btree (namespace_id, lower(name))']] end + + include_examples 'database schema assertions for', 'fetch_index_by_name', 'index_exists?', 'indexes' end context 'when having triggers' do let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Trigger } + let(:valid_schema_object_name) { 'my_trigger' } let(:results) do - { 'my_trigger' => 'CREATE TRIGGER my_trigger BEFORE INSERT ON todos FOR EACH ROW EXECUTE FUNCTION trigger()' } + [['my_trigger', 'CREATE TRIGGER my_trigger BEFORE INSERT ON todos FOR EACH ROW EXECUTE FUNCTION trigger()']] end - before do - allow(database).to receive(:fetch_triggers).and_return(results) - end - - describe '#fetch_trigger_by_name' do - context 'when trigger does not exist' do - it 'returns nil' do - expect(database.fetch_trigger_by_name('non_existing_trigger')).to be_nil - end - end - - it 'returns trigger by name' do - expect(database.fetch_trigger_by_name('my_trigger').name).to eq('my_trigger') - end - end + include_examples 'database schema assertions for', 'fetch_trigger_by_name', 'trigger_exists?', 'triggers' + end - describe '#trigger_exists?' do - context 'when trigger exists' do - it 'returns true' do - expect(database.trigger_exists?('my_trigger')).to be_truthy - end - end - - context 'when trigger does not exist' do - it 'returns false' do - expect(database.trigger_exists?('non_existing_trigger')).to be_falsey - end - end + context 'when having tables' do + let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Table } + let(:valid_schema_object_name) { 'my_table' } + let(:results) do + [ + { + 'table_name' => 'my_table', + 'column_name' => 'id', + 'not_null' => true, + 'data_type' => 'bigint', + 'column_default' => "nextval('audit_events_id_seq'::regclass)" + }, + { + 'table_name' => 'my_table', + 'column_name' => 'details', + 'not_null' => false, + 'data_type' => 'text', + 'column_default' => nil + } + ] end - describe '#triggers' do - it 'returns triggers' do - triggers = database.triggers - - expect(triggers).to all(be_a(schema_object)) - expect(triggers.map(&:name)).to eq(['my_trigger']) - end - end + include_examples 'database schema assertions for', 'fetch_table_by_name', 'table_exists?', 'tables' end end diff --git a/spec/lib/gitlab/database/schema_validation/inconsistency_spec.rb b/spec/lib/gitlab/database/schema_validation/inconsistency_spec.rb new file mode 100644 index 00000000000..cb3df75b3fb --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/inconsistency_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Inconsistency, feature_category: :database do + let(:validator) { Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionIndexes } + + let(:database_statement) { 'CREATE INDEX index_name ON public.achievements USING btree (namespace_id)' } + let(:structure_sql_statement) { 'CREATE INDEX index_name ON public.achievements USING btree (id)' } + + let(:structure_stmt) { PgQuery.parse(structure_sql_statement).tree.stmts.first.stmt.index_stmt } + let(:database_stmt) { PgQuery.parse(database_statement).tree.stmts.first.stmt.index_stmt } + + let(:structure_sql_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index.new(structure_stmt) } + let(:database_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index.new(database_stmt) } + + subject(:inconsistency) { described_class.new(validator, structure_sql_object, database_object) } + + describe '#object_name' do + it 'returns the index name' do + expect(inconsistency.object_name).to eq('index_name') + end + end + + describe '#diff' do + it 'returns a diff between the structure.sql and the database' do + expect(inconsistency.diff).to be_a(Diffy::Diff) + expect(inconsistency.diff.string1).to eq("#{structure_sql_statement}\n") + expect(inconsistency.diff.string2).to eq("#{database_statement}\n") + end + end + + describe '#error_message' do + it 'returns the error message' do + stub_const "#{validator}::ERROR_MESSAGE", 'error message %s' + + expect(inconsistency.error_message).to eq('error message index_name') + end + end + + describe '#type' do + it 'returns the type of the validator' do + expect(inconsistency.type).to eq('different_definition_indexes') + end + end + + describe '#table_name' do + it 'returns the table name' do + expect(inconsistency.table_name).to eq('achievements') + end + end + + describe '#inspect' do + let(:expected_output) do + <<~MSG + ------------------------------------------------------ + The index_name index has a different statement between structure.sql and database + Diff: + \e[31m-CREATE INDEX index_name ON public.achievements USING btree (id)\e[0m + \e[32m+CREATE INDEX index_name ON public.achievements USING btree (namespace_id)\e[0m + + ------------------------------------------------------ + MSG + end + + it 'prints the inconsistency message' do + expect(inconsistency.inspect).to eql(expected_output) + end + end +end diff --git a/spec/lib/gitlab/database/schema_validation/runner_spec.rb b/spec/lib/gitlab/database/schema_validation/runner_spec.rb index ddbdedcd8b4..f5d1c6ba31b 100644 --- a/spec/lib/gitlab/database/schema_validation/runner_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/runner_spec.rb @@ -28,7 +28,7 @@ RSpec.describe Gitlab::Database::SchemaValidation::Runner, feature_category: :da subject(:inconsistencies) { described_class.new(structure_sql, database, validators: validators).execute } let(:class_name) { 'Gitlab::Database::SchemaValidation::Validators::ExtraIndexes' } - let(:inconsistency_class_name) { 'Gitlab::Database::SchemaValidation::Validators::BaseValidator::Inconsistency' } + let(:inconsistency_class_name) { 'Gitlab::Database::SchemaValidation::Inconsistency' } let(:extra_indexes) { class_double(class_name) } let(:instace_extra_index) { instance_double(class_name, execute: [inconsistency]) } diff --git a/spec/lib/gitlab/database/schema_validation/schema_inconsistency_spec.rb b/spec/lib/gitlab/database/schema_validation/schema_inconsistency_spec.rb new file mode 100644 index 00000000000..7d6a279def9 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/schema_inconsistency_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::SchemaInconsistency, type: :model, feature_category: :database do + it { is_expected.to be_a ApplicationRecord } + + describe 'associations' do + it { is_expected.to belong_to(:issue) } + end + + describe "Validations" do + it { is_expected.to validate_presence_of(:object_name) } + it { is_expected.to validate_presence_of(:valitador_name) } + it { is_expected.to validate_presence_of(:table_name) } + end +end diff --git a/spec/lib/gitlab/database/schema_validation/schema_objects/column_spec.rb b/spec/lib/gitlab/database/schema_validation/schema_objects/column_spec.rb new file mode 100644 index 00000000000..74bc5f43b50 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/schema_objects/column_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::SchemaObjects::Column, feature_category: :database do + subject(:column) { described_class.new(adapter) } + + let(:database_adapter) { 'Gitlab::Database::SchemaValidation::Adapters::ColumnDatabaseAdapter' } + let(:adapter) do + instance_double(database_adapter, name: 'id', table_name: 'projects', + data_type: 'bigint', default: nil, nullable: 'NOT NULL') + end + + describe '#name' do + it { expect(column.name).to eq('id') } + end + + describe '#table_name' do + it { expect(column.table_name).to eq('projects') } + end + + describe '#statement' do + it { expect(column.statement).to eq('id bigint NOT NULL') } + end +end diff --git a/spec/lib/gitlab/database/schema_validation/schema_objects/index_spec.rb b/spec/lib/gitlab/database/schema_validation/schema_objects/index_spec.rb index 1aaa994e3bb..43d8fa38ec8 100644 --- a/spec/lib/gitlab/database/schema_validation/schema_objects/index_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/schema_objects/index_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::SchemaValidation::SchemaObjects::Index, feature_category: :database do let(:statement) { 'CREATE INDEX index_name ON public.achievements USING btree (namespace_id)' } let(:name) { 'index_name' } + let(:table_name) { 'achievements' } include_examples 'schema objects assertions for', 'index_stmt' end diff --git a/spec/lib/gitlab/database/schema_validation/schema_objects/table_spec.rb b/spec/lib/gitlab/database/schema_validation/schema_objects/table_spec.rb new file mode 100644 index 00000000000..6c2efee056b --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/schema_objects/table_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::SchemaObjects::Table, feature_category: :database do + subject(:table) { described_class.new(name, columns) } + + let(:name) { 'my_table' } + let(:column_class) { 'Gitlab::Database::SchemaValidation::SchemaObjects::Column' } + let(:columns) do + [ + instance_double(column_class, name: 'id', statement: 'id bigint NOT NULL'), + instance_double(column_class, name: 'col', statement: 'col text') + ] + end + + describe '#name' do + it { expect(table.name).to eq('my_table') } + end + + describe '#table_name' do + it { expect(table.table_name).to eq('my_table') } + end + + describe '#statement' do + it { expect(table.statement).to eq('CREATE TABLE my_table (id bigint NOT NULL, col text)') } + end + + describe '#fetch_column_by_name' do + it { expect(table.fetch_column_by_name('col')).not_to be_nil } + + it { expect(table.fetch_column_by_name('invalid')).to be_nil } + end + + describe '#column_exists?' do + it { expect(table.column_exists?('col')).to eq(true) } + + it { expect(table.column_exists?('invalid')).to eq(false) } + end +end diff --git a/spec/lib/gitlab/database/schema_validation/schema_objects/trigger_spec.rb b/spec/lib/gitlab/database/schema_validation/schema_objects/trigger_spec.rb index 8000a54ee27..3c2481dfae0 100644 --- a/spec/lib/gitlab/database/schema_validation/schema_objects/trigger_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/schema_objects/trigger_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::SchemaValidation::SchemaObjects::Trigger, feature_category: :database do let(:statement) { 'CREATE TRIGGER my_trigger BEFORE INSERT ON todos FOR EACH ROW EXECUTE FUNCTION trigger()' } let(:name) { 'my_trigger' } + let(:table_name) { 'todos' } include_examples 'schema objects assertions for', 'create_trig_stmt' end diff --git a/spec/lib/gitlab/database/schema_validation/structure_sql_spec.rb b/spec/lib/gitlab/database/schema_validation/structure_sql_spec.rb index cc0bd4125ef..b0c056ff5db 100644 --- a/spec/lib/gitlab/database/schema_validation/structure_sql_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/structure_sql_spec.rb @@ -2,81 +2,65 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::SchemaValidation::StructureSql, feature_category: :database do - let(:structure_file_path) { Rails.root.join('spec/fixtures/structure.sql') } - let(:schema_name) { 'public' } - +RSpec.shared_examples 'structure sql schema assertions for' do |object_exists_method, all_objects_method| subject(:structure_sql) { described_class.new(structure_file_path, schema_name) } - context 'when having indexes' do - describe '#index_exists?' do - subject(:index_exists) { structure_sql.index_exists?(index_name) } + let(:structure_file_path) { Rails.root.join('spec/fixtures/structure.sql') } + let(:schema_name) { 'public' } - context 'when the index does not exist' do - let(:index_name) { 'non-existent-index' } + describe "##{object_exists_method}" do + it 'returns true when schema object exists' do + expect(structure_sql.public_send(object_exists_method, valid_schema_object_name)).to be_truthy + end - it 'returns false' do - expect(index_exists).to be_falsey - end - end + it 'returns false when schema object does not exists' do + expect(structure_sql.public_send(object_exists_method, 'invalid-object-name')).to be_falsey + end + end - context 'when the index exists' do - let(:index_name) { 'index' } + describe "##{all_objects_method}" do + it 'returns all the schema objects' do + schema_objects = structure_sql.public_send(all_objects_method) - it 'returns true' do - expect(index_exists).to be_truthy - end - end + expect(schema_objects).to all(be_a(schema_object)) + expect(schema_objects.map(&:name)).to eq(expected_objects) end + end +end - describe '#indexes' do - it 'returns indexes' do - indexes = structure_sql.indexes +RSpec.describe Gitlab::Database::SchemaValidation::StructureSql, feature_category: :database do + let(:structure_file_path) { Rails.root.join('spec/fixtures/structure.sql') } + let(:schema_name) { 'public' } - expected_indexes = %w[ - missing_index - wrong_index - index - index_namespaces_public_groups_name_id - index_on_deploy_keys_id_and_type_and_public - index_users_on_public_email_excluding_null_and_empty - ] + subject(:structure_sql) { described_class.new(structure_file_path, schema_name) } - expect(indexes).to all(be_a(Gitlab::Database::SchemaValidation::SchemaObjects::Index)) - expect(indexes.map(&:name)).to eq(expected_indexes) - end + context 'when having indexes' do + let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index } + let(:valid_schema_object_name) { 'index' } + let(:expected_objects) do + %w[missing_index wrong_index index index_namespaces_public_groups_name_id + index_on_deploy_keys_id_and_type_and_public index_users_on_public_email_excluding_null_and_empty] end + + include_examples 'structure sql schema assertions for', 'index_exists?', 'indexes' end context 'when having triggers' do - describe '#trigger_exists?' do - subject(:trigger_exists) { structure_sql.trigger_exists?(name) } + let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Trigger } + let(:valid_schema_object_name) { 'trigger' } + let(:expected_objects) { %w[trigger wrong_trigger missing_trigger_1 projects_loose_fk_trigger] } - context 'when the trigger does not exist' do - let(:name) { 'non-existent-trigger' } - - it 'returns false' do - expect(trigger_exists).to be_falsey - end - end - - context 'when the trigger exists' do - let(:name) { 'trigger' } + include_examples 'structure sql schema assertions for', 'trigger_exists?', 'triggers' + end - it 'returns true' do - expect(trigger_exists).to be_truthy - end - end + context 'when having tables' do + let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Table } + let(:valid_schema_object_name) { 'test_table' } + let(:expected_objects) do + %w[test_table ci_project_mirrors wrong_table extra_table_columns missing_table missing_table_columns + operations_user_lists] end - describe '#triggers' do - it 'returns triggers' do - triggers = structure_sql.triggers - expected_triggers = %w[trigger wrong_trigger missing_trigger_1 projects_loose_fk_trigger] - - expect(triggers).to all(be_a(Gitlab::Database::SchemaValidation::SchemaObjects::Trigger)) - expect(triggers.map(&:name)).to eq(expected_triggers) - end - end + include_examples 'structure sql schema assertions for', 'table_exists?', 'tables' end end diff --git a/spec/lib/gitlab/database/schema_validation/track_inconsistency_spec.rb b/spec/lib/gitlab/database/schema_validation/track_inconsistency_spec.rb new file mode 100644 index 00000000000..84db721fc2d --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/track_inconsistency_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::TrackInconsistency, feature_category: :database do + describe '#execute' do + let(:validator) { Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionIndexes } + + let(:database_statement) { 'CREATE INDEX index_name ON public.achievements USING btree (namespace_id)' } + let(:structure_sql_statement) { 'CREATE INDEX index_name ON public.achievements USING btree (id)' } + + let(:structure_stmt) { PgQuery.parse(structure_sql_statement).tree.stmts.first.stmt.index_stmt } + let(:database_stmt) { PgQuery.parse(database_statement).tree.stmts.first.stmt.index_stmt } + + let(:structure_sql_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index.new(structure_stmt) } + let(:database_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index.new(database_stmt) } + + let(:inconsistency) do + Gitlab::Database::SchemaValidation::Inconsistency.new(validator, structure_sql_object, database_object) + end + + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + subject(:execute) { described_class.new(inconsistency, project, user).execute } + + before do + stub_spam_services + end + + context 'when is not GitLab.com' do + it 'does not create a schema inconsistency record' do + allow(Gitlab).to receive(:com?).and_return(false) + + expect { execute }.not_to change { Gitlab::Database::SchemaValidation::SchemaInconsistency.count } + end + end + + context 'when the issue creation fails' do + let(:issue_creation) { instance_double(Mutations::Issues::Create, resolve: { errors: 'error' }) } + + before do + allow(Mutations::Issues::Create).to receive(:new).and_return(issue_creation) + end + + it 'does not create a schema inconsistency record' do + allow(Gitlab).to receive(:com?).and_return(true) + + expect { execute }.not_to change { Gitlab::Database::SchemaValidation::SchemaInconsistency.count } + end + end + + context 'when a new inconsistency is found' do + before do + project.add_developer(user) + end + + it 'creates a new schema inconsistency record' do + allow(Gitlab).to receive(:com?).and_return(true) + + expect { execute }.to change { Gitlab::Database::SchemaValidation::SchemaInconsistency.count } + end + end + + context 'when the schema inconsistency already exists' do + before do + project.add_developer(user) + end + + let!(:schema_inconsistency) do + create(:schema_inconsistency, object_name: 'index_name', table_name: 'achievements', + valitador_name: 'different_definition_indexes') + end + + it 'does not create a schema inconsistency record' do + allow(Gitlab).to receive(:com?).and_return(true) + + expect { execute }.not_to change { Gitlab::Database::SchemaValidation::SchemaInconsistency.count } + end + end + end +end diff --git a/spec/lib/gitlab/database/schema_validation/validators/base_validator_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/base_validator_spec.rb index 2f38c25cf68..036ad6424f0 100644 --- a/spec/lib/gitlab/database/schema_validation/validators/base_validator_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/validators/base_validator_spec.rb @@ -8,10 +8,15 @@ RSpec.describe Gitlab::Database::SchemaValidation::Validators::BaseValidator, fe it 'returns an array of all validators' do expect(all_validators).to eq([ + Gitlab::Database::SchemaValidation::Validators::ExtraTables, + Gitlab::Database::SchemaValidation::Validators::ExtraTableColumns, Gitlab::Database::SchemaValidation::Validators::ExtraIndexes, Gitlab::Database::SchemaValidation::Validators::ExtraTriggers, + Gitlab::Database::SchemaValidation::Validators::MissingTables, + Gitlab::Database::SchemaValidation::Validators::MissingTableColumns, Gitlab::Database::SchemaValidation::Validators::MissingIndexes, Gitlab::Database::SchemaValidation::Validators::MissingTriggers, + Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTables, Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionIndexes, Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTriggers ]) diff --git a/spec/lib/gitlab/database/schema_validation/validators/different_definition_tables_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/different_definition_tables_spec.rb new file mode 100644 index 00000000000..746418b757e --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/different_definition_tables_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTables, feature_category: :database do + include_examples 'table validators', described_class, ['wrong_table'] +end diff --git a/spec/lib/gitlab/database/schema_validation/validators/extra_table_columns_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/extra_table_columns_spec.rb new file mode 100644 index 00000000000..9d17a2fffa9 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/extra_table_columns_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::ExtraTableColumns, feature_category: :database do + include_examples 'table validators', described_class, ['extra_table_columns'] +end diff --git a/spec/lib/gitlab/database/schema_validation/validators/extra_tables_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/extra_tables_spec.rb new file mode 100644 index 00000000000..edaf79e3c93 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/extra_tables_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::ExtraTables, feature_category: :database do + include_examples 'table validators', described_class, ['extra_table'] +end diff --git a/spec/lib/gitlab/database/schema_validation/validators/missing_table_columns_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/missing_table_columns_spec.rb new file mode 100644 index 00000000000..de2956b4dd9 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/missing_table_columns_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::MissingTableColumns, feature_category: :database do + include_examples 'table validators', described_class, ['missing_table_columns'] +end diff --git a/spec/lib/gitlab/database/schema_validation/validators/missing_tables_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/missing_tables_spec.rb new file mode 100644 index 00000000000..7c80923e860 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/missing_tables_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::MissingTables, feature_category: :database do + missing_tables = %w[ci_project_mirrors missing_table operations_user_lists test_table] + + include_examples 'table validators', described_class, missing_tables +end diff --git a/spec/lib/gitlab/database/tables_locker_spec.rb b/spec/lib/gitlab/database/tables_locker_spec.rb index 30f0f9376c8..aaafe27f7ca 100644 --- a/spec/lib/gitlab/database/tables_locker_spec.rb +++ b/spec/lib/gitlab/database/tables_locker_spec.rb @@ -3,9 +3,13 @@ require 'spec_helper' RSpec.describe Gitlab::Database::TablesLocker, :suppress_gitlab_schemas_validate_connection, :silence_stdout, - feature_category: :pods do + feature_category: :cell do let(:default_lock_writes_manager) do - instance_double(Gitlab::Database::LockWritesManager, lock_writes: nil, unlock_writes: nil) + instance_double( + Gitlab::Database::LockWritesManager, + lock_writes: { action: 'any action' }, + unlock_writes: { action: 'unlocked' } + ) end before do @@ -81,6 +85,10 @@ RSpec.describe Gitlab::Database::TablesLocker, :suppress_gitlab_schemas_validate subject end + + it 'returns list of actions' do + expect(subject).to include({ action: 'any action' }) + end end shared_examples "unlock tables" do |gitlab_schema, database_name| @@ -110,6 +118,10 @@ RSpec.describe Gitlab::Database::TablesLocker, :suppress_gitlab_schemas_validate subject end + + it 'returns list of actions' do + expect(subject).to include({ action: 'unlocked' }) + end end shared_examples "lock partitions" do |partition_identifier, database_name| @@ -154,7 +166,7 @@ RSpec.describe Gitlab::Database::TablesLocker, :suppress_gitlab_schemas_validate context 'when running on single database' do before do - skip_if_multiple_databases_are_setup(:ci) + skip_if_database_exists(:ci) end describe '#lock_writes' do @@ -191,7 +203,7 @@ RSpec.describe Gitlab::Database::TablesLocker, :suppress_gitlab_schemas_validate context 'when running on multiple databases' do before do - skip_if_multiple_databases_not_setup(:ci) + skip_if_shared_database(:ci) end describe '#lock_writes' do diff --git a/spec/lib/gitlab/database/tables_truncate_spec.rb b/spec/lib/gitlab/database/tables_truncate_spec.rb index 3bb2f4e982c..bcbed0332e2 100644 --- a/spec/lib/gitlab/database/tables_truncate_spec.rb +++ b/spec/lib/gitlab/database/tables_truncate_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_base, - :suppress_gitlab_schemas_validate_connection, feature_category: :pods do + :suppress_gitlab_schemas_validate_connection, feature_category: :cell do include MigrationsHelpers let(:min_batch_size) { 1 } @@ -48,7 +48,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba end before do - skip_if_multiple_databases_not_setup(:ci) + skip_if_shared_database(:ci) # Creating some test tables on the main database main_tables_sql = <<~SQL @@ -79,8 +79,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba ALTER TABLE _test_gitlab_hook_logs DETACH PARTITION gitlab_partitions_dynamic._test_gitlab_hook_logs_202201; SQL - main_connection.execute(main_tables_sql) - ci_connection.execute(main_tables_sql) + execute_on_each_database(main_tables_sql) ci_tables_sql = <<~SQL CREATE TABLE _test_gitlab_ci_items (id serial NOT NULL PRIMARY KEY); @@ -92,15 +91,13 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba ); SQL - main_connection.execute(ci_tables_sql) - ci_connection.execute(ci_tables_sql) + execute_on_each_database(ci_tables_sql) internal_tables_sql = <<~SQL CREATE TABLE _test_gitlab_shared_items (id serial NOT NULL PRIMARY KEY); SQL - main_connection.execute(internal_tables_sql) - ci_connection.execute(internal_tables_sql) + execute_on_each_database(internal_tables_sql) # Filling the tables 5.times do |i| @@ -314,8 +311,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba context 'when running with multiple shared databases' do before do skip_if_multiple_databases_not_setup(:ci) - ci_db_config = Ci::ApplicationRecord.connection_db_config - allow(::Gitlab::Database).to receive(:db_config_share_with).with(ci_db_config).and_return('main') + skip_if_database_exists(:ci) end it 'raises an error when truncating the main database that it is a single database setup' do diff --git a/spec/lib/gitlab/database/transaction_timeout_settings_spec.rb b/spec/lib/gitlab/database/transaction_timeout_settings_spec.rb index 5b68f9a3757..2725b22ca9d 100644 --- a/spec/lib/gitlab/database/transaction_timeout_settings_spec.rb +++ b/spec/lib/gitlab/database/transaction_timeout_settings_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::TransactionTimeoutSettings, feature_category: :pods do +RSpec.describe Gitlab::Database::TransactionTimeoutSettings, feature_category: :cell do let(:connection) { ActiveRecord::Base.connection } subject { described_class.new(connection) } |