Skip to content

Commit

Permalink
Reset current scopes in AR using an on_unload callback
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
fxn committed Mar 20, 2022
1 parent d28cb5d commit 7d87086
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 50 deletions.
10 changes: 10 additions & 0 deletions activerecord/lib/active_record/railtie.rb
Expand Up @@ -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

This comment has been minimized.

Copy link
@casperisfine

casperisfine Mar 21, 2022

Contributor

ActiveRecord::Base > value would avoid failure if some autoloaded class defines <

This comment has been minimized.

Copy link
@fxn

fxn Mar 21, 2022

Author Member

Good point, we'll reduce the assumptions to a sane is_a? :).

This comment has been minimized.

Copy link
@casperisfine

casperisfine Mar 21, 2022

Contributor

For that you can use Class === value.

This comment has been minimized.

Copy link
@fxn

fxn Mar 21, 2022

Author Member

Refactored in b764fc2 and backported. Thanks @casperisfine!

end
end
end
end
end
end
4 changes: 0 additions & 4 deletions activerecord/lib/active_record/scoping/default.rb
Expand Up @@ -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.
Expand Down
14 changes: 0 additions & 14 deletions activerecord/test/cases/base_test.rb
Expand Up @@ -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)
Expand Down
4 changes: 0 additions & 4 deletions railties/lib/rails/application/finisher.rb
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions railties/test/application/initializers/frameworks_test.rb
Expand Up @@ -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
28 changes: 0 additions & 28 deletions railties/test/application/zeitwerk_integration_test.rb
Expand Up @@ -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
Expand Down

0 comments on commit 7d87086

Please sign in to comment.