Merge pull request #13582 from schneems/schneems/preserve-connection-behavior

Ensure Active Record connection consistency

7 files changed, 268 insertions, 22 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index bd7dcb6167..d9f8ee7097 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,64 @@
+*   Currently Active Record can be configured via the environment variable
+    `DATABASE_URL` or by manually injecting a hash of values which is what Rails does,
+    reading in `database.yml` and setting Active Record appropriately. Active Record
+    expects to be able to use `DATABASE_URL` without the use of Rails, and we cannot
+    rip out this functionality without deprecating. This presents a problem though
+    when both config is set, and a `DATABASE_URL` is present. Currently the
+    `DATABASE_URL` should "win" and none of the values in `database.yml` are
+    used. This is somewhat unexpected, if one were to set values such as
+    `pool` in the `production:` group of `database.yml` they are ignored.
+
+    There are many ways that Active Record initiates a connection today:
+
+    - Stand Alone (without rails)
+      - `rake db:<tasks>`
+      - `ActiveRecord.establish_connection`
+
+    - With Rails
+      - `rake db:<tasks>`
+      - `rails <server> | <console>`
+      - `rails dbconsole`
+
+    Now all of these behave exactly the same way. The best way to do
+    this is to put all of this logic in one place so it is guaranteed to be used.
+
+    Here is the matrix of how this behavior works:
+
+    ```
+    No database.yml
+    No DATABASE_URL
+    => Error
+    ```
+
+    ```
+    database.yml present
+    No DATABASE_URL
+    => Use database.yml configuration
+    ```
+
+    ```
+    No database.yml
+    DATABASE_URL present
+    => use DATABASE_URL configuration
+    ```
+
+    ```
+    database.yml present
+    DATABASE_URL present
+    => Merged into `url` sub key. If both specify `url` sub key, the `database.yml` `url`
+       sub key "wins". If other paramaters `adapter` or `database` are specified in YAML,
+       they are discarded as the `url` sub key "wins".
+    ```
+
+    Current implementation uses `ActiveRecord::Base.configurations` to resolve and merge
+    all connection information before returning. This is achieved through a utility
+    class: `ActiveRecord::ConnectionHandling::MergeAndResolveDefaultUrlConfig`.
+
+    To understand the exact behavior of this class, it is best to review the
+    behavior in `activerecord/test/cases/connection_adapters/connection_handler_test.rb`
+
+    *Richard Schneeman*
+
 *   Make `change_column_null` revertable. Fixes #13576.
 
     *Yves Senn*, *Nishant Modak*, *Prathamesh Sonpatki*
diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb
index 9f210c5f33..3f8b14bf67 100644
--- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb
+++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb
@@ -123,13 +123,22 @@ module ActiveRecord
         def resolve(config)
           if config
             resolve_connection config
-          elsif defined?(Rails.env)
-            resolve_env_connection Rails.env.to_sym
+          elsif env = ActiveRecord::ConnectionHandling::RAILS_ENV.call
+            resolve_env_connection env.to_sym
           else
             raise AdapterNotSpecified
           end
         end
 
+        # Expands each key in @configurations hash into fully resolved hash
+        def resolve_all
+          config = configurations.dup
+          config.each do |key, value|
+            config[key] = resolve(value) if value
+          end
+          config
+        end
+
         # Returns an instance of ConnectionSpecification for a given adapter.
         # Accepts a hash one layer deep that contains all connection information.
         #
@@ -219,7 +228,7 @@ module ActiveRecord
           elsif spec.is_a?(String)
             resolve_string_connection(spec)
           else
-            raise(AdapterNotSpecified, "#{spec} database is not configured")
+            raise(AdapterNotSpecified, "'#{spec}' database is not configured. Available configuration: #{configurations.inspect}")
           end
         end
 
diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb
index c4afadbd9b..11f6a47158 100644
--- a/activerecord/lib/active_record/connection_handling.rb
+++ b/activerecord/lib/active_record/connection_handling.rb
@@ -1,5 +1,8 @@
 module ActiveRecord
   module ConnectionHandling
+    RAILS_ENV   = -> { Rails.env if defined?(Rails) }
+    DEFAULT_ENV = -> { RAILS_ENV.call || "default_env" }
+
     # Establishes the connection to the database. Accepts a hash as input where
     # the <tt>:adapter</tt> key must be specified with the name of a database adapter (in lower-case)
     # example for regular databases (MySQL, Postgresql, etc):
