diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 16a053c4cb405..8e9f634af6758 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/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 diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index e2bef02c77a20..4f0478c48766f 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/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") diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 65c6a929e8053..775ae821064b3 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/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 diff --git a/activerecord/test/models/treasure.rb b/activerecord/test/models/treasure.rb index b51db56c373bf..cb9253a369814 100644 --- a/activerecord/test/models/treasure.rb +++ b/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