Skip to content

Commit

Permalink
ActiveRecord::Delegation prevent overriding existing methods
Browse files Browse the repository at this point in the history
I discovered this while working on #47800

The bug is quite subtle.

If you call `Model.all.delete(id)`, `all` is an `ActiveRecord::Relation`
which does not respond to `delete`, so it delegates to `Model.delete`
and generate that method in the `GeneratedRelationMethods` module.

The problem however, is that `CollectionProxy` does define `delete`,
so after that initial call, the `Model::ActiveRecord_CollectionProxy`
subclass now has its `delete` method overridden, and now delegate
to the model.

Here I chose to keep that method generation caching, but I'm honestly
not convinced it's really needed. I question how much of a hotspot
these methods are, and we're busting method caches and generating
a lot of code to save on a minor `method_missing` call.

I think we should just remove that caching.
  • Loading branch information
byroot committed Mar 30, 2023
1 parent 59f2b06 commit e4009c8
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 7 deletions.
28 changes: 21 additions & 7 deletions activerecord/lib/active_record/relation/delegation.rb
Expand Up @@ -5,19 +5,31 @@

module ActiveRecord
module Delegation # :nodoc:
class << self
def delegated_classes
[
ActiveRecord::Relation,
ActiveRecord::Associations::CollectionProxy,
ActiveRecord::AssociationRelation,
ActiveRecord::DisableJoinsAssociationRelation,
]
end

def uncacheable_methods
@uncacheable_methods ||= (
delegated_classes.flat_map(&:public_instance_methods) - ActiveRecord::Relation.public_instance_methods
).to_set.freeze
end
end

module DelegateCache # :nodoc:
def relation_delegate_class(klass)
@relation_delegate_cache[klass]
end

def initialize_relation_delegate_cache
@relation_delegate_cache = cache = {}
[
ActiveRecord::Relation,
ActiveRecord::Associations::CollectionProxy,
ActiveRecord::AssociationRelation,
ActiveRecord::DisableJoinsAssociationRelation
].each do |klass|
Delegation.delegated_classes.each do |klass|
delegate = Class.new(klass) {
include ClassSpecificRelation
}
Expand Down Expand Up @@ -104,7 +116,9 @@ def name
private
def method_missing(method, *args, &block)
if @klass.respond_to?(method)
@klass.generate_relation_method(method)
unless Delegation.uncacheable_methods.include?(method)
@klass.generate_relation_method(method)
end
scoping { @klass.public_send(method, *args, &block) }
else
super
Expand Down
18 changes: 18 additions & 0 deletions activerecord/test/cases/relation/delegation_test.rb
Expand Up @@ -3,6 +3,8 @@
require "cases/helper"
require "models/post"
require "models/comment"
require "models/project"
require "models/developer"

module ActiveRecord
module DelegationTests
Expand Down Expand Up @@ -72,4 +74,20 @@ def test_delegate_querying_methods
end
end
end

class DelegationCachingTest < ActiveRecord::TestCase
fixtures :projects, :developers

test "delegation doesn't override methods defined in other relation subclasses" do
# precondition, some methods are available on ActiveRecord::Relation subclasses
# but not ActiveRecord::Relation itself. Here `delete` is just an example.
assert_equal false, ActiveRecord::Relation.method_defined?(:delete)
assert_equal true, ActiveRecord::Associations::CollectionProxy.method_defined?(:delete)

project = projects(:active_record)
original_owner = project.developers_with_callbacks.method(:delete).owner
Developer.all.delete(12345)
assert_equal original_owner, project.developers_with_callbacks.method(:delete).owner
end
end
end
1 change: 1 addition & 0 deletions activerecord/test/models/developer.rb
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require "ostruct"
require "models/computer"

class Developer < ActiveRecord::Base
module TimestampAliases
Expand Down

0 comments on commit e4009c8

Please sign in to comment.