Merge pull request #25877 from kamipo/delegate_to_scope_rather_than_merge

Delegate to `scope` rather than `merge!` for collection proxy

5 files changed, 29 insertions, 34 deletions

diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb
index 84d0493a60..1cb2b2d7c6 100644
--- a/activerecord/lib/active_record/associations/association.rb
+++ b/activerecord/lib/active_record/associations/association.rb
@@ -83,7 +83,7 @@ module ActiveRecord
       end
 
       def scope
-        target_scope.merge(association_scope)
+        target_scope.merge!(association_scope)
       end
 
       # The scope for this association.
diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 0437a79b84..77282e6463 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -30,13 +30,7 @@ module ActiveRecord
           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
@@ -315,9 +309,9 @@ module ActiveRecord
         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
 
diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb
index 0d84805b4d..55bf2e0ff0 100644
--- a/activerecord/lib/active_record/associations/collection_proxy.rb
+++ b/activerecord/lib/active_record/associations/collection_proxy.rb
@@ -28,12 +28,9 @@ module ActiveRecord
     # 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
@@ -956,19 +953,10 @@ module ActiveRecord
         @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
@@ -1100,6 +1088,7 @@ module ActiveRecord
       #   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
@@ -1121,11 +1110,21 @@ module ActiveRecord
       #   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 find_nth_with_limit(index, limit)
@@ -1149,6 +1148,18 @@ module ActiveRecord
         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
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index cbecfa84ff..ede3a44090 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -611,21 +611,16 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
 
   def test_update_all_on_association_accessed_before_save
     firm = Firm.new(name: "Firm")
-    clients_proxy_id = firm.clients.object_id
     firm.clients << Client.first
     firm.save!
     assert_equal firm.clients.count, firm.clients.update_all(description: "Great!")
-    assert_not_equal clients_proxy_id, firm.clients.object_id
   end
 
   def test_update_all_on_association_accessed_before_save_with_explicit_foreign_key
-    # We can use the same cached proxy object because the id is available for the scope
     firm = Firm.new(name: "Firm", id: 100)
-    clients_proxy_id = firm.clients.object_id
     firm.clients << Client.first
     firm.save!
     assert_equal firm.clients.count, firm.clients.update_all(description: "Great!")
-    assert_equal clients_proxy_id, firm.clients.object_id
   end
 
   def test_belongs_to_sanity
diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb
index a223b4338f..26056f6f63 100644
--- a/activerecord/test/cases/associations_test.rb
+++ b/activerecord/test/cases/associations_test.rb
@@ -220,11 +220,6 @@ class AssociationProxyTest < ActiveRecord::TestCase
     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