@@ -41,9 +44,10 @@ 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 configurations
-      spec = resolver.spec(spec)
+    def establish_connection(spec = nil)
+      spec     ||= DEFAULT_ENV.call.to_sym
+      resolver =   ConnectionAdapters::ConnectionSpecification::Resolver.new configurations
+      spec     =   resolver.spec(spec)
 
       unless respond_to?(spec.adapter_method)
         raise AdapterNotFound, "database configuration specifies nonexistent #{spec.config[:adapter]} adapter"
@@ -53,6 +57,56 @@ module ActiveRecord
       connection_handler.establish_connection self, spec
     end
 
+    class MergeAndResolveDefaultUrlConfig # :nodoc:
+      def initialize(raw_configurations, url = ENV['DATABASE_URL'])
+        @raw_config = raw_configurations.dup
+        @url        = url
+      end
+
+      # Returns fully resolved connection hashes.
+      # Merges connection information from `ENV['DATABASE_URL']` if available.
+      def resolve
+        ConnectionAdapters::ConnectionSpecification::Resolver.new(config).resolve_all
+      end
+
+      private
+        def config
+          if @url
+            raw_merged_into_default
+          else
+            @raw_config
+          end
+        end
+
+        def raw_merged_into_default
+          default = default_url_hash
+
+          @raw_config.each do |env, values|
+            default[env] = values || {}
+            default[env].merge!("url" => @url) { |h, v1, v2| v1 || v2 } if default[env].is_a?(Hash)
+          end
+          default
+        end
+
+        # When the raw configuration is not present and ENV['DATABASE_URL']
+        # is available we return a hash with the connection information in
+        # the connection URL. This hash responds to any string key with
+        # resolved connection information.
+        def default_url_hash
+          if @raw_config.blank?
+            Hash.new do |hash, key|
+              hash[key] = if key.is_a? String
+                 ActiveRecord::ConnectionAdapters::ConnectionSpecification::ConnectionUrlResolver.new(@url).to_hash
+              else
+                nil
+              end
+            end
+          else
+            {}
+          end
+        end
+    end
+
     # Returns the connection currently associated with the class. This can
     # also be used to "borrow" the connection to do database work unrelated
     # to any of the specific Active Records.
diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index 18ee77f6fe..cd8690d500 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -42,9 +42,16 @@ module ActiveRecord
       #         'database' => 'db/production.sqlite3'
       #      }
       #   }
-      mattr_accessor :configurations, instance_writer: false
+      def self.configurations=(config)
+        @@configurations = ActiveRecord::ConnectionHandling::MergeAndResolveDefaultUrlConfig.new(config).resolve
+      end
       self.configurations = {}
 
+      # Returns fully resolved configurations hash
+      def self.configurations
+        @@configurations
+      end
+
       ##
       # :singleton-method:
       # Determines whether to use Time.utc (using :utc) or Time.local (using :local) when pulling
diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb
index ec85b3c843..11b564f8f9 100644
--- a/activerecord/lib/active_record/railtie.rb
+++ b/activerecord/lib/active_record/railtie.rb
@@ -40,19 +40,7 @@ module ActiveRecord
 
       namespace :db do
         task :load_config do
-          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
+          ActiveRecord::Tasks::DatabaseTasks.database_configuration = Rails.application.config.database_configuration
 
           if defined?(ENGINE_PATH) && engine = Rails::Engine.find(ENGINE_PATH)
             if engine.paths['db/migrate'].existent
@@ -137,7 +125,7 @@ module ActiveRecord
           end
         end
 
-        self.configurations = app.config.database_configuration || {}
+        self.configurations = Rails.application.config.database_configuration
         establish_connection
       end
     end
diff --git a/activerecord/lib/active_record/railties/databases.rake b/activerecord/lib/active_record/railties/databases.rake
index 58dfa2c5a5..561387a179 100644
--- a/activerecord/lib/active_record/railties/databases.rake
+++ b/activerecord/lib/active_record/railties/databases.rake
@@ -2,7 +2,7 @@ require 'active_record'
 
 db_namespace = namespace :db do
   task :load_config do
-    ActiveRecord::Base.configurations = ActiveRecord::Tasks::DatabaseTasks.database_configuration || {}
+    ActiveRecord::Base.configurations       = ActiveRecord::Tasks::DatabaseTasks.database_configuration || {}
     ActiveRecord::Migrator.migrations_paths = ActiveRecord::Tasks::DatabaseTasks.migrations_paths
   end
 
