Merge pull request #24965 from arthurnn/arthurnn/conn_api

Change some establish_connection logic

7 files changed, 43 insertions, 37 deletions

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 f437dafec2..c341773be1 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -837,7 +837,11 @@ module ActiveRecord
       end
       alias :connection_pools :connection_pool_list
 
-      def establish_connection(spec)
+      def establish_connection(config)
+        resolver = ConnectionSpecification::Resolver.new(Base.configurations)
+        spec = resolver.spec(config)
+
+        remove_connection(spec.name)
         owner_to_pool[spec.name] = ConnectionAdapters::ConnectionPool.new(spec)
       end
 
@@ -871,9 +875,9 @@ module ActiveRecord
       # for (not necessarily the current class).
       def retrieve_connection(spec_name) #:nodoc:
         pool = retrieve_connection_pool(spec_name)
-        raise ConnectionNotEstablished, "No connection pool with id #{spec_name} found." unless pool
+        raise ConnectionNotEstablished, "No connection pool with id '#{spec_name}' found." unless pool
         conn = pool.connection
-        raise ConnectionNotEstablished, "No connection for #{spec_name} in connection pool" unless conn
+        raise ConnectionNotEstablished, "No connection for '#{spec_name}' in connection pool" unless conn
         conn
       end
 
@@ -907,7 +911,7 @@ module ActiveRecord
             # A connection was established in an ancestor process that must have
             # subsequently forked. We can't reuse the connection, but we can copy
             # the specification and establish a new connection with it.
-            establish_connection(ancestor_pool.spec).tap do |pool|
+            establish_connection(ancestor_pool.spec.to_hash).tap do |pool|
               pool.schema_cache = ancestor_pool.schema_cache if ancestor_pool.schema_cache
             end
           else
diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb
index 901c98b22b..346916337e 100644
--- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb
+++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb
@@ -13,6 +13,10 @@ module ActiveRecord
         @config = original.config.dup
       end
 
+      def to_hash
+        @config.merge(name: @name)
+      end
+
       # Expands a connection string into a hash.
       class ConnectionUrlResolver # :nodoc:
 
@@ -164,7 +168,7 @@ module ActiveRecord
         #   spec.config
         #   # => { "host" => "localhost", "database" => "foo", "adapter" => "sqlite3" }
         #
-        def spec(config, name = nil)
+        def spec(config)
           spec = resolve(config).symbolize_keys
 
           raise(AdapterNotSpecified, "database configuration does not specify adapter") unless spec.key?(:adapter)
@@ -180,13 +184,11 @@ module ActiveRecord
 
           adapter_method = "#{spec[:adapter]}_connection"
 
-          name ||=
-            if config.is_a?(Symbol)
-              config.to_s
-            else
-              "primary"
-            end
-          ConnectionSpecification.new(name, spec, adapter_method)
+          unless ActiveRecord::Base.respond_to?(adapter_method)
+            raise AdapterNotFound, "database configuration specifies nonexistent #{spec.config[:adapter]} adapter"
+          end
+
+          ConnectionSpecification.new(spec.delete(:name) || "primary", spec, adapter_method)
         end
 
         private
@@ -231,7 +233,7 @@ module ActiveRecord
         #
         def resolve_symbol_connection(spec)
           if config = configurations[spec.to_s]
-            resolve_connection(config)
+            resolve_connection(config).merge("name" => spec.to_s)
           else
             raise(AdapterNotSpecified, "'#{spec}' database is not configured. Available: #{configurations.keys.inspect}")
           end
diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb
index f932deb18d..086c678af5 100644
--- a/activerecord/lib/active_record/connection_handling.rb
+++ b/activerecord/lib/active_record/connection_handling.rb
@@ -44,21 +44,18 @@ module ActiveRecord
     #
     # The exceptions AdapterNotSpecified, AdapterNotFound and +ArgumentError+
     # may be returned on an error.
-    def establish_connection(spec = nil)
+    def establish_connection(config = nil)
       raise "Anonymous class is not allowed." unless name
 
-      spec     ||= DEFAULT_ENV.call.to_sym
-      resolver =   ConnectionAdapters::ConnectionSpecification::Resolver.new configurations
-      # TODO: uses name on establish_connection, for backwards compatibility
-      spec     =   resolver.spec(spec, self == Base ? "primary" : name)
+      config ||= DEFAULT_ENV.call.to_sym
+      spec_name = self == Base ? "primary" : name
+      self.connection_specification_name = spec_name
 
-      unless respond_to?(spec.adapter_method)
-        raise AdapterNotFound, "database configuration specifies nonexistent #{spec.config[:adapter]} adapter"
-      end
+      resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new(Base.configurations)
+      spec = resolver.resolve(config).symbolize_keys
+      spec[:name] = spec_name
 
-      remove_connection(spec.name)
-      self.connection_specification_name = spec.name
-      connection_handler.establish_connection spec
+      connection_handler.establish_connection(spec)
     end
 
     class MergeAndResolveDefaultUrlConfig # :nodoc:
