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

Fixes #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`

8 files changed, 76 insertions, 5 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index a17148f1f7..3a0c32e66d 100644
--- a/activerecord/CHANGELOG.md
+++ b/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*
diff --git a/activerecord/lib/active_record/validations/length.rb b/activerecord/lib/active_record/validations/length.rb
index ef5a6cbbe7..5991fbad8e 100644
--- a/activerecord/lib/active_record/validations/length.rb
+++ b/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
diff --git a/activerecord/lib/active_record/validations/presence.rb b/activerecord/lib/active_record/validations/presence.rb
index 61b30749d9..75d5bd5a35 100644
--- a/activerecord/lib/active_record/validations/presence.rb
+++ b/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)
diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb
index 8f7a46f2c7..ad56f637e3 100644
--- a/activerecord/lib/active_record/validations/uniqueness.rb
+++ b/activerecord/lib/active_record/validations/uniqueness.rb
@@ -11,6 +11,7 @@ module ActiveRecord
       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)
diff --git a/activerecord/test/cases/validations/association_validation_test.rb b/activerecord/test/cases/validations/association_validation_test.rb
index e4edc437e6..bff5ffa65e 100644
--- a/activerecord/test/cases/validations/association_validation_test.rb
+++ b/activerecord/test/cases/validations/association_validation_test.rb
@@ -50,7 +50,7 @@ class AssociationValidationTest < ActiveRecord::TestCase
     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 @@ class AssociationValidationTest < ActiveRecord::TestCase
       assert interest.valid?, "Expected interest to be valid, but was not. Interest should have a man object associated"
     end
   end
-
 end
diff --git a/activerecord/test/cases/validations/length_validation_test.rb b/activerecord/test/cases/validations/length_validation_test.rb
index 2c0e282761..8b69a6815e 100644
--- a/activerecord/test/cases/validations/length_validation_test.rb
+++ b/activerecord/test/cases/validations/length_validation_test.rb
@@ -37,7 +37,7 @@ class LengthValidationTest < ActiveRecord::TestCase
 
   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 @@ class LengthValidationTest < ActiveRecord::TestCase
     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 @@ class LengthValidationTest < ActiveRecord::TestCase
     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
diff --git a/activerecord/test/cases/validations/presence_validation_test.rb b/activerecord/test/cases/validations/presence_validation_test.rb
index 4f38849131..b5b16d7a9b 100644
--- a/activerecord/test/cases/validations/presence_validation_test.rb
+++ b/activerecord/test/cases/validations/presence_validation_test.rb
@@ -65,4 +65,20 @@ class PresenceValidationTest < ActiveRecord::TestCase
 
     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
diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb
index 524f59876e..d41b26a2e5 100644
--- a/activerecord/test/cases/validations/uniqueness_validation_test.rb
+++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb
@@ -386,4 +386,21 @@ class UniquenessValidationTest < ActiveRecord::TestCase
     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