Revert "Prevent double save of cyclic associations"

Commit a1a5d37 doesn't properly set the
foreign key on associations. Instead of trying to patch the change
revert it for now, so we can investigate a better solution.

This reverts commit a1a5d37

It also reverts the follow-up commits:

* Revert "Rename internal `@saving` state to `@_saving`"
  This reverts commit 2eb5458

* Revert "Add `_` prefix for the internal methods"
  This reverts commit 12c0bec.

* Revert "protected :can_save?"
  This reverts commit 8cd3b65.

* Revert "Exclude #saving? from API docs"
  This reverts commit 35d3923.

Author: p8

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