Merge pull request rails#50893 from rails/rm-fix-schema-cache-multiple-configs

 Unify the logic to determine the default schema cache path for a database configuration

Author: rafaelfranca

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Deprecate passing strings to `ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename`.
+
+    A `ActiveRecord::DatabaseConfigurations::DatabaseConfig` object should be passed instead.
+
+    *Rafael Mendonça França*
+
 *   Add row_count field to sql.active_record notification
 
     This field returns the amount of rows returned by the query that emitted the notification.

activerecord/lib/active_record/database_configurations/hash_config.rb

@@ -1,53 +1,54 @@
 # frozen_string_literal: true
 
+# :markup: markdown
+
 module ActiveRecord
   class DatabaseConfigurations
-    # = Active Record Database Hash Config
+    # # Active Record Database Hash Config
     #
-    # A +HashConfig+ object is created for each database configuration entry that
-    # is created from a hash.
+    # A `HashConfig` object is created for each database configuration entry that is
+    # created from a hash.
     #
     # A hash config:
     #
-    #   { "development" => { "database" => "db_name" } }
+    #     { "development" => { "database" => "db_name" } }
     #
     # Becomes:
     #
-    #   #<ActiveRecord::DatabaseConfigurations::HashConfig:0x00007fd1acbded10
-    #     @env_name="development", @name="primary", @config={database: "db_name"}>
+    #     #<ActiveRecord::DatabaseConfigurations::HashConfig:0x00007fd1acbded10
+    #       @env_name="development", @name="primary", @config={database: "db_name"}>
     #
     # See ActiveRecord::DatabaseConfigurations for more info.
     class HashConfig < DatabaseConfig
       attr_reader :configuration_hash
 
-
-      # Initialize a new +HashConfig+ object
+      # Initialize a new `HashConfig` object
+      #
+      # #### Parameters
       #
-      # ==== Options
+      # *   `env_name` - The Rails environment, i.e. "development".
+      # *   `name` - The db config name. In a standard two-tier database configuration
+      #     this will default to "primary". In a multiple database three-tier database
+      #     configuration this corresponds to the name used in the second tier, for
+      #     example "primary_readonly".
+      # *   `configuration_hash` - The config hash. This is the hash that contains the
+      #     database adapter, name, and other important information for database
+      #     connections.
       #
-      # * <tt>:env_name</tt> - The \Rails environment, i.e. "development".
-      # * <tt>:name</tt> - The db config name. In a standard two-tier
-      #   database configuration this will default to "primary". In a multiple
-      #   database three-tier database configuration this corresponds to the name
-      #   used in the second tier, for example "primary_readonly".
-      # * <tt>:config</tt> - The config hash. This is the hash that contains the
-      #   database adapter, name, and other important information for database
-      #   connections.
       def initialize(env_name, name, configuration_hash)
         super(env_name, name)
         @configuration_hash = configuration_hash.symbolize_keys.freeze
       end
 
       # Determines whether a database configuration is for a replica / readonly
-      # connection. If the +replica+ key is present in the config, +replica?+ will
-      # return +true+.
+      # connection. If the `replica` key is present in the config, `replica?` will
+      # return `true`.
       def replica?
         configuration_hash[:replica]
       end
 
-      # The migrations paths for a database configuration. If the
-      # +migrations_paths+ key is present in the config, +migrations_paths+
-      # will return its value.
+      # The migrations paths for a database configuration. If the `migrations_paths`
+      # key is present in the config, `migrations_paths` will return its value.
       def migrations_paths
         configuration_hash[:migrations_paths]
       end
@@ -92,8 +93,8 @@ def checkout_timeout
         (configuration_hash[:checkout_timeout] || 5).to_f
       end
 
-      # +reaping_frequency+ is configurable mostly for historical reasons, but it could
-      # also be useful if someone wants a very low +idle_timeout+.
+      # `reaping_frequency` is configurable mostly for historical reasons, but it
+      # could also be useful if someone wants a very low `idle_timeout`.
       def reaping_frequency
         configuration_hash.fetch(:reaping_frequency, 60)&.to_f
       end
@@ -107,15 +108,18 @@ def adapter
         configuration_hash[:adapter]&.to_s
       end
 
