Only update dirty attributes once for cyclic autosave callbacks

Calling save on a record with cyclic autosave callbacks, can call other
callbacks and hooks multiple times. This can lead to unexpected
behaviour.

For example `save` gets called twice on Post in the following example.
This results in `changes_applied` getting called twice.

    class Post < ApplicationRecord
      belongs_to :postable, polymorphic: true, inverse_of: :post
    end

    class Message < ApplicationRecord
      has_one :post, as: :postable
    end

    post = Post.create!(postable: Message.new(subject: "Hello, world!"))
    # the following would return false when true is expected
    post.id_previously_changed?

`save` gets called twice because Post autosaves Message, which
autosaves Post again.

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. This requires us to track the `@_saving` state of a record.
if `@_saving` is true we know we the record is being saved.

To track if a method has already been called we reuse the
@_already_called hash that is already used for this purpose.

activerecord/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   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
+          changes_applied if _apply_changes?(attribute_names)
           affected_rows
         end
 
@@ -200,6 +200,10 @@ 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; @_already_called ||= {}
+            result = true
             # Loop prevention for validation of associations
             unless @_already_called[name]
               begin
@@ -235,9 +235,18 @@ 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.
@@ -274,6 +283,17 @@ module ActiveRecord
     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.
@@ -509,5 +529,15 @@ module ActiveRecord
       def _ensure_no_duplicate_errors
         errors.uniq!
       end
+
+      def changes_applied
+        @_already_called[:changes_applied] = true
+        super
+      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

@@ -851,9 +851,11 @@ 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,6 +404,7 @@ 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

@@ -1958,3 +1958,71 @@ 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