Merge pull request #33673 from tgxworld/regression_setting_children_record_in_parent_before_save

Fix regression setting children record in parent before_save callback.

activerecord/lib/active_record/autosave_association.rb

@@ -392,7 +392,7 @@ def save_collection_association(reflection)
               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 @@ def save_collection_association(reflection)
                 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

activerecord/test/cases/autosave_association_test.rb

@@ -558,6 +558,13 @@ def test_valid_adding_with_validate_false
     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 @@ def test_parent_should_not_get_saved_with_duplicate_children_records
         ])
 
         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

activerecord/test/models/company.rb

@@ -204,4 +204,12 @@ class SpecialClient < Client
 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"

activerecord/test/models/contract.rb

@@ -20,3 +20,7 @@ def bye
     @bye_count += 1
   end
 end
+
+class NewContract < Contract
+  validates :company_id, presence: true
+end