Merge pull request #33673 from tgxworld/regression_setting_children_record_in_parent_before_save

Fix regression setting children record in parent before_save callback.

4 files changed, 31 insertions, 6 deletions

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index a405f05e0b..783a8366ce 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -392,7 +392,7 @@ module ActiveRecord
               records -= records_to_destroy
             end
 
-            records.each_with_index do |record, index|
+            records.each do |record|
               next if record.destroyed?
 
               saved = true
@@ -401,11 +401,11 @@ module ActiveRecord
                 if autosave
                   saved = association.insert_record(record, false)
                 elsif !reflection.nested?
+                  association_saved = association.insert_record(record)
+
                   if reflection.validate?
-                    valid = association_valid?(reflection, record, index)
-                    saved = valid ? association.insert_record(record, false) : false
-                  else
-                    association.insert_record(record)
+                    errors.add(reflection.name) unless association_saved
+                    saved = association_saved
                   end
                 end
               elsif autosave
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index ade1f4b44d..fa618735d7 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -558,6 +558,13 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa
     assert_equal no_of_clients + 1, Client.count
   end
 
+  def test_parent_should_save_children_record_with_foreign_key_validation_set_in_before_save_callback
+    company = NewlyContractedCompany.new(name: "test")
+
+    assert company.save
+    assert_not_empty company.reload.new_contracts
+  end
+
   def test_parent_should_not_get_saved_with_duplicate_children_records
     assert_no_difference "Reply.count" do
       assert_no_difference "SillyUniqueReply.count" do
@@ -568,7 +575,13 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa
         ])
 
         assert_not reply.save
-        assert_not_empty reply.errors
+        assert_equal ["is invalid"], reply.errors[:silly_unique_replies]
+        assert_empty reply.silly_unique_replies.first.errors
+
+        assert_equal(
+          ["has already been taken"],
+          reply.silly_unique_replies.last.errors[:content]
+        )
       end
     end
   end
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index d4d5275b78..485b35d58b 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -204,4 +204,12 @@ end
 class VerySpecialClient < SpecialClient
 end
 
+class NewlyContractedCompany < Company
+  has_many :new_contracts, foreign_key: "company_id"
+
+  before_save do
+    self.new_contracts << NewContract.new
+  end
+end
+
 require "models/account"
diff --git a/activerecord/test/models/contract.rb b/activerecord/test/models/contract.rb
index f273badd85..3f663375c4 100644
--- a/activerecord/test/models/contract.rb
+++ b/activerecord/test/models/contract.rb
@@ -20,3 +20,7 @@ class Contract < ActiveRecord::Base
     @bye_count += 1
   end
 end
+
+class NewContract < Contract
+  validates :company_id, presence: true
+end