Prevent persisting invalid record:

- Fix rails#54267
- In rails#53951, the goal was to allow saving a record and its association
  even if the association had existing invalid records in the DB.
  We would not validate the association in that case.

  The problem of not validating, is that it stops the validation
  chain and it's possible that other records in subsequent relations
  are being modified and are invalid.
  e.g. `Company -has_many-> Developers -has_many-> Computers`, if
  a computer is changed with invalid value and the company
  get saved, this would bypass the computer validation and persist it.

  This commit fixes that by ensuring there are no validation errors
  from *changed* records in the whole association chain.

Author: Edouard-chin

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