diff options
Diffstat (limited to 'spec/lib/gitlab/database')
22 files changed, 624 insertions, 114 deletions
diff --git a/spec/lib/gitlab/database/background_migration/batched_background_migration_dictionary_spec.rb b/spec/lib/gitlab/database/background_migration/batched_background_migration_dictionary_spec.rb new file mode 100644 index 00000000000..b3aa0c194d2 --- /dev/null +++ b/spec/lib/gitlab/database/background_migration/batched_background_migration_dictionary_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Gitlab::Database::BackgroundMigration::BatchedBackgroundMigrationDictionary, feature_category: :database do + describe '.entry' do + it 'returns a single dictionary entry for the given migration job' do + entry = described_class.entry('MigrateHumanUserType') + expect(entry.migration_job_name).to eq('MigrateHumanUserType') + expect(entry.finalized_by).to eq(20230523101514) + end + end +end diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb index f70b38377d8..ffede2b6759 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -911,4 +911,18 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m expect(actual).to contain_exactly(migration) end end + + describe '#finalize_command' do + let_it_be(:migration) do + create( + :batched_background_migration, + gitlab_schema: :gitlab_main, + job_arguments: [['column_1'], ['column_1_convert_to_bigint']] + ) + end + + it 'generates the correct finalize command' do + expect(migration.finalize_command).to eq("sudo gitlab-rake gitlab:background_migrations:finalize[CopyColumnUsingBackgroundMigrationJob,events,id,'[[\"column_1\"]\\,[\"column_1_convert_to_bigint\"]]']") + end + end end diff --git a/spec/lib/gitlab/database/decomposition/migrate_spec.rb b/spec/lib/gitlab/database/decomposition/migrate_spec.rb new file mode 100644 index 00000000000..fa2248e8d84 --- /dev/null +++ b/spec/lib/gitlab/database/decomposition/migrate_spec.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Decomposition::Migrate, :delete, query_analyzers: false, feature_category: :cell do + let(:ci_database_name) do + config = ActiveRecord::Base.configurations.find_db_config(Rails.env).configuration_hash + + "#{config[:database]}_ci" + end + + let(:ci_connection) do + database_model = self.class.const_set(:TestCiApplicationRecord, Class.new(ApplicationRecord)) + + database_model.establish_connection( + ActiveRecord::DatabaseConfigurations::HashConfig.new( + ActiveRecord::Base.connection_db_config.env_name, + 'ci', + ActiveRecord::Base.connection_db_config.configuration_hash.dup.merge(database: ci_database_name) + ) + ) + + Gitlab::Database::LoadBalancing::Setup.new(database_model).setup + + database_model.connection + end + + let(:backup_location_postfix) { SecureRandom.alphanumeric(10) } + + before do + skip_if_database_exists(:ci) + + allow(SecureRandom).to receive(:alphanumeric).with(10).and_return(backup_location_postfix) + end + + after do + Milestone.delete_all + Ci::Pipeline.delete_all + end + + describe '#new' do + context 'when backup_location is not specified' do + subject(:instance) { described_class.new } + + it 'defaults to subdirectory of configured backup location' do + expect(instance.instance_variable_get(:@backup_location)).to eq( + File.join(Gitlab.config.backup.path, "migration_#{backup_location_postfix}") + ) + end + end + + context 'when backup_location is specified' do + let(:backup_base_location) { Rails.root.join('tmp') } + + subject(:instance) { described_class.new(backup_base_location: backup_base_location) } + + it 'uses subdirectory of specified backup_location' do + expect(instance.instance_variable_get(:@backup_location)).to eq( + File.join(backup_base_location, "migration_#{backup_location_postfix}") + ) + end + + context 'when specified_backup_location does not exist' do + let(:backup_base_location) { Rails.root.join('tmp', SecureRandom.alphanumeric(10)) } + + context 'and creation of the directory succeeds' do + it 'uses subdirectory of specified backup_location' do + expect(instance.instance_variable_get(:@backup_location)).to eq( + File.join(backup_base_location, "migration_#{backup_location_postfix}") + ) + end + end + + context 'and creation of the directory fails' do + before do + allow(FileUtils).to receive(:mkdir_p).with(backup_base_location).and_raise(Errno::EROFS.new) + end + + it 'raises error' do + expect { instance.process! }.to raise_error( + Gitlab::Database::Decomposition::MigrateError, + "Failed to create directory #{backup_base_location}: Read-only file system" + ) + end + end + end + end + end + + describe '#process!' do + subject(:process) { described_class.new.process! } + + before do + # Database `ci` is not configured. But it can still exist. So drop and create it + ActiveRecord::Base.connection.execute("DROP DATABASE IF EXISTS #{ci_database_name} WITH (FORCE)") + ActiveRecord::Base.connection.execute("CREATE DATABASE #{ci_database_name}") + end + + context 'when the checks pass' do + let!(:milestone) { create(:milestone) } + let!(:ci_pipeline) { create(:ci_pipeline) } + + it 'copies main database to ci database' do + process + + ci_milestones = ci_connection.execute("SELECT COUNT(*) FROM milestones").getvalue(0, 0) + ci_pipelines = ci_connection.execute("SELECT COUNT(*) FROM ci_pipelines").getvalue(0, 0) + + expect(ci_milestones).to be(Milestone.count) + expect(ci_pipelines).to be(Ci::Pipeline.count) + end + end + + context 'when local diskspace is not enough' do + let(:backup_location) { described_class.new.backup_location } + let(:fake_stats) { instance_double(Sys::Filesystem::Stat, bytes_free: 1000) } + + before do + allow(Sys::Filesystem).to receive(:stat).with(File.expand_path("#{backup_location}/../")).and_return(fake_stats) + end + + it 'raises error' do + expect { process }.to raise_error( + Gitlab::Database::Decomposition::MigrateError, + /Not enough diskspace available on #{backup_location}: Available: (.+?), Needed: (.+?)/ + ) + end + end + + context 'when connection to ci database fails' do + before do + ActiveRecord::Base.connection.execute("DROP DATABASE IF EXISTS #{ci_database_name} WITH (FORCE)") + end + + it 'raises error' do + host = ActiveRecord::Base.configurations.find_db_config(Rails.env).configuration_hash[:host] + expect { process }.to raise_error( + Gitlab::Database::Decomposition::MigrateError, + "Can't connect to database '#{ci_database_name} on host '#{host}'. Ensure the database has been created.") + end + end + + context 'when ci database is not empty' do + before do + ci_connection.execute("CREATE TABLE IF NOT EXISTS _test_table (id integer, primary key (id))") + end + + it 'raises error' do + expect { process }.to raise_error( + Gitlab::Database::Decomposition::MigrateError, + "Database '#{ci_database_name}' is not empty" + ) + end + end + + context 'when already on decomposed setup' do + before do + allow(Gitlab::Database).to receive(:database_mode).and_return(Gitlab::Database::MODE_MULTIPLE_DATABASES) + end + + it 'raises error' do + expect { process }.to raise_error( + Gitlab::Database::Decomposition::MigrateError, + "GitLab is already configured to run on multiple databases" + ) + end + end + + context 'when not all background migrations are finished' do + let!(:batched_migration) { create(:batched_background_migration, :active) } + + it 'raises error' do + expect { process }.to raise_error( + Gitlab::Database::Decomposition::MigrateError, + "Found 1 unfinished Background Migration(s). Please wait until they are finished." + ) + end + end + end +end diff --git a/spec/lib/gitlab/database/dictionary_spec.rb b/spec/lib/gitlab/database/dictionary_spec.rb index 6d2de41468b..261cf27ed69 100644 --- a/spec/lib/gitlab/database/dictionary_spec.rb +++ b/spec/lib/gitlab/database/dictionary_spec.rb @@ -3,81 +3,104 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Dictionary, feature_category: :database do - subject(:database_dictionary) { described_class.new(file_path) } + describe '.entries' do + it 'all tables and views are unique' do + table_and_view_names = described_class.entries('') + table_and_view_names += described_class.entries('views') + + # ignore gitlab_internal due to `ar_internal_metadata`, `schema_migrations` + table_and_view_names = table_and_view_names + .reject { |database_dictionary| database_dictionary.schema?('gitlab_internal') } + + duplicated_tables = table_and_view_names + .group_by(&:key_name) + .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 - context 'for a table' do - let(:file_path) { 'db/docs/application_settings.yml' } + describe '::Entry' do + subject(:database_dictionary) { described_class::Entry.new(file_path) } - describe '#name_and_schema' do - it 'returns the name of the table and its gitlab schema' do - expect(database_dictionary.name_and_schema).to match_array(['application_settings', :gitlab_main_clusterwide]) + context 'for a table' do + let(:file_path) { 'db/docs/application_settings.yml' } + + describe '#name_and_schema' do + it 'returns the name of the table and its gitlab schema' do + expect(database_dictionary.name_and_schema).to match_array(['application_settings', :gitlab_main_clusterwide]) + end end - end - describe '#table_name' do - it 'returns the name of the table' do - expect(database_dictionary.table_name).to eq('application_settings') + describe '#table_name' do + it 'returns the name of the table' do + expect(database_dictionary.table_name).to eq('application_settings') + end end - end - describe '#view_name' do - it 'returns nil' do - expect(database_dictionary.view_name).to be_nil + describe '#view_name' do + it 'returns nil' do + expect(database_dictionary.view_name).to be_nil + end end - end - describe '#milestone' do - it 'returns the milestone in which the table was introduced' do - expect(database_dictionary.milestone).to eq('7.7') + describe '#milestone' do + it 'returns the milestone in which the table was introduced' do + expect(database_dictionary.milestone).to eq('7.7') + end end - end - describe '#gitlab_schema' do - it 'returns the gitlab_schema of the table' do - expect(database_dictionary.table_name).to eq('application_settings') + describe '#gitlab_schema' do + it 'returns the gitlab_schema of the table' do + expect(database_dictionary.table_name).to eq('application_settings') + end end - end - describe '#schema?' do - it 'checks if the given schema matches the schema of the table' do - expect(database_dictionary.schema?('gitlab_main')).to eq(false) - expect(database_dictionary.schema?('gitlab_main_clusterwide')).to eq(true) + describe '#schema?' do + it 'checks if the given schema matches the schema of the table' do + expect(database_dictionary.schema?('gitlab_main')).to eq(false) + expect(database_dictionary.schema?('gitlab_main_clusterwide')).to eq(true) + end end - end - describe '#key_name' do - it 'returns the value of the name of the table' do - expect(database_dictionary.key_name).to eq('application_settings') + describe '#key_name' do + it 'returns the value of the name of the table' do + expect(database_dictionary.key_name).to eq('application_settings') + end end - end - describe '#validate!' do - it 'raises an error if the gitlab_schema is empty' do - allow(database_dictionary).to receive(:gitlab_schema).and_return(nil) + describe '#validate!' do + it 'raises an error if the gitlab_schema is empty' do + allow(database_dictionary).to receive(:gitlab_schema).and_return(nil) - expect { database_dictionary.validate! }.to raise_error(Gitlab::Database::GitlabSchema::UnknownSchemaError) + expect { database_dictionary.validate! }.to raise_error(Gitlab::Database::GitlabSchema::UnknownSchemaError) + end end end - end - context 'for a view' do - let(:file_path) { 'db/docs/views/postgres_constraints.yml' } + context 'for a view' do + let(:file_path) { 'db/docs/views/postgres_constraints.yml' } - describe '#table_name' do - it 'returns nil' do - expect(database_dictionary.table_name).to be_nil + describe '#table_name' do + it 'returns nil' do + expect(database_dictionary.table_name).to be_nil + end end - end - describe '#view_name' do - it 'returns the name of the view' do - expect(database_dictionary.view_name).to eq('postgres_constraints') + describe '#view_name' do + it 'returns the name of the view' do + expect(database_dictionary.view_name).to eq('postgres_constraints') + end end - end - describe '#key_name' do - it 'returns the value of the name of the view' do - expect(database_dictionary.key_name).to eq('postgres_constraints') + describe '#key_name' do + it 'returns the value of the name of the view' do + expect(database_dictionary.key_name).to eq('postgres_constraints') + end end end end diff --git a/spec/lib/gitlab/database/gitlab_schema_spec.rb b/spec/lib/gitlab/database/gitlab_schema_spec.rb index a47e53c18a5..7fca47c707c 100644 --- a/spec/lib/gitlab/database/gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/gitlab_schema_spec.rb @@ -1,13 +1,6 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.shared_examples 'validate path globs' do |path_globs| - it 'returns an array of path globs' do - expect(path_globs).to be_an(Array) - expect(path_globs).to all(be_an(Pathname)) - end -end - RSpec.shared_examples 'validate schema data' do |tables_and_views| it 'all tables and views have assigned a known gitlab_schema' do expect(tables_and_views).to all( @@ -88,32 +81,6 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do end end 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') - - # ignore gitlab_internal due to `ar_internal_metadata`, `schema_migrations` - table_and_view_names = table_and_view_names - .reject { |database_dictionary| database_dictionary.schema?('gitlab_internal') } - - duplicated_tables = table_and_view_names - .group_by(&:key_name) - .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 '.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 @@ -306,4 +273,16 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do end end end + + describe '.cell_local?' do + it 'is true for cell local tables and false otherwise' do + expect(described_class.cell_local?('gitlab_ci')).to eq(true) + expect(described_class.cell_local?('gitlab_pm')).to eq(true) + expect(described_class.cell_local?('gitlab_main_cell')).to eq(true) + expect(described_class.cell_local?('gitlab_main')).to eq(false) + expect(described_class.cell_local?('gitlab_main_clusterwide')).to eq(false) + expect(described_class.cell_local?('gitlab_shared')).to eq(false) + expect(described_class.cell_local?('gitlab_internal')).to eq(false) + end + end end diff --git a/spec/lib/gitlab/database/health_status/indicators/autovacuum_active_on_table_spec.rb b/spec/lib/gitlab/database/health_status/indicators/autovacuum_active_on_table_spec.rb index cd145bd5c0f..328cdede794 100644 --- a/spec/lib/gitlab/database/health_status/indicators/autovacuum_active_on_table_spec.rb +++ b/spec/lib/gitlab/database/health_status/indicators/autovacuum_active_on_table_spec.rb @@ -19,6 +19,7 @@ RSpec.describe Gitlab::Database::HealthStatus::Indicators::AutovacuumActiveOnTab before do swapout_view_for_table(:postgres_autovacuum_activity, connection: connection) + stub_feature_flags(skip_autovacuum_health_check_for_ci_builds: false) end let(:tables) { [table] } @@ -59,10 +60,34 @@ RSpec.describe Gitlab::Database::HealthStatus::Indicators::AutovacuumActiveOnTab expect(subject.indicator_class).to eq(described_class) end - it 'returns NoSignal signal in case the feature flag is disabled' do - stub_feature_flags(batched_migrations_health_status_autovacuum: false) + context 'with specific feature flags' do + it 'returns NotAvailable on batched_migrations_health_status_autovacuum FF being disable' do + stub_feature_flags(batched_migrations_health_status_autovacuum: false) - expect(subject).to be_a(Gitlab::Database::HealthStatus::Signals::NotAvailable) + expect(subject).to be_a(Gitlab::Database::HealthStatus::Signals::NotAvailable) + end + + context 'with skip_autovacuum_health_check_for_ci_builds FF being enabled' do + before do + stub_feature_flags(skip_autovacuum_health_check_for_ci_builds: true) + end + + context 'for ci_builds table' do + let(:table) { 'ci_builds' } + + it 'returns NotAvailable' do + expect(subject).to be_a(Gitlab::Database::HealthStatus::Signals::NotAvailable) + end + end + + context 'for users table' do + let(:table) { 'users' } + + it 'returns Stop signal' do + expect(subject).to be_a(Gitlab::Database::HealthStatus::Signals::Stop) + end + end + end end end end diff --git a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb index c975f5b5ee4..3c14dc23a80 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -92,8 +92,20 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store, fe end end + shared_examples 'restrict within concurrent ruby' do |lb_method| + it 'raises an exception when running within a concurrent Ruby thread' do + Thread.current[:restrict_within_concurrent_ruby] = true + + expect { |b| lb.public_send(lb_method, &b) }.to raise_error(Gitlab::Utils::ConcurrentRubyThreadIsUsedError, + "Cannot run 'db' if running from `Concurrent::Promise`.") + + Thread.current[:restrict_within_concurrent_ruby] = nil + end + end + describe '#read' do it_behaves_like 'logs service discovery thread interruption', :read + it_behaves_like 'restrict within concurrent ruby', :read it 'yields a connection for a read' do connection = double(:connection) @@ -227,6 +239,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store, fe describe '#read_write' do it_behaves_like 'logs service discovery thread interruption', :read_write + it_behaves_like 'restrict within concurrent ruby', :read_write it 'yields a connection for a write' do connection = ActiveRecord::Base.connection_pool.connection diff --git a/spec/lib/gitlab/database/migration_spec.rb b/spec/lib/gitlab/database/migration_spec.rb index 18bbc6c1dd3..8390a5ff19e 100644 --- a/spec/lib/gitlab/database/migration_spec.rb +++ b/spec/lib/gitlab/database/migration_spec.rb @@ -34,6 +34,12 @@ RSpec.describe Gitlab::Database::Migration do # untouched. expect(described_class[described_class.current_version]).to be < ActiveRecord::Migration::Current end + + it 'matches the version used by Rubocop' do + require 'rubocop' + load 'rubocop/cop/migration/versioned_migration_class.rb' + expect(described_class.current_version).to eq(RuboCop::Cop::Migration::VersionedMigrationClass::CURRENT_MIGRATION_VERSION) + end end describe Gitlab::Database::Migration::LockRetriesConcern do 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 a81ccf9583a..5c98379d852 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 @@ -71,8 +71,11 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers, end context "when the migration doesn't exist already" do + let(:version) { '20231204101122' } + before do allow(Gitlab::Database::PgClass).to receive(:for_table).with(:projects).and_return(pgclass_info) + allow(migration).to receive(:version).and_return(version) end subject(:enqueue_batched_background_migration) do @@ -81,7 +84,6 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers, :projects, :id, job_interval: 5.minutes, - queued_migration_version: format("%.14d", 123), batch_min_value: 5, batch_max_value: 1000, batch_class_name: 'MyBatchClass', @@ -115,7 +117,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers, status_name: :active, total_tuple_count: pgclass_info.cardinality_estimate, gitlab_schema: 'gitlab_ci', - queued_migration_version: format("%.14d", 123) + queued_migration_version: version ) end end diff --git a/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb b/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb index 33e83ea2575..a9ef28a4b51 100644 --- a/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb +++ b/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb @@ -18,6 +18,17 @@ RSpec.describe Gitlab::Database::Migrations::PgBackendPid, feature_category: :da expect { |b| patched_instance.with_advisory_lock_connection(&b) }.to yield_with_args(:conn) end + + it 're-yields with same arguments and wraps it with calls to .say even when error is raised' do + patched_instance = klass.prepend(described_class).new + expect(Gitlab::Database::Migrations::PgBackendPid).to receive(:say).twice + + expect do + patched_instance.with_advisory_lock_connection do + raise ActiveRecord::ConcurrentMigrationError, 'test' + end + end.to raise_error ActiveRecord::ConcurrentMigrationError + end end describe '.patch!' do 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 c57b8bb5992..60934eb06a5 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 @@ -11,7 +11,6 @@ RSpec.describe 'cross-database foreign keys' do # should be added as a comment along with the name of the column. let!(:allowed_cross_database_foreign_keys) do [ - 'events.author_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/429803 'gitlab_subscriptions.hosted_plan_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422012 'group_import_states.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/421210 'identities.saml_provider_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422010 @@ -27,10 +26,8 @@ RSpec.describe 'cross-database foreign keys' do 'namespace_commit_emails.email_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/429804 'namespace_commit_emails.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/429804 'path_locks.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/429380 - 'project_authorizations.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422044 'protected_branch_push_access_levels.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/431054 'protected_branch_merge_access_levels.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/431055 - 'security_orchestration_policy_configurations.bot_user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/429438 'user_group_callouts.user_id' # https://gitlab.com/gitlab-org/gitlab/-/issues/421287 ] end @@ -59,4 +56,17 @@ RSpec.describe 'cross-database foreign keys' do end end end + + it 'only allows existing foreign keys to be present in the exempted list', :aggregate_failures do + allowed_cross_database_foreign_keys.each do |entry| + table, _ = entry.split('.') + + all_foreign_keys_for_table = foreign_keys_for(table) + fk_entry = all_foreign_keys_for_table.find { |fk| "#{fk.from_table}.#{fk.column}" == entry } + + expect(fk_entry).to be_present, + "`#{entry}` is no longer a foreign key. " \ + "You must remove this entry from the `allowed_cross_database_foreign_keys` list." + end + end end diff --git a/spec/lib/gitlab/database/no_new_tables_with_gitlab_main_schema_spec.rb b/spec/lib/gitlab/database/no_new_tables_with_gitlab_main_schema_spec.rb index 338475fa9c4..d1d7aa12c46 100644 --- a/spec/lib/gitlab/database/no_new_tables_with_gitlab_main_schema_spec.rb +++ b/spec/lib/gitlab/database/no_new_tables_with_gitlab_main_schema_spec.rb @@ -11,7 +11,9 @@ RSpec.describe 'new tables with gitlab_main schema', feature_category: :cell do # Specific tables can be exempted from this requirement, and such tables must be added to the `exempted_tables` list. let!(:exempted_tables) do - [] + [ + "audit_events_instance_amazon_s3_configurations" # https://gitlab.com/gitlab-org/gitlab/-/issues/431327 + ] end let!(:starting_from_milestone) { 16.7 } @@ -48,16 +50,16 @@ RSpec.describe 'new tables with gitlab_main schema', feature_category: :cell do end def tables_having_gitlab_main_schema(starting_from_milestone:) - selected_data = gitlab_main_schema_tables.select do |database_dictionary| - database_dictionary.milestone.to_f >= starting_from_milestone + selected_data = gitlab_main_schema_tables.select do |entry| + entry.milestone.to_f >= starting_from_milestone end selected_data.map(&:table_name) end def gitlab_main_schema_tables - ::Gitlab::Database::GitlabSchema.build_dictionary('').select do |database_dictionary| - database_dictionary.schema?('gitlab_main') + ::Gitlab::Database::Dictionary.entries.select do |entry| + entry.schema?('gitlab_main') end end end diff --git a/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb b/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb index fa7645d581c..56899924b60 100644 --- a/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb +++ b/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb @@ -53,11 +53,11 @@ RSpec.describe Gitlab::Database::ObsoleteIgnoredColumns, feature_category: :data expect(subject.execute).to eq( [ ['Testing::A', { - 'unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0'), - 'also_unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-02-01'), '12.1') + 'unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0', false), + 'also_unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-02-01'), '12.1', false) }], ['Testing::B', { - 'other' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0') + 'other' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0', false) }] ]) end diff --git a/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb index 79c2c9e32d2..337749446ed 100644 --- a/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb @@ -6,8 +6,8 @@ RSpec.describe Gitlab::Database::Partitioning::CiSlidingListStrategy, feature_ca let(:connection) { ActiveRecord::Base.connection } let(:table_name) { :_test_gitlab_ci_partitioned_test } let(:model) { class_double(ApplicationRecord, table_name: table_name, connection: connection) } - let(:next_partition_if) { nil } - let(:detach_partition_if) { nil } + let(:next_partition_if) { ->(_) { false } } + let(:detach_partition_if) { ->(_) { false } } subject(:strategy) do described_class.new(model, :partition, @@ -62,6 +62,16 @@ RSpec.describe Gitlab::Database::Partitioning::CiSlidingListStrategy, feature_ca it 'is the partition with the largest value' do expect(strategy.active_partition.value).to eq(101) end + + context 'when there are no partitions' do + before do + drop_partitions + end + + it 'is the initial partition' do + expect(strategy.active_partition.value).to eq(100) + end + end end describe '#missing_partitions' do @@ -74,6 +84,17 @@ RSpec.describe Gitlab::Database::Partitioning::CiSlidingListStrategy, feature_ca expect(extra.length).to eq(1) expect(extra.first.value).to eq(102) end + + context 'when there are no partitions for the table' do + it 'returns partitions for value 100 and 101' do + drop_partitions + + missing_partitions = strategy.missing_partitions + + expect(missing_partitions.size).to eq(2) + expect(missing_partitions.map(&:value)).to match_array([100, 101]) + end + end end context 'when next_partition_if returns false' do @@ -85,8 +106,8 @@ RSpec.describe Gitlab::Database::Partitioning::CiSlidingListStrategy, feature_ca end context 'when there are no partitions for the table' do - it 'returns a partition for value 1' do - connection.execute("drop table #{table_name}_100; drop table #{table_name}_101;") + it 'returns a partition for value 100' do + drop_partitions missing_partitions = strategy.missing_partitions @@ -201,4 +222,8 @@ RSpec.describe Gitlab::Database::Partitioning::CiSlidingListStrategy, feature_ca }) end end + + def drop_partitions + connection.execute("drop table #{table_name}_100; drop table #{table_name}_101;") + end end diff --git a/spec/lib/gitlab/database/postgres_index_spec.rb b/spec/lib/gitlab/database/postgres_index_spec.rb index 2e654a33a58..fd2455e25c0 100644 --- a/spec/lib/gitlab/database/postgres_index_spec.rb +++ b/spec/lib/gitlab/database/postgres_index_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::PostgresIndex do +RSpec.describe Gitlab::Database::PostgresIndex, feature_category: :database do let(:schema) { 'public' } let(:name) { 'foo_idx' } let(:identifier) { "#{schema}.#{name}" } @@ -13,6 +13,9 @@ RSpec.describe Gitlab::Database::PostgresIndex do CREATE UNIQUE INDEX bar_key ON public.users (id); CREATE TABLE _test_gitlab_main_example_table (id serial primary key); + + CREATE TABLE _test_partitioned (id bigserial primary key not null) PARTITION BY LIST (id); + CREATE TABLE _test_partitioned_1 PARTITION OF _test_partitioned FOR VALUES IN (1); SQL end @@ -25,8 +28,8 @@ RSpec.describe Gitlab::Database::PostgresIndex do it { is_expected.to be_a Gitlab::Database::SharedModel } describe '.reindexing_support' do - it 'only non partitioned indexes' do - expect(described_class.reindexing_support).to all(have_attributes(partitioned: false)) + it 'includes partitioned indexes' do + expect(described_class.reindexing_support.where("name = '_test_partitioned_1_pkey'")).not_to be_empty end it 'only indexes that dont serve an exclusion constraint' do diff --git a/spec/lib/gitlab/database/postgres_sequences_spec.rb b/spec/lib/gitlab/database/postgres_sequences_spec.rb new file mode 100644 index 00000000000..2373edaea18 --- /dev/null +++ b/spec/lib/gitlab/database/postgres_sequences_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::PostgresSequence, type: :model, feature_category: :database do + # PostgresSequence does not `behaves_like 'a postgres model'` because it does not correspond 1-1 with a single entry + # in pg_class + let(:schema) { ActiveRecord::Base.connection.current_schema } + let(:table_name) { '_test_table' } + let(:table_name_without_sequence) { '_test_table_without_sequence' } + + before do + ActiveRecord::Base.connection.execute(<<~SQL) + CREATE TABLE #{table_name} ( + id bigserial PRIMARY KEY NOT NULL + ); + + CREATE TABLE #{table_name_without_sequence} ( + id bigint PRIMARY KEY NOT NULL + ); + SQL + end + + describe '#by_table_name' do + context 'when table does not have a sequence' do + it 'returns an empty collection' do + expect(described_class.by_table_name(table_name_without_sequence)).to be_empty + end + end + + it 'returns the sequence for a given table' do + expect(described_class.by_table_name(table_name).first[:table_name]).to eq(table_name) + end + end +end diff --git a/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb b/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb index 3650ca1d904..9570a25238e 100644 --- a/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb +++ b/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb @@ -15,7 +15,9 @@ RSpec.describe Gitlab::Database::PostgresqlAdapter::ForceDisconnectableMixin, :d end let(:config) { ActiveRecord::Base.configurations.find_db_config(Rails.env).configuration_hash.merge(pool: 1) } - let(:pool) { model.establish_connection(config) } + let(:pool) do + model.establish_connection(ActiveRecord::DatabaseConfigurations::HashConfig.new(Rails.env, 'main', config)) + end it 'calls the force disconnect callback on checkin' do connection = pool.connection diff --git a/spec/lib/gitlab/database/query_analyzers/prevent_set_operator_mismatch_spec.rb b/spec/lib/gitlab/database/query_analyzers/prevent_set_operator_mismatch_spec.rb index 28c155c1eb1..7fcdc59b691 100644 --- a/spec/lib/gitlab/database/query_analyzers/prevent_set_operator_mismatch_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/prevent_set_operator_mismatch_spec.rb @@ -42,12 +42,21 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventSetOperatorMismatch, que end context 'when SQL does not include a set operator' do - let(:sql) { 'SELECT 1' } + where(:sql) do + [ + 'SELECT 1', + 'SELECT union_station', + 'SELECT intersection', + 'SELECT deny_all_requests_except_allowed from application_settings' + ] + end - it 'does not parse SQL' do - expect(described_class::SelectStmt).not_to receive(:new) + with_them do + it 'does not parse SQL' do + expect(described_class::SelectStmt).not_to receive(:new) - process_sql sql + process_sql sql + end end end diff --git a/spec/lib/gitlab/database/schema_migrations/context_spec.rb b/spec/lib/gitlab/database/schema_migrations/context_spec.rb index 6a614e2488f..ed83ed9e744 100644 --- a/spec/lib/gitlab/database/schema_migrations/context_spec.rb +++ b/spec/lib/gitlab/database/schema_migrations/context_spec.rb @@ -25,12 +25,15 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do context 'multiple databases', :reestablished_active_record_base do before do - connection_class.establish_connection( + db_config = ActiveRecord::Base .connection_pool .db_config .configuration_hash .merge(configuration_overrides) + + connection_class.establish_connection( + ActiveRecord::DatabaseConfigurations::HashConfig.new(Rails.env, 'main', db_config) ) end diff --git a/spec/lib/gitlab/database/sharding_key_spec.rb b/spec/lib/gitlab/database/sharding_key_spec.rb new file mode 100644 index 00000000000..b47f5ea5df0 --- /dev/null +++ b/spec/lib/gitlab/database/sharding_key_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'new tables missing sharding_key', feature_category: :cell do + # Specific tables can be temporarily exempt from this requirement. You must add an issue link in a comment next to + # the table name to remove this once a decision has been made. + let(:allowed_to_be_missing_sharding_key) do + [ + 'abuse_report_assignees', # https://gitlab.com/gitlab-org/gitlab/-/issues/432365 + 'sbom_occurrences_vulnerabilities' # https://gitlab.com/gitlab-org/gitlab/-/issues/432900 + ] + end + + # Specific tables can be temporarily exempt from this requirement. You must add an issue link in a comment next to + # the table name to remove this once a decision has been made. + let(:allowed_to_be_missing_not_null) do + [ + 'labels.project_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/434356 + 'labels.group_id' # https://gitlab.com/gitlab-org/gitlab/-/issues/434356 + ] + end + + let(:starting_from_milestone) { 16.6 } + + let(:allowed_sharding_key_referenced_tables) { %w[projects namespaces organizations] } + + it 'requires a sharding_key for all cell-local tables, after milestone 16.6', :aggregate_failures do + tables_missing_sharding_key(starting_from_milestone: starting_from_milestone).each do |table_name| + expect(allowed_to_be_missing_sharding_key).to include(table_name), error_message(table_name) + end + end + + it 'ensures all sharding_key columns exist and reference projects, namespaces or organizations', + :aggregate_failures do + all_tables_to_sharding_key.each do |table_name, sharding_key| + sharding_key.each do |column_name, referenced_table_name| + expect(column_exists?(table_name, column_name)).to eq(true), + "Could not find sharding key column #{table_name}.#{column_name}" + expect(referenced_table_name).to be_in(allowed_sharding_key_referenced_tables) + end + end + end + + it 'ensures all sharding_key columns are not nullable or have a not null check constraint', + :aggregate_failures do + all_tables_to_sharding_key.each do |table_name, sharding_key| + sharding_key.each do |column_name, _| + not_nullable = not_nullable?(table_name, column_name) + has_null_check_constraint = has_null_check_constraint?(table_name, column_name) + + if allowed_to_be_missing_not_null.include?("#{table_name}.#{column_name}") + expect(not_nullable || has_null_check_constraint).to eq(false), + "You must remove `#{table_name}.#{column_name}` from allowed_to_be_missing_not_null" \ + "since it now has a valid constraint." + else + expect(not_nullable || has_null_check_constraint).to eq(true), + "Missing a not null constraint for `#{table_name}.#{column_name}` . " \ + "All sharding keys must be not nullable or have a NOT NULL check constraint" + end + end + end + end + + it 'only allows `allowed_to_be_missing_sharding_key` to include tables that are missing a sharding_key', + :aggregate_failures do + allowed_to_be_missing_sharding_key.each do |exempted_table| + expect(tables_missing_sharding_key(starting_from_milestone: starting_from_milestone)).to include(exempted_table), + "`#{exempted_table}` is not missing a `sharding_key`. " \ + "You must remove this table from the `allowed_to_be_missing_sharding_key` list." + end + end + + private + + def error_message(table_name) + <<~HEREDOC + The table `#{table_name}` is missing a `sharding_key` in the `db/docs` YML file. + Starting from GitLab #{starting_from_milestone}, we expect all new tables to define a `sharding_key`. + + To choose an appropriate sharding_key for this table please refer + to our guidelines at https://docs.gitlab.com/ee/development/database/multiple_databases.html#defining-a-sharding-key-for-all-cell-local-tables, or consult with the Tenant Scale group. + HEREDOC + end + + def tables_missing_sharding_key(starting_from_milestone:) + ::Gitlab::Database::Dictionary.entries.select do |entry| + entry.sharding_key.blank? && + entry.milestone.to_f >= starting_from_milestone && + ::Gitlab::Database::GitlabSchema.cell_local?(entry.gitlab_schema) + end.map(&:table_name) + end + + def all_tables_to_sharding_key + entries_with_sharding_key = ::Gitlab::Database::Dictionary.entries.select do |entry| + entry.sharding_key.present? + end + + entries_with_sharding_key.to_h do |entry| + [entry.table_name, entry.sharding_key] + end + end + + def not_nullable?(table_name, column_name) + sql = <<~SQL + SELECT 1 + FROM information_schema.columns + WHERE table_schema = 'public' AND + table_name = '#{table_name}' AND + column_name = '#{column_name}' AND + is_nullable = 'NO' + SQL + + result = ApplicationRecord.connection.execute(sql) + + result.count > 0 + end + + def has_null_check_constraint?(table_name, column_name) + # This is a heuristic query to look for all check constraints on the table and see if any of them contain a clause + # column IS NOT NULL. This is to match tables that will have multiple sharding keys where either of them can be not + # null. Such cases may look like: + # (project_id IS NOT NULL) OR (group_id IS NOT NULL) + # It's possible that this will sometimes incorrectly find a check constraint that isn't exactly as strict as we want + # but it should be pretty unlikely. + sql = <<~SQL + SELECT 1 + FROM pg_constraint + INNER JOIN pg_class ON pg_constraint.conrelid = pg_class.oid + WHERE pg_class.relname = '#{table_name}' + AND contype = 'c' + AND pg_get_constraintdef(pg_constraint.oid) ILIKE '%#{column_name} IS NOT NULL%' + SQL + + result = ApplicationRecord.connection.execute(sql) + + result.count > 0 + end + + def column_exists?(table_name, column_name) + sql = <<~SQL + SELECT 1 + FROM information_schema.columns + WHERE table_schema = 'public' AND + table_name = '#{table_name}' AND + column_name = '#{column_name}'; + SQL + + result = ApplicationRecord.connection.execute(sql) + + result.count > 0 + end +end diff --git a/spec/lib/gitlab/database/transaction/observer_spec.rb b/spec/lib/gitlab/database/transaction/observer_spec.rb index 778212add66..2d5a59a2d5d 100644 --- a/spec/lib/gitlab/database/transaction/observer_spec.rb +++ b/spec/lib/gitlab/database/transaction/observer_spec.rb @@ -21,6 +21,8 @@ RSpec.describe Gitlab::Database::Transaction::Observer, feature_category: :datab it 'tracks transaction data', :aggregate_failures do ActiveRecord::Base.transaction do + User.first + ActiveRecord::Base.transaction(requires_new: true) do User.first diff --git a/spec/lib/gitlab/database/with_lock_retries_spec.rb b/spec/lib/gitlab/database/with_lock_retries_spec.rb index 7e0435c815b..624e2b5c144 100644 --- a/spec/lib/gitlab/database/with_lock_retries_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_spec.rb @@ -244,9 +244,9 @@ RSpec.describe Gitlab::Database::WithLockRetries, feature_category: :database do it 'executes `SET LOCAL lock_timeout` using the configured timeout value in milliseconds' do expect(connection).to receive(:execute).with("RESET idle_in_transaction_session_timeout; RESET lock_timeout").and_call_original - expect(connection).to receive(:execute).with("SAVEPOINT active_record_1", "TRANSACTION").and_call_original + expect(connection).to receive(:create_savepoint).with('active_record_1') expect(connection).to receive(:execute).with("SET LOCAL lock_timeout TO '15ms'").and_call_original - expect(connection).to receive(:execute).with("RELEASE SAVEPOINT active_record_1", "TRANSACTION").and_call_original + expect(connection).to receive(:release_savepoint).with('active_record_1') subject.run {} end |