Merge pull request #23794 from matthewd/mutation-safe-to_a

Mutating the result of Relation#to_a should not affect the relation

12 files changed, 50 insertions, 24 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 641e9195c6..c18403865f 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Ensure that mutations of the array returned from `ActiveRecord::Relation#to_a`
+    do not affect the original relation, by returning a duplicate array each time.
+
+    This brings the behavior in line with `CollectionProxy#to_a`, which was
+    already more careful.
+
+    *Matthew Draper*
+
 *   Fixed `where` for polymorphic associations when passed an array containing different types.
 
     Fixes #17011.
diff --git a/activerecord/lib/active_record/association_relation.rb b/activerecord/lib/active_record/association_relation.rb
index ee0bb8fafe..c18e88e4cf 100644
--- a/activerecord/lib/active_record/association_relation.rb
+++ b/activerecord/lib/active_record/association_relation.rb
@@ -10,7 +10,7 @@ module ActiveRecord
     end
 
     def ==(other)
-      other == to_a
+      other == records
     end
 
     def build(*args, &block)
diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb
index 2a9627a474..b9aed05135 100644
--- a/activerecord/lib/active_record/associations/collection_proxy.rb
+++ b/activerecord/lib/active_record/associations/collection_proxy.rb
@@ -979,6 +979,10 @@ module ActiveRecord
       end
       alias_method :to_a, :to_ary
 
+      def records # :nodoc:
+        load_target
+      end
+
       # Adds one or more +records+ to the collection by setting their foreign keys
       # to the association's primary key. Returns +self+, so several appends may be
       # chained together.
diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb
index c7cc48ba16..f913f0852a 100644
--- a/activerecord/lib/active_record/associations/singular_association.rb
+++ b/activerecord/lib/active_record/associations/singular_association.rb
@@ -45,7 +45,7 @@ module ActiveRecord
         end
 
         def get_records
-          return scope.limit(1).to_a if skip_statement_cache?
+          return scope.limit(1).records if skip_statement_cache?
 
           conn = klass.connection
           sc = reflection.association_scope_cache(conn, owner) do
diff --git a/activerecord/lib/active_record/null_relation.rb b/activerecord/lib/active_record/null_relation.rb
index 0b500346bc..1ab4e0404f 100644
--- a/activerecord/lib/active_record/null_relation.rb
+++ b/activerecord/lib/active_record/null_relation.rb
@@ -1,7 +1,7 @@
 module ActiveRecord
   module NullRelation # :nodoc:
     def exec_queries
-      @records = []
+      @records = [].freeze
     end
 
     def pluck(*column_names)
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 7e842668c6..09afdc6c69 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -253,17 +253,21 @@ module ActiveRecord
 
     # Converts relation objects to Array.
     def to_a
+      records.dup
+    end
+
+    def records # :nodoc:
       load
       @records
     end
 
     # Serializes the relation objects Array.
     def encode_with(coder)
-      coder.represent_seq(nil, to_a)
+      coder.represent_seq(nil, records)
     end
 
     def as_json(options = nil) #:nodoc:
-      to_a.as_json(options)
+      records.as_json(options)
     end
 
     # Returns size of the records.
@@ -298,13 +302,13 @@ module ActiveRecord
     # Returns true if there is exactly one record.
     def one?
       return super if block_given?
-      limit_value ? to_a.one? : size == 1
+      limit_value ? records.one? : size == 1
     end
 
     # Returns true if there is more than one record.
     def many?
       return super if block_given?
-      limit_value ? to_a.many? : size > 1
+      limit_value ? records.many? : size > 1
     end
 
     # Returns a cache key that can be used to identify the records fetched by
@@ -418,7 +422,7 @@ module ActiveRecord
       if id.is_a?(Array)
         id.map.with_index { |one_id, idx| update(one_id, attributes[idx]) }
       elsif id == :all
-        to_a.each { |record| record.update(attributes) }
+        records.each { |record| record.update(attributes) }
       else
         if ActiveRecord::Base === id
           id = id.id
@@ -457,7 +461,7 @@ module ActiveRecord
         MESSAGE
         where(conditions).destroy_all
       else
-        to_a.each(&:destroy).tap { reset }
+        records.each(&:destroy).tap { reset }
       end
     end
 
@@ -587,7 +591,7 @@ module ActiveRecord
     def reset
       @last = @to_sql = @order_clause = @scope_for_create = @arel = @loaded = nil
       @should_eager_load = @join_dependency = nil
-      @records = []
+      @records = [].freeze
       @offsets = {}
       self
     end
@@ -654,21 +658,21 @@ module ActiveRecord
     def ==(other)
       case other
       when Associations::CollectionProxy, AssociationRelation
-        self == other.to_a
+        self == other.records
       when Relation
         other.to_sql == to_sql
       when Array
