From 081d95c02163f5f2ac594df320c6b8a374e33c43 Mon Sep 17 00:00:00 2001
From: Yorick Peterse <git@yorickpeterse.com>
Date: Wed, 15 Dec 2021 17:10:37 +0100
Subject: [PATCH] Disallow the use of X.establish_connection
This adds a RuboCop rule that disallows the usage of
`SomeModel.establish_connection`, as this pattern can break database
configurations and slows down the tests.
See https://gitlab.com/gitlab-org/gitlab/-/issues/338653 for more
information.
---
.../initializers/active_record_lifecycle.rb | 2 +-
config/initializers/database_config.rb | 2 +-
ee/lib/gitlab/geo/database_tasks.rb | 4 +--
lib/tasks/gitlab/db.rake | 2 +-
rubocop/cop/database/establish_connection.rb | 20 +++++++++++++
scripts/insert-rspec-profiling-data | 2 +-
spec/lib/gitlab/database/bulk_update_spec.rb | 2 +-
.../cop/database/establish_connection_spec.rb | 29 +++++++++++++++++++
spec/support/db_cleaner.rb | 2 +-
.../database/multiple_databases_spec.rb | 8 ++---
10 files changed, 61 insertions(+), 12 deletions(-)
create mode 100644 rubocop/cop/database/establish_connection.rb
create mode 100644 spec/rubocop/cop/database/establish_connection_spec.rb
diff --git a/config/initializers/active_record_lifecycle.rb b/config/initializers/active_record_lifecycle.rb
index 8d4b6d61abe3..92cc1d816179 100644
--- a/config/initializers/active_record_lifecycle.rb
+++ b/config/initializers/active_record_lifecycle.rb
@@ -5,7 +5,7 @@
if defined?(ActiveRecord::Base) && !Gitlab::Runtime.sidekiq?
Gitlab::Cluster::LifecycleEvents.on_worker_start do
ActiveSupport.on_load(:active_record) do
- ActiveRecord::Base.establish_connection
+ ActiveRecord::Base.establish_connection # rubocop: disable Database/EstablishConnection
Gitlab::AppLogger.debug("ActiveRecord connection established")
end
diff --git a/config/initializers/database_config.rb b/config/initializers/database_config.rb
index a3172fae027c..050ab1d9b3ec 100644
--- a/config/initializers/database_config.rb
+++ b/config/initializers/database_config.rb
@@ -13,6 +13,6 @@
# The Geo::TrackingBase model does not yet use connects_to. So,
# this will not properly support geo: from config/databse.yml
# file yet. This is ACK of the current state and will be fixed.
- Geo::TrackingBase.establish_connection(Gitlab::Database.geo_db_config_with_default_pool_size)
+ Geo::TrackingBase.establish_connection(Gitlab::Database.geo_db_config_with_default_pool_size) # rubocop: disable Database/EstablishConnection
end
end
diff --git a/ee/lib/gitlab/geo/database_tasks.rb b/ee/lib/gitlab/geo/database_tasks.rb
index 31d8e97d10f5..7a4b7a5154b5 100644
--- a/ee/lib/gitlab/geo/database_tasks.rb
+++ b/ee/lib/gitlab/geo/database_tasks.rb
@@ -132,7 +132,7 @@ def load
ActiveRecord::Tasks::DatabaseTasks.load_schema(ActiveRecord::Base.configurations.configs_for(env_name: 'test').first, :sql, ENV['SCHEMA'])
ensure
if should_reconnect
- ActiveRecord::Base.establish_connection(Gitlab::Geo::DatabaseTasks.db_config)
+ ActiveRecord::Base.establish_connection(Gitlab::Geo::DatabaseTasks.db_config) # rubocop: disable Database/EstablishConnection
end
end
end
@@ -204,7 +204,7 @@ def set_db_env(settings)
ActiveRecord::Migrator.migrations_paths = ActiveRecord::Tasks::DatabaseTasks.migrations_paths
Gitlab::Database::SchemaMigrations::Context.default_schema_migrations_path = settings[:schema_migrations_path]
- ActiveRecord::Base.establish_connection(db_config)
+ ActiveRecord::Base.establish_connection(db_config) # rubocop: disable Database/EstablishConnection
end
class SeedLoader
diff --git a/lib/tasks/gitlab/db.rake b/lib/tasks/gitlab/db.rake
index 9e733fc3a0fc..efb0e1ef1e1a 100644
--- a/lib/tasks/gitlab/db.rake
+++ b/lib/tasks/gitlab/db.rake
@@ -170,7 +170,7 @@ namespace :gitlab do
# the `ActiveRecord::Base.connection` might be switched to another one
# This is due to `if should_reconnect`:
# https://github.com/rails/rails/blob/a81aeb63a007ede2fe606c50539417dada9030c7/activerecord/lib/active_record/railties/databases.rake#L622
- ActiveRecord::Base.establish_connection :main
+ ActiveRecord::Base.establish_connection :main # rubocop: disable Database/EstablishConnection
Rake::Task['gitlab:db:create_dynamic_partitions'].invoke
end
diff --git a/rubocop/cop/database/establish_connection.rb b/rubocop/cop/database/establish_connection.rb
new file mode 100644
index 000000000000..20454887ce2d
--- /dev/null
+++ b/rubocop/cop/database/establish_connection.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module RuboCop
+ module Cop
+ module Database
+ class EstablishConnection < RuboCop::Cop::Cop
+ MSG = "Don't establish new database connections, as this slows down " \
+ 'tests and may result in new connections using an incorrect configuration'
+
+ def_node_matcher :establish_connection?, <<~PATTERN
+ (send (const ...) :establish_connection ...)
+ PATTERN
+
+ def on_send(node)
+ add_offense(node, location: :expression) if establish_connection?(node)
+ end
+ end
+ end
+ end
+end
diff --git a/scripts/insert-rspec-profiling-data b/scripts/insert-rspec-profiling-data
index b20118585588..be25972644c8 100755
--- a/scripts/insert-rspec-profiling-data
+++ b/scripts/insert-rspec-profiling-data
@@ -12,7 +12,7 @@ module RspecProfiling
# This disables the automatic creation of the database and
# table. In the future, we may want a way to specify the host of
# the database to connect so that we can call #install.
- Result.establish_connection(results_url)
+ Result.establish_connection(results_url) # rubocop: disable Database/EstablishConnection
end
def results_url
diff --git a/spec/lib/gitlab/database/bulk_update_spec.rb b/spec/lib/gitlab/database/bulk_update_spec.rb
index 9a6463c99fa0..08b4d50f83b5 100644
--- a/spec/lib/gitlab/database/bulk_update_spec.rb
+++ b/spec/lib/gitlab/database/bulk_update_spec.rb
@@ -101,7 +101,7 @@
before do
configuration_hash = ActiveRecord::Base.connection_db_config.configuration_hash
- ActiveRecord::Base.establish_connection(
+ ActiveRecord::Base.establish_connection( # rubocop: disable Database/EstablishConnection
configuration_hash.merge(prepared_statements: prepared_statements)
)
end
diff --git a/spec/rubocop/cop/database/establish_connection_spec.rb b/spec/rubocop/cop/database/establish_connection_spec.rb
new file mode 100644
index 000000000000..a3c27d33cb0e
--- /dev/null
+++ b/spec/rubocop/cop/database/establish_connection_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative '../../../../rubocop/cop/database/establish_connection'
+
+RSpec.describe RuboCop::Cop::Database::EstablishConnection do
+ subject(:cop) { described_class.new }
+
+ it 'flags the use of ActiveRecord::Base.establish_connection' do
+ expect_offense(<<~CODE)
+ ActiveRecord::Base.establish_connection
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't establish new database [...]
+ CODE
+ end
+
+ it 'flags the use of ActiveRecord::Base.establish_connection with arguments' do
+ expect_offense(<<~CODE)
+ ActiveRecord::Base.establish_connection(:foo)
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't establish new database [...]
+ CODE
+ end
+
+ it 'flags the use of SomeModel.establish_connection' do
+ expect_offense(<<~CODE)
+ SomeModel.establish_connection
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't establish new database [...]
+ CODE
+ end
+end
diff --git a/spec/support/db_cleaner.rb b/spec/support/db_cleaner.rb
index 316d645f99ff..fb70f82ef875 100644
--- a/spec/support/db_cleaner.rb
+++ b/spec/support/db_cleaner.rb
@@ -67,7 +67,7 @@ def recreate_all_databases!
# Migrate each database individually
with_reestablished_active_record_base do
all_connection_classes.each do |connection_class|
- ActiveRecord::Base.establish_connection(connection_class.connection_db_config)
+ ActiveRecord::Base.establish_connection(connection_class.connection_db_config) # rubocop: disable Database/EstablishConnection
ActiveRecord::Tasks::DatabaseTasks.migrate
end
diff --git a/spec/support_specs/database/multiple_databases_spec.rb b/spec/support_specs/database/multiple_databases_spec.rb
index 660563594587..b4cfa2538131 100644
--- a/spec/support_specs/database/multiple_databases_spec.rb
+++ b/spec/support_specs/database/multiple_databases_spec.rb
@@ -7,13 +7,13 @@
context 'when doing establish_connection' do
context 'on ActiveRecord::Base' do
it 'raises exception' do
- expect { ActiveRecord::Base.establish_connection(:main) }.to raise_error /Cannot re-establish/
+ expect { ActiveRecord::Base.establish_connection(:main) }.to raise_error /Cannot re-establish/ # rubocop: disable Database/EstablishConnection
end
context 'when using with_reestablished_active_record_base' do
it 'does not raise exception' do
with_reestablished_active_record_base do
- expect { ActiveRecord::Base.establish_connection(:main) }.not_to raise_error
+ expect { ActiveRecord::Base.establish_connection(:main) }.not_to raise_error # rubocop: disable Database/EstablishConnection
end
end
end
@@ -25,13 +25,13 @@
end
it 'raises exception' do
- expect { Ci::ApplicationRecord.establish_connection(:ci) }.to raise_error /Cannot re-establish/
+ expect { Ci::ApplicationRecord.establish_connection(:ci) }.to raise_error /Cannot re-establish/ # rubocop: disable Database/EstablishConnection
end
context 'when using with_reestablished_active_record_base' do
it 'does not raise exception' do
with_reestablished_active_record_base do
- expect { Ci::ApplicationRecord.establish_connection(:main) }.not_to raise_error
+ expect { Ci::ApplicationRecord.establish_connection(:main) }.not_to raise_error # rubocop: disable Database/EstablishConnection
end
end
end
--
GitLab