Remove ConnectionAdapters::Resolver in favor of DatabaseConfigurations

We have these two objects, `ConnectionAdapters::Resolver` and
`DatabaseConfiguratons` that implement a lot of the same logic. One of
them is used for configurations defined in `config/database.yml` and the
other is used when passing raw configurations `String` or `Hash` objects
into methods like `establish_connection`.

Over time these two have diverged a bit. In the interest of less code
complexity, and more consistency for users this commit brings them back
together.

* Remove `Resolver` altogether and replace its primary method with
  `DatabaseConfigurations#resolve`.

* Move `resolve_pool_config` over to the `ConnectionPool` alongside the code
  that uses it.

activerecord/CHANGELOG.md

+*   Calling methods like `establish_connection` with a `Hash` which is invalid (eg: no `adapter`) will now raise an error the same way as connections defined in `config/database.yml`.
+
+    *John Crepezzi*
+
 *   Specifying `implicit_order_column` now subsorts the records by primary key if available to ensure deterministic results.
 
     *Paweł Urbanek*

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

@@ -1040,8 +1040,7 @@ def connection_pool_list
       alias :connection_pools :connection_pool_list
 
       def establish_connection(config, pool_key = :default)
-        resolver = Resolver.new(Base.configurations)
-        pool_config = resolver.resolve_pool_config(config)
+        pool_config = resolve_pool_config(config)
         db_config = pool_config.db_config
 
         remove_connection(pool_config.connection_specification_name, pool_key)
@@ -1146,6 +1145,51 @@ def retrieve_connection_pool(spec_name, pool_key = :default)
 
       private
         attr_reader :owner_to_pool_manager
+
+        # Returns an instance of PoolConfig for a given adapter.
+        # Accepts a hash one layer deep that contains all connection information.
+        #
+        # == Example
+        #
+        #   config = { "production" => { "host" => "localhost", "database" => "foo", "adapter" => "sqlite3" } }
+        #   pool_config = Base.configurations.resolve_pool_config(:production)
+        #   pool_config.db_config.configuration_hash
+        #   # => { host: "localhost", database: "foo", adapter: "sqlite3" }
+        #
+        def resolve_pool_config(config)
+          pool_name = config if config.is_a?(Symbol)
+
+          db_config = Base.configurations.resolve(config, pool_name)
+
+          raise(AdapterNotSpecified, "database configuration does not specify adapter") unless db_config.adapter
+
+          # Require the adapter itself and give useful feedback about
+          #   1. Missing adapter gems and
+          #   2. Adapter gems' missing dependencies.
+          path_to_adapter = "active_record/connection_adapters/#{db_config.adapter}_adapter"
+          begin
+            require path_to_adapter
+          rescue LoadError => e
+            # We couldn't require the adapter itself. Raise an exception that
+            # points out config typos and missing gems.
+            if e.path == path_to_adapter
+              # We can assume that a non-builtin adapter was specified, so it's
+              # either misspelled or missing from Gemfile.
+              raise LoadError, "Could not load the '#{db_config.adapter}' Active Record adapter. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile.", e.backtrace
+
+              # Bubbled up from the adapter require. Prefix the exception message
+              # with some guidance about how to address it and reraise.
+            else
+              raise LoadError, "Error loading the '#{db_config.adapter}' Active Record adapter. Missing a gem it depends on? #{e.message}", e.backtrace
+            end
+          end
+
+          unless ActiveRecord::Base.respond_to?(db_config.adapter_method)
+            raise AdapterNotFound, "database configuration specifies nonexistent #{db_config.adapter} adapter"
+          end
+
+          ConnectionAdapters::PoolConfig.new(db_config.configuration_hash.delete(:name) || "primary", db_config)
+        end
     end
   end
 end

activerecord/lib/active_record/connection_adapters/resolver.rb

-# frozen_string_literal: true
-
-module ActiveRecord
-  module ConnectionAdapters
-    # Builds a PoolConfig from user input.
-    class Resolver # :nodoc:
-      attr_reader :configurations
-
-      # Accepts a list of db config objects.
-      def initialize(configurations)
-        @configurations = configurations
-      end
-
-      # Returns an instance of PoolConfig for a given adapter.
-      # Accepts a hash one layer deep that contains all connection information.
-      #
-      # == Example
-      #
-      #   config = { "production" => { "host" => "localhost", "database" => "foo", "adapter" => "sqlite3" } }
-      #   pool_config = Resolver.new(config).resolve_pool_config(:production)
-      #   pool_config.db_config.configuration_hash
-      #   # => { host: "localhost", database: "foo", adapter: "sqlite3" }
-      #
-      def resolve_pool_config(config)
-        pool_name = config if config.is_a?(Symbol)
-
-        db_config = resolve(config, pool_name)
-
-        raise(AdapterNotSpecified, "database configuration does not specify adapter") unless db_config.adapter
-
-        # Require the adapter itself and give useful feedback about
-        #   1. Missing adapter gems and
-        #   2. Adapter gems' missing dependencies.
-        path_to_adapter = "active_record/connection_adapters/#{db_config.adapter}_adapter"
-        begin
-          require path_to_adapter
-        rescue LoadError => e
-          # We couldn't require the adapter itself. Raise an exception that
-          # points out config typos and missing gems.
-          if e.path == path_to_adapter
-            # We can assume that a non-builtin adapter was specified, so it's
-            # either misspelled or missing from Gemfile.
-            raise LoadError, "Could not load the '#{db_config.adapter}' Active Record adapter. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile.", e.backtrace
-
-            # Bubbled up from the adapter require. Prefix the exception message
-            # with some guidance about how to address it and reraise.
-          else
-            raise LoadError, "Error loading the '#{db_config.adapter}' Active Record adapter. Missing a gem it depends on? #{e.message}", e.backtrace
-          end
-        end
-
-        unless ActiveRecord::Base.respond_to?(db_config.adapter_method)
-          raise AdapterNotFound, "database configuration specifies nonexistent #{db_config.adapter} adapter"
-        end
-
-        PoolConfig.new(db_config.configuration_hash.delete(:name) || "primary", db_config)
-      end
-
-      # Returns fully resolved connection, accepts hash, string or symbol.
-      # Always returns a DatabaseConfiguration::DatabaseConfig
-      #
-      # == Examples
-      #
-      # Symbol representing current environment.
-      #
-      #   Resolver.new("production" => {}).resolve(:production)
-      #   # => DatabaseConfigurations::HashConfig.new(env_name: "production", config: {})
-      #
-      # One layer deep hash of connection values.
-      #
-      #   Resolver.new({}).resolve("adapter" => "sqlite3")
-      #   # => DatabaseConfigurations::HashConfig.new(config: {"adapter" => "sqlite3"})
-      #
-      # Connection URL.
-      #
-      #   Resolver.new({}).resolve("postgresql://localhost/foo")
-      #   # => DatabaseConfigurations::UrlConfig.new(config: {"adapter" => "postgresql", "host" => "localhost", "database" => "foo"})
-      #
-      def resolve(config_or_env, pool_name = nil)
-        env = ActiveRecord::ConnectionHandling::DEFAULT_ENV.call.to_s
-
-        case config_or_env
-        when Symbol
-          resolve_symbol_connection(config_or_env, pool_name)
-        when String
-          DatabaseConfigurations::UrlConfig.new(env, "primary", config_or_env)
-        when Hash
-          resolve_hash_configuration(env, config_or_env.symbolize_keys)
-        when DatabaseConfigurations::DatabaseConfig
-          config_or_env
-        else
-          raise TypeError, "Invalid type for configuration. Expected Symbol, String, or Hash. Got #{config_or_env.inspect}"
-        end
-      end
-
-      private
-        # Resolve a hash to a valid configuration object. This method will
-        # either return a HashConfig, or a UrlConfig if the passed Hash
-        # contains a `:url` key.
-        def resolve_hash_configuration(env, config)
-          if config.has_key?(:url)
-            url = config[:url]
-            config_without_url = config.dup
-            config_without_url.delete :url
-            DatabaseConfigurations::UrlConfig.new(env, "primary", url, config)
-          else
-            DatabaseConfigurations::HashConfig.new(env, "primary", config)
-          end
-        end
-
-        # Takes the environment such as +:production+ or +:development+ and a
-        # pool name the corresponds to the name given by the connection pool
-        # to the connection. That pool name is merged into the hash with the
-        # name key.
-        #
-        # This requires that the @configurations was initialized with a key that
-        # matches.
-        #
-        #   configurations = #<ActiveRecord::DatabaseConfigurations:0x00007fd9fdace3e0
-        #     @configurations=[
-        #       #<ActiveRecord::DatabaseConfigurations::HashConfig:0x00007fd9fdace250
-        #         @env_name="production", @spec_name="primary", @config={database: "my_db"}>
-        #       ]>
-        #
-        #   Resolver.new(configurations).resolve_symbol_connection(:production, "primary")
-        #   # => DatabaseConfigurations::HashConfig(config: database: "my_db", env_name: "production", spec_name: "primary")
-        def resolve_symbol_connection(env_name, pool_name)
-          db_config = configurations.find_db_config(env_name)
-
-          if db_config
-            config = db_config.configuration_hash.merge(name: pool_name.to_s)
-            DatabaseConfigurations::HashConfig.new(db_config.env_name, db_config.spec_name, config)
-          else
-            raise AdapterNotSpecified, <<~MSG
-            The `#{env_name}` database is not configured for the `#{ActiveRecord::ConnectionHandling::DEFAULT_ENV.call}` environment.
-
-              Available databases configurations are:
-
-              #{build_configuration_sentence}
-              MSG
-          end
-        end
-
-        def build_configuration_sentence # :nodoc:
-          configs = configurations.configs_for(include_replicas: true)
-
-          configs.group_by(&:env_name).map do |env, config|
-            namespaces = config.map(&:spec_name)
-            if namespaces.size > 1
-              "#{env}: #{namespaces.join(", ")}"
-            else
-              env
-            end
-          end.join("\n")
-        end
-    end
-  end
-end

activerecord/lib/active_record/connection_handling.rb

@@ -256,9 +256,7 @@ def resolve_config_for_connection(config_or_env)
         pool_name = primary_class? ? "primary" : name
         self.connection_specification_name = pool_name
 
-        resolver = ConnectionAdapters::Resolver.new(Base.configurations)
-
-        db_config = resolver.resolve(config_or_env, pool_name)
+        db_config = Base.configurations.resolve(config_or_env, pool_name)
         db_config.configuration_hash[:name] = pool_name
         db_config
       end

activerecord/lib/active_record/database_configurations.rb

@@ -106,6 +106,39 @@ def first
       [config.env_name, config.configuration_hash]
     end
 
+    # Returns fully resolved connection, accepts hash, string or symbol.
+    # Always returns a DatabaseConfiguration::DatabaseConfig
+    #
+    # == Examples
+    #
+    # Symbol representing current environment.
+    #
+    #   DatabaseConfigurations.new("production" => {}).resolve(:production)
+    #   # => DatabaseConfigurations::HashConfig.new(env_name: "production", config: {})
+    #
+    # One layer deep hash of connection values.
+    #
+    #   DatabaseConfigurations.new({}).resolve("adapter" => "sqlite3")
+    #   # => DatabaseConfigurations::HashConfig.new(config: {"adapter" => "sqlite3"})
+    #
+    # Connection URL.
+    #
+    #   DatabaseConfigurations.new({}).resolve("postgresql://localhost/foo")
+    #   # => DatabaseConfigurations::UrlConfig.new(config: {"adapter" => "postgresql", "host" => "localhost", "database" => "foo"})
+    def resolve(config, pool_name = nil) # :nodoc:
+      return config if DatabaseConfigurations::DatabaseConfig === config
+
+      case config
+      when Symbol
+        resolve_symbol_connection(config, pool_name)
+      when Hash, String
+        env = ActiveRecord::ConnectionHandling::DEFAULT_ENV.call.to_s
+        build_db_config_from_raw_config(env, "primary", config)
+      else
+        raise TypeError, "Invalid type for configuration. Expected Symbol, String, or Hash. Got #{config.inspect}"
+      end
+    end
+
     private
       def env_with_configs(env = nil)
         if env
@@ -142,6 +175,36 @@ def walk_configs(env_name, config)
         end
       end
 
+      def resolve_symbol_connection(env_name, pool_name)
+        db_config = find_db_config(env_name)
+
+        if db_config
+          config = db_config.configuration_hash.merge(name: pool_name.to_s)
+          DatabaseConfigurations::HashConfig.new(db_config.env_name, db_config.spec_name, config)
+        else
+          raise AdapterNotSpecified, <<~MSG
+            The `#{env_name}` database is not configured for the `#{ActiveRecord::ConnectionHandling::DEFAULT_ENV.call}` environment.
+
+              Available databases configurations are:
+
+              #{build_configuration_sentence}
+          MSG
+        end
+      end
+
+      def build_configuration_sentence
+        configs = configs_for(include_replicas: true)
+
+        configs.group_by(&:env_name).map do |env, config|
+          namespaces = config.map(&:spec_name)
+          if namespaces.size > 1
+            "#{env}: #{namespaces.join(", ")}"
+          else
+            env
+          end
+        end.join("\n")
+      end
+
       def build_db_config_from_raw_config(env_name, spec_name, config)
         case config
         when String

activerecord/lib/active_record/tasks/database_tasks.rb

@@ -456,8 +456,7 @@ def dump_schema_cache(conn, filename)
 
       private
         def resolve_configuration(configuration)
-          resolver = ConnectionAdapters::Resolver.new(ActiveRecord::Base.configurations)
-          resolver.resolve(configuration)
+          Base.configurations.resolve(configuration)
         end
 
         def verbose?

activerecord/test/cases/connection_adapters/connection_handler_test.rb

@@ -31,8 +31,7 @@ def test_establish_connection_uses_config_hash_with_spec_name
         old_config = ActiveRecord::Base.configurations
         config = { "readonly" => { "adapter" => "sqlite3", "pool" => "5" } }
         ActiveRecord::Base.configurations = config
-        resolver = ConnectionAdapters::Resolver.new(ActiveRecord::Base.configurations)
-        config_hash = resolver.resolve(config["readonly"], "readonly").configuration_hash
+        config_hash = ActiveRecord::Base.configurations.resolve(config["readonly"], "readonly").configuration_hash
         config_hash[:name] = "readonly"
         @handler.establish_connection(config_hash)
 

activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb

@@ -24,8 +24,7 @@ def resolve_config(config, env_name = ActiveRecord::ConnectionHandling::DEFAULT_
 
       def resolve_spec(spec, config)
         configs = ActiveRecord::DatabaseConfigurations.new(config)
-        resolver = ConnectionAdapters::Resolver.new(configs)
-        resolver.resolve(spec, spec).configuration_hash
+        configs.resolve(spec, spec).configuration_hash
       end
 
       def test_invalid_string_config

activerecord/test/cases/pool_config/resolver_test.rb → activerecord/test/cases/database_configurations/resolver_test.rb

@@ -8,19 +8,12 @@ class PoolConfig
       class ResolverTest < ActiveRecord::TestCase
         def resolve(pool_config, config = {})
           configs = ActiveRecord::DatabaseConfigurations.new(config)
-          resolver = ConnectionAdapters::Resolver.new(configs)
-          resolver.resolve(pool_config, pool_config).configuration_hash
-        end
-
-        def resolve_pool_config(pool_config, config = {})
-          configs = ActiveRecord::DatabaseConfigurations.new(config)
-          resolver = ConnectionAdapters::Resolver.new(configs)
-          resolver.resolve_pool_config(pool_config)
+          configs.resolve(pool_config, pool_config).configuration_hash
         end
 
         def test_url_invalid_adapter
           error = assert_raises(LoadError) do
-            resolve_pool_config "ridiculous://foo?encoding=utf8"
+            Base.connection_handler.establish_connection "ridiculous://foo?encoding=utf8"
           end
 
           assert_match "Could not load the 'ridiculous' Active Record adapter. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile.", error.message
@@ -28,7 +21,7 @@ def test_url_invalid_adapter
 
         def test_error_if_no_adapter_method
           error = assert_raises(AdapterNotFound) do
-            resolve_pool_config "abstract://foo?encoding=utf8"
+            Base.connection_handler.establish_connection "abstract://foo?encoding=utf8"
           end
 
           assert_match "database configuration specifies nonexistent abstract adapter", error.message
@@ -79,8 +72,9 @@ def test_url_host_no_db
         end
 
         def test_url_missing_scheme
-          pool_config = resolve "foo"
-          assert_equal({ database: "foo" }, pool_config)
+          assert_raises ActiveRecord::DatabaseConfigurations::InvalidConfigurationError do
+            resolve "foo"
+          end
         end
 
         def test_url_host_db
@@ -140,19 +134,9 @@ def test_url_sub_key_for_sqlite3
           }, pool_config)
         end
 
-        def test_pool_config_connection_specification_name_on_key_lookup
-          pool_config = resolve_pool_config(:readonly, "readonly" => { "adapter" => "sqlite3" })
-          assert_equal "readonly", pool_config.connection_specification_name
-        end
-
-        def test_pool_config_connection_specification_name_with_inline_config
-          pool_config = resolve_pool_config("adapter" => "sqlite3")
-          assert_equal "primary", pool_config.connection_specification_name, "should default to primary id"
-        end
-
         def test_pool_config_with_invalid_type
           assert_raises TypeError do
-            resolve_pool_config(Object.new)
+            Base.connection_handler.establish_connection(Object.new)
           end
         end
       end