-        to_a == other
+        records == other
       end
     end
 
     def pretty_print(q)
-      q.pp(self.to_a)
+      q.pp(self.records)
     end
 
     # Returns true if relation is blank.
     def blank?
-      to_a.blank?
+      records.blank?
     end
 
     def values
@@ -676,7 +680,7 @@ module ActiveRecord
     end
 
     def inspect
-      entries = to_a.take([limit_value, 11].compact.min).map!(&:inspect)
+      entries = records.take([limit_value, 11].compact.min).map!(&:inspect)
       entries[10] = '...' if entries.size == 11
 
       "#<#{self.class.name} [#{entries.join(', ')}]>"
@@ -685,14 +689,14 @@ module ActiveRecord
     protected
 
       def load_records(records)
-        @records = records
+        @records = records.freeze
         @loaded = true
       end
 
     private
 
     def exec_queries
-      @records = eager_loading? ? find_with_associations : @klass.find_by_sql(arel, bound_attributes)
+      @records = eager_loading? ? find_with_associations.freeze : @klass.find_by_sql(arel, bound_attributes).freeze
 
       preload = preload_values
       preload +=  includes_values unless eager_loading?
diff --git a/activerecord/lib/active_record/relation/batches.rb b/activerecord/lib/active_record/relation/batches.rb
index de005e2810..243ef0eae9 100644
--- a/activerecord/lib/active_record/relation/batches.rb
+++ b/activerecord/lib/active_record/relation/batches.rb
@@ -187,7 +187,7 @@ module ActiveRecord
 
       loop do
         if load
-          records = batch_relation.to_a
+          records = batch_relation.records
           ids = records.map(&:id)
           yielded_relation = self.where(primary_key => ids)
           yielded_relation.load_records(records)
diff --git a/activerecord/lib/active_record/relation/batches/batch_enumerator.rb b/activerecord/lib/active_record/relation/batches/batch_enumerator.rb
index c6e39814dd..13393dc605 100644
--- a/activerecord/lib/active_record/relation/batches/batch_enumerator.rb
+++ b/activerecord/lib/active_record/relation/batches/batch_enumerator.rb
@@ -35,7 +35,7 @@ module ActiveRecord
         return to_enum(:each_record) unless block_given?
 
         @relation.to_enum(:in_batches, of: @of, start: @start, finish: @finish, load: true).each do |relation|
-          relation.to_a.each { |record| yield record }
+          relation.records.each { |record| yield record }
         end
       end
 
diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb
index 2b1ac42395..f2578f5f96 100644
--- a/activerecord/lib/active_record/relation/delegation.rb
+++ b/activerecord/lib/active_record/relation/delegation.rb
@@ -38,7 +38,7 @@ module ActiveRecord
 
     delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :join,
              :[], :&, :|, :+, :-, :sample, :reverse, :compact, :in_groups, :in_groups_of,
-             :shuffle, :split, to: :to_a
+             :shuffle, :split, to: :records
 
     delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key,
              :connection, :columns_hash, :to => :klass
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 16563515f6..0037398554 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -506,7 +506,7 @@ module ActiveRecord
     def find_some_ordered(ids)
       ids = ids.slice(offset_value || 0, limit_value || ids.size) || []
 
-      result = except(:limit, :offset).where(primary_key => ids).to_a
+      result = except(:limit, :offset).where(primary_key => ids).records
 
       if result.size == ids.size
         pk_type = @klass.type_for_attribute(primary_key)
@@ -522,7 +522,7 @@ module ActiveRecord
       if loaded?
         @records.first
       else
-        @take ||= limit(1).to_a.first
+        @take ||= limit(1).records.first
       end
     end
 
@@ -573,7 +573,7 @@ module ActiveRecord
     end
 
     def find_last(limit)
-      limit ? to_a.last(limit) : to_a.last
+      limit ? records.last(limit) : records.last
     end
   end
 end
diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb
index 67d7f83cb4..d5c18a2a4a 100644
--- a/activerecord/lib/active_record/relation/spawn_methods.rb
+++ b/activerecord/lib/active_record/relation/spawn_methods.rb
@@ -29,7 +29,7 @@ module ActiveRecord
     # This is mainly intended for sharing common conditions between multiple associations.
     def merge(other)
       if other.is_a?(Array)
-        to_a & other
+        records & other
       elsif other
         spawn.merge!(other)
       else
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 090b885dd5..4c15ab6644 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1273,6 +1273,16 @@ class RelationTest < ActiveRecord::TestCase
     assert posts.loaded?
   end
 
+  def test_to_a_should_dup_target
+    posts = Post.all
+
+    original_size = posts.size
+    removed = posts.to_a.pop
+
+    assert_equal original_size, posts.size
+    assert_includes posts.to_a, removed
+  end
+
   def test_build
     posts = Post.all