From b74fbe4e516c86f14cfbc39d2df244aeff5f03da Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 7 Jan 2020 16:40:15 -0500 Subject: [PATCH] Deprecate "primary" as a connection_specification_name for ActiveRecord::Base As multiple databases have evolved it's becoming more and more confusing that we have a `connection_specification_name` that defaults to "primary" and a `spec_name` on the database objects that defaults to "primary" (my bad). Even more confusing is that we use the class name for all non-ActiveRecord::Base abstract classes that establish connections. For example connections established on `class MyOtherDatabaseModel < ApplicationRecord` will use `"MyOtherDatabaseModel"` as it's connection specification name while `ActiveRecord::Base` uses `"primary"`. This PR deprecates the use of the name `"primary"` as the `connection_specification_name` for `ActiveRecord::Base` in favor of using `"ActiveRecord::Base"`. In this PR the following is true: * If `handler.establish_connection(:primary)` is called, `"primary"` will not throw a deprecation warning and can still be used for the `connection_specification_name`. This also fixes a bug where using this method to establish a connection could accidentally overwrite the actual `ActiveRecord::Base` connection IF that connection was not using a configuration named `:primary`. * Calling `handler.retrieve_connection "primary"` when `handler.establish_connection :primary` has never been called will return the connection for `ActiveRecord::Base` and throw a deprecation warning. * Calling `handler.remove_connection "primary"` when `handler.establish_connection :primary` has never been called will remove the connection for `ActiveRecord::Base` and throw a deprecation warning. See #38179 for details on more motivations for this change. Co-authored-by: John Crepezzi --- activerecord/CHANGELOG.md | 6 +++ .../abstract/connection_pool.rb | 38 ++++++++++++---- .../lib/active_record/connection_handling.rb | 4 +- .../connection_handler_test.rb | 45 +++++++++++++++---- .../connection_handlers_multi_db_test.rb | 38 ++++++++-------- activerecord/test/cases/fixtures_test.rb | 2 +- activerecord/test/cases/multiple_db_test.rb | 2 +- activerecord/test/cases/query_cache_test.rb | 2 +- activerecord/test/cases/unconnected_test.rb | 2 +- 9 files changed, 97 insertions(+), 42 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5a7a3b6029..1ea45c74ed 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Deprecate "primary" as the connection_specification_name for ActiveRecord::Base + + `"primary"` has been deprecated as the `connection_specification_name` for `ActiveRecord::Base` in favor of using `"ActiveRecord::Base"`. This change affects calls to `ActiveRecord::Base.connection_handler.retrieve_connection` and `ActiveRecord::Base.connection_handler.remove_connection`. If you're calling these methods with `"primary"`, please switch to `"ActiveRecord::Base"`. + + *Eileen M. Uchitelle*, *John Crepezzi* + * Add `ActiveRecord::Validations::NumericalityValidator` with support for casting floats using a database columns' precision value. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index a1bba37610..1535b9d437 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -1043,7 +1043,11 @@ module ActiveRecord pool_config = resolve_pool_config(config) db_config = pool_config.db_config - remove_connection(pool_config.connection_specification_name, pool_key) + # Protects the connection named `ActiveRecord::Base` from being removed + # if the user calls `establish_connection :primary`. + if owner_to_pool_manager.key?(pool_config.connection_specification_name) + remove_connection(pool_config.connection_specification_name, pool_key) + end message_bus = ActiveSupport::Notifications.instrumenter payload = {} @@ -1053,7 +1057,7 @@ module ActiveRecord end owner_to_pool_manager[pool_config.connection_specification_name] ||= PoolManager.new - pool_manager = owner_to_pool_manager[pool_config.connection_specification_name] + pool_manager = get_pool_manager(pool_config.connection_specification_name) pool_manager.set_pool_config(pool_key, pool_config) message_bus.instrument("!connection.active_record", payload) do @@ -1102,9 +1106,9 @@ module ActiveRecord unless pool # multiple database application if ActiveRecord::Base.connection_handler != ActiveRecord::Base.default_connection_handler - raise ConnectionNotEstablished, "No connection pool with '#{spec_name}' found for the '#{ActiveRecord::Base.current_role}' role." + raise ConnectionNotEstablished, "No connection pool for '#{spec_name}' found for the '#{ActiveRecord::Base.current_role}' role." else - raise ConnectionNotEstablished, "No connection pool with '#{spec_name}' found." + raise ConnectionNotEstablished, "No connection pool for '#{spec_name}' found." end end @@ -1122,8 +1126,8 @@ module ActiveRecord # connection and the defined connection (if they exist). The result # can be used as an argument for #establish_connection, for easily # re-establishing the connection. - def remove_connection(spec_name, pool_key = :default) - if pool_manager = owner_to_pool_manager[spec_name] + def remove_connection(owner, pool_key = :default) + if pool_manager = get_pool_manager(owner) pool_config = pool_manager.remove_pool_config(pool_key) if pool_config @@ -1136,14 +1140,30 @@ module ActiveRecord # Retrieving the connection pool happens a lot, so we cache it in @owner_to_pool_manager. # This makes retrieving the connection pool O(1) once the process is warm. # When a connection is established or removed, we invalidate the cache. - def retrieve_connection_pool(spec_name, pool_key = :default) - pool_config = owner_to_pool_manager[spec_name]&.get_pool_config(pool_key) + def retrieve_connection_pool(owner, pool_key = :default) + pool_config = get_pool_manager(owner)&.get_pool_config(pool_key) pool_config&.pool end private attr_reader :owner_to_pool_manager + # Returns the pool manager for an owner. + # + # Using `"primary"` to look up the pool manager for `ActiveRecord::Base` is + # deprecated in favor of looking it up by `"ActiveRecord::Base"`. + # + # During the deprecation period, if `"primary"` is passed, the pool manager + # for `ActiveRecord::Base` will still be returned. + def get_pool_manager(owner) + return owner_to_pool_manager[owner] if owner_to_pool_manager.key?(owner) + + if owner == "primary" + ActiveSupport::Deprecation.warn("Using `\"primary\"` as a `connection_specification_name` is deprecated and will be removed in Rails 6.2.0. Please use `ActiveRecord::Base`.") + owner_to_pool_manager[Base.name] + end + end + # Returns an instance of PoolConfig for a given adapter. # Accepts a hash one layer deep that contains all connection information. # @@ -1186,7 +1206,7 @@ module ActiveRecord raise AdapterNotFound, "database configuration specifies nonexistent #{db_config.adapter} adapter" end - pool_name = db_config.owner_name || "primary" + pool_name = db_config.owner_name || Base.name db_config.owner_name = nil ConnectionAdapters::PoolConfig.new(pool_name, db_config) end diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 335824e479..0721dbe06c 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -181,7 +181,7 @@ module ActiveRecord # Return the specification name from the current class or its parent. def connection_specification_name if !defined?(@connection_specification_name) || @connection_specification_name.nil? - return self == Base ? "primary" : superclass.connection_specification_name + return self == Base ? Base.name : superclass.connection_specification_name end @connection_specification_name end @@ -249,7 +249,7 @@ module ActiveRecord raise "Anonymous class is not allowed." unless name config_or_env ||= DEFAULT_ENV.call.to_sym - pool_name = primary_class? ? "primary" : name + pool_name = primary_class? ? Base.name : name self.connection_specification_name = pool_name db_config = Base.configurations.resolve(config_or_env, pool_name) diff --git a/activerecord/test/cases/connection_adapters/connection_handler_test.rb b/activerecord/test/cases/connection_adapters/connection_handler_test.rb index 843bd4f22b..aef0efaa64 100644 --- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb @@ -12,7 +12,7 @@ module ActiveRecord def setup @handler = ConnectionHandler.new - @spec_name = "primary" + @owner_name = "ActiveRecord::Base" @pool = @handler.establish_connection(ActiveRecord::Base.configurations["arunit"]) end @@ -75,6 +75,35 @@ module ActiveRecord end unless in_memory_db? + def test_establish_connection_with_primary_works_without_deprecation + old_config = ActiveRecord::Base.configurations + config = { "primary" => { "adapter" => "sqlite3", "database" => "db/primary.sqlite3" } } + ActiveRecord::Base.configurations = config + + @handler.establish_connection(:primary) + + assert_not_deprecated do + @handler.retrieve_connection("primary") + @handler.remove_connection("primary") + end + ensure + ActiveRecord::Base.configurations = old_config + end + + def test_retrieve_connection_shows_primary_deprecation_warning_when_established_on_active_record_base + old_config = ActiveRecord::Base.configurations + config = { "primary" => { "adapter" => "sqlite3", "database" => "db/primary.sqlite3" } } + ActiveRecord::Base.configurations = config + + ActiveRecord::Base.establish_connection(:primary) + + assert_deprecated { @handler.retrieve_connection("primary") } + assert_deprecated { @handler.remove_connection("primary") } + ensure + ActiveRecord::Base.configurations = old_config + ActiveRecord::Base.establish_connection(:arunit) + end + def test_establish_connection_using_3_level_config_defaults_to_default_env_primary_db previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "default_env" @@ -182,19 +211,19 @@ module ActiveRecord end def test_retrieve_connection - assert @handler.retrieve_connection(@spec_name) + assert @handler.retrieve_connection(@owner_name) end def test_active_connections? assert_not_predicate @handler, :active_connections? - assert @handler.retrieve_connection(@spec_name) + assert @handler.retrieve_connection(@owner_name) assert_predicate @handler, :active_connections? @handler.clear_active_connections! assert_not_predicate @handler, :active_connections? end def test_retrieve_connection_pool - assert_not_nil @handler.retrieve_connection_pool(@spec_name) + assert_not_nil @handler.retrieve_connection_pool(@owner_name) end def test_retrieve_connection_pool_with_invalid_id @@ -236,8 +265,8 @@ module ActiveRecord assert_equal klassB.connection_specification_name, klassA.connection_specification_name assert_equal klassC.connection_specification_name, klassA.connection_specification_name - assert_equal "primary", klassA.connection_specification_name - assert_equal "primary", klassC.connection_specification_name + assert_equal "ActiveRecord::Base", klassA.connection_specification_name + assert_equal "ActiveRecord::Base", klassC.connection_specification_name klassA.connection_specification_name = "readonly" assert_equal "readonly", klassB.connection_specification_name @@ -246,7 +275,7 @@ module ActiveRecord assert_equal "readonly", klassC.connection_specification_name ensure Object.send :remove_const, :ApplicationRecord - ActiveRecord::Base.connection_specification_name = "primary" + ActiveRecord::Base.connection_specification_name = "ActiveRecord::Base" end def test_remove_connection_should_not_remove_parent @@ -362,7 +391,7 @@ module ActiveRecord pid = fork { rd.close - pool = @handler.retrieve_connection_pool(@spec_name) + pool = @handler.retrieve_connection_pool(@owner_name) wr.write Marshal.dump pool.schema_cache.size wr.close exit! diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb index 8993e250ec..1ef3f965af 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb @@ -14,7 +14,7 @@ module ActiveRecord @handlers = { writing: ConnectionHandler.new, reading: ConnectionHandler.new } @rw_handler = @handlers[:writing] @ro_handler = @handlers[:reading] - @spec_name = "primary" + @owner_name = "ActiveRecord::Base" @rw_pool = @handlers[:writing].establish_connection(ActiveRecord::Base.configurations["arunit"]) @ro_pool = @handlers[:reading].establish_connection(ActiveRecord::Base.configurations["arunit"]) end @@ -81,11 +81,11 @@ module ActiveRecord ActiveRecord::Base.connects_to(database: { writing: :default, reading: :readonly }) - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("primary") + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base") assert_equal "db/primary.sqlite3", pool.db_config.database assert_equal "default", pool.db_config.spec_name - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("primary") + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base") assert_equal "db/readonly.sqlite3", pool.db_config.database assert_equal "readonly", pool.db_config.spec_name ensure @@ -141,10 +141,10 @@ module ActiveRecord ActiveRecord::Base.connects_to(database: { default: :primary, readonly: :readonly }) - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:default].retrieve_connection_pool("primary") + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:default].retrieve_connection_pool("ActiveRecord::Base") assert_equal "db/primary.sqlite3", pool.db_config.database - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:readonly].retrieve_connection_pool("primary") + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:readonly].retrieve_connection_pool("ActiveRecord::Base") assert_equal "db/readonly.sqlite3", pool.db_config.database ensure ActiveRecord::Base.configurations = @prev_configs @@ -163,7 +163,7 @@ module ActiveRecord handler = ActiveRecord::Base.connection_handler assert_equal handler, ActiveRecord::Base.connection_handlers[:writing] - assert_not_nil pool = handler.retrieve_connection_pool("primary") + assert_not_nil pool = handler.retrieve_connection_pool("ActiveRecord::Base") assert_equal({ adapter: "postgresql", database: "bar", host: "localhost" }, pool.db_config.configuration_hash) ensure ActiveRecord::Base.establish_connection(:arunit) @@ -182,7 +182,7 @@ module ActiveRecord handler = ActiveRecord::Base.connection_handler assert_equal handler, ActiveRecord::Base.connection_handlers[:writing] - assert_not_nil pool = handler.retrieve_connection_pool("primary") + assert_not_nil pool = handler.retrieve_connection_pool("ActiveRecord::Base") assert_equal(config, pool.db_config.configuration_hash) ensure ActiveRecord::Base.establish_connection(:arunit) @@ -223,7 +223,7 @@ module ActiveRecord handler = ActiveRecord::Base.connection_handler assert_equal handler, ActiveRecord::Base.connection_handlers[:writing] - assert_not_nil pool = handler.retrieve_connection_pool("primary") + assert_not_nil pool = handler.retrieve_connection_pool("ActiveRecord::Base") assert_equal(config["default_env"]["animals"], pool.db_config.configuration_hash) ensure ActiveRecord::Base.configurations = @prev_configs @@ -249,7 +249,7 @@ module ActiveRecord handler = ActiveRecord::Base.connection_handler assert_equal handler, ActiveRecord::Base.connection_handlers[:writing] - assert_not_nil pool = handler.retrieve_connection_pool("primary") + assert_not_nil pool = handler.retrieve_connection_pool("ActiveRecord::Base") assert_equal(config["default_env"]["primary"], pool.db_config.configuration_hash) ensure ActiveRecord::Base.configurations = @prev_configs @@ -283,7 +283,7 @@ module ActiveRecord ActiveRecord::Base.connects_to database: { writing: :development, reading: :development_readonly } - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("primary") + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base") assert_equal "db/readonly.sqlite3", pool.db_config.database ensure ActiveRecord::Base.configurations = @prev_configs @@ -301,8 +301,8 @@ module ActiveRecord assert_equal( [ - ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("primary"), - ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("primary") + ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base"), + ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base") ], result ) @@ -318,16 +318,16 @@ module ActiveRecord end def test_retrieve_connection - assert @rw_handler.retrieve_connection(@spec_name) - assert @ro_handler.retrieve_connection(@spec_name) + assert @rw_handler.retrieve_connection(@owner_name) + assert @ro_handler.retrieve_connection(@owner_name) end def test_active_connections? assert_not_predicate @rw_handler, :active_connections? assert_not_predicate @ro_handler, :active_connections? - assert @rw_handler.retrieve_connection(@spec_name) - assert @ro_handler.retrieve_connection(@spec_name) + assert @rw_handler.retrieve_connection(@owner_name) + assert @ro_handler.retrieve_connection(@owner_name) assert_predicate @rw_handler, :active_connections? assert_predicate @ro_handler, :active_connections? @@ -340,8 +340,8 @@ module ActiveRecord end def test_retrieve_connection_pool - assert_not_nil @rw_handler.retrieve_connection_pool(@spec_name) - assert_not_nil @ro_handler.retrieve_connection_pool(@spec_name) + assert_not_nil @rw_handler.retrieve_connection_pool(@owner_name) + assert_not_nil @ro_handler.retrieve_connection_pool(@owner_name) end def test_retrieve_connection_pool_with_invalid_id @@ -393,7 +393,7 @@ module ActiveRecord end end - assert_equal "No connection pool with 'primary' found for the 'reading' role.", error.message + assert_equal "No connection pool for 'ActiveRecord::Base' found for the 'reading' role.", error.message end def test_default_handlers_are_writing_and_reading diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index de9292f7d9..805e43fbb8 100644 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -1406,7 +1406,7 @@ class MultipleDatabaseFixturesTest < ActiveRecord::TestCase private def with_temporary_connection_pool - pool_config = ActiveRecord::Base.connection_handler.send(:owner_to_pool_manager).fetch("primary").get_pool_config(:default) + pool_config = ActiveRecord::Base.connection_handler.send(:owner_to_pool_manager).fetch("ActiveRecord::Base").get_pool_config(:default) new_pool = ActiveRecord::ConnectionAdapters::ConnectionPool.new(pool_config) pool_config.stub(:pool, new_pool) do diff --git a/activerecord/test/cases/multiple_db_test.rb b/activerecord/test/cases/multiple_db_test.rb index f11c441c65..18c97090c7 100644 --- a/activerecord/test/cases/multiple_db_test.rb +++ b/activerecord/test/cases/multiple_db_test.rb @@ -27,7 +27,7 @@ class MultipleDbTest < ActiveRecord::TestCase end def test_swapping_the_connection - old_spec_name, Course.connection_specification_name = Course.connection_specification_name, "primary" + old_spec_name, Course.connection_specification_name = Course.connection_specification_name, "ActiveRecord::Base" assert_equal(Entrant.connection, Course.connection) ensure Course.connection_specification_name = old_spec_name diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index b82e5e1655..81a2f98243 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -636,7 +636,7 @@ class QueryCacheTest < ActiveRecord::TestCase private def with_temporary_connection_pool - pool_config = ActiveRecord::Base.connection_handler.send(:owner_to_pool_manager).fetch("primary").get_pool_config(:default) + pool_config = ActiveRecord::Base.connection_handler.send(:owner_to_pool_manager).fetch("ActiveRecord::Base").get_pool_config(:default) new_pool = ActiveRecord::ConnectionAdapters::ConnectionPool.new(pool_config) pool_config.stub(:pool, new_pool) do diff --git a/activerecord/test/cases/unconnected_test.rb b/activerecord/test/cases/unconnected_test.rb index f9b3d79709..75d0964292 100644 --- a/activerecord/test/cases/unconnected_test.rb +++ b/activerecord/test/cases/unconnected_test.rb @@ -37,7 +37,7 @@ class TestUnconnectedAdapter < ActiveRecord::TestCase TestRecord.find(1) end - assert_equal "No connection pool with 'primary' found.", error.message + assert_equal "No connection pool for 'ActiveRecord::Base' found.", error.message end def test_underlying_adapter_no_longer_active