From 901d62c586c20ab38b0f18f4bd9a4419902768c4 Mon Sep 17 00:00:00 2001
From: Julian Nadeau <jules2689@github.com>
Date: Mon, 6 Jan 2020 12:51:56 -0500
Subject: [PATCH] Only assign @new_record_before_save once in
 autosave_association

If we defined a callback before an association that updates the object, then this may end up being
manipulated to being `false` when it should be `true`. We guard this be only defining it once.

The implication of it being false, in this case, is that the children aren't updated with the parent_id
and so they fail to associate to one another.

See https://github.com/rails/rails/issues/38120 for more details
---
 .../lib/active_record/autosave_association.rb |  7 +++++-
 .../test/cases/autosave_association_test.rb   | 24 +++++++++++++++++++
 guides/source/active_record_callbacks.md      |  2 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 0c9c210bf2..6d6650ba57 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -365,7 +365,12 @@ module ActiveRecord
       # Is used as a before_save callback to check while saving a collection
       # association whether or not the parent was a new record before saving.
       def before_save_collection_association
-        @new_record_before_save = new_record?
+        # If we defined a callback that updates the object before we defined the association, then this ivar may end up being
+        # manipulated to being `false` when it should be `true`. We guard this be only defining it once.
+        # See https://github.com/rails/rails/issues/38120 for more details
+        unless defined?(@new_record_before_save)
+          @new_record_before_save = new_record?
+        end
       end
 
       def after_save_collection_association
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index e7c0ef0d25..4f988e2050 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -35,6 +35,30 @@ require "models/tuning_peg"
 require "models/reply"
 
 class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase
+  def test_autosave_works_even_when_other_callbacks_update_the_parent_model
+    reference = Class.new(ActiveRecord::Base) do
+      self.table_name = "references"
+      def self.name; "Reference"; end
+    end
+
+    person = Class.new(ActiveRecord::Base) do
+      self.table_name = "people"
+      def self.name; "Person"; end
+
+      # It is necessary that the after_create is before the has_many _and_ that it updates the model.
+      # This replicates a bug found in https://github.com/rails/rails/issues/38120
+      after_create { update(first_name: "first name") }
+      has_many :references, autosave: true, anonymous_class: reference
+    end
+
+    reference_instance = reference.create!
+    person_instance = person.create!(first_name: "foo", references: [reference_instance])
+
+    reference_instance.reload
+    assert_equal person_instance.id, reference_instance.person_id
+    assert_equal "first name", person_instance.first_name # Make sure the after_create is actually called
+  end
+
   def test_autosave_does_not_pass_through_non_custom_validation_contexts
     person = Class.new(ActiveRecord::Base) {
       self.table_name = "people"
diff --git a/guides/source/active_record_callbacks.md b/guides/source/active_record_callbacks.md
index 1518c704ff..e9df505f21 100644
--- a/guides/source/active_record_callbacks.md
+++ b/guides/source/active_record_callbacks.md
@@ -117,6 +117,8 @@ Here is a list with all the available Active Record callbacks, listed in the sam
 
 WARNING. `after_save` runs both on create and update, but always _after_ the more specific callbacks `after_create` and `after_update`, no matter the order in which the macro calls were executed.
 
+WARNING. Care should be taken in callbacks to avoid updating attributes. For example, avoid running `update(attribute: "value")` and similar code during callbacks. This can alter the state of the model and may result in unexpected side effects during commit. Instead, you should try to assign values in the `before_create` or earlier callbacks.
+
 NOTE: `before_destroy` callbacks should be placed before `dependent: :destroy`
 associations (or use the `prepend: true` option), to ensure they execute before
 the records are deleted by `dependent: :destroy`.