Merge pull request rails#41759 from p8/rollback-a1a5d37749

Revert "Prevent double save of cyclic associations"

Author: rafaelfranca

activerecord/CHANGELOG.md

@@ -17,13 +17,6 @@
 
     *Dinah Shi*
 
-*   Prevent double saves in autosave of cyclic associations.
-
-    Adds an internal saving state which tracks if a record is currently being saved.
-    If a state is set to true, the record won't be saved by the autosave callbacks.
-
-    *Petrik de Heus*
-
 *   Fix Float::INFINITY assignment to datetime column with postgresql adapter
 
     Before:

activerecord/lib/active_record/autosave_association.rb

@@ -233,20 +233,11 @@ def define_autosave_validation_callbacks(reflection)
 
     # Reloads the attributes of the object as usual and clears <tt>marked_for_destruction</tt> flag.
     def reload(options = nil)
-      @_saving = false
       @marked_for_destruction = false
       @destroyed_by_association = nil
       super
     end
 
-    def save(**options) # :nodoc
-      _saving { super }
-    end
-
-    def save!(**options) # :nodoc:
-      _saving { super }
-    end
-
     # Marks this record to be destroyed as part of the parent's save transaction.
     # This does _not_ actually destroy the record instantly, rather child record will be destroyed
     # when <tt>parent.save</tt> is called.
@@ -282,21 +273,7 @@ def changed_for_autosave?
       new_record? || has_changes_to_save? || marked_for_destruction? || nested_records_changed_for_autosave?
     end
 
-    protected
-      def _can_save? # :nodoc:
-        !destroyed? && !@_saving
-      end
-
     private
-      # Track if this record is being saved. If it is being saved we
-      # can skip saving it in the autosave callbacks.
-      def _saving
-        previously_saving, @_saving = @_saving, true
-        yield
-      ensure
-        @_saving = previously_saving
-      end
-
       # Returns the record for an association collection that should be validated
       # or saved. If +autosave+ is +false+ only new records will be returned,
       # unless the parent is/was a new record itself.
@@ -423,7 +400,7 @@ def save_collection_association(reflection)
             end
 
             records.each do |record|
-              next unless record._can_save?
+              next if record.destroyed?
 
               saved = true
 
@@ -460,7 +437,7 @@ def save_has_one_association(reflection)
         association = association_instance_get(reflection.name)
         record      = association && association.load_target
 
-        if record&._can_save?
+        if record && !record.destroyed?
           autosave = reflection.options[:autosave]
 
           if autosave && record.marked_for_destruction?
@@ -505,7 +482,7 @@ def save_belongs_to_association(reflection)
         return unless association && association.loaded? && !association.stale_target?
 
         record = association.load_target
-        if record&._can_save?
+        if record && !record.destroyed?
           autosave = reflection.options[:autosave]
 
           if autosave && record.marked_for_destruction?

activerecord/lib/active_record/core.rb

@@ -841,7 +841,6 @@ def init_internals
         @readonly                 = false
         @previously_new_record    = false
         @destroyed                = false
-        @_saving                  = false
         @marked_for_destruction   = false
         @destroyed_by_association = nil
         @_start_transaction_state = nil

activerecord/lib/active_record/transactions.rb

@@ -404,7 +404,6 @@ def restore_transaction_record_state(force_restore_state = false)
               attr = attr.with_value_from_user(value) if attr.value != value
               attr
             end
-            @_saving = false
             @mutations_from_database = nil
             @mutations_before_last_save = nil
             if @attributes.fetch_value(@primary_key) != restore_state[:id]

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -1556,7 +1556,7 @@ def test_circular_autosave_association_correctly_saves_multiple_records
     fall.sections << sections
     fall.save!
     fall.reload
-    assert_equal sections.sort_by(&:id), fall.sections.sort_by(&:id)
+    assert_equal sections, fall.sections.sort_by(&:id)
   end
 
   private

activerecord/test/cases/autosave_association_test.rb

@@ -1951,72 +1951,3 @@ def test_should_update_children_when_association_redefined_in_subclass
     assert_equal "Updated", valid_project.name
   end
 end
-
-class TestCyclicAutosaveAssociationsOnlySaveOnce < ActiveRecord::TestCase
-  test "child is saved only once if child is the inverse has_one of parent" do
-    ship_reflection = Ship.reflect_on_association(:pirate)
-    pirate_reflection = Pirate.reflect_on_association(:ship)
-    assert_equal ship_reflection, pirate_reflection.inverse_of, "The pirate reflection's inverse should be the ship reflection"
-
-    child = Ship.new(name: "Nights Dirty Lightning")
-    parent = child.build_pirate(catchphrase: "Aye")
-    child.save!
-    assert_predicate child, :previously_new_record?
-    assert_predicate parent, :previously_new_record?
-  end
-
-  test "child is saved only once if child is an inverse has_many of parent" do
-    child = FamousShip.new(name: "Poison Orchid")
-    parent = child.build_famous_pirate(catchphrase: "Aye")
-    child.save!
-    assert_predicate child, :previously_new_record?
-    assert_predicate parent, :previously_new_record?
-  end
-
-  test "similar children are saved in the autosave" do
-    child1 = FamousShip.new(name: "Poison Orchid")
-    parent = child1.build_famous_pirate(catchphrase: "Aye")
-    child2 = parent.famous_ships.build(name: "Red Messenger")
-    child1.save!
-    assert_predicate child2, :persisted?
-    assert_predicate child1, :previously_new_record?
-    assert_predicate parent, :previously_new_record?
-    assert_equal [child2, child1], parent.reload.famous_ships
-  end
-
-  test "parent is saved only once" do
-    child = Ship.new(name: "Nights Dirty Lightning")
-    parent = child.build_pirate(catchphrase: "Aye")
-    parent.save!
-    assert_predicate child, :previously_new_record?
-    assert_predicate parent, :previously_new_record?
-  end
-
-  test "saving? is reset to false if validations fail" do
-    child = Ship.new(name: "Nights Dirty Lightning")
-    child.build_pirate
-    assert_not child.save
-    assert_not child.instance_variable_get(:@_saving)
-  end
-
-  test "saving? is set to false after multiple nested saves" do
-    autosave_saving_stack = []
-
-    ship_with_saving_stack = Class.new(Ship) do
-      before_save { autosave_saving_stack << @_saving }
-      after_save  { autosave_saving_stack << @_saving }
-    end
-
-    pirate_with_callbacks = Class.new(Pirate) do
-      after_save   { ship.save }
-      after_create { ship.save }
-      after_commit { ship.save }
-    end
-
-    child = ship_with_saving_stack.new(name: "Nights Dirty Lightning")
-    child.pirate = pirate_with_callbacks.new(catchphrase: "Aye")
-    child.save!
-    assert_equal [true] * 8, autosave_saving_stack
-    assert_not child.instance_variable_get(:@_saving)
-  end
-end