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: NJohn Crepezzi <john.crepezzi@gmail.com>

activerecord/CHANGELOG.md

+*   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.
 

activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb

@@ -1043,7 +1043,11 @@ def establish_connection(config, pool_key = :default)
         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 @@ def establish_connection(config, pool_key = :default)
         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 @@ def retrieve_connection(spec_name) #:nodoc:
         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 @@ def connected?(spec_name, pool_key = :default)
       # 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 @@ def remove_connection(spec_name, pool_key = :default)
       # 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 @@ def resolve_pool_config(config)
             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

activerecord/lib/active_record/connection_handling.rb

@@ -181,7 +181,7 @@ def connection
     # 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 @@ def resolve_config_for_connection(config_or_env)
         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)

activerecord/test/cases/connection_adapters/connection_handler_test.rb

@@ -12,7 +12,7 @@ class ConnectionHandlerTest < ActiveRecord::TestCase
 
       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 @@ def test_establish_connection_using_3_levels_config
       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 @@ def test_symbolized_configurations_assignment
       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 @@ def test_connection_specification_name_should_fallback_to_parent
         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 @@ def test_connection_specification_name_should_fallback_to_parent
         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 @@ def test_retrieve_connection_pool_copies_schema_cache_from_ancestor_pool
 
           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!

activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb

@@ -14,7 +14,7 @@ def setup
         @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 @@ def test_establish_connection_using_3_levels_config
 
           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 @@ def test_establish_connection_using_3_levels_config_with_non_default_handlers
 
           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 @@ def test_switching_connections_with_database_url
           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 @@ def test_switching_connections_with_database_config_hash
           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 @@ def test_switching_connections_with_database_symbol_uses_default_role
           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 @@ def test_switching_connections_with_database_hash_uses_passed_role_and_database
           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 @@ def test_connects_to_using_top_level_key_in_two_level_config
 
           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 @@ def test_connects_to_returns_array_of_established_connections
 
           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 @@ def test_connection_pools
       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 @@ def test_active_connections?
       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 @@ def test_calling_connected_to_on_a_non_existent_handler_raises
           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

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

activerecord/test/cases/multiple_db_test.rb

@@ -27,7 +27,7 @@ def test_proper_connection
   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

activerecord/test/cases/query_cache_test.rb

@@ -636,7 +636,7 @@ def test_clear_query_cache_is_called_on_all_connections
 
   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

activerecord/test/cases/unconnected_test.rb

@@ -37,7 +37,7 @@ def test_error_message_when_connection_not_established
       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