diff --git a/activerecord/test/cases/connection_adapters/connection_handler_test.rb b/activerecord/test/cases/connection_adapters/connection_handler_test.rb
index 3e33b30144..3365ad1294 100644
--- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb
+++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb
@@ -2,6 +2,133 @@ require "cases/helper"
 
 module ActiveRecord
   module ConnectionAdapters
+
+    class MergeAndResolveDefaultUrlConfigTest < ActiveRecord::TestCase
+
+      def klass
+        ActiveRecord::ConnectionHandling::MergeAndResolveDefaultUrlConfig
+      end
+
+      def setup
+        @previous_database_url = ENV.delete("DATABASE_URL")
+      end
+
+      def teardown
+        ENV["DATABASE_URL"] = @previous_database_url if @previous_database_url
+      end
+
+      def test_string_connection
+        config   = { "production" => "postgres://localhost/foo" }
+        actual   = klass.new(config).resolve
+        expected = { "production" =>
+                     { "adapter"  => "postgresql",
+                       "database" => "foo",
+                       "host"     => "localhost"
+                      }
+                    }
+        assert_equal expected, actual
+      end
+
+      def test_url_sub_key
+        config   = { "production" => { "url" => "postgres://localhost/foo" } }
+        actual   = klass.new(config).resolve
+        expected = { "production" =>
+                     { "adapter"  => "postgresql",
+                       "database" => "foo",
+                       "host"     => "localhost"
+                      }
+                    }
+        assert_equal expected, actual
+      end
+
+      def test_hash
+        config = { "production" => { "adapter" => "postgres", "database" => "foo" } }
+        actual = klass.new(config).resolve
+        assert_equal config, actual
+      end
+
+      def test_blank
+        config = {}
+        actual = klass.new(config).resolve
+        assert_equal config, actual
+      end
+
+      def test_blank_with_database_url
+        ENV['DATABASE_URL'] = "postgres://localhost/foo"
+
+        config   = {}
+        actual   = klass.new(config).resolve
+        expected = { "adapter"  => "postgresql",
+                     "database" => "foo",
+                     "host"     => "localhost" }
+        assert_equal expected, actual["production"]
+        assert_equal expected, actual["development"]
+        assert_equal expected, actual["test"]
+        assert_equal nil,      actual[:production]
+        assert_equal nil,      actual[:development]
+        assert_equal nil,      actual[:test]
+      end
+
+      def test_sting_with_database_url
+        ENV['DATABASE_URL'] = "NOT-POSTGRES://localhost/NOT_FOO"
+
+        config   = { "production" => "postgres://localhost/foo" }
+        actual   = klass.new(config).resolve
+
+        expected = { "production" =>
+                     { "adapter"  => "postgresql",
+                       "database" => "foo",
+                       "host"     => "localhost"
+                      }
+                    }
+        assert_equal expected, actual
+      end
+
+      def test_url_sub_key_with_database_url
+        ENV['DATABASE_URL'] = "NOT-POSTGRES://localhost/NOT_FOO"
+
+        config   = { "production" => { "url" => "postgres://localhost/foo" } }
+        actual   = klass.new(config).resolve
+        expected = { "production" =>
+                    { "adapter"  => "postgresql",
+                       "database" => "foo",
+                       "host"     => "localhost"
+                      }
+                    }
+        assert_equal expected, actual
+      end
+
+      def test_merge_no_conflicts_with_database_url
+        ENV['DATABASE_URL'] = "postgres://localhost/foo"
+
+        config   = {"production" => { "pool" => "5" } }
+        actual   = klass.new(config).resolve
+        expected = { "production" =>
+                     { "adapter"  => "postgresql",
+                       "database" => "foo",
+                       "host"     => "localhost",
+                       "pool"     => "5"
+                      }
+                    }
+        assert_equal expected, actual
+      end
+
+      def test_merge_conflicts_with_database_url
+        ENV['DATABASE_URL'] = "postgres://localhost/foo"
+
+        config   = {"production" => { "adapter" => "NOT-POSTGRES", "database" => "NOT-FOO", "pool" => "5" } }
+        actual   = klass.new(config).resolve
+        expected = { "production" =>
+                     { "adapter"  => "postgresql",
+                       "database" => "foo",
+                       "host"     => "localhost",
+                       "pool"     => "5"
+                      }
+                    }
+        assert_equal expected, actual
+      end
+    end
+
     class ConnectionHandlerTest < ActiveRecord::TestCase
       def setup
         @klass    = Class.new(Base)   { def self.name; 'klass';    end }