From c05098852c5424a2357d1d4bb0990c432b3d138a Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 7 Jan 2020 12:23:34 -0500 Subject: [PATCH] Restore previous behavior of parallel test databases This commit is somewhat of a bandaid fix for a bug that was revealed in #38029 and then #38151. #38151 can cause problems in certain cases when an app has a 3-tier config, with replicas, because it reorders the configuration and changes the implict default connection that gets picked up. If an app calls `establish_connection` with no arguments or doesn't call `connects_to` in `ApplicationRecord` AND uses parallel testing databases, the application may pick up the wrong configuration. This is because when the code in #38151 loops through the configurations it will update the non-replica configurations and then put them at the end of the list. If you don't specify which connection you want, Rails will pick up the _first_ connection for that environment. So given the following configuration: ``` test: primary: database: my_db replica: database: my_db replica: true ``` The database configurations will get reordered to be `replica`, `primary` and when Rails calls `establish_connection` with no arguments it will pick up `replica` because it's first in the list. Looking at this bug it shows that calling `establish_connection` with no arguments (which will pick up the default env + first configuration in the list) OR when `establish_connection` is called with an environment like `:test` it will also pick up that env's first configuration. This can have surprising behavior in a couple cases: 1) In the parallel testing changes we saw users getting the wrong db configuration and hitting an `ActiveRecord::ReadOnlyError` 2) Writing a configuration that puts `replica` before `primary`, also resulting in a `ActiveRecord::ReadOnlyError` The real fix for this issue is to deprecate calling `establish_connection` with an env or nothing and require an explcit configuration (like `primary`). This would also involve blessing `:primary` as the default connection Rails looks for on boot. In addition, this would require us deprecating connection specification name "primary" in favor of the class name always since that will get mega-confusing (seriously, it's already mega-confusing). We'll work on fixing these underlying issues, but wanted to get a fix out that restores previous behavior. Co-authored-by: John Crepezzi --- .../database_config.rb | 4 +++ .../database_configurations/hash_config.rb | 4 +++ .../lib/active_record/test_databases.rb | 17 ++--------- .../test/cases/test_databases_test.rb | 28 ++++++++++++++++++- 4 files changed, 38 insertions(+), 15 deletions(-) diff --git a/activerecord/lib/active_record/database_configurations/database_config.rb b/activerecord/lib/active_record/database_configurations/database_config.rb index 14ca5eee31..e52933a456 100644 --- a/activerecord/lib/active_record/database_configurations/database_config.rb +++ b/activerecord/lib/active_record/database_configurations/database_config.rb @@ -30,6 +30,10 @@ def database raise NotImplementedError end + def _database=(database) + raise NotImplementedError + end + def adapter raise NotImplementedError end diff --git a/activerecord/lib/active_record/database_configurations/hash_config.rb b/activerecord/lib/active_record/database_configurations/hash_config.rb index 841a97edbe..25177c17eb 100644 --- a/activerecord/lib/active_record/database_configurations/hash_config.rb +++ b/activerecord/lib/active_record/database_configurations/hash_config.rb @@ -61,6 +61,10 @@ def database configuration_hash[:database] end + def _database=(database) # :nodoc: + @config = configuration_hash.dup.merge(database: database).freeze + end + def pool (configuration_hash[:pool] || 5).to_i end diff --git a/activerecord/lib/active_record/test_databases.rb b/activerecord/lib/active_record/test_databases.rb index 2c6cb69145..ab4732bb80 100644 --- a/activerecord/lib/active_record/test_databases.rb +++ b/activerecord/lib/active_record/test_databases.rb @@ -12,23 +12,12 @@ def self.create_and_load_schema(i, env_name:) old, ENV["VERBOSE"] = ENV["VERBOSE"], "false" ActiveRecord::Base.configurations.configs_for(env_name: env_name).each do |db_config| - database = "#{db_config.database}-#{i}" + db_config._database = "#{db_config.database}-#{i}" - db_config_copy = ActiveRecord::DatabaseConfigurations::HashConfig.new( - env_name, - db_config.spec_name, - db_config.configuration_hash.merge(database: database) - ) - - # Reconstruct with the new configuration - ActiveRecord::Tasks::DatabaseTasks.reconstruct_from_schema(db_config_copy, ActiveRecord::Base.schema_format, nil) - - # Replace the original configuration with our replacement - ActiveRecord::Base.configurations.configurations.delete(db_config) - ActiveRecord::Base.configurations.configurations.push(db_config_copy) + ActiveRecord::Tasks::DatabaseTasks.reconstruct_from_schema(db_config, ActiveRecord::Base.schema_format, nil) end ensure - ActiveRecord::Base.establish_connection(ActiveRecord::ConnectionHandling::DEFAULT_ENV.call.to_sym) + ActiveRecord::Base.establish_connection ENV["VERBOSE"] = old end end diff --git a/activerecord/test/cases/test_databases_test.rb b/activerecord/test/cases/test_databases_test.rb index 19f5cd0256..61910b202b 100644 --- a/activerecord/test/cases/test_databases_test.rb +++ b/activerecord/test/cases/test_databases_test.rb @@ -25,6 +25,7 @@ def test_databases_are_created ActiveRecord::Base.configurations = prev_configs ActiveRecord::Base.establish_connection(:arunit) ENV["RAILS_ENV"] = previous_env + FileUtils.rm_rf("db") end def test_create_databases_after_fork @@ -45,12 +46,37 @@ def test_create_databases_after_fork ActiveSupport::Testing::Parallelization.after_fork_hooks.each { |cb| cb.call(idx) } end - # Updates the databse configuration + # Updates the database configuration assert_equal expected_database, ActiveRecord::Base.configurations.configs_for(env_name: "arunit", spec_name: "primary").database ensure ActiveRecord::Base.configurations = prev_configs ActiveRecord::Base.establish_connection(:arunit) ENV["RAILS_ENV"] = previous_env + FileUtils.rm_rf("db") + end + + def test_order_of_configurations_isnt_changed_by_test_databases + previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "arunit" + prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, { + "arunit" => { + "primary" => { "adapter" => "sqlite3", "database" => "db/primary.sqlite3" }, + "replica" => { "adapter" => "sqlite3", "database" => "db/primary.sqlite3" } + } + } + + idx = 42 + base_configs_order = ActiveRecord::Base.configurations.configs_for(env_name: "arunit").map(&:spec_name) + + ActiveRecord::Tasks::DatabaseTasks.stub(:reconstruct_from_schema, ->(db_config, _, _) { + assert_equal base_configs_order, ActiveRecord::Base.configurations.configs_for(env_name: "arunit").map(&:spec_name) + }) do + ActiveSupport::Testing::Parallelization.after_fork_hooks.each { |cb| cb.call(idx) } + end + ensure + ActiveRecord::Base.configurations = prev_configs + ActiveRecord::Base.establish_connection(:arunit) + ENV["RAILS_ENV"] = previous_env + FileUtils.rm_rf("db") end end end -- GitLab