Revert "Only update dirty attributes once for cyclic autosave callbacks"

This reverts commit ae56ecd467.

activerecord/CHANGELOG.md

@@ -70,14 +70,6 @@
 
     *Matt Duszynski*
 
-*   Only update dirty attributes once for cyclic autosave callbacks.
-
-    Instead of calling `changes_applied` everytime `save` is called,
-    we can skip it if it has already been called once in the current saving
-    cycle.
-
-    *Petrik de Heus*
-
 *   Allow passing SQL as `on_duplicate` value to `#upsert_all` to make it possible to use raw SQL to update columns on conflict:
 
     ```ruby

activerecord/lib/active_record/attribute_methods/dirty.rb

@@ -187,7 +187,7 @@ module ActiveRecord
 
         def _update_record(attribute_names = attribute_names_for_partial_writes)
           affected_rows = super
-          changes_applied if _apply_changes?(attribute_names)
+          changes_applied
           affected_rows
         end
 
@@ -200,10 +200,6 @@ module ActiveRecord
         def attribute_names_for_partial_writes
           partial_writes? ? changed_attribute_names_to_save : attribute_names
         end
-
-        def _apply_changes?(attribute_names)
-          true
-        end
     end
   end
 end

activerecord/lib/active_record/autosave_association.rb

@@ -159,7 +159,7 @@ module ActiveRecord
           return if method_defined?(name, false)
 
           define_method(name) do |*args|
-            result = true
+            result = true; @_already_called ||= {}
             # Loop prevention for validation of associations
             unless @_already_called[name]
               begin
@@ -235,18 +235,9 @@ module ActiveRecord
     def reload(options = nil)
       @marked_for_destruction = false
       @destroyed_by_association = nil
-      @_saving = false
       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,23 +273,7 @@ module ActiveRecord
       new_record? || has_changes_to_save? || marked_for_destruction? || nested_records_changed_for_autosave?
     end
 
-    def changes_applied # :nodoc:
-      @_already_called[:changes_applied] = true
-      super
-    end
-
     private
-      # Track if this record is currently being saved.
-      # Autosave can call save multiple times on the same record. Some methods
-      # like +changes_applied+ should be called only once while saving.
-      def _saving
-        previously_saving, @_saving = @_saving, true
-        yield
-      ensure
-        @_saving = previously_saving
-        @_already_called[:changes_applied] = false unless @_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.
@@ -534,10 +509,5 @@ module ActiveRecord
       def _ensure_no_duplicate_errors
         errors.uniq!
       end
-
-      # Call +changes_applied+ at least once or if attributes changed
-      def _apply_changes?(attribute_names)
-        !@_already_called[:changes_applied] || attribute_names.present?
-      end
   end
 end

activerecord/lib/active_record/core.rb

@@ -860,11 +860,9 @@ module ActiveRecord
         @readonly                 = false
         @previously_new_record    = false
         @destroyed                = false
-        @_saving                  = false
         @marked_for_destruction   = false
         @destroyed_by_association = nil
         @_start_transaction_state = nil
-        @_already_called          = {}
 
         klass = self.class
 

activerecord/lib/active_record/transactions.rb

@@ -404,7 +404,6 @@ module ActiveRecord
               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/autosave_association_test.rb

@@ -2008,71 +2008,3 @@ class TestAutosaveAssociationOnAHasManyAssociationDefinedInSubclassWithAcceptsNe
     assert_equal "Updated", valid_project.name
   end
 end
-
-class TestAutosaveAssociationTracksSavingState < ActiveRecord::TestCase
-  test "@_saving is set to true during 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] * 10, autosave_saving_stack
-    assert_not child.instance_variable_get(:@_saving)
-  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
-end
-
-class TestCyclicAutosaveAssociationsApplyDirtyChangesOnce < ActiveRecord::TestCase
-  test "dirty changes are applied once on child 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, :id_previously_changed?
-    assert_predicate parent, :id_previously_changed?
-  end
-
-  test "dirty changes are applied once on child 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, :id_previously_changed?
-    assert_predicate parent, :id_previously_changed?
-  end
-
-  test "dirty changes on parent are applied only once" do
-    child = Ship.new(name: "Nights Dirty Lightning")
-    parent = child.build_pirate(catchphrase: "Aye")
-    parent.save!
-    assert_predicate child, :id_previously_changed?
-    assert_predicate parent, :id_previously_changed?
-  end
-
-  test "dirty changes are applied once on child if child is an inverse has_many of parent with touch" do
-    child = LineItem.new
-    parent = child.build_invoice
-    child.save!
-    assert_predicate child, :id_previously_changed?
-    assert_predicate parent, :id_previously_changed?
-  end
-end