Merge pull request rails#54273 from Edouard-chin/ec-no-invalid-persist

Prevent persisting invalid record:

Author: byroot

activerecord/lib/active_record/autosave_association.rb

@@ -370,27 +370,29 @@ def validate_collection_association(reflection)
       # enabled records if they're marked_for_destruction? or destroyed.
       def association_valid?(association, record)
         return true if record.destroyed? || (association.options[:autosave] && record.marked_for_destruction?)
-        if custom_validation_context?
-          context = validation_context
+
+        context = validation_context if custom_validation_context?
+        return true if record.valid?(context)
+
+        if record.changed? || record.new_record? || context
+          associated_errors = record.errors.objects
         else
-          # If the associated record is unchanged we shouldn't auto validate it.
-          # Even if a record is invalid you should still be able to create new references
-          # to it.
-          return true if !record.new_record? && !record.changed?
+          # If there are existing invalid records in the DB, we should still be able to reference them.
+          # Unless a record (no matter where in the association chain) is invalid and is being changed.
+          associated_errors = record.errors.objects.select { |error| error.is_a?(Associations::NestedError) }
         end
 
-        unless valid = record.valid?(context)
-          if association.options[:autosave]
-            record.errors.each { |error|
-              self.errors.objects.append(
-                Associations::NestedError.new(association, error)
-              )
-            }
-          else
-            errors.add(association.reflection.name)
-          end
+        if association.options[:autosave]
+          associated_errors.each { |error|
+            errors.objects.append(
+              Associations::NestedError.new(association, error)
+            )
+          }
+        elsif associated_errors.any?
+          errors.add(association.reflection.name)
         end
-        valid
+
+        errors.any?
       end
 
       # Is used as an around_save callback to check while saving a collection

activerecord/test/cases/autosave_association_test.rb

@@ -19,6 +19,7 @@
 require "models/parrot"
 require "models/pirate"
 require "models/project"
+require "models/price_estimate"
 require "models/ship"
 require "models/ship_part"
 require "models/squeak"
@@ -1490,7 +1491,7 @@ def test_should_skip_validation_on_habtm_if_destroyed
     assert_predicate @pirate, :valid?
   end
 
-  def test_should_skip_validation_on_habtm_if_persisted_and_unchanged
+  def test_should_be_valid_on_habtm_if_persisted_and_unchanged
     parrot = @pirate.parrots.create!(name: "parrots_1")
     parrot.update_column(:name, "")
     parrot.reload
@@ -1501,6 +1502,73 @@ def test_should_skip_validation_on_habtm_if_persisted_and_unchanged
     new_pirate.save!
   end
 
+  def test_should_be_invalid_on_habtm_when_any_record_in_the_association_chain_is_invalid_and_was_changed
+    treasure = @pirate.treasures.create!(name: "gold")
+    estimate = treasure.price_estimates.create!(price: 1)
+    estimate.update_columns(price: "not a number")
+
+    assert_not_predicate estimate, :valid?
+
+    treasures = @pirate.treasures.eager_load(:price_estimates).to_a
+    treasures.first.price_estimates.first.price = "not a price"
+    new_pirate = Pirate.new(
+      catchphrase: "Arr",
+      treasures: treasures,
+    )
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      new_pirate.save!
+    end
+    assert_equal(["Treasures is invalid"], new_pirate.errors.full_messages)
+  end
+
+  def test_should_be_invalid_on_habtm_when_any_record_in_the_association_chain_is_invalid_and_was_changed_with_autosave
+    super_pirate = Class.new(Pirate) do
+      self.table_name = "pirates"
+      has_many :great_treasures, class_name: "Treasure", foreign_key: "looter_id", autosave: true
+
+      def self.name
+        "SuperPirate"
+      end
+    end
+    @pirate = super_pirate.create(catchphrase: "Don' botharrr talkin' like one, savvy?")
+    treasure = @pirate.great_treasures.create!(name: "gold")
+    estimate = treasure.price_estimates.create!(price: 1)
+    estimate.update_columns(price: "not a number")
+
+    assert_not_predicate estimate, :valid?
+
+    treasures = @pirate.great_treasures.eager_load(:price_estimates).to_a
+    treasures.first.price_estimates.first.price = "not a price"
+    new_pirate = super_pirate.new(
+      catchphrase: "Arr",
+      great_treasures: treasures,
+    )
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      new_pirate.save!
+    end
+    assert_equal(["Great treasures price estimates price is not a number"], new_pirate.errors.full_messages)
+  end
+
+  def test_should_be_valid_on_habtm_when_any_record_in_the_association_chain_is_invalid_but_was_not_changed
+    treasure = @pirate.treasures.create!(name: "gold")
+    estimate = treasure.price_estimates.create!(price: 1)
+    estimate.update_columns(price: "not a number")
+
+    assert_not_predicate estimate, :valid?
+
+    treasures = @pirate.treasures.eager_load(:price_estimates).to_a
+    new_pirate = Pirate.new(
+      catchphrase: "Arr",
+      treasures: treasures,
+    )
+
+    assert_nothing_raised do
+      new_pirate.save!
+    end
+  end
+
   def test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_habtm
     @pirate.parrots.create!(name: "parrots_1")
 

activerecord/test/cases/nested_attributes_test.rb

@@ -1206,12 +1206,19 @@ def self.name; "Guitar"; end
 end
 
 class TestNestedAttributesWithExtend < ActiveRecord::TestCase
-  setup do
-    Pirate.accepts_nested_attributes_for :treasures
-  end
-
   def test_extend_affects_nested_attributes
-    pirate = Pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?")
+    super_pirate = Class.new(ActiveRecord::Base) do
+      self.table_name = "pirates"
+
+      has_many :treasures, as: :looter, extend: Pirate::PostTreasuresExtension
+      self.accepts_nested_attributes_for :treasures
+
+      def self.name
+        "SuperPirate"
+      end
+    end
+
+    pirate = super_pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?")
     pirate.treasures_attributes = [{ id: nil }]
     assert_equal "from extension", pirate.treasures[0].name
   end

activerecord/test/models/treasure.rb

@@ -6,7 +6,7 @@ class Treasure < ActiveRecord::Base
   # No counter_cache option given
   belongs_to :ship
 
-  has_many :price_estimates, as: :estimate_of
+  has_many :price_estimates, as: :estimate_of, autosave: true
   has_and_belongs_to_many :rich_people, join_table: "peoples_treasures", validate: false
 
   accepts_nested_attributes_for :looter