From 5a3e34ed1db155964b2a2e037c34df3b16cf8e06 Mon Sep 17 00:00:00 2001 From: Tekin Suleyman Date: Wed, 25 Sep 2019 10:07:45 +0200 Subject: [PATCH] Ensure Contextual validations fire on associations If a custom validation context is used, the validations on dependent association records should fire regardless of whether those record have `changed_for_autosave?` or not as the use of a custom validation context increases the chance that validations for the context will be different to those from when the record was first saved. 6ea80b6 changed the autosave behaviour for single associations such that validations would only fire on an associated record if the record was `changed_for_autosave?`. This brought the behaviour inline with that for collection associations, but introduce a regression in that validations would no longer fire on dependent associations, even when using a custom validation context. This change updates the behaviour for both single and collection associations. This commit also updates another related regression test that became a potential false-positive when 6ea80b6 was merged. The original test was written to protect against non-custom validations firing on associations (see #14106). At the time validations on autosaving singular associations would always fire, even when the association was not dirty, whereas after 6ea80b6 the validations only fire if the association is dirty. This update to the test makes the association record dirty to ensure the validations will fire so that the code responsible for excluding non-custom contexts is correctly exercised. --- .../lib/active_record/autosave_association.rb | 10 +++++--- .../test/cases/autosave_association_test.rb | 24 ++++++++++++++++--- activerecord/test/models/account.rb | 5 ++++ activerecord/test/models/pirate.rb | 2 +- activerecord/test/models/ship.rb | 2 +- 5 files changed, 35 insertions(+), 8 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 3196692b57..2c8bf526af 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -270,7 +270,7 @@ def changed_for_autosave? # or saved. If +autosave+ is +false+ only new records will be returned, # unless the parent is/was a new record itself. def associated_records_to_validate_or_save(association, new_record, autosave) - if new_record + if new_record || custom_validation_context? association && association.target elsif autosave association.target.find_all(&:changed_for_autosave?) @@ -302,7 +302,7 @@ def nested_records_changed_for_autosave? def validate_single_association(reflection) association = association_instance_get(reflection.name) record = association && association.reader - association_valid?(reflection, record) if record && record.changed_for_autosave? + association_valid?(reflection, record) if record && (record.changed_for_autosave? || custom_validation_context?) end # Validate the associated records if :validate or @@ -322,7 +322,7 @@ def validate_collection_association(reflection) def association_valid?(reflection, record, index = nil) return true if record.destroyed? || (reflection.options[:autosave] && record.marked_for_destruction?) - context = validation_context unless [:create, :update].include?(validation_context) + context = validation_context if custom_validation_context? unless valid = record.valid?(context) if reflection.options[:autosave] @@ -492,6 +492,10 @@ def save_belongs_to_association(reflection) end end + def custom_validation_context? + validation_context && [:create, :update].exclude?(validation_context) + end + def _ensure_no_duplicate_errors errors.uniq! end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index a771817da9..e7c0ef0d25 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -35,7 +35,7 @@ require "models/reply" class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase - def test_autosave_validation + def test_autosave_does_not_pass_through_non_custom_validation_contexts person = Class.new(ActiveRecord::Base) { self.table_name = "people" validate :should_be_cool, on: :create @@ -55,8 +55,11 @@ def self.name; "Reference"; end } u = person.create!(first_name: "cool") - u.update!(first_name: "nah") # still valid because validation only applies on 'create' - assert_predicate reference.create!(person: u), :persisted? + u.first_name = "nah" + + assert_predicate u, :valid? + r = reference.new(person: u) + assert_predicate r, :valid? end def test_should_not_add_the_same_callbacks_multiple_times_for_has_one @@ -1736,6 +1739,14 @@ def setup assert_equal(author_count_before_save, Author.count) assert_equal(book_count_before_save, Book.count) end + + def test_validations_still_fire_on_unchanged_association_with_custom_validation_context + pirate = FamousPirate.create!(catchphrase: "Avast Ye!") + pirate.famous_ships.create! + + assert pirate.valid? + assert_not pirate.valid?(:conference) + end end class TestAutosaveAssociationValidationsOnAHasOneAssociation < ActiveRecord::TestCase @@ -1780,6 +1791,13 @@ def setup @pirate.non_validated_parrot = Parrot.new(name: "") assert_predicate @pirate, :valid? end + + def test_validations_still_fire_on_unchanged_association_with_custom_validation_context + firm_with_low_credit = Firm.create!(name: "Something", account: Account.new(credit_limit: 50)) + + assert firm_with_low_credit.valid? + assert_not firm_with_low_credit.valid?(:bank_loan) + end end class TestAutosaveAssociationValidationsOnAHABTMAssociation < ActiveRecord::TestCase diff --git a/activerecord/test/models/account.rb b/activerecord/test/models/account.rb index 639e395743..ffc197e08b 100644 --- a/activerecord/test/models/account.rb +++ b/activerecord/test/models/account.rb @@ -21,12 +21,17 @@ def self.destroyed_account_ids end validate :check_empty_credit_limit + validate :ensure_good_credit, on: :bank_loan private def check_empty_credit_limit errors.add("credit_limit", :blank) if credit_limit.blank? end + def ensure_good_credit + errors.add(:credit_limit, "too low") unless credit_limit > 10_000 + end + def private_method "Sir, yes sir!" end diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index 8733398697..6b903bc19f 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -95,7 +95,7 @@ class DestructivePirate < Pirate class FamousPirate < ActiveRecord::Base self.table_name = "pirates" - has_many :famous_ships + has_many :famous_ships, inverse_of: :famous_pirate, foreign_key: :pirate_id validates_presence_of :catchphrase, on: :conference end diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb index 6bab7a1eb9..773892edad 100644 --- a/activerecord/test/models/ship.rb +++ b/activerecord/test/models/ship.rb @@ -37,6 +37,6 @@ class Prisoner < ActiveRecord::Base class FamousShip < ActiveRecord::Base self.table_name = "ships" - belongs_to :famous_pirate + belongs_to :famous_pirate, foreign_key: :pirate_id validates_presence_of :name, on: :conference end