diff --git a/activerecord/lib/active_record/tasks/database_tasks.rb b/activerecord/lib/active_record/tasks/database_tasks.rb
index 0df46d54df..e3e665e149 100644
--- a/activerecord/lib/active_record/tasks/database_tasks.rb
+++ b/activerecord/lib/active_record/tasks/database_tasks.rb
@@ -120,7 +120,7 @@ module ActiveRecord
         old_pool = ActiveRecord::Base.connection_handler.retrieve_connection_pool(ActiveRecord::Base.connection_specification_name)
         each_local_configuration { |configuration| create configuration }
         if old_pool
-          ActiveRecord::Base.connection_handler.establish_connection(old_pool.spec)
+          ActiveRecord::Base.connection_handler.establish_connection(old_pool.spec.to_hash)
         end
       end
 
diff --git a/activerecord/test/cases/connection_adapters/connection_handler_test.rb b/activerecord/test/cases/connection_adapters/connection_handler_test.rb
index 50f942f5aa..a019cc6490 100644
--- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb
+++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb
@@ -5,16 +5,15 @@ module ActiveRecord
     class ConnectionHandlerTest < ActiveRecord::TestCase
       def setup
         @handler = ConnectionHandler.new
-        resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new Base.configurations
         @spec_name = "primary"
-        @pool    = @handler.establish_connection(resolver.spec(:arunit, @spec_name))
+        @pool    = @handler.establish_connection(ActiveRecord::Base.configurations['arunit'])
       end
 
       def test_establish_connection_uses_spec_name
         config = {"readonly" => {"adapter" => 'sqlite3'}}
         resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new(config)
         spec =   resolver.spec(:readonly)
-        @handler.establish_connection(spec)
+        @handler.establish_connection(spec.to_hash)
 
         assert_not_nil @handler.retrieve_connection_pool('readonly')
       ensure
diff --git a/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb
index 9ee92a3cd2..f25b85e8a7 100644
--- a/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb
+++ b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb
@@ -27,7 +27,7 @@ module ActiveRecord
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
         config   = { "not_production" => {  "adapter" => "not_postgres", "database" => "not_foo" } }
         actual   = resolve_spec(:default_env, config)
-        expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" }
+        expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost", "name"=>"default_env" }
         assert_equal expected, actual
       end
 
@@ -37,7 +37,7 @@ module ActiveRecord
 
         config   = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } }
         actual   = resolve_spec(:foo, config)
-        expected = { "adapter" => "postgresql", "database" => "foo", "host" => "localhost" }
+        expected = { "adapter" => "postgresql", "database" => "foo", "host" => "localhost","name"=>"foo" }
         assert_equal expected, actual
       end
 
@@ -47,7 +47,7 @@ module ActiveRecord
 
         config   = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } }
         actual   = resolve_spec(:foo, config)
-        expected = { "adapter" => "postgresql", "database" => "foo", "host" => "localhost" }
+        expected = { "adapter" => "postgresql", "database" => "foo", "host" => "localhost","name"=>"foo" }
         assert_equal expected, actual
       end
 
@@ -55,7 +55,7 @@ module ActiveRecord
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
         config   = { "production" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } }
         actual   = resolve_spec(:production, config)
-        expected = { "adapter"=>"not_postgres", "database"=>"not_foo", "host"=>"localhost" }
+        expected = { "adapter"=>"not_postgres", "database"=>"not_foo", "host"=>"localhost", "name"=>"production" }
         assert_equal expected, actual
       end
 
@@ -93,7 +93,7 @@ module ActiveRecord
         ENV['DATABASE_URL'] = "ibm-db://localhost/foo"
         config   = { "default_env" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } }
         actual   = resolve_spec(:default_env, config)
-        expected = { "adapter"=>"ibm_db", "database"=>"foo", "host"=>"localhost" }
+        expected = { "adapter"=>"ibm_db", "database"=>"foo", "host"=>"localhost", "name"=>"default_env" }
         assert_equal expected, actual
       end
 
diff --git a/activerecord/test/cases/connection_specification/resolver_test.rb b/activerecord/test/cases/connection_specification/resolver_test.rb
index 3bddaf32ec..b30a83d9ce 100644
--- a/activerecord/test/cases/connection_specification/resolver_test.rb
+++ b/activerecord/test/cases/connection_specification/resolver_test.rb
@@ -28,7 +28,8 @@ module ActiveRecord
           assert_equal({
             "adapter"  =>  "abstract",
             "host"     =>  "foo",
-            "encoding" => "utf8" }, spec)
+            "encoding" => "utf8",
+            "name"     => "production"}, spec)
         end
 
         def test_url_sub_key
@@ -36,7 +37,8 @@ module ActiveRecord
           assert_equal({
             "adapter"  => "abstract",
             "host"     => "foo",
-            "encoding" => "utf8" }, spec)
+            "encoding" => "utf8",
+            "name"     => "production"}, spec)
         end
 
         def test_url_sub_key_merges_correctly
@@ -46,7 +48,8 @@ module ActiveRecord
             "adapter"  => "abstract",
             "host"     => "foo",
             "encoding" => "utf8",
-            "pool"     => "3" }, spec)
+            "pool"     => "3",
+            "name"     => "production"}, spec)
         end
 
         def test_url_host_no_db
@@ -113,7 +116,8 @@ module ActiveRecord
           assert_equal({
             "adapter"  => "sqlite3",
             "database" => "foo",
-            "encoding" => "utf8" }, spec)
+            "encoding" => "utf8",
+            "name"     => "production"}, spec)
         end
 
         def test_spec_name_on_key_lookup