Less ambition, more deprecation

The "DATABASE_URL_*" idea was moving in the wrong direction.

Instead, let's deprecate the situation where we end up using
ENV['DATABASE_URL'] at all: the Right Way is to explicitly include it in
database.yml with ERB.

2 files changed, 27 insertions, 75 deletions

diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb
index f83829bb26..3667a42864 100644
--- a/activerecord/lib/active_record/connection_handling.rb
+++ b/activerecord/lib/active_record/connection_handling.rb
@@ -72,24 +72,24 @@ module ActiveRecord
       private
         def config
           @raw_config.dup.tap do |cfg|
-            urls_in_environment.each do |key, url|
-              cfg[key] ||= {}
-              cfg[key]["url"] ||= url
-            end
-          end
-        end
-
-        def urls_in_environment
-          {}.tap do |mapping|
-            ENV.each do |k, v|
-              if k =~ /\ADATABASE_URL_(.*)/
-                mapping[$1.downcase] = v
+            if url = ENV['DATABASE_URL']
+              if cfg[@env]
+                if cfg[@env]["url"]
+                  # Nothing to do
+                else
+                  ActiveSupport::Deprecation.warn "Overriding database configuration with DATABASE_URL without using an ERB tag in database.yml is deprecated. Please update the entry for #{@env.inspect}:\n\n" \
+                    "  #{@env}:\n    url: <%= ENV['DATABASE_URL'] %>\n\n"\
+                    "This will be required in Rails 4.2"
+                  cfg[@env]["url"] = url
+                end
+              else
+                cfg[@env] = {}
+                ActiveSupport::Deprecation.warn "Supplying DATABASE_URL without a matching entry in database.yml is deprecated. Please add an entry for #{@env.inspect}:\n\n" \
+                  "  #{@env}:\n    url: <%= ENV['DATABASE_URL'] %>\n\n"\
+                  "This will be required in Rails 4.2"
+                cfg[@env]["url"] ||= url
               end
             end
-
-            # Check for this last, because it is prioritised over the
-            # longer "DATABASE_URL_#{@env}" spelling
-            mapping[@env] = ENV['DATABASE_URL'] if ENV['DATABASE_URL']
           end
         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 d6abc13c0d..a7002bd13d 100644
--- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb
+++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb
@@ -11,14 +11,10 @@ module ActiveRecord
 
       def setup
         @previous_database_url = ENV.delete("DATABASE_URL")
-        @previous_database_url_default_env = ENV.delete("DATABASE_URL_DEFAULT_ENV")
-        @previous_database_url_production = ENV.delete("DATABASE_URL_PRODUCTION")
       end
 
       teardown do
         ENV["DATABASE_URL"] = @previous_database_url
-        ENV["DATABASE_URL_DEFAULT_ENV"] = @previous_database_url_default_env
-        ENV["DATABASE_URL_PRODUCTION"] = @previous_database_url_production
       end
 
       def resolve(spec, config)
@@ -32,24 +28,7 @@ module ActiveRecord
       def test_resolver_with_database_uri_and_current_env_symbol_key
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
         config   = { "not_production" => {  "adapter" => "not_postgres", "database" => "not_foo" } }
-        actual   = resolve(:default_env, config)
-        expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" }
-        assert_equal expected, actual
-      end
-
-      def test_resolver_with_environment_database_uri_and_current_env_symbol_key
-        ENV['DATABASE_URL_DEFAULT_ENV'] = "postgres://localhost/foo"
-        config   = { "default_env" => {  "adapter" => "not_postgres", "database" => "not_foo" } }
-        actual   = resolve(:default_env, config)
-        expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" }
-        assert_equal expected, actual
-      end
-
-      def test_resolver_with_environment_database_uri_and_global_database_uri_and_current_env_symbol_key
-        ENV['DATABASE_URL'] = "postgres://localhost/foo"
-        ENV['DATABASE_URL_DEFAULT_ENV'] = "mysql://host/foo_bar"
-        config   = { "default_env" => {  "adapter" => "not_postgres", "database" => "not_foo" } }
-        actual   = resolve(:default_env, config)
+        actual   = assert_deprecated { resolve(:default_env, config) }
         expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" }
         assert_equal expected, actual
       end
@@ -65,32 +44,18 @@ module ActiveRecord
       def test_resolver_with_database_uri_and_known_key
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
         config   = { "production" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } }
-        actual   = resolve(:production, config)
+        actual   = assert_deprecated { resolve(:production, config) }
         expected = { "adapter"=>"not_postgres", "database"=>"not_foo", "host"=>"localhost" }
         assert_equal expected, actual
       end
 
-      def test_resolver_with_custom_database_uri_and_no_matching_config
-        ENV['DATABASE_URL_PRODUCTION'] = "postgres://localhost/foo"
-        config   = {}
-        actual   = resolve(:production, config)
-        expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" }
-        assert_equal expected, actual
-      end
-
-      def test_resolver_with_custom_database_uri_and_custom_key
-        ENV['DATABASE_URL_PRODUCTION'] = "postgres://localhost/foo"
-        config   = { "production" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } }
-        actual   = 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" } }
-        assert_raises AdapterNotSpecified do
-          resolve(:production, config)
+        assert_deprecated do
+          assert_raises AdapterNotSpecified do
+            resolve(:production, config)
+          end
         end
       end
 
@@ -107,7 +72,7 @@ module ActiveRecord
       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)
+        actual   = assert_deprecated { resolve("postgres://localhost/foo", config) }
         expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" }
         assert_equal expected, actual
       end
@@ -121,24 +86,11 @@ module ActiveRecord
       def test_environment_does_not_exist_in_config_url_does_exist
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
         config      = { "not_default_env" => {  "adapter" => "not_postgres", "database" => "not_foo" } }
-        actual      = klass.new(config).resolve
+        actual      = assert_deprecated { klass.new(config).resolve }
         expect_prod = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" }
         assert_equal expect_prod, actual["default_env"]
       end
 
-      def test_environment_key_available_immediately
-        ENV['DATABASE_URL_PRODUCTION'] = "postgres://localhost/foo"
-        config   = {}
-        actual   = klass.new(config).resolve
-        expected = { "production" =>
-                     { "adapter"  => "postgresql",
-                       "database" => "foo",
-                       "host"     => "localhost"
-                     }
-                   }
-        assert_equal expected, actual
-      end
-
       def test_string_connection
         config   = { "default_env" => "postgres://localhost/foo" }
         actual   = klass.new(config).resolve
@@ -179,7 +131,7 @@ module ActiveRecord
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
 
         config   = {}
-        actual   = klass.new(config).resolve
+        actual   = assert_deprecated { klass.new(config).resolve }
         expected = { "adapter"  => "postgresql",
                      "database" => "foo",
                      "host"     => "localhost" }
@@ -210,7 +162,7 @@ module ActiveRecord
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
 
         config   = {"default_env" => { "pool" => "5" } }
-        actual   = klass.new(config).resolve
+        actual   = assert_deprecated { klass.new(config).resolve }
         expected = { "default_env" =>
                      { "adapter"  => "postgresql",
                        "database" => "foo",
@@ -225,7 +177,7 @@ module ActiveRecord
         ENV['DATABASE_URL'] = "postgres://localhost/foo"
 
         config   = {"default_env" => { "adapter" => "NOT-POSTGRES", "database" => "NOT-FOO", "pool" => "5" } }
-        actual   = klass.new(config).resolve
+        actual   = assert_deprecated { klass.new(config).resolve }
         expected = { "default_env" =>
                      { "adapter"  => "postgresql",
                        "database" => "foo",