Merge pull request #18612 from...

Merge pull request #18612 from eileencodes/fix-validates-on-associated-record-if-parent-is-validate-false

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

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

+*   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