aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
authoreileencodes <eileencodes@gmail.com>2016-02-23 15:12:57 -0500
committereileencodes <eileencodes@gmail.com>2016-02-23 15:21:46 -0500
commit2c02bc0a47777ad8cf98e1465c08b1a68151803e (patch)
tree33f657c78b31370c283f130fc60d2ff65e28be09 /activerecord
parente9b96f0d666adfd3484641a4a55feb1c774d3378 (diff)
downloadrails-2c02bc0a47777ad8cf98e1465c08b1a68151803e.tar.gz
rails-2c02bc0a47777ad8cf98e1465c08b1a68151803e.tar.bz2
rails-2c02bc0a47777ad8cf98e1465c08b1a68151803e.zip
Revert changes to validations from PR #18612
In order to fix issue #17621 we added a check to validations that determined if a record should be validated. Based on the existing tests and behavior we wanted we determined the best way to do that was by checking if `!record.peristed? || record.changed? || record.marked_for_destruction?` This change didn't make it into a release until now. When #23790 was opened we realized that `valid?` and `invalid?` were broken and did not work on persisted records because of the `!record.persisted?`. While there is still a bug that #17621 brought up, this change was too drastic and should not be a RC blocker. I will work on fixing this so that we don't break `valid?` but also aren't validating parent records through child records if that parent record is validate false. This change removes the code changes to validate and the corresponding tests. It adds tests for two of the bugs found since Rails 5 beta2 release. Fixes #17621
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/lib/active_record/validations/absence.rb1
-rw-r--r--activerecord/lib/active_record/validations/length.rb12
-rw-r--r--activerecord/lib/active_record/validations/presence.rb1
-rw-r--r--activerecord/lib/active_record/validations/uniqueness.rb1
-rw-r--r--activerecord/test/cases/validations/absence_validation_test.rb16
-rw-r--r--activerecord/test/cases/validations/length_validation_test.rb14
-rw-r--r--activerecord/test/cases/validations/presence_validation_test.rb37
-rw-r--r--activerecord/test/cases/validations/uniqueness_validation_test.rb17
8 files changed, 21 insertions, 78 deletions
diff --git a/activerecord/lib/active_record/validations/absence.rb b/activerecord/lib/active_record/validations/absence.rb
index 376d743c92..641d041f3d 100644
--- a/activerecord/lib/active_record/validations/absence.rb
+++ b/activerecord/lib/active_record/validations/absence.rb
@@ -2,7 +2,6 @@ module ActiveRecord
module Validations
class AbsenceValidator < ActiveModel::Validations::AbsenceValidator # :nodoc:
def validate_each(record, attribute, association_or_value)
- return unless should_validate?(record) || unknown_attribute?(record, attribute)
if record.class._reflect_on_association(attribute)
association_or_value = Array.wrap(association_or_value).reject(&:marked_for_destruction?)
end
diff --git a/activerecord/lib/active_record/validations/length.rb b/activerecord/lib/active_record/validations/length.rb
index fe34e4875c..0e0cebce4a 100644
--- a/activerecord/lib/active_record/validations/length.rb
+++ b/activerecord/lib/active_record/validations/length.rb
@@ -2,23 +2,11 @@ module ActiveRecord
module Validations
class LengthValidator < ActiveModel::Validations::LengthValidator # :nodoc:
def validate_each(record, attribute, association_or_value)
- return unless should_validate?(record) || unknown_attribute?(record, attribute) || 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 e34d2d70ab..ad82ea66c4 100644
--- a/activerecord/lib/active_record/validations/presence.rb
+++ b/activerecord/lib/active_record/validations/presence.rb
@@ -2,7 +2,6 @@ module ActiveRecord
module Validations
class PresenceValidator < ActiveModel::Validations::PresenceValidator # :nodoc:
def validate_each(record, attribute, association_or_value)
- return unless should_validate?(record) || unknown_attribute?(record, attribute)
if record.class._reflect_on_association(attribute)
association_or_value = Array.wrap(association_or_value).reject(&:marked_for_destruction?)
end
diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb
index 1eb5005791..13053beb78 100644
--- a/activerecord/lib/active_record/validations/uniqueness.rb
+++ b/activerecord/lib/active_record/validations/uniqueness.rb
@@ -11,7 +11,6 @@ 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/absence_validation_test.rb b/activerecord/test/cases/validations/absence_validation_test.rb
index 180acbcb6a..c0b3750bcc 100644
--- a/activerecord/test/cases/validations/absence_validation_test.rb
+++ b/activerecord/test/cases/validations/absence_validation_test.rb
@@ -57,22 +57,6 @@ class AbsenceValidationTest < ActiveRecord::TestCase
assert_nothing_raised { boy_klass.new(face: face_with_to_a).valid? }
end
- def test_does_not_validate_if_parent_record_is_validate_false
- repair_validations(Interest) do
- Interest.validates_absence_of(:topic)
- interest = Interest.new(topic: Topic.new(title: "Math"))
- 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
-
def test_validates_absence_of_virtual_attribute_on_model
repair_validations(Interest) do
Interest.send(:attr_accessor, :token)
diff --git a/activerecord/test/cases/validations/length_validation_test.rb b/activerecord/test/cases/validations/length_validation_test.rb
index 4b6470393e..78263fd955 100644
--- a/activerecord/test/cases/validations/length_validation_test.rb
+++ b/activerecord/test/cases/validations/length_validation_test.rb
@@ -61,20 +61,6 @@ 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
-
def test_validates_length_of_virtual_attribute_on_model
repair_validations(Pet) do
Pet.send(:attr_accessor, :nickname)
diff --git a/activerecord/test/cases/validations/presence_validation_test.rb b/activerecord/test/cases/validations/presence_validation_test.rb
index 691f10a635..2ae30c7fd3 100644
--- a/activerecord/test/cases/validations/presence_validation_test.rb
+++ b/activerecord/test/cases/validations/presence_validation_test.rb
@@ -65,22 +65,6 @@ 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
-
def test_validates_presence_of_virtual_attribute_on_model
repair_validations(Interest) do
Interest.send(:attr_accessor, :abbreviation)
@@ -95,4 +79,25 @@ class PresenceValidationTest < ActiveRecord::TestCase
assert interest.invalid?
end
end
+
+ def test_validations_run_on_persisted_record
+ repair_validations(Interest) do
+ interest = Interest.new
+ interest.save!
+ assert_predicate interest, :valid?
+
+ Interest.validates_presence_of(:topic)
+
+ assert_not_predicate interest, :valid?
+ end
+ end
+
+ def test_validates_prescence_with_on_context
+ repair_validations(Interest) do
+ Interest.validates_presence_of(:topic, on: :required_name)
+ interest = Interest.new
+ interest.save!
+ assert_not interest.valid?(:required_name)
+ end
+ end
end
diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb
index 2217e52f4b..8abb6c9844 100644
--- a/activerecord/test/cases/validations/uniqueness_validation_test.rb
+++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb
@@ -417,23 +417,6 @@ class UniquenessValidationTest < ActiveRecord::TestCase
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
-
def test_validate_uniqueness_of_custom_primary_key
klass = Class.new(ActiveRecord::Base) do
self.table_name = "keyboards"