From 7d87086f10ed15aa0c574f575fa834e271c64b04 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Sun, 20 Mar 2022 21:28:20 +0100 Subject: [PATCH] Reset current scopes in AR using an on_unload callback Rails 6.0 and Rails 6.1 didn't support the undocumented `before_remove_const` in `zeitwerk` mode. I noticed this cleanup in AR was not being executed, and restored the original code for Rails 7. However, invoking `respond_to?` in an `on_unload` callback may have unexpected side-effects, as seen in #44125. So, this patch reimplements the cleanup in a more modern way. Fixes #44125. --- activerecord/lib/active_record/railtie.rb | 10 +++++++ .../lib/active_record/scoping/default.rb | 4 --- activerecord/test/cases/base_test.rb | 14 ---------- railties/lib/rails/application/finisher.rb | 4 --- .../initializers/frameworks_test.rb | 13 +++++++++ .../application/zeitwerk_integration_test.rb | 28 ------------------- 6 files changed, 23 insertions(+), 50 deletions(-) diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 1f58d0049415a..d88be4effcbb5 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -389,5 +389,15 @@ class Railtie < Rails::Railtie # :nodoc: end end end + + initializer "active_record.unregister_current_scopes_on_unload" do |app| + config.after_initialize do + unless app.config.cache_classes + Rails.autoloaders.main.on_unload do |_cpath, value, _abspath| + value.current_scope = nil if value.is_a?(Class) && value < ActiveRecord::Base + end + end + end + end end end diff --git a/activerecord/lib/active_record/scoping/default.rb b/activerecord/lib/active_record/scoping/default.rb index 52895d28e96c8..f6466947b54c4 100644 --- a/activerecord/lib/active_record/scoping/default.rb +++ b/activerecord/lib/active_record/scoping/default.rb @@ -48,10 +48,6 @@ def scope_attributes? # :nodoc: super || default_scopes.any? || respond_to?(:default_scope) end - def before_remove_const # :nodoc: - self.current_scope = nil - end - # Checks if the model has any default scopes. If all_queries # is set to true, the method will check if there are any # default_scopes for the model where +all_queries+ is true. diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 8e0629078eb19..c6320b93367b1 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1300,20 +1300,6 @@ def test_clear_cache! assert_equal c1, c2 end - def test_before_remove_const_resets_the_current_scope - # Done this way because a class cannot be defined in a method using the - # class keyword. - Object.const_set(:ReloadableModel, Class.new(ActiveRecord::Base)) - ReloadableModel.current_scope = ReloadableModel.all - assert_not_nil ActiveRecord::Scoping::ScopeRegistry.current_scope(ReloadableModel) # precondition - - ReloadableModel.before_remove_const - - assert_nil ActiveRecord::Scoping::ScopeRegistry.current_scope(ReloadableModel) - ensure - Object.send(:remove_const, :ReloadableModel) - end - def test_marshal_round_trip expected = posts(:welcome) marshalled = Marshal.dump(expected) diff --git a/railties/lib/rails/application/finisher.rb b/railties/lib/rails/application/finisher.rb index 96c85dcd3ecc2..00a8d2882ea02 100644 --- a/railties/lib/rails/application/finisher.rb +++ b/railties/lib/rails/application/finisher.rb @@ -35,10 +35,6 @@ module Finisher ActiveSupport::Dependencies._autoloaded_tracked_classes << value end end - - autoloader.on_unload do |_cpath, value, _abspath| - value.before_remove_const if value.respond_to?(:before_remove_const) - end end autoloader.setup diff --git a/railties/test/application/initializers/frameworks_test.rb b/railties/test/application/initializers/frameworks_test.rb index 4e6b66c15e597..8a29c6c52a31b 100644 --- a/railties/test/application/initializers/frameworks_test.rb +++ b/railties/test/application/initializers/frameworks_test.rb @@ -338,5 +338,18 @@ def show require "#{app_path}/config/environment" assert_not_predicate ActiveRecord::Base.connection_pool, :active_connection? end + + test "Current scopes in AR models are reset on reloading" do + rails %w(generate model post) + rails %w(db:migrate) + require "#{app_path}/config/environment" + + Post.current_scope = Post + assert_not_nil ActiveRecord::Scoping::ScopeRegistry.current_scope(Post) # precondition + + ActiveSupport::Dependencies.clear + + assert_nil ActiveRecord::Scoping::ScopeRegistry.current_scope(Post) + end end end diff --git a/railties/test/application/zeitwerk_integration_test.rb b/railties/test/application/zeitwerk_integration_test.rb index 09f83f45e3589..4e949e9aed575 100644 --- a/railties/test/application/zeitwerk_integration_test.rb +++ b/railties/test/application/zeitwerk_integration_test.rb @@ -262,34 +262,6 @@ def once_autoloader.reload assert_equal %i(main_autoloader), $zeitwerk_integration_reload_test end - test "reloading invokes before_remove_const" do - $before_remove_const_invoked = false - - app_file "app/models/foo.rb", <<~RUBY - # While the most common use case is classes/modules, the contract does not - # require values to be so. Let's weaken the test down to Object.new. - Foo = Object.new - def Foo.before_remove_const - $before_remove_const_invoked = true - end - RUBY - - app_file "app/models/bar.rb", <<~RUBY - # This object does not implement before_remove_const. We define it to make - # sure reloading does not raise. That is, it does not blindly invoke the - # hook on all unloaded objects. - Bar = Object.new - RUBY - - boot - - assert Foo - assert Bar - ActiveSupport::Dependencies.clear - - assert $before_remove_const_invoked - end - test "reloading clears autoloaded tracked classes" do eval <<~RUBY class Parent