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.

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