Guarantee the connection resolver handles string values

This commit also cleans up the rake tasks that were checking
for DATABASE_URL in different places.

In fact, it would be nice to deprecate DATABASE_URL usage in the long
term, considering the direction we are moving of allowing those in .yml
files.

activerecord/lib/active_record/connection_adapters/connection_specification.rb

@@ -16,34 +16,43 @@ module ActiveRecord
       ##
       # Builds a ConnectionSpecification from user input
       class Resolver # :nodoc:
-        attr_reader :config, :klass, :configurations
+        attr_reader :configurations
 
-        def initialize(config, configurations)
-          @config         = config
+        def initialize(configurations)
           @configurations = configurations
         end
 
-        def spec
-          case config
-          when nil
-            raise AdapterNotSpecified unless defined?(Rails.env)
-            resolve_string_connection Rails.env
-          when Symbol, String
-            resolve_string_connection config.to_s
-          when Hash
-            resolve_hash_connection config
+        def resolve(config)
+          if config
+            resolve_connection config
+          elsif defined?(Rails.env)
+            resolve_env_connection Rails.env
+          else
+            raise AdapterNotSpecified
           end
         end
 
         private
-        def resolve_string_connection(spec) # :nodoc:
-          hash = configurations.fetch(spec) do |k|
-            connection_url_to_hash(k)
+
+        def resolve_connection(spec) #:nodoc:
+          case spec
+          when Symbol, String
+            resolve_env_connection spec.to_s
+          when Hash
+            resolve_hash_connection spec
           end
+        end
 
-          raise(AdapterNotSpecified, "#{spec} database is not configured") unless hash
-
-          resolve_hash_connection hash
+        def resolve_env_connection(spec) # :nodoc:
+          # Rails has historically accepted a string to mean either
+          # an environment key or a url spec. So we support both for
+          # now but it would be nice to limit the environment key only
+          # for symbols.
+          spec = configurations.fetch(spec.to_s) do
+            resolve_string_connection(spec) if spec.is_a?(String)
+          end
+          raise(AdapterNotSpecified, "#{spec} database is not configured") unless spec
+          resolve_connection spec
         end
 
         def resolve_hash_connection(spec) # :nodoc:
@@ -65,8 +74,8 @@ module ActiveRecord
           ConnectionSpecification.new(spec, adapter_method)
         end
 
-        def connection_url_to_hash(url) # :nodoc:
-          config = URI.parse url
+        def resolve_string_connection(spec) # :nodoc:
+          config = URI.parse spec
           adapter = config.scheme
           adapter = "postgresql" if adapter == "postgres"
 

activerecord/lib/active_record/connection_handling.rb

@@ -35,8 +35,8 @@ module ActiveRecord
     # The exceptions AdapterNotSpecified, AdapterNotFound and ArgumentError
     # may be returned on an error.
     def establish_connection(spec = ENV["DATABASE_URL"])
-      resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new spec, configurations
-      spec = resolver.spec
+      resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new configurations
+      spec = resolver.resolve(spec)
 
       unless respond_to?(spec.adapter_method)
         raise AdapterNotFound, "database configuration specifies nonexistent #{spec.config[:adapter]} adapter"

activerecord/lib/active_record/railtie.rb

@@ -43,11 +43,24 @@ module ActiveRecord
       namespace :db do
         task :load_config do
           ActiveRecord::Tasks::DatabaseTasks.db_dir = Rails.application.config.paths["db"].first
-          ActiveRecord::Tasks::DatabaseTasks.database_configuration = Rails.application.config.database_configuration
           ActiveRecord::Tasks::DatabaseTasks.migrations_paths = Rails.application.paths['db/migrate'].to_a
           ActiveRecord::Tasks::DatabaseTasks.fixtures_path = File.join Rails.root, 'test', 'fixtures'
           ActiveRecord::Tasks::DatabaseTasks.root = Rails.root
 