-      # The path to the schema cache dump file for a database.
-      # If omitted, the filename will be read from ENV or a
-      # default will be derived.
+      # The path to the schema cache dump file for a database. If omitted, the
+      # filename will be read from ENV or a default will be derived.
       def schema_cache_path
         configuration_hash[:schema_cache_path]
       end
 
-      def default_schema_cache_path
-        "db/schema_cache.yml"
+      def default_schema_cache_path(db_dir = "db")
+        if primary?
+          File.join(db_dir, "schema_cache.yml")
+        else
+          File.join(db_dir, "#{name}_schema_cache.yml")
+        end
       end
 
       def lazy_schema_cache_path
@@ -126,14 +130,14 @@ def primary? # :nodoc:
         Base.configurations.primary?(name)
       end
 
-      # Determines whether to dump the schema/structure files and the
-      # filename that should be used.
+      # Determines whether to dump the schema/structure files and the filename that
+      # should be used.
       #
-      # If +configuration_hash[:schema_dump]+ is set to +false+ or +nil+
-      # the schema will not be dumped.
+      # If `configuration_hash[:schema_dump]` is set to `false` or `nil` the schema
+      # will not be dumped.
       #
-      # If the config option is set that will be used. Otherwise \Rails
-      # will generate the filename from the database config name.
+      # If the config option is set that will be used. Otherwise Rails will generate
+      # the filename from the database config name.
       def schema_dump(format = ActiveRecord.schema_format)
         if configuration_hash.key?(:schema_dump)
           if config = configuration_hash[:schema_dump]

activerecord/lib/active_record/railtie.rb

@@ -148,10 +148,7 @@ class Railtie < Rails::Railtie # :nodoc:
           ActiveSupport.on_load(:active_record) do
             db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env).first
 
-            filename = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(
-              db_config.name,
-              schema_cache_path: db_config.schema_cache_path
-            )
+            filename = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(db_config)
 
             cache = ActiveRecord::ConnectionAdapters::SchemaCache._load_from(filename)
             next if cache.nil?

activerecord/lib/active_record/railties/databases.rake

@@ -509,7 +509,7 @@ db_namespace = namespace :db do
       task dump: :load_config do
         ActiveRecord::Tasks::DatabaseTasks.with_temporary_connection_for_each do |conn|
           db_config = conn.pool.db_config
-          filename = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(db_config.name, schema_cache_path: db_config.schema_cache_path)
+          filename = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(db_config)
 
           ActiveRecord::Tasks::DatabaseTasks.dump_schema_cache(conn, filename)
         end
@@ -518,10 +518,7 @@ db_namespace = namespace :db do
       desc "Clear a db/schema_cache.yml file."
       task clear: :load_config do
         ActiveRecord::Base.configurations.configs_for(env_name: ActiveRecord::Tasks::DatabaseTasks.env).each do |db_config|
-          filename = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(
-            db_config.name,
-            schema_cache_path: db_config.schema_cache_path,
-          )
+          filename = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(db_config)
           ActiveRecord::Tasks::DatabaseTasks.clear_schema_cache(
             filename,
           )

activerecord/lib/active_record/tasks/database_tasks.rb

@@ -437,14 +437,26 @@ def schema_dump_path(db_config, format = ActiveRecord.schema_format)
         end
       end
 
-      def cache_dump_filename(db_config_name, schema_cache_path: nil)
-        filename = if ActiveRecord::Base.configurations.primary?(db_config_name)
-          "schema_cache.yml"
+      def cache_dump_filename(db_config_or_name, schema_cache_path: nil)
+        if db_config_or_name.is_a?(DatabaseConfigurations::DatabaseConfig)
+          schema_cache_path ||
+            db_config_or_name.schema_cache_path ||
+            ENV["SCHEMA_CACHE"] ||
+            db_config_or_name.default_schema_cache_path(ActiveRecord::Tasks::DatabaseTasks.db_dir)
         else
-          "#{db_config_name}_schema_cache.yml"
-        end
+          ActiveRecord.deprecator.deprecation_warning(<<~MSG.squish)
+            Passing a database name to `cache_dump_filename` is deprecated and will be removed in Rails 7.3. Pass a
+            `ActiveRecord::DatabaseConfigurations::DatabaseConfig` object instead.
+          MSG
 
