Merge pull request #32952 from mechanicles/32940-fix

Fix parent record should not get saved with duplicate children records

5 files changed, 35 insertions, 3 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index dda7d19915..5cefb5dfd7 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Fix parent record should not get saved with duplicate children records.
+
+    Fixes #32940.
+
+    *Santosh Wadghule*
+
 *   Fix logic on disabling commit callbacks so they are not called unexpectedly when errors occur.
 
     *Brian Durand*
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 9575cc24c8..a405f05e0b 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 do |record|
+            records.each_with_index do |record, index|
               next if record.destroyed?
 
               saved = true
@@ -401,9 +401,11 @@ module ActiveRecord
                 if autosave
                   saved = association.insert_record(record, false)
                 elsif !reflection.nested?
-                  association_saved = association.insert_record(record)
                   if reflection.validate?
-                    saved = association_saved
+                    valid = association_valid?(reflection, record, index)
+                    saved = valid ? association.insert_record(record, false) : false
+                  else
+                    association.insert_record(record)
                   end
                 end
               elsif autosave
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 7355f4cd62..cf93d42942 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -27,6 +27,8 @@ require "models/member_detail"
 require "models/organization"
 require "models/guitar"
 require "models/tuning_peg"
+require "models/topic"
+require "models/reply"
 
 class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase
   def test_autosave_validation
@@ -557,6 +559,22 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa
     assert_equal no_of_clients + 1, Client.count
   end
 
+  def test_parent_should_not_get_saved_with_duplicate_children_records
+    Topic.delete_all
+
+    content = "Best content"
+    reply1 = ValidateUniqueContentReply.new(content: content)
+    reply2 = ValidateUniqueContentReply.new(content: content)
+
+    topic = Topic.new(validate_unique_content_replies: [reply1, reply2])
+
+    assert_not topic.save
+    assert topic.errors.any?
+
+    assert_equal 0, Topic.count
+    assert_equal 0, ValidateUniqueContentReply.count
+  end
+
   def test_invalid_build
     new_client = companies(:first_firm).clients_of_firm.build
     assert_not_predicate new_client, :persisted?
diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb
index d1cee58788..d178413fd7 100644
--- a/activerecord/test/models/reply.rb
+++ b/activerecord/test/models/reply.rb
@@ -16,6 +16,11 @@ end
 class SillyUniqueReply < UniqueReply
 end
 
+class ValidateUniqueContentReply < Reply
+  belongs_to :topic, foreign_key: "parent_id"
+  validates :content, uniqueness: true
+end
+
 class WrongReply < Reply
   validate :errors_on_empty_content
   validate :title_is_wrong_create, on: :create
diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb
index 72699046f9..8368405cef 100644
--- a/activerecord/test/models/topic.rb
+++ b/activerecord/test/models/topic.rb
@@ -47,6 +47,7 @@ class Topic < ActiveRecord::Base
 
   has_many :unique_replies, dependent: :destroy, foreign_key: "parent_id"
   has_many :silly_unique_replies, dependent: :destroy, foreign_key: "parent_id"
+  has_many :validate_unique_content_replies, dependent: :destroy, foreign_key: "parent_id"
 
   serialize :content