From 88e60a48ec7f3447225e9f5e47a251d6e1af78e2 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Thu, 3 Apr 2014 03:33:40 +1030 Subject: Avoid a spurious deprecation warning for database URLs This is all about the case where we have a `DATABASE_URL`, and we have a `database.yml` present, but the latter doesn't contain the key we're looking for. If the key is a symbol, we'll always connect to `DATABASE_URL`, per the new behaviour in 283a2edec2f8ccdf90fb58025608f02a63948fa0. If the key is a string, on the other hand, it should always be a URL: the ability to specify a name not present in `database.yml` is new in this version of Rails, and that ability does not stretch to the deprecated use of a string in place of a symbol. Uncovered by @guilleiguaran while investigating #14495 -- this actually may be related to the original report, but we don't have enough info to confirm. --- .../connection_specification.rb | 50 ++++++++++++---------- .../connection_adapters/connection_handler_test.rb | 48 +++++++++++++++++++++ 2 files changed, 75 insertions(+), 23 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb index 9a133168f8..09ef0f389a 100644 --- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb +++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb @@ -124,7 +124,7 @@ module ActiveRecord if config resolve_connection config elsif env = ActiveRecord::ConnectionHandling::RAILS_ENV.call - resolve_env_connection env.to_sym + resolve_symbol_connection env.to_sym else raise AdapterNotSpecified end @@ -193,40 +193,39 @@ module ActiveRecord # def resolve_connection(spec) case spec - when Symbol, String - resolve_env_connection spec + when Symbol + resolve_symbol_connection spec + when String + resolve_string_connection spec when Hash resolve_hash_connection spec end end + def resolve_string_connection(spec) + # Rails has historically accepted a string to mean either + # an environment key or a URL spec, so we have deprecated + # this ambiguous behaviour and in the future this function + # can be removed in favor of resolve_url_connection. + if configurations.key?(spec) + ActiveSupport::Deprecation.warn "Passing a string to ActiveRecord::Base.establish_connection " \ + "for a configuration lookup is deprecated, please pass a symbol (#{spec.to_sym.inspect}) instead" + resolve_connection(configurations[spec]) + else + resolve_url_connection(spec) + end + end + # Takes the environment such as `:production` or `:development`. # This requires that the @configurations was initialized with a key that # matches. # - # - # Resolver.new("production" => {}).resolve_env_connection(:production) + # Resolver.new("production" => {}).resolve_symbol_connection(:production) # # => {} # - # Takes a connection URL. - # - # Resolver.new({}).resolve_env_connection("postgresql://localhost/foo") - # # => { "host" => "localhost", "database" => "foo", "adapter" => "postgresql" } - # - def resolve_env_connection(spec) - # Rails has historically accepted a string to mean either - # an environment key or a URL spec, so we have deprecated - # this ambiguous behaviour and in the future this function - # can be removed in favor of resolve_string_connection and - # resolve_symbol_connection. + def resolve_symbol_connection(spec) if config = configurations[spec.to_s] - if spec.is_a?(String) - ActiveSupport::Deprecation.warn "Passing a string to ActiveRecord::Base.establish_connection " \ - "for a configuration lookup is deprecated, please pass a symbol (#{spec.to_sym.inspect}) instead" - end resolve_connection(config) - elsif spec.is_a?(String) - resolve_string_connection(spec) else raise(AdapterNotSpecified, "'#{spec}' database is not configured. Available configuration: #{configurations.inspect}") end @@ -244,7 +243,12 @@ module ActiveRecord spec end - def resolve_string_connection(url) + # Takes a connection URL. + # + # Resolver.new({}).resolve_url_connection("postgresql://localhost/foo") + # # => { "host" => "localhost", "database" => "foo", "adapter" => "postgresql" } + # + def resolve_url_connection(url) ConnectionUrlResolver.new(url).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 2992ceb211..e097449029 100644 --- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb @@ -17,6 +17,54 @@ module ActiveRecord ENV["DATABASE_URL"] = @previous_database_url end + def resolve(spec, config) + ConnectionSpecification::Resolver.new(klass.new(config).resolve).resolve(spec) + end + + def spec(spec, config) + ConnectionSpecification::Resolver.new(klass.new(config).resolve).spec(spec) + end + + def test_resolver_with_database_uri_and_known_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "production" => { "adapter" => "not_postgres", "database" => "not_foo" } } + actual = resolve(:production, config) + expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_resolver_with_database_uri_and_known_string_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "production" => { "adapter" => "not_postgres", "database" => "not_foo" } } + actual = assert_deprecated { resolve("production", config) } + expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_resolver_with_database_uri_and_unknown_symbol_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } + actual = resolve(:production, config) + expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_resolver_with_database_uri_and_unknown_string_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } + assert_raises AdapterNotSpecified do + spec("production", config) + end + end + + def test_resolver_with_database_uri_and_supplied_url + ENV['DATABASE_URL'] = "not-postgres://not-localhost/not_foo" + config = { "production" => { "adapter" => "also_not_postgres", "database" => "also_not_foo" } } + actual = resolve("postgres://localhost/foo", config) + expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + def test_jdbc_url config = { "production" => { "url" => "jdbc:postgres://localhost/foo" } } actual = klass.new(config).resolve -- cgit v1.2.3 From ad1415381e4d6a74ac418b1bf4ba042b1f369765 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Thu, 3 Apr 2014 06:19:18 +1030 Subject: Avoid including DB details in exception messages The keys are quite sufficient; we shouldn't be throwing passwords around. --- .../lib/active_record/connection_adapters/connection_specification.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb index 09ef0f389a..d60d31eda4 100644 --- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb +++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb @@ -227,7 +227,7 @@ module ActiveRecord if config = configurations[spec.to_s] resolve_connection(config) else - raise(AdapterNotSpecified, "'#{spec}' database is not configured. Available configuration: #{configurations.inspect}") + raise(AdapterNotSpecified, "'#{spec}' database is not configured. Available: #{configurations.keys.inspect}") end end -- cgit v1.2.3