-        schema_cache_path || ENV["SCHEMA_CACHE"] || File.join(ActiveRecord::Tasks::DatabaseTasks.db_dir, filename)
+          filename = if ActiveRecord::Base.configurations.primary?(db_config_or_name)
+            "schema_cache.yml"
+          else
+            "#{db_config_or_name}_schema_cache.yml"
+          end
+
+          schema_cache_path || ENV["SCHEMA_CACHE"] || File.join(ActiveRecord::Tasks::DatabaseTasks.db_dir, filename)
+        end
       end
 
       def load_schema_current(format = ActiveRecord.schema_format, file = nil, environment = env)

activerecord/test/cases/database_configurations/hash_config_test.rb

@@ -141,11 +141,31 @@ def test_schema_cache_path_default_for_primary
         assert_equal "db/schema_cache.yml", config.default_schema_cache_path
       end
 
+      def test_schema_cache_path_default_for_custom_name
+        config = HashConfig.new("default_env", "alternate", adapter: "abstract")
+        assert_equal "db/alternate_schema_cache.yml", config.default_schema_cache_path
+      end
+
+      def test_schema_cache_path_default_for_different_db_dir
+        config = HashConfig.new("default_env", "alternate", adapter: "abstract")
+        assert_equal "my_db/alternate_schema_cache.yml", config.default_schema_cache_path("my_db")
+      end
+
       def test_schema_cache_path_configuration_hash
         config = HashConfig.new("default_env", "primary", { schema_cache_path: "db/config_schema_cache.yml", adapter: "abstract" })
         assert_equal "db/config_schema_cache.yml", config.schema_cache_path
       end
 
+      def test_lazy_schema_cache_path
+        config = HashConfig.new("default_env", "primary", { schema_cache_path: "db/config_schema_cache.yml", adapter: "abstract" })
+        assert_equal "db/config_schema_cache.yml", config.lazy_schema_cache_path
+      end
+
+      def test_lazy_schema_cache_path_uses_default_if_config_is_not_present
+        config = HashConfig.new("default_env", "alternate", { adapter: "abstract" })
+        assert_equal "db/alternate_schema_cache.yml", config.lazy_schema_cache_path
+      end
+
       def test_validate_checks_the_adapter_exists
         config = HashConfig.new("default_env", "primary", adapter: "abstract")
         assert config.validate!

activerecord/test/cases/tasks/database_tasks_test.rb

@@ -315,8 +315,38 @@ def test_cache_dump_default_filename
       old_path = ENV["SCHEMA_CACHE"]
       ENV.delete("SCHEMA_CACHE")
 
+      config = DatabaseConfigurations::HashConfig.new("development", "primary", {})
+
+      ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
+        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(config)
+        assert_equal "db/schema_cache.yml", path
+      end
+    ensure
+      ENV["SCHEMA_CACHE"] = old_path
+    end
+
+    def test_cache_dump_default_filename_with_custom_db_dir
+      old_path = ENV["SCHEMA_CACHE"]
+      ENV.delete("SCHEMA_CACHE")
+
+      config = DatabaseConfigurations::HashConfig.new("development", "primary", {})
+
+      ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "my_db") do
+        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(config)
+        assert_equal "my_db/schema_cache.yml", path
+      end
+    ensure
+      ENV["SCHEMA_CACHE"] = old_path
+    end
+
+    def test_deprecated_cache_dump_default_filename
+      old_path = ENV["SCHEMA_CACHE"]
+      ENV.delete("SCHEMA_CACHE")
+
       ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
-        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("primary")
+        path = assert_deprecated(ActiveRecord.deprecator) do
+          ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("primary")
+        end
         assert_equal "db/schema_cache.yml", path
       end
     ensure
@@ -327,8 +357,24 @@ def test_cache_dump_alternate_filename
       old_path = ENV["SCHEMA_CACHE"]
       ENV.delete("SCHEMA_CACHE")
 
+      config = DatabaseConfigurations::HashConfig.new("development", "alternate", {})
+
       ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
-        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("alternate")
+        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(config)
+        assert_equal "db/alternate_schema_cache.yml", path
+      end
+    ensure
+      ENV["SCHEMA_CACHE"] = old_path
+    end
+
+    def test_deprecated_cache_dump_alternate_filename
+      old_path = ENV["SCHEMA_CACHE"]
+      ENV.delete("SCHEMA_CACHE")
+
+      ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
+        path = assert_deprecated(ActiveRecord.deprecator) do
+          ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("alternate")
+        end
         assert_equal "db/alternate_schema_cache.yml", path
       end
     ensure
