Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport "Delegate to scope rather than merge! for collection proxy" #28246

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/associations/association.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def target=(target)
end

def scope
target_scope.merge(association_scope)
target_scope.merge!(association_scope)
end

# The scope for this association.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,7 @@ def reader(force_reload = false)
reload
end

if null_scope?
# Cache the proxy separately before the owner has an id
# or else a post-save proxy will still lack the id
@null_proxy ||= CollectionProxy.create(klass, self)
else
@proxy ||= CollectionProxy.create(klass, self)
end
CollectionProxy.create(klass, self)
end

# Implements the writer method, e.g. foo.items= for Foo.has_many :items
Expand Down Expand Up @@ -455,9 +449,9 @@ def replace_on_target(record, index, skip_callbacks)
record
end

def scope(opts = {})
scope = super()
scope.none! if opts.fetch(:nullify, true) && null_scope?
def scope
scope = super
scope.none! if null_scope?
scope
end

Expand Down
37 changes: 24 additions & 13 deletions activerecord/lib/active_record/associations/collection_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,9 @@ module Associations
# is computed directly through SQL and does not trigger by itself the
# instantiation of the actual post records.
class CollectionProxy < Relation
delegate :exists?, :update_all, :arel, to: :scope

def initialize(klass, association) #:nodoc:
@association = association
super klass, klass.arel_table, klass.predicate_builder
merge! association.scope(nullify: false)
end

def target
Expand Down Expand Up @@ -902,19 +899,10 @@ def proxy_association
@association
end

# We don't want this object to be put on the scoping stack, because
# that could create an infinite loop where we call an @association
# method, which gets the current scope, which is this object, which
# delegates to @association, and so on.
def scoping
@association.scope.scoping { yield }
end

# Returns a <tt>Relation</tt> object for the records in this association
def scope
@association.scope
@scope ||= @association.scope
end
alias spawn scope

# Equivalent to <tt>Array#==</tt>. Returns +true+ if the two arrays
# contain the same number of elements and if each element is equal
Expand Down Expand Up @@ -1046,6 +1034,7 @@ def clear
# person.pets(true) # fetches pets from the database
# # => [#<Pet id: 1, name: "Snoop", group: "dogs", person_id: 1>]
def reload
@scope = nil
proxy_association.reload
self
end
Expand All @@ -1067,11 +1056,21 @@ def reload
# person.pets # fetches pets from the database
# # => [#<Pet id: 1, name: "Snoop", group: "dogs", person_id: 1>]
def reset
@scope = nil
proxy_association.reset
proxy_association.reset_scope
self
end

delegate_methods = [
QueryMethods,
SpawnMethods,
].flat_map { |klass|
klass.public_instance_methods(false)
} - self.public_instance_methods(false) + [:scoping]

delegate(*delegate_methods, to: :scope)

private

def null_scope?
Expand All @@ -1081,6 +1080,18 @@ def null_scope?
def exec_queries
load_target
end

def respond_to_missing?(method, _)
scope.respond_to?(method) || super
end

def method_missing(method, *args, &block)
if scope.respond_to?(method)
scope.public_send(method, *args, &block)
else
super
end
end
end
end
end
11 changes: 11 additions & 0 deletions activerecord/test/cases/associations/has_many_associations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,17 @@ def test_update_all_on_association_accessed_before_save_with_explicit_foreign_ke
firm.save!
assert_equal firm.clients.count, firm.clients.update_all(description: 'Great!')
assert_equal clients_proxy_id, firm.clients.object_id
firm = Firm.new(name: "Firm")
firm.clients << Client.first
firm.save!
assert_equal firm.clients.count, firm.clients.update_all(description: "Great!")
end

def test_update_all_on_association_accessed_before_save_with_explicit_foreign_key
firm = Firm.new(name: "Firm", id: 100)
firm.clients << Client.first
firm.save!
assert_equal firm.clients.count, firm.clients.update_all(description: "Great!")
end

def test_belongs_to_sanity
Expand Down
5 changes: 0 additions & 5 deletions activerecord/test/cases/associations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,6 @@ def test_scoped_allows_conditions
assert_equal david.projects, david.projects.scope
end

test "proxy object is cached" do
david = developers(:david)
assert david.projects.equal?(david.projects)
end

test "inverses get set of subsets of the association" do
man = Man.create
man.interests.create
Expand Down