Fix validations on child record when record parent has validate: false

Fixes rails#17621. This 5 year old (or older) issue causes validations to fire
when a parent record has `validate: false` option and a child record is
saved. It's not the responsibility of the model to validate an
associated object unless the object was created or modified by the
parent.

Clean up tests related to validations

`assert_nothing_raised` is not benefiting us in these tests
Corrected spelling of "respects"
It's better to use `assert_not_operator` over `assert !r.valid`

activemodel/lib/active_model/validator.rb

@@ -163,6 +163,10 @@ def validate_each(record, attribute, value)
     # +ArgumentError+ when invalid options are supplied.
     def check_validity!
     end
+
+    def should_validate?(record) # :nodoc:
+      !record.persisted? || record.changed? || record.marked_for_destruction?
+    end
   end
 
   # +BlockValidator+ is a special +EachValidator+ which receives a block on initialization

activerecord/CHANGELOG.md

@@ -1,3 +1,15 @@
+*   Validation errors would be raised for parent records when an association
+    was saved when the parent had `validate: false`. It should not be the
+    responsibility of the model to validate an associated object unless the
+    object was created or modified by the parent.
+
+    This fixes the issue by skipping validations if the parent record is
+    persisted, not changed, and not marked for destruction.
+
+    Fixes #17621.
+
+    *Eileen M. Uchitelle, Aaron Patterson*
+
 *   Fix n+1 query problem when eager loading nil associations (fixes #18312)
 
     *Sammy Larbi*

activerecord/lib/active_record/validations/length.rb

@@ -2,11 +2,23 @@ module ActiveRecord
   module Validations
     class LengthValidator < ActiveModel::Validations::LengthValidator # :nodoc:
       def validate_each(record, attribute, association_or_value)
+        return unless should_validate?(record) || associations_are_dirty?(record)
         if association_or_value.respond_to?(:loaded?) && association_or_value.loaded?
           association_or_value = association_or_value.target.reject(&:marked_for_destruction?)
         end
         super
       end
+
+      def associations_are_dirty?(record)
+        attributes.any? do |attribute|
+          value = record.read_attribute_for_validation(attribute)
+          if value.respond_to?(:loaded?) && value.loaded?
+            value.target.any?(&:marked_for_destruction?)
+          else
+            false
+          end
+        end
+      end
     end
 
     module ClassMethods

activerecord/lib/active_record/validations/presence.rb

@@ -2,6 +2,7 @@ module ActiveRecord
   module Validations
     class PresenceValidator < ActiveModel::Validations::PresenceValidator # :nodoc:
       def validate(record)
+        return unless should_validate?(record)
         super
         attributes.each do |attribute|
           next unless record.class._reflect_on_association(attribute)

activerecord/lib/active_record/validations/uniqueness.rb

@@ -11,6 +11,7 @@ def initialize(options)
       end
 
       def validate_each(record, attribute, value)
+        return unless should_validate?(record)
         finder_class = find_finder_class_for(record)
         table = finder_class.arel_table
         value = map_enum_attribute(finder_class, attribute, value)

activerecord/test/cases/validations/association_validation_test.rb

@@ -50,7 +50,7 @@ def test_validates_associated_with_custom_message_using_quotes
     Topic.validates_presence_of :content
     r = Reply.create("title" => "A reply", "content" => "with content!")
     r.topic = Topic.create("title" => "uhohuhoh")
-    assert !r.valid?
+    assert_not_operator r, :valid?
     assert_equal ["This string contains 'single' and \"double\" quotes"], r.errors[:topic]
   end
 
@@ -82,5 +82,4 @@ def test_validates_presence_of_belongs_to_association__existing_parent
       assert interest.valid?, "Expected interest to be valid, but was not. Interest should have a man object associated"
     end
   end
-
 end

activerecord/test/cases/validations/length_validation_test.rb

@@ -37,7 +37,7 @@ def test_validates_size_of_association_using_within
 
   def test_validates_size_of_association_utf8
     repair_validations Owner do
-      assert_nothing_raised { Owner.validates_size_of :pets, :minimum => 1 }
+      Owner.validates_size_of :pets, :minimum => 1
       o = Owner.new('name' => 'あいうえおかきくけこ')
       assert !o.save
       assert o.errors[:pets].any?
@@ -46,8 +46,8 @@ def test_validates_size_of_association_utf8
     end
   end
 
-  def test_validates_size_of_reprects_records_marked_for_destruction
-    assert_nothing_raised { Owner.validates_size_of :pets, minimum: 1 }
+  def test_validates_size_of_respects_records_marked_for_destruction
+    Owner.validates_size_of :pets, minimum: 1
     owner = Owner.new
     assert_not owner.save
     assert owner.errors[:pets].any?
@@ -62,4 +62,17 @@ def test_validates_size_of_reprects_records_marked_for_destruction
     assert_equal pet_count, Pet.count
   end
 
+  def test_does_not_validate_length_of_if_parent_record_is_validate_false
+    Owner.validates_length_of :name, minimum: 1
+    owner = Owner.new
+    owner.save!(validate: false)
+    assert owner.persisted?
+
+    pet = Pet.new(owner_id: owner.id)
+    pet.save!
+
+    assert_equal owner.pets.size, 1
+    assert owner.valid?
+    assert pet.valid?
+  end
 end

activerecord/test/cases/validations/presence_validation_test.rb

@@ -65,4 +65,20 @@ def dash.to_a; ['(/)', '(\)']; end
 
     assert_nothing_raised { s.valid? }
   end
+
+  def test_does_not_validate_presence_of_if_parent_record_is_validate_false
+    repair_validations(Interest) do
+      Interest.validates_presence_of(:topic)
+      interest = Interest.new
+      interest.save!(validate: false)
+      assert interest.persisted?
+
+      man = Man.new(interest_ids: [interest.id])
+      man.save!
+
+      assert_equal man.interests.size, 1
+      assert interest.valid?
+      assert man.valid?
+    end
+  end
 end

activerecord/test/cases/validations/uniqueness_validation_test.rb

@@ -386,4 +386,21 @@ def test_validate_uniqueness_on_empty_relation
     topic = TopicWithUniqEvent.new
     assert topic.valid?
   end
+
+  def test_does_not_validate_uniqueness_of_if_parent_record_is_validate_false
+    Reply.validates_uniqueness_of(:content)
+
+    Reply.create!(content: "Topic Title")
+
+    reply = Reply.new(content: "Topic Title")
+    reply.save!(validate: false)
+    assert reply.persisted?
+
+    topic = Topic.new(reply_ids: [reply.id])
+    topic.save!
+
+    assert_equal topic.replies.size, 1
+    assert reply.valid?
+    assert topic.valid?
+  end
 end