@@ -339,8 +385,24 @@ def test_cache_dump_filename_with_env_override
       old_path = ENV["SCHEMA_CACHE"]
       ENV["SCHEMA_CACHE"] = "tmp/something.yml"
 
+      config = DatabaseConfigurations::HashConfig.new("development", "primary", {})
+
+      ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
+        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(config)
+        assert_equal "tmp/something.yml", path
+      end
+    ensure
+      ENV["SCHEMA_CACHE"] = old_path
+    end
+
+    def test_deprecated_cache_dump_filename_with_env_override
+      old_path = ENV["SCHEMA_CACHE"]
+      ENV["SCHEMA_CACHE"] = "tmp/something.yml"
+
       ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
-        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("primary")
+        path = assert_deprecated(ActiveRecord.deprecator) do
+          ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("primary")
+        end
         assert_equal "tmp/something.yml", path
       end
     ensure
@@ -351,8 +413,39 @@ def test_cache_dump_filename_with_path_from_db_config
       old_path = ENV["SCHEMA_CACHE"]
       ENV.delete("SCHEMA_CACHE")
 
+      config = DatabaseConfigurations::HashConfig.new("development", "primary", { schema_cache_path:  "tmp/something.yml" })
+
+      ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
+        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(config)
+        assert_equal "tmp/something.yml", path
+      end
+    ensure
+      ENV["SCHEMA_CACHE"] = old_path
+    end
+
+
+    def test_cache_dump_filename_with_path_from_the_argument_has_precedence
+      old_path = ENV["SCHEMA_CACHE"]
+      ENV.delete("SCHEMA_CACHE")
+
+      config = DatabaseConfigurations::HashConfig.new("development", "primary", { schema_cache_path:  "tmp/something.yml" })
+
       ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
-        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("primary", schema_cache_path: "tmp/something.yml")
+        path = ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename(config, schema_cache_path: "tmp/another.yml")
+        assert_equal "tmp/another.yml", path
+      end
+    ensure
+      ENV["SCHEMA_CACHE"] = old_path
+    end
+
+    def test_deprecated_cache_dump_filename_with_path_from_the_argument
+      old_path = ENV["SCHEMA_CACHE"]
+      ENV.delete("SCHEMA_CACHE")
+
+      ActiveRecord::Tasks::DatabaseTasks.stub(:db_dir, "db") do
+        path = assert_deprecated(ActiveRecord.deprecator) do
+          ActiveRecord::Tasks::DatabaseTasks.cache_dump_filename("primary", schema_cache_path: "tmp/something.yml")
+        end
         assert_equal "tmp/something.yml", path
       end
     ensure

railties/test/application/rake/multi_dbs_test.rb

@@ -810,9 +810,7 @@ class TwoMigration < ActiveRecord::Migration::Current
         rails "db:drop" rescue nil
       end
 
-      # Note that schema cache loader depends on the connection and
-      # does not work for all connections.
-      test "schema_cache is loaded on primary db in multi-db app" do
+      test "schema_cache is loaded on all connection db in multi-db app if it exists for the connection" do
         require "#{app_path}/config/environment"
         db_migrate_and_schema_cache_dump
 
@@ -822,11 +820,8 @@ class TwoMigration < ActiveRecord::Migration::Current
         cache_tables_a = rails("runner", "p ActiveRecord::Base.connection.schema_cache.columns('books')").strip
         assert_includes cache_tables_a, "title", "expected cache_tables_a to include a title entry"
 
-        expired_warning = capture(:stderr) do
-          cache_size_b = rails("runner", "p AnimalsBase.connection.schema_cache.size", stderr: true).strip
-          assert_equal "0", cache_size_b
-        end
-        assert_match(/Ignoring .*\.yml because it has expired/, expired_warning)
+        cache_size_b = rails("runner", "p AnimalsBase.connection.schema_cache.size", stderr: true).strip
+        assert_equal "12", cache_size_b, "expected the cache size for animals to be valid since it was dumped"
 
         cache_tables_b = rails("runner", "p AnimalsBase.connection.schema_cache.columns('dogs')").strip
         assert_includes cache_tables_b, "name", "expected cache_tables_b to include a name entry"