Skip to content
Snippets Groups Projects
Verified Commit b57b31dd authored by Omar Qunsul's avatar Omar Qunsul :two:
Browse files

Fixing Database/MultipleDatabases offenses in specs

We still have many ignored Rubocop Offenses for
Database/MultipleDatabases in specs. This is a MR
to address validate and fix all of them.

Addressing Issue: #362031

Changelog: other
parent 9147f60c
No related branches found
No related tags found
1 merge request!107967Fixing Database/MultipleDatabases offenses in specs - Part 2
Showing
with 61 additions and 72 deletions
......@@ -409,6 +409,7 @@ Database/MultipleDatabases:
- 'ee/lib/ee/gitlab/background_migration/**/*.rb'
- 'spec/lib/gitlab/background_migration/**/*.rb'
- 'spec/lib/gitlab/database/**/*.rb'
- 'spec/tasks/gitlab/db_rake_spec.rb'
Migration/BatchMigrationsPostOnly:
Enabled: true
......
......@@ -5,10 +5,3 @@ Database/MultipleDatabases:
- 'db/post_migrate/20210811122206_update_external_project_bots.rb'
- 'db/post_migrate/20210812013042_remove_duplicate_project_authorizations.rb'
- 'ee/spec/services/ee/merge_requests/update_service_spec.rb'
- 'spec/support/caching.rb'
- 'spec/support/helpers/database/database_helpers.rb'
- 'spec/support/helpers/database/table_schema_helpers.rb'
- 'spec/support/helpers/migrations_helpers.rb'
- 'spec/support/helpers/query_recorder.rb'
- 'spec/support/helpers/usage_data_helpers.rb'
- 'spec/tasks/gitlab/db_rake_spec.rb'
......@@ -3156,7 +3156,6 @@ Layout/LineLength:
- 'lib/gitlab/database/reflection.rb'
- 'lib/gitlab/database/reindexing.rb'
- 'lib/gitlab/database/reindexing/coordinator.rb'
- 'lib/gitlab/database/reindexing/grafana_notifier.rb'
- 'lib/gitlab/database/reindexing/reindex_concurrently.rb'
- 'lib/gitlab/database/schema_migrations/context.rb'
- 'lib/gitlab/database/similarity_score.rb'
......@@ -4458,7 +4457,6 @@ Layout/LineLength:
- 'spec/lib/gitlab/database/query_analyzer_spec.rb'
- 'spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb'
- 'spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb'
- 'spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb'
- 'spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb'
- 'spec/lib/gitlab/database/reindexing_spec.rb'
- 'spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb'
......
......@@ -60,7 +60,9 @@ def annotate(payload)
"Authorization": "Bearer #{@api_key}"
}
success = Gitlab::HTTP.post("#{@api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true).success?
success = Gitlab::HTTP.post(
"#{@api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true
).success?
log_error("Response code #{response.code}") unless success
......
......@@ -2,7 +2,8 @@
require 'spec_helper'
RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::AutovacuumActiveOnTable do
RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::AutovacuumActiveOnTable,
feature_category: :database do
include Database::DatabaseHelpers
let(:connection) { Gitlab::Database.database_base_models[:main].connection }
......@@ -17,7 +18,7 @@
subject { described_class.new(context).evaluate }
before do
swapout_view_for_table(:postgres_autovacuum_activity)
swapout_view_for_table(:postgres_autovacuum_activity, connection: connection)
end
let(:tables) { [table] }
......
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model do
RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model, feature_category: :database do
include Database::DatabaseHelpers
it { is_expected.to be_a Gitlab::Database::SharedModel }
......@@ -13,7 +13,7 @@
let(:tables) { %w[foo test] }
before do
swapout_view_for_table(:postgres_autovacuum_activity)
swapout_view_for_table(:postgres_autovacuum_activity, connection: ApplicationRecord.connection)
# unrelated
create(:postgres_autovacuum_activity, table: 'bar')
......
......@@ -2,13 +2,15 @@
require 'spec_helper'
RSpec.describe Gitlab::Database::Reindexing::Coordinator do
RSpec.describe Gitlab::Database::Reindexing::Coordinator, feature_category: :database do
include Database::DatabaseHelpers
include ExclusiveLeaseHelpers
let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) }
let(:index) { create(:postgres_index) }
let(:connection) { index.connection }
let(:notifier) do
instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil)
end
let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) }
let(:lease_key) { "gitlab/database/reindexing/coordinator/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" }
......@@ -19,14 +21,11 @@
model = Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME]
Gitlab::Database::SharedModel.using_connection(model.connection) do
swapout_view_for_table(:postgres_indexes, connection: model.connection)
example.run
end
end
before do
swapout_view_for_table(:postgres_indexes)
end
describe '#perform' do
subject { described_class.new(index, notifier).perform }
......
......@@ -12,7 +12,7 @@
let(:action) { create(:reindex_action) }
before do
swapout_view_for_table(:postgres_indexes)
swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection)
end
let(:headers) do
......@@ -25,7 +25,9 @@
let(:response) { double('response', success?: true) }
def expect_api_call(payload)
expect(Gitlab::HTTP).to receive(:post).with("#{api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true).and_return(response)
expect(Gitlab::HTTP).to receive(:post).with(
"#{api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true
).and_return(response)
end
shared_examples_for 'interacting with Grafana annotations API' do
......@@ -109,7 +111,9 @@ def expect_api_call(payload)
end
context 'additional tag is provided' do
subject { described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_start(action) }
subject do
described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_start(action)
end
let(:payload) do
{
......@@ -163,7 +167,9 @@ def expect_api_call(payload)
end
context 'additional tag is provided' do
subject { described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_end(action) }
subject do
described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_end(action)
end
let(:payload) do
{
......
......@@ -2,14 +2,16 @@
require 'spec_helper'
RSpec.describe Gitlab::Database::Reindexing::IndexSelection do
RSpec.describe Gitlab::Database::Reindexing::IndexSelection, feature_category: :database do
include Database::DatabaseHelpers
subject { described_class.new(Gitlab::Database::PostgresIndex.all).to_a }
let(:connection) { ApplicationRecord.connection }
before do
swapout_view_for_table(:postgres_index_bloat_estimates)
swapout_view_for_table(:postgres_indexes)
swapout_view_for_table(:postgres_index_bloat_estimates, connection: connection)
swapout_view_for_table(:postgres_indexes, connection: connection)
create_list(:postgres_index, 10, ondisk_size_bytes: 10.gigabytes).each_with_index do |index, i|
create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 2.gigabyte * (i + 1))
......@@ -17,7 +19,7 @@
end
def execute(sql)
ActiveRecord::Base.connection.execute(sql)
connection.execute(sql)
end
it 'orders by highest relative bloat first' do
......
......@@ -2,13 +2,13 @@
require 'spec_helper'
RSpec.describe Gitlab::Database::Reindexing::ReindexAction do
RSpec.describe Gitlab::Database::Reindexing::ReindexAction, feature_category: :database do
include Database::DatabaseHelpers
let(:index) { create(:postgres_index) }
before_all do
swapout_view_for_table(:postgres_indexes)
swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection)
end
it { is_expected.to be_a Gitlab::Database::SharedModel }
......
......@@ -76,7 +76,7 @@
let(:limit) { 5 }
before_all do
swapout_view_for_table(:postgres_indexes)
swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection)
end
before do
......@@ -147,7 +147,7 @@
subject { described_class.perform_from_queue(maximum_records: limit) }
before_all do
swapout_view_for_table(:postgres_indexes)
swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection)
end
let(:limit) { 2 }
......
......@@ -374,7 +374,7 @@ def expect_error(exception, message, &block)
context 'when query timeout' do
subject do
with_statement_timeout(0.001) do
with_statement_timeout(0.001, connection: ApplicationRecord.connection) do
relation = AlertManagement::HttpIntegration.select('pg_sleep(0.002)')
described_class.histogram(relation, column, buckets: 1..100)
end
......
......@@ -188,7 +188,13 @@
before do
factories_based_on_view.each do |factory|
view = build(factory).class.table_name
swapout_view_for_table(view)
view_gitlab_schema = Gitlab::Database::GitlabSchema.table_schema(view)
Gitlab::Database.database_base_models.each_value.select do |base_model|
connection = base_model.connection
next unless Gitlab::Database.gitlab_schemas_for_connection(connection).include?(view_gitlab_schema)
swapout_view_for_table(view, connection: connection)
end
end
end
......
......@@ -37,8 +37,8 @@
end
config.around(:each, :use_sql_query_cache) do |example|
ActiveRecord::Base.cache do
example.run
end
base_models = Gitlab::Database.database_base_models_with_gitlab_shared.values
inner_proc = proc { example.run }
base_models.inject(inner_proc) { |proc, base_model| proc { base_model.cache { proc.call } } }.call
end
end
......@@ -4,9 +4,7 @@ module Database
module DatabaseHelpers
# In order to directly work with views using factories,
# we can swapout the view for a table of identical structure.
def swapout_view_for_table(view, connection: nil)
connection ||= ActiveRecord::Base.connection
def swapout_view_for_table(view, connection:)
connection.execute(<<~SQL.squish)
CREATE TABLE #{view}_copy (LIKE #{view});
DROP VIEW #{view};
......@@ -28,21 +26,20 @@ def swapout_view_for_table(view, connection: nil)
# with_statement_timeout(0.1) do
# model.select('pg_sleep(0.11)')
# end
def with_statement_timeout(timeout)
def with_statement_timeout(timeout, connection:)
# Force a positive value and a minimum of 1ms for very small values.
timeout = (timeout * 1000).abs.ceil
raise ArgumentError, 'Using a timeout of `0` means to disable statement timeout.' if timeout == 0
previous_timeout = ActiveRecord::Base.connection
.exec_query('SHOW statement_timeout')[0].fetch('statement_timeout')
previous_timeout = connection.select_value('SHOW statement_timeout')
set_statement_timeout("#{timeout}ms")
connection.execute(format(%(SET LOCAL statement_timeout = '%s'), timeout))
yield
ensure
begin
set_statement_timeout(previous_timeout)
connection.execute(format(%(SET LOCAL statement_timeout = '%s'), previous_timeout))
rescue ActiveRecord::StatementInvalid
# After a transaction was canceled/aborted due to e.g. a statement
# timeout commands are ignored and will raise in PG::InFailedSqlTransaction.
......@@ -50,22 +47,5 @@ def with_statement_timeout(timeout)
# for the currrent transaction which will be closed anyway.
end
end
# Set statement timeout for the current transaction.
#
# Note, that it does not restore the previous statement timeout.
# Use `with_statement_timeout` instead.
#
# @param timeout - Statement timeout in seconds
#
# Example:
#
# set_statement_timeout(0.1)
# model.select('pg_sleep(0.11)')
def set_statement_timeout(timeout)
ActiveRecord::Base.connection.execute(
format(%(SET LOCAL statement_timeout = '%s'), timeout)
)
end
end
end
......@@ -3,7 +3,9 @@
module Database
module TableSchemaHelpers
def connection
ActiveRecord::Base.connection
# We use ActiveRecord::Base.connection here because this is mainly used for database migrations
# where we override the connection on ActiveRecord::Base.connection
ActiveRecord::Base.connection # rubocop:disable Database/MultipleDatabases
end
def expect_table_to_be_replaced(original_table:, replacement_table:, archived_table:)
......
......@@ -104,7 +104,7 @@ def use_fake_application_settings
# We stub this way because we can't stub on
# `current_application_settings` due to `method_missing` is
# depending on current_application_settings...
allow(ActiveRecord::Base.connection)
allow(Gitlab::Database::Migration::V1_0::MigrationRecord.connection)
.to receive(:active?)
.and_return(false)
allow(Gitlab::Runtime)
......@@ -158,10 +158,10 @@ def disable_migrations_output
end
def migrate!
open_transactions = ActiveRecord::Base.connection.open_transactions
open_transactions = Gitlab::Database::Migration::V1_0::MigrationRecord.connection.open_transactions
allow_next_instance_of(described_class) do |migration|
allow(migration).to receive(:transaction_open?) do
ActiveRecord::Base.connection.open_transactions > open_transactions
Gitlab::Database::Migration::V1_0::MigrationRecord.connection.open_transactions > open_transactions
end
end
......
......@@ -19,9 +19,7 @@ def initialize(skip_cached: true, skip_schema_queries: true, log_file: nil, quer
def record(&block)
# force replacement of bind parameters to give tests the ability to check for ids
ActiveRecord::Base.connection.unprepared_statement do
ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block)
end
ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block)
end
def show_backtrace(values, duration)
......
......@@ -116,8 +116,9 @@ module UsageDataHelpers
).freeze
def stub_usage_data_connections
allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false)
allow(::Ci::ApplicationRecord.connection).to receive(:transaction_open?).and_return(false) if ::Ci::ApplicationRecord.connection_class?
Gitlab::Database.database_base_models_with_gitlab_shared.each_value do |base_model|
allow(base_model.connection).to receive(:transaction_open?).and_return(false)
end
allow(Gitlab::Prometheus::Internal).to receive(:prometheus_enabled?).and_return(false)
end
......
......@@ -22,7 +22,7 @@
describe 'mark_migration_complete' do
context 'with a single database' do
let(:main_model) { ActiveRecord::Base }
let(:main_model) { ApplicationRecord }
before do
skip_if_multiple_databases_are_setup
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment