From 8d5a4ff6a7025751bb2ad67f1a00363745f9dbb5 Mon Sep 17 00:00:00 2001 From: John Crepezzi Date: Mon, 11 Nov 2019 14:17:54 -0700 Subject: [PATCH] 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 | 4 + .../abstract/connection_pool.rb | 48 +++++- .../connection_adapters/resolver.rb | 158 ------------------ .../lib/active_record/connection_handling.rb | 4 +- .../active_record/database_configurations.rb | 63 +++++++ .../lib/active_record/tasks/database_tasks.rb | 3 +- .../connection_handler_test.rb | 3 +- ...rge_and_resolve_default_url_config_test.rb | 3 +- .../resolver_test.rb | 30 +--- 9 files changed, 124 insertions(+), 192 deletions(-) delete mode 100644 activerecord/lib/active_record/connection_adapters/resolver.rb rename activerecord/test/cases/{pool_config => database_configurations}/resolver_test.rb (78%) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 95c1ea8258..66b4a47cad 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* 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* 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 c9fd12cd8b..69e5d447c9 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -1040,8 +1040,7 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/connection_adapters/resolver.rb b/activerecord/lib/active_record/connection_adapters/resolver.rb deleted file mode 100644 index 18700f7fcc..0000000000 --- a/activerecord/lib/active_record/connection_adapters/resolver.rb +++ /dev/null @@ -1,158 +0,0 @@ -# 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 = # - # ]> - # - # 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 diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 2432cfb951..d9c18a2379 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -256,9 +256,7 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/database_configurations.rb b/activerecord/lib/active_record/database_configurations.rb index 2a23f3939b..7d7475331f 100644 --- a/activerecord/lib/active_record/database_configurations.rb +++ b/activerecord/lib/active_record/database_configurations.rb @@ -106,6 +106,39 @@ module ActiveRecord [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 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/tasks/database_tasks.rb b/activerecord/lib/active_record/tasks/database_tasks.rb index 358a7d12b7..73b2c8ce48 100644 --- a/activerecord/lib/active_record/tasks/database_tasks.rb +++ b/activerecord/lib/active_record/tasks/database_tasks.rb @@ -456,8 +456,7 @@ module ActiveRecord private def resolve_configuration(configuration) - resolver = ConnectionAdapters::Resolver.new(ActiveRecord::Base.configurations) - resolver.resolve(configuration) + Base.configurations.resolve(configuration) end def verbose? diff --git a/activerecord/test/cases/connection_adapters/connection_handler_test.rb b/activerecord/test/cases/connection_adapters/connection_handler_test.rb index 29088cc2f9..2eeaaa9f3f 100644 --- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb @@ -31,8 +31,7 @@ module ActiveRecord 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) 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 5550ccaecc..43e73fa2d0 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 @@ -24,8 +24,7 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/pool_config/resolver_test.rb b/activerecord/test/cases/database_configurations/resolver_test.rb similarity index 78% rename from activerecord/test/cases/pool_config/resolver_test.rb rename to activerecord/test/cases/database_configurations/resolver_test.rb index b45e2a13bd..e610607f96 100644 --- a/activerecord/test/cases/pool_config/resolver_test.rb +++ b/activerecord/test/cases/database_configurations/resolver_test.rb @@ -8,19 +8,12 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord }, 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