diff options
Diffstat (limited to 'spec/lib/gitlab/database')
48 files changed, 1376 insertions, 361 deletions
diff --git a/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb b/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb index 51a09ba0b5e..0454e7e72f4 100644 --- a/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb @@ -12,11 +12,12 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator, feature_category: : let(:index_model) { Gitlab::Database::AsyncIndexes::PostgresAsyncIndex } - let(:model) { Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME] } + let(:connection_name) { Gitlab::Database::PRIMARY_DATABASE_NAME } + let(:model) { Gitlab::Database.database_base_models[connection_name] } let(:connection) { model.connection } let!(:lease) { stub_exclusive_lease(lease_key, :uuid, timeout: lease_timeout) } - let(:lease_key) { "gitlab/database/asyncddl/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{connection_name}" } let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } around do |example| @@ -51,7 +52,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator, feature_category: : expect(Gitlab::AppLogger) .to have_received(:info) - .with(a_hash_including(message: expected_message)) + .with(a_hash_including(message: expected_message, connection_name: connection_name.to_s)) end end @@ -85,11 +86,11 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator, feature_category: : expect(Gitlab::AppLogger) .to have_received(:info) - .with(a_hash_including(message: 'Starting async index creation')) + .with(a_hash_including(message: 'Starting async index creation', connection_name: connection_name.to_s)) expect(Gitlab::AppLogger) .to have_received(:info) - .with(a_hash_including(message: 'Finished async index creation')) + .with(a_hash_including(message: 'Finished async index creation', connection_name: connection_name.to_s)) end end end diff --git a/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb b/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb index 7f0febdcacd..384c541256c 100644 --- a/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb @@ -12,11 +12,12 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor, feature_category let(:index_model) { Gitlab::Database::AsyncIndexes::PostgresAsyncIndex } - let(:model) { Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME] } + let(:connection_name) { Gitlab::Database::PRIMARY_DATABASE_NAME } + let(:model) { Gitlab::Database.database_base_models[connection_name] } let(:connection) { model.connection } let!(:lease) { stub_exclusive_lease(lease_key, :uuid, timeout: lease_timeout) } - let(:lease_key) { "gitlab/database/asyncddl/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{connection_name}" } let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } before do @@ -55,7 +56,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor, feature_category expect(Gitlab::AppLogger) .to have_received(:info) - .with(a_hash_including(message: expected_message)) + .with(a_hash_including(message: expected_message, connection_name: connection_name.to_s)) end end @@ -91,11 +92,11 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor, feature_category expect(Gitlab::AppLogger) .to have_received(:info) - .with(a_hash_including(message: 'Starting async index removal')) + .with(a_hash_including(message: 'Starting async index removal', connection_name: connection_name.to_s)) expect(Gitlab::AppLogger) .to have_received(:info) - .with(a_hash_including(message: 'Finished async index removal')) + .with(a_hash_including(message: 'Finished async index removal', connection_name: connection_name.to_s)) end end end diff --git a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb index 5e9d4f78a4a..9e37124ba28 100644 --- a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb @@ -6,6 +6,9 @@ RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model, it { is_expected.to be_a Gitlab::Database::SharedModel } describe 'validations' do + subject(:model) { build(:postgres_async_index) } + + let(:table_name_limit) { described_class::MAX_TABLE_NAME_LENGTH } let(:identifier_limit) { described_class::MAX_IDENTIFIER_LENGTH } let(:definition_limit) { described_class::MAX_DEFINITION_LENGTH } let(:last_error_limit) { described_class::MAX_LAST_ERROR_LENGTH } @@ -13,10 +16,45 @@ RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model, it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_length_of(:name).is_at_most(identifier_limit) } it { is_expected.to validate_presence_of(:table_name) } - it { is_expected.to validate_length_of(:table_name).is_at_most(identifier_limit) } + it { is_expected.to validate_length_of(:table_name).is_at_most(table_name_limit) } it { is_expected.to validate_presence_of(:definition) } it { is_expected.to validate_length_of(:definition).is_at_most(definition_limit) } it { is_expected.to validate_length_of(:last_error).is_at_most(last_error_limit) } + + shared_examples 'table_name is invalid' do + before do + model.table_name = table_name + end + + it 'is invalid' do + expect(model).to be_invalid + expect(model.errors).to have_key(:table_name) + end + end + + context 'when passing a long schema name' do + let(:table_name) { "#{'schema_name' * 10}.table_name" } + + it_behaves_like 'table_name is invalid' + end + + context 'when passing a long table name' do + let(:table_name) { "schema_name.#{'table_name' * 10}" } + + it_behaves_like 'table_name is invalid' + end + + context 'when passing a long table name and schema name' do + let(:table_name) { "#{'schema_name' * 10}.#{'table_name' * 10}" } + + it_behaves_like 'table_name is invalid' + end + + context 'when invalid table name is given' do + let(:table_name) { 'a.b.c' } + + it_behaves_like 'table_name is invalid' + end end describe 'scopes' do diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb index 4ef2e7f936b..0faa468233d 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do +RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner, feature_category: :database do let(:connection) { Gitlab::Database.database_base_models[:main].connection } let(:migration_wrapper) { double('test wrapper') } @@ -15,8 +15,8 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end before do - normal_signal = instance_double(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Normal, stop?: false) - allow(Gitlab::Database::BackgroundMigration::HealthStatus).to receive(:evaluate).and_return([normal_signal]) + normal_signal = instance_double(Gitlab::Database::HealthStatus::Signals::Normal, stop?: false) + allow(Gitlab::Database::HealthStatus).to receive(:evaluate).and_return([normal_signal]) end describe '#run_migration_job' do @@ -65,7 +65,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end context 'migration health' do - let(:health_status) { Gitlab::Database::BackgroundMigration::HealthStatus } + let(:health_status) { Gitlab::Database::HealthStatus } let(:stop_signal) { health_status::Signals::Stop.new(:indicator, reason: 'Take a break') } let(:normal_signal) { health_status::Signals::Normal.new(:indicator, reason: 'All good') } let(:not_available_signal) { health_status::Signals::NotAvailable.new(:indicator, reason: 'Indicator is disabled') } 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 546f9353808..213dee0d19d 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -46,6 +46,12 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m expect(batched_migration.status_name).to be :finished end + + it 'updates the finished_at' do + freeze_time do + expect { batched_migration.finish! }.to change(batched_migration, :finished_at).from(nil).to(Time.current) + end + end end end @@ -173,52 +179,6 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end end - describe '.active_migration' do - let(:connection) { Gitlab::Database.database_base_models[:main].connection } - let!(:migration1) { create(:batched_background_migration, :finished) } - - subject(:active_migration) { described_class.active_migration(connection: connection) } - - around do |example| - Gitlab::Database::SharedModel.using_connection(connection) do - example.run - end - end - - context 'when there are no migrations on hold' do - let!(:migration2) { create(:batched_background_migration, :active) } - let!(:migration3) { create(:batched_background_migration, :active) } - - it 'returns the first active migration according to queue order' do - expect(active_migration).to eq(migration2) - end - end - - context 'when there are migrations on hold' do - let!(:migration2) { create(:batched_background_migration, :active, on_hold_until: 10.minutes.from_now) } - let!(:migration3) { create(:batched_background_migration, :active, on_hold_until: 2.minutes.ago) } - - it 'returns the first active migration that is not on hold according to queue order' do - expect(active_migration).to eq(migration3) - end - end - - context 'when there are migrations not available for the current connection' do - let!(:migration2) { create(:batched_background_migration, :active, gitlab_schema: :gitlab_not_existing) } - let!(:migration3) { create(:batched_background_migration, :active, gitlab_schema: :gitlab_main) } - - it 'returns the first active migration that is available for the current connection' do - expect(active_migration).to eq(migration3) - end - end - - context 'when there are no active migrations available' do - it 'returns nil' do - expect(active_migration).to eq(nil) - end - end - end - describe '.find_executable' do let(:connection) { Gitlab::Database.database_base_models[:main].connection } let(:migration_id) { migration.id } diff --git a/spec/lib/gitlab/database/background_migration/health_status_spec.rb b/spec/lib/gitlab/database/background_migration/health_status_spec.rb deleted file mode 100644 index 4d6c729f080..00000000000 --- a/spec/lib/gitlab/database/background_migration/health_status_spec.rb +++ /dev/null @@ -1,114 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus, feature_category: :database do - let(:connection) { Gitlab::Database.database_base_models[:main].connection } - - around do |example| - Gitlab::Database::SharedModel.using_connection(connection) do - example.run - end - end - - describe '.evaluate' do - subject(:evaluate) { described_class.evaluate(migration, [autovacuum_indicator_class]) } - - let(:migration) { build(:batched_background_migration, :active) } - - 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) - end - - context 'with default indicators' do - subject(:evaluate) { described_class.evaluate(migration) } - - it 'returns a collection of signals' do - normal_signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) - not_available_signal = instance_double("#{health_status}::Signals::NotAvailable", log_info?: false) - - 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, not_available_signal) - end - end - - it 'returns a collection of signals' do - signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) - - expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) - - expect(evaluate).to contain_exactly(signal) - end - - it 'logs interesting signals' do - signal = instance_double( - "#{health_status}::Signals::Stop", - log_info?: true, - indicator_class: autovacuum_indicator_class, - short_name: 'Stop', - reason: 'Test Exception' - ) - - expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) - - expect(Gitlab::BackgroundMigration::Logger).to receive(:info).with( - migration_id: migration.id, - health_status_indicator: autovacuum_indicator_class.to_s, - indicator_signal: 'Stop', - signal_reason: 'Test Exception', - message: "#{migration} signaled: #{signal}" - ) - - evaluate - end - - it 'does not log signals of no interest' do - signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) - - expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) - expect(described_class).not_to receive(:log_signal) - - evaluate - end - - context 'on indicator error' do - let(:error) { RuntimeError.new('everything broken') } - - before do - expect(autovacuum_indicator).to receive(:evaluate).and_raise(error) - end - - it 'does not fail' do - expect { evaluate }.not_to raise_error - end - - it 'returns Unknown signal' do - signal = evaluate.first - - expect(signal).to be_an_instance_of(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) - expect(signal.reason).to eq("unexpected error: everything broken (RuntimeError)") - end - - it 'reports the exception to error tracking' do - expect(Gitlab::ErrorTracking).to receive(:track_exception) - .with(error, migration_id: migration.id, job_class_name: migration.job_class_name) - - evaluate - end - end - end -end diff --git a/spec/lib/gitlab/database/convert_feature_category_to_group_label_spec.rb b/spec/lib/gitlab/database/convert_feature_category_to_group_label_spec.rb new file mode 100644 index 00000000000..32766b0d937 --- /dev/null +++ b/spec/lib/gitlab/database/convert_feature_category_to_group_label_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::ConvertFeatureCategoryToGroupLabel, feature_category: :database do + describe '#execute' do + subject(:group_label) { described_class.new(feature_category).execute } + + let_it_be(:stages_fixture) do + { stages: { manage: { groups: { database: { categories: ['database'] } } } } } + end + + before do + stub_request(:get, 'https://gitlab.com/gitlab-com/www-gitlab-com/-/raw/master/data/stages.yml') + .to_return(status: 200, body: stages_fixture.to_json, headers: {}) + end + + context 'when the group label exists' do + let(:feature_category) { 'database' } + + it 'returns a group label' do + expect(group_label).to eql 'group::database' + end + end + + context 'when the group label does not exist' do + let(:feature_category) { 'non_existing_feature_category_test' } + + it 'returns nil' do + expect(group_label).to be nil + end + end + end +end diff --git a/spec/lib/gitlab/database/database_connection_info_spec.rb b/spec/lib/gitlab/database/database_connection_info_spec.rb new file mode 100644 index 00000000000..c87fd61268d --- /dev/null +++ b/spec/lib/gitlab/database/database_connection_info_spec.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::DatabaseConnectionInfo, feature_category: :cell do + let(:default_attributes) do + { + name: 'main', + gitlab_schemas: ['gitlab_main'], + klass: 'ActiveRecord::Base' + } + end + + let(:attributes) { default_attributes } + + subject { described_class.new(attributes) } + + describe '.new' do + let(:attributes) { default_attributes.merge(fallback_database: 'fallback') } + + it 'does convert attributes into symbols and objects' do + expect(subject.name).to be_a(Symbol) + expect(subject.gitlab_schemas).to all(be_a(Symbol)) + expect(subject.klass).to be(ActiveRecord::Base) + expect(subject.fallback_database).to be_a(Symbol) + expect(subject.db_dir).to be_a(Pathname) + end + + it 'does raise error when using invalid argument' do + expect { described_class.new(invalid: 'aa') }.to raise_error ArgumentError, /unknown keywords: invalid/ + end + end + + describe '.load_file' do + it 'does load YAML file and has file_path specified' do + file_path = Rails.root.join('db/database_connections/main.yaml') + db_info = described_class.load_file(file_path) + + expect(db_info).not_to be_nil + expect(db_info.file_path).to eq(file_path) + end + end + + describe '#connection_class' do + context 'when klass is "ActiveRecord::Base"' do + let(:attributes) { default_attributes.merge(klass: 'ActiveRecord::Base') } + + it 'does always return "ActiveRecord::Base"' do + expect(subject.connection_class).to eq(ActiveRecord::Base) + end + end + + context 'when klass is "Ci::ApplicationRecord"' do + let(:attributes) { default_attributes.merge(klass: 'Ci::ApplicationRecord') } + + it 'does return "Ci::ApplicationRecord" when it is connection_class' do + expect(Ci::ApplicationRecord).to receive(:connection_class).and_return(true) + + expect(subject.connection_class).to eq(Ci::ApplicationRecord) + end + + it 'does return nil when it is not connection_class' do + expect(Ci::ApplicationRecord).to receive(:connection_class).and_return(false) + + expect(subject.connection_class).to eq(nil) + end + end + end + + describe '#order' do + using RSpec::Parameterized::TableSyntax + + let(:configs_for) { %w[main ci geo] } + + before do + hash_configs = configs_for.map do |x| + instance_double(ActiveRecord::DatabaseConfigurations::HashConfig, name: x) + end + allow(::ActiveRecord::Base).to receive(:configurations).and_return( + instance_double(ActiveRecord::DatabaseConfigurations, configs_for: hash_configs) + ) + end + + where(:name, :order) do + :main | 0 + :ci | 1 + :undefined | 1000 + end + + with_them do + let(:attributes) { default_attributes.merge(name: name) } + + it { expect(subject.order).to eq(order) } + end + end + + describe '#connection_class_or_fallback' do + let(:all_databases) do + { + main: described_class.new( + name: 'main', gitlab_schemas: [], klass: 'ActiveRecord::Base'), + ci: described_class.new( + name: 'ci', gitlab_schemas: [], klass: 'Ci::ApplicationRecord', fallback_database: 'main') + } + end + + context 'for "main"' do + it 'does return ActiveRecord::Base' do + expect(all_databases[:main].connection_class_or_fallback(all_databases)) + .to eq(ActiveRecord::Base) + end + end + + context 'for "ci"' do + it 'does return "Ci::ApplicationRecord" when it is connection_class' do + expect(Ci::ApplicationRecord).to receive(:connection_class).and_return(true) + + expect(all_databases[:ci].connection_class_or_fallback(all_databases)) + .to eq(Ci::ApplicationRecord) + end + + it 'does return "ActiveRecord::Base" (fallback to "main") when it is not connection_class' do + expect(Ci::ApplicationRecord).to receive(:connection_class).and_return(false) + + expect(all_databases[:ci].connection_class_or_fallback(all_databases)) + .to eq(ActiveRecord::Base) + end + end + end + + describe '#has_gitlab_shared?' do + using RSpec::Parameterized::TableSyntax + + where(:gitlab_schemas, :result) do + %w[gitlab_main] | false + %w[gitlab_main gitlab_shared] | true + end + + with_them do + let(:attributes) { default_attributes.merge(gitlab_schemas: gitlab_schemas) } + + it { expect(subject.has_gitlab_shared?).to eq(result) } + end + end + + describe 'db_docs_dir' do + let(:attributes) { default_attributes.merge(db_dir: db_dir) } + + context 'when db_dir is specified' do + let(:db_dir) { 'ee/my/db' } + + it { expect(subject.db_docs_dir).to eq(Rails.root.join(db_dir, 'docs')) } + end + + context 'when db_dir is not specified fallbacks to "db/docs"' do + let(:db_dir) { nil } + + it { expect(subject.db_docs_dir).to eq(Rails.root.join('db/docs')) } + end + end +end diff --git a/spec/lib/gitlab/database/each_database_spec.rb b/spec/lib/gitlab/database/each_database_spec.rb index 75b543bee85..2653297c81a 100644 --- a/spec/lib/gitlab/database/each_database_spec.rb +++ b/spec/lib/gitlab/database/each_database_spec.rb @@ -70,11 +70,13 @@ RSpec.describe Gitlab::Database::EachDatabase do # Clear the memoization because the return of Gitlab::Database#schemas_to_base_models depends stubbed value clear_memoization(:@schemas_to_base_models) - clear_memoization(:@schemas_to_base_models_ee) end it 'only yields the unshared connections' do - expect(Gitlab::Database).to receive(:db_config_share_with).exactly(3).times.and_return(nil, 'main', 'main') + # if this is `non-main` connection make it shared with `main` + allow(Gitlab::Database).to receive(:db_config_share_with) do |db_config| + db_config.name != 'main' ? 'main' : nil + end expect { |b| described_class.each_database_connection(include_shared: false, &b) } .to yield_successive_args([ActiveRecord::Base.connection, 'main']) diff --git a/spec/lib/gitlab/database/gitlab_schema_info_spec.rb b/spec/lib/gitlab/database/gitlab_schema_info_spec.rb new file mode 100644 index 00000000000..b37aec46de8 --- /dev/null +++ b/spec/lib/gitlab/database/gitlab_schema_info_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::GitlabSchemaInfo, feature_category: :cell do + describe '.new' do + it 'does ensure that name is always symbol' do + schema_info = described_class.new(name: 'gitlab_main') + expect(schema_info.name).to eq(:gitlab_main) + end + + it 'does raise error when using invalid argument' do + expect { described_class.new(invalid: 'aa') }.to raise_error ArgumentError, /unknown keywords: invalid/ + end + end + + describe '.load_file' do + it 'does load YAML file and has file_path specified' do + file_path = Rails.root.join('db/gitlab_schemas/gitlab_main.yaml') + schema_info = described_class.load_file(file_path) + + expect(schema_info).not_to be_nil + expect(schema_info.file_path).to eq(file_path) + end + end +end diff --git a/spec/lib/gitlab/database/gitlab_schema_spec.rb b/spec/lib/gitlab/database/gitlab_schema_spec.rb index 5d3260a77c9..48f5cdb995b 100644 --- a/spec/lib/gitlab/database/gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/gitlab_schema_spec.rb @@ -20,12 +20,6 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do shared_examples 'maps table name to table schema' do using RSpec::Parameterized::TableSyntax - before do - ApplicationRecord.connection.execute(<<~SQL) - CREATE INDEX index_name_on_table_belonging_to_gitlab_main ON public.projects (name); - SQL - end - where(:name, :classification) do 'ci_builds' | :gitlab_ci 'my_schema.ci_builds' | :gitlab_ci @@ -37,7 +31,6 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do '_test_gitlab_ci_table' | :gitlab_ci '_test_my_table' | :gitlab_shared 'pg_attribute' | :gitlab_internal - 'index_name_on_table_belonging_to_gitlab_main' | :gitlab_main end with_them do @@ -52,53 +45,72 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do describe '.views_and_tables_to_schema' do include_examples 'validate schema data', described_class.views_and_tables_to_schema - # This being run across different databases indirectly also tests - # a general consistency of structure across databases - Gitlab::Database.database_base_models.except(:geo).each do |db_config_name, db_class| - context "for #{db_config_name} using #{db_class}" do - let(:db_data_sources) { db_class.connection.data_sources } + # group configurations by db_docs_dir, since then we expect all sharing this + # to contain exactly those tables + Gitlab::Database.all_database_connections.values.group_by(&:db_docs_dir).each do |db_docs_dir, db_infos| + context "for #{db_docs_dir}" do + let(:all_gitlab_schemas) { db_infos.flat_map(&:gitlab_schemas).to_set } - # The embedding and Geo databases do not share the same structure as all decomposed databases - subject do - described_class.views_and_tables_to_schema.reject { |_, v| v == :gitlab_embedding || v == :gitlab_geo } + let(:tables_for_gitlab_schemas) do + described_class.views_and_tables_to_schema.select do |_, gitlab_schema| + all_gitlab_schemas.include?(gitlab_schema) + end end - it 'new data sources are added' do - missing_data_sources = db_data_sources.to_set - subject.keys - - expect(missing_data_sources).to be_empty, \ - "Missing table/view(s) #{missing_data_sources.to_a} not found in " \ - "#{described_class}.views_and_tables_to_schema. " \ - "Any new tables or views must be added to the database dictionary. " \ - "More info: https://docs.gitlab.com/ee/development/database/database_dictionary.html" - end - - it 'non-existing data sources are removed' do - extra_data_sources = subject.keys.to_set - db_data_sources - - expect(extra_data_sources).to be_empty, \ - "Extra table/view(s) #{extra_data_sources.to_a} found in #{described_class}.views_and_tables_to_schema. " \ - "Any removed or renamed tables or views must be removed from the database dictionary. " \ - "More info: https://docs.gitlab.com/ee/development/database/database_dictionary.html" + db_infos.to_h { |db_info| [db_info.name, db_info.connection_class] } + .compact.each do |db_config_name, connection_class| + context "validates '#{db_config_name}' using '#{connection_class}'" do + let(:data_sources) { connection_class.connection.data_sources } + + it 'new data sources are added' do + missing_data_sources = data_sources.to_set - tables_for_gitlab_schemas.keys + + expect(missing_data_sources).to be_empty, \ + "Missing table/view(s) #{missing_data_sources.to_a} not found in " \ + "#{described_class}.views_and_tables_to_schema. " \ + "Any new tables or views must be added to the database dictionary. " \ + "More info: https://docs.gitlab.com/ee/development/database/database_dictionary.html" + end + + it 'non-existing data sources are removed' do + extra_data_sources = tables_for_gitlab_schemas.keys.to_set - data_sources + + expect(extra_data_sources).to be_empty, \ + "Extra table/view(s) #{extra_data_sources.to_a} found in " \ + "#{described_class}.views_and_tables_to_schema. " \ + "Any removed or renamed tables or views must be removed from the database dictionary. " \ + "More info: https://docs.gitlab.com/ee/development/database/database_dictionary.html" + end + end end end end - end - describe '.dictionary_path_globs' do - include_examples 'validate path globs', described_class.dictionary_path_globs - end + it 'all tables and views are unique' do + table_and_view_names = described_class.build_dictionary('') + table_and_view_names += described_class.build_dictionary('views') - describe '.view_path_globs' do - include_examples 'validate path globs', described_class.view_path_globs - end + # ignore gitlab_internal due to `ar_internal_metadata`, `schema_migrations` + table_and_view_names = table_and_view_names + .reject { |_, gitlab_schema| gitlab_schema == :gitlab_internal } - describe '.deleted_tables_path_globs' do - include_examples 'validate path globs', described_class.deleted_tables_path_globs + duplicated_tables = table_and_view_names + .group_by(&:first) + .select { |_, schemas| schemas.count > 1 } + .keys + + expect(duplicated_tables).to be_empty, \ + "Duplicated table(s) #{duplicated_tables.to_a} found in #{described_class}.views_and_tables_to_schema. " \ + "Any duplicated table must be removed from db/docs/ or ee/db/docs/. " \ + "More info: https://docs.gitlab.com/ee/development/database/database_dictionary.html" + end end - describe '.deleted_views_path_globs' do - include_examples 'validate path globs', described_class.deleted_views_path_globs + describe '.dictionary_path_globs' do + include_examples 'validate path globs', described_class.dictionary_path_globs('') + include_examples 'validate path globs', described_class.dictionary_path_globs('views') + include_examples 'validate path globs', described_class.dictionary_path_globs('deleted_views') + include_examples 'validate path globs', described_class.dictionary_path_globs('deleted_tables') end describe '.tables_to_schema' do @@ -128,7 +140,7 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do end describe '.table_schemas!' do - let(:tables) { %w[users projects ci_builds] } + let(:tables) { %w[projects issues ci_builds] } subject { described_class.table_schemas!(tables) } @@ -137,7 +149,7 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do end context 'when one of the tables does not have a matching table schema' do - let(:tables) { %w[users projects unknown ci_builds] } + let(:tables) { %w[namespaces projects unknown ci_builds] } it 'raises error' do expect { subject }.to raise_error(/Could not find gitlab schema for table unknown/) @@ -155,6 +167,18 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do it { is_expected.to be_nil } end + + context 'when an index name is used as the table name' do + before do + ApplicationRecord.connection.execute(<<~SQL) + CREATE INDEX index_on_projects ON public.projects USING gin (name gin_trgm_ops) + SQL + end + + let(:name) { 'index_on_projects' } + + it { is_expected.to be_nil } + end end describe '.table_schema!' do @@ -175,4 +199,82 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do end end end + + context 'when testing cross schema access' do + using RSpec::Parameterized::TableSyntax + + before do + allow(Gitlab::Database).to receive(:all_gitlab_schemas).and_return( + [ + Gitlab::Database::GitlabSchemaInfo.new( + name: "gitlab_main_clusterwide", + allow_cross_joins: %i[gitlab_shared gitlab_main], + allow_cross_transactions: %i[gitlab_internal gitlab_shared gitlab_main], + allow_cross_foreign_keys: %i[gitlab_main] + ), + Gitlab::Database::GitlabSchemaInfo.new( + name: "gitlab_main", + allow_cross_joins: %i[gitlab_shared], + allow_cross_transactions: %i[gitlab_internal gitlab_shared], + allow_cross_foreign_keys: %i[] + ), + Gitlab::Database::GitlabSchemaInfo.new( + name: "gitlab_ci", + allow_cross_joins: %i[gitlab_shared], + allow_cross_transactions: %i[gitlab_internal gitlab_shared], + allow_cross_foreign_keys: %i[] + ) + ].index_by(&:name) + ) + end + + describe '.cross_joins_allowed?' do + where(:schemas, :result) do + %i[] | true + %i[gitlab_main_clusterwide gitlab_main] | true + %i[gitlab_main_clusterwide gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_main gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_internal] | false + %i[gitlab_main gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_main gitlab_shared] | true + %i[gitlab_main_clusterwide gitlab_shared] | true + end + + with_them do + it { expect(described_class.cross_joins_allowed?(schemas)).to eq(result) } + end + end + + describe '.cross_transactions_allowed?' do + where(:schemas, :result) do + %i[] | true + %i[gitlab_main_clusterwide gitlab_main] | true + %i[gitlab_main_clusterwide gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_main gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_internal] | true + %i[gitlab_main gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_main gitlab_shared] | true + %i[gitlab_main_clusterwide gitlab_shared] | true + end + + with_them do + it { expect(described_class.cross_transactions_allowed?(schemas)).to eq(result) } + end + end + + describe '.cross_foreign_key_allowed?' do + where(:schemas, :result) do + %i[] | false + %i[gitlab_main_clusterwide gitlab_main] | true + %i[gitlab_main_clusterwide gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_internal] | false + %i[gitlab_main gitlab_ci] | false + %i[gitlab_main_clusterwide gitlab_shared] | false + end + + with_them do + it { expect(described_class.cross_foreign_key_allowed?(schemas)).to eq(result) } + end + end + end end diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb b/spec/lib/gitlab/database/health_status/indicators/autovacuum_active_on_table_spec.rb index 1c0f5a0c420..cd145bd5c0f 100644 --- a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb +++ b/spec/lib/gitlab/database/health_status/indicators/autovacuum_active_on_table_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::AutovacuumActiveOnTable, +RSpec.describe Gitlab::Database::HealthStatus::Indicators::AutovacuumActiveOnTable, feature_category: :database do include Database::DatabaseHelpers @@ -23,11 +23,18 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: let(:tables) { [table] } let(:table) { 'users' } - let(:context) { Gitlab::Database::BackgroundMigration::HealthStatus::Context.new(connection, tables) } + let(:context) do + Gitlab::Database::HealthStatus::Context.new( + described_class, + connection, + tables, + :gitlab_main + ) + end context 'without autovacuum activity' do it 'returns Normal signal' do - expect(subject).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Normal) + expect(subject).to be_a(Gitlab::Database::HealthStatus::Signals::Normal) end it 'remembers the indicator class' do @@ -41,7 +48,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: end it 'returns Stop signal' do - expect(subject).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Stop) + expect(subject).to be_a(Gitlab::Database::HealthStatus::Signals::Stop) end it 'explains why' do @@ -55,7 +62,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: it 'returns NoSignal signal in case the feature flag is disabled' do stub_feature_flags(batched_migrations_health_status_autovacuum: false) - expect(subject).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::NotAvailable) + expect(subject).to be_a(Gitlab::Database::HealthStatus::Signals::NotAvailable) end end end diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/patroni_apdex_spec.rb b/spec/lib/gitlab/database/health_status/indicators/patroni_apdex_spec.rb index d3102a105ea..e0e3a0a7c23 100644 --- a/spec/lib/gitlab/database/background_migration/health_status/indicators/patroni_apdex_spec.rb +++ b/spec/lib/gitlab/database/health_status/indicators/patroni_apdex_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::PatroniApdex, :aggregate_failures, feature_category: :database do # rubocop:disable Layout/LineLength +RSpec.describe Gitlab::Database::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 } @@ -19,8 +19,12 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: let(:prometheus_client) { instance_double(Gitlab::PrometheusClient) } let(:context) do - Gitlab::Database::BackgroundMigration::HealthStatus::Context - .new(connection, ['users'], gitlab_schema) + Gitlab::Database::HealthStatus::Context.new( + described_class, + connection, + ['users'], + gitlab_schema + ) end let(:gitlab_schema) { "gitlab_#{schema}" } @@ -61,7 +65,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: 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).to be_a(Gitlab::Database::HealthStatus::Signals::NotAvailable) expect(evaluate.reason).to include('indicator disabled') end @@ -69,7 +73,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: let(:database_apdex_settings) { nil } it 'returns Unknown signal' do - expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate).to be_a(Gitlab::Database::HealthStatus::Signals::Unknown) expect(evaluate.reason).to include('Patroni Apdex Settings not configured') end end @@ -78,7 +82,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: let(:client_ready) { false } it 'returns Unknown signal' do - expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate).to be_a(Gitlab::Database::HealthStatus::Signals::Unknown) expect(evaluate.reason).to include('Prometheus client is not ready') end end @@ -87,7 +91,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: 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).to be_a(Gitlab::Database::HealthStatus::Signals::Unknown) expect(evaluate.reason).to include('Apdex SLI query is not configured') end end @@ -96,7 +100,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: let(:"database_apdex_slo_#{schema}") { nil } it 'returns Unknown signal' do - expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(evaluate).to be_a(Gitlab::Database::HealthStatus::Signals::Unknown) expect(evaluate.reason).to include('Apdex SLO is not configured') end end @@ -105,7 +109,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: 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).to be_a(Gitlab::Database::HealthStatus::Signals::Normal) expect(evaluate.reason).to include('Patroni service apdex is above SLO') end @@ -113,7 +117,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: 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).to be_a(Gitlab::Database::HealthStatus::Signals::Stop) expect(evaluate.reason).to include('Patroni service apdex is below SLO') end @@ -131,7 +135,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: 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).to be_a(Gitlab::Database::HealthStatus::Signals::Unknown) expect(evaluate.reason).to include('Patroni service apdex can not be calculated') end end diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/write_ahead_log_spec.rb b/spec/lib/gitlab/database/health_status/indicators/write_ahead_log_spec.rb index 650f11e3cd5..aa2aee4f94a 100644 --- a/spec/lib/gitlab/database/background_migration/health_status/indicators/write_ahead_log_spec.rb +++ b/spec/lib/gitlab/database/health_status/indicators/write_ahead_log_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::WriteAheadLog do +RSpec.describe Gitlab::Database::HealthStatus::Indicators::WriteAheadLog, feature_category: :database do let(:connection) { Gitlab::Database.database_base_models[:main].connection } around do |example| @@ -14,7 +14,14 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: describe '#evaluate' do let(:tables) { [table] } let(:table) { 'users' } - let(:context) { Gitlab::Database::BackgroundMigration::HealthStatus::Context.new(connection, tables) } + let(:context) do + Gitlab::Database::HealthStatus::Context.new( + described_class, + connection, + tables, + :gitlab_main + ) + end subject(:evaluate) { described_class.new(context).evaluate } @@ -25,14 +32,14 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: it 'returns NoSignal signal in case the feature flag is disabled' do stub_feature_flags(batched_migrations_health_status_wal: false) - expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::NotAvailable) + expect(evaluate).to be_a(Gitlab::Database::HealthStatus::Signals::NotAvailable) expect(evaluate.reason).to include('indicator disabled') end it 'returns NoSignal signal when WAL archive queue can not be calculated' do expect(connection).to receive(:execute).and_return([{ 'pending_wal_count' => nil }]) - expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::NotAvailable) + expect(evaluate).to be_a(Gitlab::Database::HealthStatus::Signals::NotAvailable) expect(evaluate.reason).to include('WAL archive queue can not be calculated') end @@ -45,7 +52,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: context 'when WAL archive queue size is below the limit' do it 'returns Normal signal' do expect(connection).to receive(:execute).and_return([{ 'pending_wal_count' => 1 }]) - expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Normal) + expect(evaluate).to be_a(Gitlab::Database::HealthStatus::Signals::Normal) expect(evaluate.reason).to include('WAL archive queue is within limit') end end @@ -53,7 +60,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: context 'when WAL archive queue size is above the limit' do it 'returns Stop signal' do expect(connection).to receive(:execute).and_return([{ 'pending_wal_count' => 420 }]) - expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Stop) + expect(evaluate).to be_a(Gitlab::Database::HealthStatus::Signals::Stop) expect(evaluate.reason).to include('WAL archive queue is too big') end end diff --git a/spec/lib/gitlab/database/health_status/logger_spec.rb b/spec/lib/gitlab/database/health_status/logger_spec.rb new file mode 100644 index 00000000000..5ae6b40cb3a --- /dev/null +++ b/spec/lib/gitlab/database/health_status/logger_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::HealthStatus::Logger, feature_category: :database do + subject { described_class.new('/dev/null') } + + it_behaves_like 'a json logger', {} + + it 'excludes context' do + expect(described_class.exclude_context?).to be(true) + end +end diff --git a/spec/lib/gitlab/database/health_status/signals_spec.rb b/spec/lib/gitlab/database/health_status/signals_spec.rb new file mode 100644 index 00000000000..5bfd8ffb91e --- /dev/null +++ b/spec/lib/gitlab/database/health_status/signals_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::HealthStatus::Signals, feature_category: :database do + shared_examples 'health status signal' do |subclass, stop_signal, log_signal| + let(:indicator) { instance_double('Gitlab::Database::HealthStatus::Indicators::PatroniApdex') } + let(:reason) { 'Test reason' } + + subject { subclass.new(indicator, reason: reason) } + + describe '#log_info?' do + it 'returns the log signal' do + expect(subject.log_info?).to eq(log_signal) + end + end + + describe '#stop?' do + it 'returns the stop signal' do + expect(subject.stop?).to eq(stop_signal) + end + end + end + + context 'with Stop signal it should stop and log' do + it_behaves_like 'health status signal', described_class::Stop, true, true + end + + context 'with Normal signal it should not stop and log' do + it_behaves_like 'health status signal', described_class::Normal, false, false + end + + context 'with NotAvailable signal it should not stop and log' do + it_behaves_like 'health status signal', described_class::NotAvailable, false, false + end + + context 'with Unknown signal it should only log and not stop' do + it_behaves_like 'health status signal', described_class::Unknown, false, true + end +end diff --git a/spec/lib/gitlab/database/health_status_spec.rb b/spec/lib/gitlab/database/health_status_spec.rb new file mode 100644 index 00000000000..bc923635b1d --- /dev/null +++ b/spec/lib/gitlab/database/health_status_spec.rb @@ -0,0 +1,172 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::HealthStatus, feature_category: :database do + let(:connection) { Gitlab::Database.database_base_models[:main].connection } + + around do |example| + Gitlab::Database::SharedModel.using_connection(connection) do + example.run + end + end + + describe '.evaluate' do + subject(:evaluate) { described_class.evaluate(health_context, [autovacuum_indicator_class]) } + + let(:migration) { build(:batched_background_migration, :active) } + let(:health_context) { migration.health_context } + + let(:health_status) { described_class } + 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(health_context).and_return(autovacuum_indicator) + end + + context 'with default indicators' do + subject(:evaluate) { described_class.evaluate(health_context) } + + it 'returns a collection of signals' do + normal_signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) + not_available_signal = instance_double("#{health_status}::Signals::NotAvailable", log_info?: false) + + expect(autovacuum_indicator).to receive(:evaluate).and_return(normal_signal) + expect(wal_indicator_class).to receive(:new).with(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(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, not_available_signal) + end + end + + it 'returns the signal of the given indicator' do + signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) + + expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) + + expect(evaluate).to contain_exactly(signal) + end + + context 'with stop signals' do + let(:stop_signal) do + instance_double( + "#{health_status}::Signals::Stop", + log_info?: true, + indicator_class: autovacuum_indicator_class, + short_name: 'Stop', + reason: 'Test Exception' + ) + end + + before do + allow(autovacuum_indicator).to receive(:evaluate).and_return(stop_signal) + end + + context 'with batched migrations as the status checker' do + it 'captures BatchedMigration class name in the log' do + expect(Gitlab::Database::HealthStatus::Logger).to receive(:info).with( + status_checker_id: migration.id, + status_checker_type: 'Gitlab::Database::BackgroundMigration::BatchedMigration', + job_class_name: migration.job_class_name, + health_status_indicator: autovacuum_indicator_class.to_s, + indicator_signal: 'Stop', + signal_reason: 'Test Exception', + message: "#{migration} signaled: #{stop_signal}" + ) + + evaluate + end + end + + context 'with sidekiq deferred job as the status checker' do + let(:deferred_worker) do + Class.new do + def self.name + 'TestDeferredWorker' + end + + include ApplicationWorker + end + end + + let(:deferred_worker_health_checker) do + Gitlab::SidekiqMiddleware::DeferJobs::DatabaseHealthStatusChecker.new( + 123, + deferred_worker.name + ) + end + + let(:health_context) do + Gitlab::Database::HealthStatus::Context.new( + deferred_worker_health_checker, + ActiveRecord::Base.connection, + :gitlab_main, + [:users] + ) + end + + it 'captures sidekiq job class in the log' do + expect(Gitlab::Database::HealthStatus::Logger).to receive(:info).with( + status_checker_id: deferred_worker_health_checker.id, + status_checker_type: 'Gitlab::SidekiqMiddleware::DeferJobs::DatabaseHealthStatusChecker', + job_class_name: deferred_worker_health_checker.job_class_name, + health_status_indicator: autovacuum_indicator_class.to_s, + indicator_signal: 'Stop', + signal_reason: 'Test Exception', + message: "#{deferred_worker_health_checker} signaled: #{stop_signal}" + ) + + evaluate + end + end + end + + it 'does not log signals of no interest' do + signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) + + expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) + expect(described_class).not_to receive(:log_signal) + + evaluate + end + + context 'on indicator error' do + let(:error) { RuntimeError.new('everything broken') } + + before do + allow(autovacuum_indicator).to receive(:evaluate).and_raise(error) + end + + it 'does not fail' do + expect { evaluate }.not_to raise_error + end + + it 'returns Unknown signal' do + signal = evaluate.first + + expect(signal).to be_an_instance_of(Gitlab::Database::HealthStatus::Signals::Unknown) + expect(signal.reason).to eq("unexpected error: everything broken (RuntimeError)") + end + + it 'reports the exception to error tracking' do + expect(Gitlab::ErrorTracking).to receive(:track_exception) + .with( + error, + status_checker_id: migration.id, + status_checker_type: 'Gitlab::Database::BackgroundMigration::BatchedMigration', + job_class_name: migration.job_class_name + ) + + evaluate + end + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/host_spec.rb b/spec/lib/gitlab/database/load_balancing/host_spec.rb index b040c7a76bd..caae06ce43a 100644 --- a/spec/lib/gitlab/database/load_balancing/host_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/host_spec.rb @@ -195,6 +195,40 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do expect(host).to be_online end + + it 'clears the cache for latest_lsn_query' do + allow(host).to receive(:replica_is_up_to_date?).and_return(true) + + expect(host) + .to receive(:query_and_release) + .with(described_class::CAN_TRACK_LOGICAL_LSN_QUERY) + .twice + .and_return({ 'allowed' => 't' }, { 'allowed' => 'f' }) + + # Should receive LATEST_LSN_WITH_LOGICAL_QUERY twice even though we only + # return 't' once above + expect(host) + .to receive(:query_and_release) + .with(a_string_including(described_class::LATEST_LSN_WITH_LOGICAL_QUERY)) + .twice + .and_call_original + + host.replication_lag_size + host.replication_lag_size + + # Clear the cache for latest_lsn_query + host.refresh_status + + # Should recieve LATEST_LSN_WITHOUT_LOGICAL_QUERY since we received 'f' + # after clearing the cache + expect(host) + .to receive(:query_and_release) + .with(a_string_including(described_class::LATEST_LSN_WITHOUT_LOGICAL_QUERY)) + .once + .and_call_original + + host.replication_lag_size + end end describe '#check_replica_status?' do @@ -289,6 +323,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do expect(host) .to receive(:query_and_release) + .with(described_class::CAN_TRACK_LOGICAL_LSN_QUERY) + .and_call_original + + expect(host) + .to receive(:query_and_release) .and_return({ 'diff' => diff }) expect(host.data_is_recent_enough?).to eq(false) @@ -325,6 +364,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do it 'returns nil when the database query returned no rows' do expect(host) .to receive(:query_and_release) + .with(described_class::CAN_TRACK_LOGICAL_LSN_QUERY) + .and_call_original + + expect(host) + .to receive(:query_and_release) .and_return({}) expect(host.replication_lag_size).to be_nil @@ -339,6 +383,54 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do expect(host.replication_lag_size).to be_nil end + + context 'when can_track_logical_lsn? is false' do + before do + allow(host).to receive(:can_track_logical_lsn?).and_return(false) + end + + it 'uses LATEST_LSN_WITHOUT_LOGICAL_QUERY' do + expect(host) + .to receive(:query_and_release) + .with(a_string_including(described_class::LATEST_LSN_WITHOUT_LOGICAL_QUERY)) + .and_call_original + + expect(host.replication_lag_size('0/00000000')).to be_an_instance_of(Integer) + end + end + + context 'when can_track_logical_lsn? is true' do + before do + allow(host).to receive(:can_track_logical_lsn?).and_return(true) + end + + it 'uses LATEST_LSN_WITH_LOGICAL_QUERY' do + expect(host) + .to receive(:query_and_release) + .with(a_string_including(described_class::LATEST_LSN_WITH_LOGICAL_QUERY)) + .and_call_original + + expect(host.replication_lag_size('0/00000000')).to be_an_instance_of(Integer) + end + end + + context 'when CAN_TRACK_LOGICAL_LSN_QUERY raises connection errors' do + before do + expect(host) + .to receive(:query_and_release) + .with(described_class::CAN_TRACK_LOGICAL_LSN_QUERY) + .and_raise(ActiveRecord::ConnectionNotEstablished) + end + + it 'uses LATEST_LSN_WITHOUT_LOGICAL_QUERY' do + expect(host) + .to receive(:query_and_release) + .with(a_string_including(described_class::LATEST_LSN_WITHOUT_LOGICAL_QUERY)) + .and_call_original + + expect(host.replication_lag_size('0/00000000')).to be_an_instance_of(Integer) + end + end end describe '#primary_write_location' do @@ -357,28 +449,41 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do it 'returns true when a host has caught up' do allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => 't' }]) - expect(host.caught_up?('foo')).to eq(true) - end + expect(connection) + .to receive(:select_all) + .with(described_class::CAN_TRACK_LOGICAL_LSN_QUERY) + .and_return([{ 'has_table_privilege' => 't' }]) - it 'returns true when a host has caught up' do - allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => true }]) + expect(connection) + .to receive(:select_all) + .and_return([{ 'diff' => -1 }]) expect(host.caught_up?('foo')).to eq(true) end - it 'returns false when a host has not caught up' do + it 'returns false when diff query returns nothing' do allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => 'f' }]) + + expect(connection) + .to receive(:select_all) + .with(described_class::CAN_TRACK_LOGICAL_LSN_QUERY) + .and_return([{ 'has_table_privilege' => 't' }]) + + expect(connection).to receive(:select_all).and_return([]) expect(host.caught_up?('foo')).to eq(false) end it 'returns false when a host has not caught up' do allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => false }]) + + expect(connection) + .to receive(:select_all) + .with(described_class::CAN_TRACK_LOGICAL_LSN_QUERY) + .and_return([{ 'has_table_privilege' => 't' }]) + + expect(connection).to receive(:select_all).and_return([{ 'diff' => 123 }]) expect(host.caught_up?('foo')).to eq(false) end diff --git a/spec/lib/gitlab/database/lock_writes_manager_spec.rb b/spec/lib/gitlab/database/lock_writes_manager_spec.rb index 2aa95372338..899f3760132 100644 --- a/spec/lib/gitlab/database/lock_writes_manager_spec.rb +++ b/spec/lib/gitlab/database/lock_writes_manager_spec.rb @@ -55,7 +55,9 @@ RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: : describe '#lock_writes' do it 'prevents any writes on the table' do - subject.lock_writes + expect(subject.lock_writes).to eq( + { action: "locked", database: "main", dry_run: dry_run, table: test_table } + ) expect do connection.execute("delete from #{test_table}") @@ -116,19 +118,13 @@ RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: : expect(connection).not_to receive(:execute).with(/CREATE TRIGGER/) expect do - subject.lock_writes + result = subject.lock_writes + expect(result).to eq({ action: "skipped", database: "main", dry_run: false, table: test_table }) end.not_to change { number_of_triggers_on(connection, test_table) } 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 } @@ -154,9 +150,10 @@ table: 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 }) + it 'returns result hash with action needs_lock' do + expect(subject.lock_writes).to eq( + { action: "needs_lock", database: "main", dry_run: true, table: test_table } + ) end end end @@ -175,13 +172,24 @@ table: test_table }) end it 'allows writing on the table again' do - subject.unlock_writes + expect(subject.unlock_writes).to eq( + { action: "unlocked", database: "main", dry_run: dry_run, table: test_table } + ) expect do connection.execute("delete from #{test_table}") end.not_to raise_error end + it 'skips unlocking the table if the table was already unlocked for writes' do + subject.unlock_writes + + expect(subject).not_to receive(:execute_sql_statement) + expect(subject.unlock_writes).to eq( + { action: "skipped", database: "main", dry_run: dry_run, table: test_table } + ) + end + it 'removes the write protection triggers from the gitlab_main tables on the ci database' do expect do subject.unlock_writes @@ -198,11 +206,6 @@ table: test_table }) 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 } @@ -225,8 +228,9 @@ table: test_table }) 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 }) + expect(subject.unlock_writes).to eq( + { action: "needs_unlock", database: "main", dry_run: true, table: test_table } + ) end end end diff --git a/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb b/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb index faf0447c054..37075c4d2df 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 @@ -78,13 +78,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a } } }, - "does add column to ci_builds in gitlab_main and gitlab_ci" => { + "does add column to p_ci_builds in gitlab_main and gitlab_ci" => { migration: ->(klass) do def change - add_column :ci_builds, :__test_column, :integer + add_column :p_ci_builds, :__test_column, :integer end end, - query_matcher: /ALTER TABLE "ci_builds" ADD "__test_column" integer/, + query_matcher: /ALTER TABLE "p_ci_builds" ADD "__test_column" integer/, expected: { no_gitlab_schema: { main: :success, diff --git a/spec/lib/gitlab/database/migration_helpers/wraparound_autovacuum_spec.rb b/spec/lib/gitlab/database/migration_helpers/wraparound_autovacuum_spec.rb new file mode 100644 index 00000000000..1cc4ff6891c --- /dev/null +++ b/spec/lib/gitlab/database/migration_helpers/wraparound_autovacuum_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::MigrationHelpers::WraparoundAutovacuum, feature_category: :database do + include Database::DatabaseHelpers + + let(:migration) do + Class.new(Gitlab::Database::Migration[2.1]) + .include(described_class) + .new + end + + describe '#can_execute_on?' do + using RSpec::Parameterized::TableSyntax + + where(:dot_com, :dev_or_test, :wraparound_prevention, :expectation) do + true | true | true | false + true | false | true | false + false | true | true | false + false | false | true | false + true | true | false | true + true | false | false | true + false | true | false | true + false | false | false | false + end + + with_them do + it 'returns true for GitLab.com, dev, or test' do + allow(Gitlab).to receive(:com?).and_return(dot_com) + allow(Gitlab).to receive(:dev_or_test_env?).and_return(dev_or_test) + allow(migration).to receive(:wraparound_prevention_on_tables?).with([:table]).and_return(wraparound_prevention) + + expect(migration.can_execute_on?(:table)).to eq(expectation) + end + end + end + + describe '#wraparound_prevention_on_tables?' do + before do + swapout_view_for_table(:postgres_autovacuum_activity, connection: ApplicationRecord.connection) + create(:postgres_autovacuum_activity, table: 'foo', wraparound_prevention: false) + create(:postgres_autovacuum_activity, table: 'bar', wraparound_prevention: true) + end + + it { expect(migration.wraparound_prevention_on_tables?([:foo])).to be_falsey } + it { expect(migration.wraparound_prevention_on_tables?([:bar])).to be_truthy } + it { expect(migration.wraparound_prevention_on_tables?([:foo, :bar])).to be_truthy } + end +end diff --git a/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb b/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb index f5ce207773f..82f77d2bb19 100644 --- a/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb @@ -428,21 +428,24 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d describe '#ensure_batched_background_migration_is_finished' do let(:job_class_name) { 'CopyColumnUsingBackgroundMigrationJob' } - let(:table) { :events } + let(:table_name) { 'events' } let(:column_name) { :id } let(:job_arguments) { [["id"], ["id_convert_to_bigint"], nil] } + let(:gitlab_schema) { Gitlab::Database::GitlabSchema.table_schema!(table_name) } let(:configuration) do { job_class_name: job_class_name, - table_name: table, + table_name: table_name, column_name: column_name, job_arguments: job_arguments } end let(:migration_attributes) do - configuration.merge(gitlab_schema: Gitlab::Database.gitlab_schemas_for_connection(migration.connection).first) + configuration.merge( + gitlab_schema: gitlab_schema + ) end before do @@ -457,7 +460,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d create(:batched_background_migration, :active, migration_attributes) allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| - allow(runner).to receive(:finalize).with(job_class_name, table, column_name, job_arguments).and_return(false) + allow(runner).to receive(:finalize).with(job_class_name, table_name, column_name, job_arguments).and_return(false) end expect { ensure_batched_background_migration_is_finished } @@ -530,7 +533,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d migration = create(:batched_background_migration, :active, configuration) allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| - expect(runner).to receive(:finalize).with(job_class_name, table, column_name, job_arguments).and_return(migration.finish!) + expect(runner).to receive(:finalize).with(job_class_name, table_name, column_name, job_arguments).and_return(migration.finish!) end ensure_batched_background_migration_is_finished @@ -543,7 +546,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d create(:batched_background_migration, :active, configuration) allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| - expect(runner).not_to receive(:finalize).with(job_class_name, table, column_name, job_arguments) + expect(runner).not_to receive(:finalize).with(job_class_name, table_name, column_name, job_arguments) end expect { migration.ensure_batched_background_migration_is_finished(**configuration.merge(finalize: false)) }.to raise_error(RuntimeError) diff --git a/spec/lib/gitlab/database/migrations/constraints_helpers_spec.rb b/spec/lib/gitlab/database/migrations/constraints_helpers_spec.rb index 07d913cf5cc..476b5f3a784 100644 --- a/spec/lib/gitlab/database/migrations/constraints_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/constraints_helpers_spec.rb @@ -679,4 +679,43 @@ RSpec.describe Gitlab::Database::Migrations::ConstraintsHelpers do end end end + + describe '#switch_constraint_names' do + before do + ActiveRecord::Migration.connection.create_table(:_test_table) do |t| + t.references :supplier, foreign_key: { to_table: :_test_table, name: :supplier_fk } + t.references :customer, foreign_key: { to_table: :_test_table, name: :customer_fk } + end + end + + context 'when inside a transaction' do + it 'raises an error' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect do + model.switch_constraint_names(:_test_table, :supplier_fk, :customer_fk) + end.to raise_error(RuntimeError) + end + end + + context 'when outside a transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + + it 'executes the statement to swap the constraint names' do + expect { model.switch_constraint_names(:_test_table, :supplier_fk, :customer_fk) } + .to change { constrained_column_for(:customer_fk) }.from(:customer_id).to(:supplier_id) + .and change { constrained_column_for(:supplier_fk) }.from(:supplier_id).to(:customer_id) + end + + def constrained_column_for(fk_name) + Gitlab::Database::PostgresForeignKey + .find_by!(referenced_table_name: :_test_table, name: fk_name) + .constrained_columns + .first + .to_sym + end + end + end end diff --git a/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb b/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb index e48937037fa..7899c1588b2 100644 --- a/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb +++ b/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb @@ -16,7 +16,9 @@ RSpec.describe 'cross-database foreign keys' do end def is_cross_db?(fk_record) - Gitlab::Database::GitlabSchema.table_schemas!([fk_record.from_table, fk_record.to_table]).many? + table_schemas = Gitlab::Database::GitlabSchema.table_schemas!([fk_record.from_table, fk_record.to_table]) + + !Gitlab::Database::GitlabSchema.cross_foreign_key_allowed?(table_schemas) end it 'onlies have allowed list of cross-database foreign keys', :aggregate_failures do diff --git a/spec/lib/gitlab/database/partitioning/list/convert_table_spec.rb b/spec/lib/gitlab/database/partitioning/list/convert_table_spec.rb index 8e2a53ea76f..b30501cce21 100644 --- a/spec/lib/gitlab/database/partitioning/list/convert_table_spec.rb +++ b/spec/lib/gitlab/database/partitioning/list/convert_table_spec.rb @@ -15,8 +15,7 @@ RSpec.describe Gitlab::Database::Partitioning::List::ConvertTable, feature_categ table_name: table_name, partitioning_column: partitioning_column, parent_table_name: parent_table_name, - zero_partition_value: partitioning_default, - lock_tables: lock_tables + zero_partition_value: partitioning_default ) end @@ -227,16 +226,6 @@ RSpec.describe Gitlab::Database::Partitioning::List::ConvertTable, feature_categ end end - context 'with locking tables' do - let(:lock_tables) { [table_name] } - - it 'locks the table' do - recorder = ActiveRecord::QueryRecorder.new { partition } - - expect(recorder.log).to include(/LOCK "_test_table_to_partition" IN ACCESS EXCLUSIVE MODE/) - end - end - context 'when an error occurs during the conversion' do before do # Set up the fault that we'd like to inject @@ -264,7 +253,6 @@ RSpec.describe Gitlab::Database::Partitioning::List::ConvertTable, feature_categ with_them do it 'recovers from a fault', :aggregate_failures do expect { converter.partition }.to raise_error(/fault/) - expect(Gitlab::Database::PostgresPartition.for_parent_table(parent_table_name).count).to eq(0) expect { converter.partition }.not_to raise_error expect(Gitlab::Database::PostgresPartition.for_parent_table(parent_table_name).count).to eq(1) @@ -286,26 +274,6 @@ RSpec.describe Gitlab::Database::Partitioning::List::ConvertTable, feature_categ 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 - - context 'with locking tables' do - let(:lock_tables) { [table_name] } - - it 'locks the table before dropping the triggers' do - recorder = ActiveRecord::QueryRecorder.new { partition } - - lock_index = recorder.log.find_index do |log| - log.start_with?('LOCK "_test_table_to_partition" IN ACCESS EXCLUSIVE MODE') - end - - trigger_index = recorder.log.find_index do |log| - log.start_with?('DROP TRIGGER IF EXISTS _test_table_to_partition_loose_fk_trigger') - end - - expect(lock_index).to be_present - expect(trigger_index).to be_present - expect(lock_index).to be < trigger_index - end - end end end diff --git a/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb index e6014f81b74..5b6967c2d14 100644 --- a/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb @@ -2,10 +2,15 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy do +RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy, feature_category: :database do + include Gitlab::Database::DynamicModelHelpers + let(:connection) { ActiveRecord::Base.connection } - let(:table_name) { :_test_partitioned_test } - let(:model) { double('model', table_name: table_name, ignored_columns: %w[partition], connection: connection) } + let(:table_name) { '_test_partitioned_test' } + let(:model) do + define_batchable_model(table_name, connection: connection).tap { |m| m.ignored_columns = %w[partition] } + end + let(:next_partition_if) { double('next_partition_if') } let(:detach_partition_if) { double('detach_partition_if') } @@ -87,6 +92,31 @@ RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy do strategy.validate_and_fix end + + context 'when the shared connection is for the wrong database' do + it 'does not attempt to fix connections' do + skip_if_shared_database(:ci) + expect(strategy.model.connection).not_to receive(:change_column_default) + + Ci::ApplicationRecord.connection.execute(<<~SQL) + create table #{table_name} + ( + id serial not null, + partition bigint not null default 1, + created_at timestamptz not null, + primary key (id, partition) + ) + partition by list(partition); + + create table #{table_name}_1 + partition of #{table_name} for values in (1); + SQL + + Gitlab::Database::SharedModel.using_connection(Ci::ApplicationRecord.connection) do + strategy.validate_and_fix + end + end + end end describe '#active_partition' do diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb index d5f4afd7ba4..5f1e8842f18 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb @@ -228,6 +228,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers end it 'validates FK for each partition' do + allow(migration).to receive(:statement_timeout_disabled?).and_return(false) expect(migration).to receive(:execute).with(/SET statement_timeout TO 0/).twice expect(migration).to receive(:execute).with(/RESET statement_timeout/).twice expect(migration).to receive(:execute) 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 571c67db597..6a947044317 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 @@ -68,7 +68,6 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe describe '#convert_table_to_first_list_partition' do it_behaves_like 'delegates to ConvertTable' do let(:lock_tables) { [source_table] } - let(:extra_options) { { lock_tables: lock_tables } } let(:expected_method) { :partition } let(:migrate) do migration.convert_table_to_first_list_partition(table_name: source_table, diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb index 9df238a0024..8724716dd3d 100644 --- a/spec/lib/gitlab/database/partitioning_spec.rb +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -112,6 +112,24 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do end end + context 'without ci database' do + it 'only creates partitions for main database' do + skip_if_database_exists(:ci) + + allow(Gitlab::Database::Partitioning::PartitionManager).to receive(:new).and_call_original + + # Also, in the case where `ci` database is shared with `main` database, + # check that we do not run PartitionManager again for ci connection as + # that is redundant. + expect(Gitlab::Database::Partitioning::PartitionManager).not_to receive(:new) + .with(anything, connection: ci_connection).and_call_original + + expect { described_class.sync_partitions(models) } + .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) + end + end + context 'when no partitioned models are given' do it 'manages partitions for each registered model' do described_class.register_models([models.first]) @@ -247,6 +265,18 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do .and change { table_exists?(table_names.last) }.from(true).to(false) end + context 'when the feature flag is disabled' do + before do + stub_feature_flags(partition_manager_sync_partitions: false) + end + + it 'does not call the DetachedPartitionDropper' do + expect(Gitlab::Database::Partitioning::DetachedPartitionDropper).not_to receive(:new) + + described_class.drop_detached_partitions + end + end + def table_exists?(table_name) table_oid(table_name).present? end diff --git a/spec/lib/gitlab/database/pg_depend_spec.rb b/spec/lib/gitlab/database/pg_depend_spec.rb index 547a2c84b76..ff5169ebabf 100644 --- a/spec/lib/gitlab/database/pg_depend_spec.rb +++ b/spec/lib/gitlab/database/pg_depend_spec.rb @@ -13,8 +13,14 @@ RSpec.describe Gitlab::Database::PgDepend, type: :model, feature_category: :data connection.execute('CREATE EXTENSION IF NOT EXISTS pg_stat_statements;') end - it 'returns pg_stat_statements', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/410508' do - expect(subject.pluck('relname')).to eq(['pg_stat_statements']) + it 'returns pg_stat_statements' do + expected_views = ['pg_stat_statements'] + + if Gitlab::Database::Reflection.new(described_class).version.to_f >= 14 + expected_views << 'pg_stat_statements_info' # View added by pg_stat_statements starting in postgres 14 + end + + expect(subject.pluck('relname')).to match_array(expected_views) end end end diff --git a/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb b/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb index f24c4559349..5367cf1fb9b 100644 --- a/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb +++ b/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb @@ -28,5 +28,15 @@ RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model, featu it 'returns autovacuum activity for queries tables' do expect(subject.map(&:table).sort).to eq(tables) end + + it 'executes the query' do + is_expected.to be_a Array + end + end + + describe '.wraparound_prevention' do + subject { described_class.wraparound_prevention } + + it { expect(subject.where_values_hash).to match(a_hash_including('wraparound_prevention' => true)) } end end diff --git a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb index 6a0c4226db8..b5e08f58608 100644 --- a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb @@ -7,6 +7,9 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasMetrics, query_ana before do allow(Gitlab::Database::QueryAnalyzer.instance).to receive(:all_analyzers).and_return([analyzer]) + ApplicationRecord.connection.execute(<<~SQL) + CREATE INDEX index_on_projects ON public.projects USING gin (name gin_trgm_ops) + SQL end it 'does not increment metrics if feature flag is disabled' do @@ -59,6 +62,11 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasMetrics, query_ana sql: "SELECT 1 FROM projects LEFT JOIN not_in_schema ON not_in_schema.project_id=projects.id", expect_error: /Could not find gitlab schema for table not_in_schema/ + }, + "for query altering an INDEX" => { + model: ApplicationRecord, + sql: "ALTER INDEX index_on_projects SET ( fastupdate = false )", + no_op: true } } end @@ -74,6 +82,10 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasMetrics, query_ana if expect_error expect { process_sql(model, sql) }.to raise_error(expect_error) + elsif no_op + expect(described_class.schemas_metrics).not_to receive(:increment) + + process_sql(model, sql) else expect(described_class.schemas_metrics).to receive(:increment) .with(expectations).and_call_original 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 02bd6b51463..3ccdb907cba 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 @@ -57,13 +57,19 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio end end - shared_examples 'cross-database modification errors' do |model:| + shared_examples 'cross-database modification errors' do |model:, sql_log_contains:| let(:model) { model } context "within #{model} transaction" do it 'raises error' do model.transaction do - expect { run_queries }.to raise_error /Cross-database data modification/ + expect { run_queries }.to raise_error do |error| + expect(error.message).to include 'Cross-database data modification' + + sql_log_contains.each do |sql_query| + expect(error.message).to match sql_query + end + end end end end @@ -87,7 +93,8 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio include_examples 'successful examples', model: Ci::Pipeline - include_examples 'cross-database modification errors', model: Project + include_examples 'cross-database modification errors', model: Project, + sql_log_contains: [/UPDATE "ci_pipelines"/] end context 'when other data is modified' do @@ -98,7 +105,8 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio include_examples 'successful examples', model: Project - include_examples 'cross-database modification errors', model: Ci::Pipeline + include_examples 'cross-database modification errors', model: Ci::Pipeline, + sql_log_contains: [/UPDATE "projects"/] end context 'when both CI and other data is modified' do @@ -112,11 +120,8 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio end context 'when data modification happens in a transaction' do - it 'raises error' do - Project.transaction do - expect { run_queries }.to raise_error /Cross-database data modification/ - end - end + include_examples 'cross-database modification errors', model: Project, + sql_log_contains: [/UPDATE "projects"/, /UPDATE "ci_pipelines"/] context 'when ci_pipelines are ignored for cross modification' do it 'does not raise error' do @@ -131,11 +136,16 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio end context 'when data modification happens in nested transactions' do - it 'raises error' do + it 'raises error, with the generated sql queries included' do Project.transaction(requires_new: true) do project.touch Project.transaction(requires_new: true) do - expect { pipeline.touch }.to raise_error /Cross-database data modification/ + expect { pipeline.touch }.to raise_error do |error| + expect(error.message).to include('Cross-database data modification') + + expect(error.message).to match(/UPDATE "projects"/) + expect(error.message).to match(/UPDATE "ci_pipelines"/) + end end end end @@ -151,11 +161,8 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio Marginalia::Comment.prepend_comment = prepend_comment_was end - it 'raises error' do - Project.transaction do - expect { run_queries }.to raise_error /Cross-database data modification/ - end - end + include_examples 'cross-database modification errors', model: Project, + sql_log_contains: [/UPDATE "projects"/, /UPDATE "ci_pipelines"/] end end @@ -170,11 +177,8 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio end context 'when data modification happens in a transaction' do - it 'raises error' do - Project.transaction do - expect { run_queries }.to raise_error /Cross-database data modification/ - end - end + include_examples 'cross-database modification errors', model: Project, + sql_log_contains: [/UPDATE "projects"/, /SELECT "ci_pipelines"."id".*FOR UPDATE/] context 'when the modification is inside a factory save! call' do let(:runner) { create(:ci_runner, :project, projects: [build(:project)]) } @@ -194,7 +198,8 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio include_examples 'successful examples', model: Ci::Pipeline - include_examples 'cross-database modification errors', model: Project + include_examples 'cross-database modification errors', model: Project, + sql_log_contains: [/INSERT INTO "ci_variables"/] end describe '.allow_cross_database_modification_within_transaction' do diff --git a/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb b/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb index 261bef58bb6..b90f60e0301 100644 --- a/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas, query_analyzers: false do +RSpec.describe Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas, + query_analyzers: false, feature_category: :database do let(:analyzer) { described_class } context 'properly analyzes queries' do @@ -15,14 +16,38 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas, query_a expected_allowed_gitlab_schemas: { no_schema: :dml_not_allowed, gitlab_main: :success, + gitlab_main_clusterwide: :success, + gitlab_main_cell: :success, gitlab_ci: :dml_access_denied # cross-schema access } }, - "for INSERT" => { + "for SELECT on namespaces" => { + sql: "SELECT 1 FROM namespaces", + expected_allowed_gitlab_schemas: { + no_schema: :dml_not_allowed, + gitlab_main: :success, + gitlab_main_clusterwide: :success, + gitlab_main_cell: :success, + gitlab_ci: :dml_access_denied # cross-schema access + } + }, + "for INSERT on projects" => { sql: "INSERT INTO projects VALUES (1)", expected_allowed_gitlab_schemas: { no_schema: :dml_not_allowed, gitlab_main: :success, + gitlab_main_clusterwide: :success, + gitlab_main_cell: :success, + gitlab_ci: :dml_access_denied # cross-schema access + } + }, + "for INSERT on namespaces" => { + sql: "INSERT INTO namespaces VALUES (1)", + expected_allowed_gitlab_schemas: { + no_schema: :dml_not_allowed, + gitlab_main: :success, + gitlab_main_clusterwide: :success, + gitlab_main_cell: :success, gitlab_ci: :dml_access_denied # cross-schema access } }, diff --git a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb index e82a2ab467d..f1d88615762 100644 --- a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb +++ b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb @@ -38,7 +38,7 @@ RSpec.describe Gitlab::Database::Reindexing::IndexSelection, feature_category: : expect(subject).not_to include(excluded.index) end - it 'excludes indexes smaller than 1 GB ondisk size' do + it 'excludes indexes smaller than 1 GiB ondisk size' do excluded = create( :postgres_index_bloat_estimate, index: create(:postgres_index, ondisk_size_bytes: 0.99.gigabytes), @@ -48,7 +48,7 @@ RSpec.describe Gitlab::Database::Reindexing::IndexSelection, feature_category: : expect(subject).not_to include(excluded.index) end - it 'includes indexes larger than 100 GB ondisk size' do + it 'includes indexes larger than 100 GiB ondisk size' do included = create( :postgres_index_bloat_estimate, index: create(:postgres_index, ondisk_size_bytes: 101.gigabytes), diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb index 2cb84e2f02a..370d03b495c 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :delete, feature_category: :subgroups do +RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :delete, feature_category: :groups_and_projects do let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb index 5b5661020b0..b00a1d4a9e1 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces, :delete, -feature_category: :subgroups do +feature_category: :groups_and_projects do let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } let(:namespace) { create(:group, name: 'the-path') } diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb index 787c9e87038..d2665664fb0 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :delete, -feature_category: :projects do +feature_category: :groups_and_projects do let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } let(:project) do diff --git a/spec/lib/gitlab/database/schema_validation/adapters/foreign_key_database_adapter_spec.rb b/spec/lib/gitlab/database/schema_validation/adapters/foreign_key_database_adapter_spec.rb new file mode 100644 index 00000000000..cfe5572fb51 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/adapters/foreign_key_database_adapter_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Adapters::ForeignKeyDatabaseAdapter, feature_category: :database do + subject(:adapter) { described_class.new(query_result) } + + let(:query_result) do + { + 'schema' => 'public', + 'foreign_key_name' => 'fk_2e88fb7ce9', + 'table_name' => 'members', + 'foreign_key_definition' => 'FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE' + } + end + + describe '#name' do + it { expect(adapter.name).to eq('public.fk_2e88fb7ce9') } + end + + describe '#table_name' do + it { expect(adapter.table_name).to eq('members') } + end + + describe '#statement' do + it { expect(adapter.statement).to eq('FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE') } + end +end diff --git a/spec/lib/gitlab/database/schema_validation/adapters/foreign_key_structure_sql_adapter_spec.rb b/spec/lib/gitlab/database/schema_validation/adapters/foreign_key_structure_sql_adapter_spec.rb new file mode 100644 index 00000000000..f7ae0c0f892 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/adapters/foreign_key_structure_sql_adapter_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Adapters::ForeignKeyStructureSqlAdapter, feature_category: :database do + subject(:adapter) { described_class.new(stmt) } + + let(:stmt) { PgQuery.parse(sql).tree.stmts.first.stmt.alter_table_stmt } + + where(:sql, :name, :table_name, :statement) do + [ + [ + 'ALTER TABLE ONLY public.issues ADD CONSTRAINT fk_05f1e72feb FOREIGN KEY (author_id) REFERENCES users (id) ' \ + 'ON DELETE SET NULL', + 'public.fk_05f1e72feb', + 'issues', + 'FOREIGN KEY (author_id) REFERENCES users(id) ON DELETE SET NULL' + ], + [ + 'ALTER TABLE public.import_failures ADD CONSTRAINT fk_9a9b9ba21c FOREIGN KEY (user_id) REFERENCES users(id) ' \ + 'ON DELETE CASCADE', + 'public.fk_9a9b9ba21c', + 'import_failures', + 'FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE' + ] + ] + end + + with_them do + describe '#name' do + it { expect(adapter.name).to eq(name) } + end + + describe '#table_name' do + it { expect(adapter.table_name).to eq(table_name) } + end + + describe '#statement' do + it { expect(adapter.statement).to eq(statement) } + end + end +end diff --git a/spec/lib/gitlab/database/schema_validation/schema_inconsistency_spec.rb b/spec/lib/gitlab/database/schema_validation/schema_inconsistency_spec.rb index 7d6a279def9..fbaf8474f22 100644 --- a/spec/lib/gitlab/database/schema_validation/schema_inconsistency_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/schema_inconsistency_spec.rb @@ -13,5 +13,29 @@ RSpec.describe Gitlab::Database::SchemaValidation::SchemaInconsistency, type: :m 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) } + it { is_expected.to validate_presence_of(:diff) } + end + + describe 'scopes' do + describe '.with_open_issues' do + subject(:inconsistencies) { described_class.with_open_issues } + + let(:closed_issue) { create(:issue, :closed) } + let(:open_issue) { create(:issue, :opened) } + + let!(:schema_inconsistency_with_issue_closed) do + create(:schema_inconsistency, object_name: 'index_name', table_name: 'achievements', + valitador_name: 'different_definition_indexes', issue: closed_issue) + end + + let!(:schema_inconsistency_with_issue_opened) do + create(:schema_inconsistency, object_name: 'index_name', table_name: 'achievements', + valitador_name: 'different_definition_indexes', issue: open_issue) + end + + it 'returns only schema inconsistencies with GitLab issues open' do + expect(inconsistencies).to eq([schema_inconsistency_with_issue_opened]) + end + end end end diff --git a/spec/lib/gitlab/database/schema_validation/schema_objects/foreign_key_spec.rb b/spec/lib/gitlab/database/schema_validation/schema_objects/foreign_key_spec.rb new file mode 100644 index 00000000000..7500ad44f82 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/schema_objects/foreign_key_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::SchemaObjects::ForeignKey, feature_category: :database do + subject(:foreign_key) { described_class.new(adapter) } + + let(:database_adapter) { 'Gitlab::Database::SchemaValidation::Adapters::ForeignKeyDatabaseAdapter' } + let(:adapter) do + instance_double(database_adapter, name: 'public.fk_1d37cddf91', table_name: 'vulnerabilities', + statement: 'FOREIGN KEY (epic_id) REFERENCES epics(id) ON DELETE SET NULL') + end + + describe '#name' do + it { expect(foreign_key.name).to eq('public.fk_1d37cddf91') } + end + + describe '#table_name' do + it { expect(foreign_key.table_name).to eq('vulnerabilities') } + end + + describe '#statement' do + it { expect(foreign_key.statement).to eq('FOREIGN KEY (epic_id) REFERENCES epics(id) ON DELETE SET NULL') } + 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 index 84db721fc2d..0b104e40c11 100644 --- a/spec/lib/gitlab/database/schema_validation/track_inconsistency_spec.rb +++ b/spec/lib/gitlab/database/schema_validation/track_inconsistency_spec.rb @@ -24,10 +24,6 @@ RSpec.describe Gitlab::Database::SchemaValidation::TrackInconsistency, feature_c 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) @@ -39,7 +35,12 @@ RSpec.describe Gitlab::Database::SchemaValidation::TrackInconsistency, feature_c context 'when the issue creation fails' do let(:issue_creation) { instance_double(Mutations::Issues::Create, resolve: { errors: 'error' }) } + let(:convert_object) do + instance_double('Gitlab::Database::ConvertFeatureCategoryToGroupLabel', execute: 'group_label') + end + before do + allow(Gitlab::Database::ConvertFeatureCategoryToGroupLabel).to receive(:new).and_return(convert_object) allow(Mutations::Issues::Create).to receive(:new).and_return(issue_creation) end @@ -51,7 +52,12 @@ RSpec.describe Gitlab::Database::SchemaValidation::TrackInconsistency, feature_c end context 'when a new inconsistency is found' do + let(:convert_object) do + instance_double('Gitlab::Database::ConvertFeatureCategoryToGroupLabel', execute: 'group_label') + end + before do + allow(Gitlab::Database::ConvertFeatureCategoryToGroupLabel).to receive(:new).and_return(convert_object) project.add_developer(user) end @@ -63,19 +69,116 @@ RSpec.describe Gitlab::Database::SchemaValidation::TrackInconsistency, feature_c end context 'when the schema inconsistency already exists' do - before do - project.add_developer(user) + let(:diff) do + "-#{structure_sql_statement}\n" \ + "+#{database_statement}\n" end let!(:schema_inconsistency) do create(:schema_inconsistency, object_name: 'index_name', table_name: 'achievements', - valitador_name: 'different_definition_indexes') + valitador_name: 'different_definition_indexes', diff: diff) end - it 'does not create a schema inconsistency record' do + before do + project.add_developer(user) + end + + context 'when the issue has the last schema inconsistency' do + it 'does not add a note' do + allow(Gitlab).to receive(:com?).and_return(true) + + expect { execute }.not_to change { schema_inconsistency.issue.notes.count } + end + end + + context 'when the issue is outdated' do + let!(:schema_inconsistency) do + create(:schema_inconsistency, object_name: 'index_name', table_name: 'achievements', + valitador_name: 'different_definition_indexes', diff: 'old_diff') + end + + it 'adds a note' do + allow(Gitlab).to receive(:com?).and_return(true) + + expect { execute }.to change { schema_inconsistency.issue.notes.count }.from(0).to(1) + end + + it 'updates the diff' do + allow(Gitlab).to receive(:com?).and_return(true) + + execute + + expect(schema_inconsistency.reload.diff).to eq(diff) + end + end + + context 'when the GitLab issue is open' do + it 'does not create a new schema inconsistency record' do + allow(Gitlab).to receive(:com?).and_return(true) + schema_inconsistency.issue.update!(state_id: Issue.available_states[:opened]) + + expect { execute }.not_to change { Gitlab::Database::SchemaValidation::SchemaInconsistency.count } + end + end + + context 'when the GitLab is not open' do + let(:convert_object) do + instance_double('Gitlab::Database::ConvertFeatureCategoryToGroupLabel', execute: 'group_label') + end + + before do + allow(Gitlab::Database::ConvertFeatureCategoryToGroupLabel).to receive(:new).and_return(convert_object) + project.add_developer(user) + end + + it 'creates a new schema inconsistency record' do + allow(Gitlab).to receive(:com?).and_return(true) + schema_inconsistency.issue.update!(state_id: Issue.available_states[:closed]) + + expect { execute }.to change { Gitlab::Database::SchemaValidation::SchemaInconsistency.count } + end + end + end + + context 'when the dictionary file is not present' do + before do + allow(Gitlab::Database::GitlabSchema).to receive(:dictionary_paths).and_return(['dictionary_not_found_path/']) + + project.add_developer(user) + end + + it 'add the default labels' do allow(Gitlab).to receive(:com?).and_return(true) - expect { execute }.not_to change { Gitlab::Database::SchemaValidation::SchemaInconsistency.count } + inconsistency = execute + + labels = inconsistency.issue.labels.map(&:name) + + expect(labels).to eq %w[database database-inconsistency-report type::maintenance severity::4] + end + end + + context 'when dictionary feature_categories are available' do + let(:convert_object) do + instance_double('Gitlab::Database::ConvertFeatureCategoryToGroupLabel', execute: 'group_label') + end + + before do + allow(Gitlab::Database::ConvertFeatureCategoryToGroupLabel).to receive(:new).and_return(convert_object) + + allow(Gitlab::Database::GitlabSchema).to receive(:dictionary_paths).and_return(['spec/fixtures/']) + + project.add_developer(user) + end + + it 'add the default labels + group labels' do + allow(Gitlab).to receive(:com?).and_return(true) + + inconsistency = execute + + labels = inconsistency.issue.labels.map(&:name) + + expect(labels).to eq %w[database database-inconsistency-report type::maintenance severity::4 group_label] 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 036ad6424f0..e8c08277d52 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 @@ -12,13 +12,16 @@ RSpec.describe Gitlab::Database::SchemaValidation::Validators::BaseValidator, fe Gitlab::Database::SchemaValidation::Validators::ExtraTableColumns, Gitlab::Database::SchemaValidation::Validators::ExtraIndexes, Gitlab::Database::SchemaValidation::Validators::ExtraTriggers, + Gitlab::Database::SchemaValidation::Validators::ExtraForeignKeys, Gitlab::Database::SchemaValidation::Validators::MissingTables, Gitlab::Database::SchemaValidation::Validators::MissingTableColumns, Gitlab::Database::SchemaValidation::Validators::MissingIndexes, Gitlab::Database::SchemaValidation::Validators::MissingTriggers, + Gitlab::Database::SchemaValidation::Validators::MissingForeignKeys, Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTables, Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionIndexes, - Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTriggers + Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTriggers, + Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionForeignKeys ]) end end diff --git a/spec/lib/gitlab/database/schema_validation/validators/different_definition_foreign_keys_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/different_definition_foreign_keys_spec.rb new file mode 100644 index 00000000000..ffebffc3ad2 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/different_definition_foreign_keys_spec.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionForeignKeys, + feature_category: :database do + include_examples 'foreign key validators', described_class, ['public.wrong_definition_fk'] +end diff --git a/spec/lib/gitlab/database/schema_validation/validators/extra_foreign_keys_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/extra_foreign_keys_spec.rb new file mode 100644 index 00000000000..053153aa214 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/extra_foreign_keys_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::ExtraForeignKeys, feature_category: :database do + include_examples 'foreign key validators', described_class, ['public.extra_fk'] +end diff --git a/spec/lib/gitlab/database/schema_validation/validators/missing_foreign_keys_spec.rb b/spec/lib/gitlab/database/schema_validation/validators/missing_foreign_keys_spec.rb new file mode 100644 index 00000000000..a47804abb91 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/validators/missing_foreign_keys_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Validators::MissingForeignKeys, feature_category: :database do + include_examples 'foreign key validators', described_class, %w[public.fk_rails_536b96bff1 public.missing_fk] +end diff --git a/spec/lib/gitlab/database/tables_locker_spec.rb b/spec/lib/gitlab/database/tables_locker_spec.rb index aaafe27f7ca..0e7e929d54b 100644 --- a/spec/lib/gitlab/database/tables_locker_spec.rb +++ b/spec/lib/gitlab/database/tables_locker_spec.rb @@ -251,6 +251,31 @@ RSpec.describe Gitlab::Database::TablesLocker, :suppress_gitlab_schemas_validate it_behaves_like 'unlock partitions', gitlab_main_detached_partition, 'ci' end + context 'when not including partitions' do + subject { described_class.new(include_partitions: false).lock_writes } + + it 'does not include any table partitions' do + gitlab_main_partition = "#{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.security_findings_test_partition" + + expect(Gitlab::Database::LockWritesManager).not_to receive(:new).with( + hash_including(table_name: gitlab_main_partition) + ) + + subject + end + + it 'does not include any detached partitions' do + detached_partition_name = "_test_gitlab_main_part_20220101" + gitlab_main_detached_partition = "#{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.#{detached_partition_name}" + + expect(Gitlab::Database::LockWritesManager).not_to receive(:new).with( + hash_including(table_name: gitlab_main_detached_partition) + ) + + subject + end + end + context 'when running in dry_run mode' do subject { described_class.new(dry_run: true).lock_writes } |