+          configuration = if ENV["DATABASE_URL"]
+            { Rails.env => ENV["DATABASE_URL"] }
+          else
+            Rails.application.config.database_configuration || {}
+          end
+
+          resolver = ActiveRecord::ConnectionAdapters::ConnectionSpecification::Resolver.new(configuration)
+
+          configuration.each do |key, value|
+            configuration[key] = resolver.resolve(value) if value
+          end
+
+          ActiveRecord::Tasks::DatabaseTasks.database_configuration = configuration
+
           if defined?(ENGINE_PATH) && engine = Rails::Engine.find(ENGINE_PATH)
             if engine.paths['db/migrate'].existent
               ActiveRecord::Tasks::DatabaseTasks.migrations_paths += engine.paths['db/migrate'].to_a

activerecord/lib/active_record/railties/databases.rake

@@ -14,11 +14,7 @@ db_namespace = namespace :db do
 
   desc 'Create the database from DATABASE_URL or config/database.yml for the current Rails.env (use db:create:all to create all databases in the config)'
   task :create => [:load_config] do
-    if ENV['DATABASE_URL']
-      ActiveRecord::Tasks::DatabaseTasks.create_database_url
-    else
-      ActiveRecord::Tasks::DatabaseTasks.create_current
-    end
+    ActiveRecord::Tasks::DatabaseTasks.create_current
   end
 
   namespace :drop do
@@ -29,11 +25,7 @@ db_namespace = namespace :db do
 
   desc 'Drops the database using DATABASE_URL or the current Rails.env (use db:drop:all to drop all databases)'
   task :drop => [:load_config] do
-    if ENV['DATABASE_URL']
-      ActiveRecord::Tasks::DatabaseTasks.drop_database_url
-    else
-      ActiveRecord::Tasks::DatabaseTasks.drop_current
-    end
+    ActiveRecord::Tasks::DatabaseTasks.drop_current
   end
 
   desc "Migrate the database (options: VERSION=x, VERBOSE=false, SCOPE=blog)."

activerecord/lib/active_record/tasks/database_tasks.rb

@@ -56,11 +56,7 @@ module ActiveRecord
         if options.has_key?(:config)
           @current_config = options[:config]
         else
-          @current_config ||= if ENV['DATABASE_URL']
-                                database_url_config
-                              else
-                                ActiveRecord::Base.configurations[options[:env]]
-                              end
+          @current_config ||= ActiveRecord::Base.configurations[options[:env]]
         end
       end
 
@@ -85,10 +81,6 @@ module ActiveRecord
         ActiveRecord::Base.establish_connection environment
       end
 
-      def create_database_url
-        create database_url_config
-      end
-
       def drop(*arguments)
         configuration = arguments.first
         class_for_adapter(configuration['adapter']).new(*arguments).drop
@@ -107,10 +99,6 @@ module ActiveRecord
         }
       end
 
-      def drop_database_url
-        drop database_url_config
-      end
-
       def charset_current(environment = env)
         charset ActiveRecord::Base.configurations[environment]
       end
@@ -165,11 +153,6 @@ module ActiveRecord
 
       private
 
-      def database_url_config
-        @database_url_config ||=
-               ConnectionAdapters::ConnectionSpecification::Resolver.new(ENV["DATABASE_URL"], {}).spec.config.stringify_keys
-      end
-
       def class_for_adapter(adapter)
         key = @tasks.keys.detect { |pattern| adapter[pattern] }
         unless key

activerecord/test/cases/connection_specification/resolver_test.rb

@@ -4,8 +4,8 @@ module ActiveRecord
   module ConnectionAdapters
     class ConnectionSpecification
       class ResolverTest < ActiveRecord::TestCase
-        def resolve(spec)
-          Resolver.new(spec, {}).spec.config
+        def resolve(spec, config={})
+          Resolver.new(config).resolve(spec).config
         end
 
         def test_url_invalid_adapter
@@ -17,6 +17,14 @@ module ActiveRecord
         # The abstract adapter is used simply to bypass the bit of code that
         # checks that the adapter file can be required in.
 
+        def test_url_from_environment
+          spec = resolve :production, 'production' => 'abstract://foo?encoding=utf8'
+          assert_equal({
+            adapter:  "abstract",
+            host:     "foo",
+            encoding: "utf8" }, spec)
+        end
+
         def test_url_host_no_db
           spec = resolve 'abstract://foo?encoding=utf8'
           assert_equal({