aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
authoreileencodes <eileencodes@gmail.com>2016-02-20 09:15:39 -0500
committereileencodes <eileencodes@gmail.com>2016-02-20 09:15:39 -0500
commit6f15b276cb69ae1241e5c021fdc8e73fd3fe1197 (patch)
tree72e4ee8e88d583fce9b2658522ea6c2dc1ef2d55 /activerecord
parent3156a7692c3c51adb846252192364172b05bd67f (diff)
downloadrails-6f15b276cb69ae1241e5c021fdc8e73fd3fe1197.tar.gz
rails-6f15b276cb69ae1241e5c021fdc8e73fd3fe1197.tar.bz2
rails-6f15b276cb69ae1241e5c021fdc8e73fd3fe1197.zip
Always validate record if validating a virtual attribute
Fixes #23645 When you're using an `attr_accessor` for a record instead of an attribute in the database there's no way for the record to know if it has `changed?` unless you tell it `attribute_will_change!("attribute")`. The change made in 27aa4dd updated validations to check if a record was `changed?` or `marked_for_destruction?` or not `persisted?`. It did not take into account virtual attributes that do not affect the model's dirty status. The only way to fix this is to always validate the record if the attribute does not belong to the set of attributes the record expects (in `record.attributes`) because virtual attributes will not be in that hash. I think we should consider deprecating this particular behavior in the future and requiring that the user mark the record dirty by noting that the virtual attribute will change. Unfortunately this isn't easy because we have no way of knowing that you did the "right thing" in your application by marking it dirty and will get the deprecation warning even if you are doing the correct thing. For now this restores expected behavior when using a virtual attribute by always validating the record, as well as adds tests for this case. I was going to add the `!record.attributes.include?(attribute)` to the `should_validate?` method but `uniqueness` cannot validate a virtual attribute with nothing to hold on to the attribute. Because of this `should_validate?` was about to become a very messy method so I decided to split them up so we can handle it specifically for each case.
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/lib/active_record/validations/absence.rb2
-rw-r--r--activerecord/lib/active_record/validations/length.rb2
-rw-r--r--activerecord/lib/active_record/validations/presence.rb2
-rw-r--r--activerecord/test/cases/validations/absence_validation_test.rb14
-rw-r--r--activerecord/test/cases/validations/length_validation_test.rb16
-rw-r--r--activerecord/test/cases/validations/presence_validation_test.rb15
6 files changed, 48 insertions, 3 deletions
diff --git a/activerecord/lib/active_record/validations/absence.rb b/activerecord/lib/active_record/validations/absence.rb
index 2e19e6dc5c..376d743c92 100644
--- a/activerecord/lib/active_record/validations/absence.rb
+++ b/activerecord/lib/active_record/validations/absence.rb
@@ -2,7 +2,7 @@ module ActiveRecord
module Validations
class AbsenceValidator < ActiveModel::Validations::AbsenceValidator # :nodoc:
def validate_each(record, attribute, association_or_value)
- return unless should_validate?(record)
+ 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 69e048eef1..fe34e4875c 100644
--- a/activerecord/lib/active_record/validations/length.rb
+++ b/activerecord/lib/active_record/validations/length.rb
@@ -2,7 +2,7 @@ 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)
+ 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
diff --git a/activerecord/lib/active_record/validations/presence.rb b/activerecord/lib/active_record/validations/presence.rb
index 7e85ed43ac..e34d2d70ab 100644
--- a/activerecord/lib/active_record/validations/presence.rb
+++ b/activerecord/lib/active_record/validations/presence.rb
@@ -2,7 +2,7 @@ module ActiveRecord
module Validations
class PresenceValidator < ActiveModel::Validations::PresenceValidator # :nodoc:
def validate_each(record, attribute, association_or_value)
- return unless should_validate?(record)
+ 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/test/cases/validations/absence_validation_test.rb b/activerecord/test/cases/validations/absence_validation_test.rb
index dd43ee358c..180acbcb6a 100644
--- a/activerecord/test/cases/validations/absence_validation_test.rb
+++ b/activerecord/test/cases/validations/absence_validation_test.rb
@@ -72,4 +72,18 @@ class AbsenceValidationTest < ActiveRecord::TestCase
assert man.valid?
end
end
+
+ def test_validates_absence_of_virtual_attribute_on_model
+ repair_validations(Interest) do
+ Interest.send(:attr_accessor, :token)
+ Interest.validates_absence_of(:token)
+
+ interest = Interest.create!(topic: 'Thought Leadering')
+ assert interest.valid?
+
+ interest.token = 'tl'
+
+ assert interest.invalid?
+ end
+ end
end
diff --git a/activerecord/test/cases/validations/length_validation_test.rb b/activerecord/test/cases/validations/length_validation_test.rb
index c5d8f8895c..624aeaaddb 100644
--- a/activerecord/test/cases/validations/length_validation_test.rb
+++ b/activerecord/test/cases/validations/length_validation_test.rb
@@ -74,4 +74,20 @@ class LengthValidationTest < ActiveRecord::TestCase
assert owner.valid?
assert pet.valid?
end
+
+ def test_validates_presence_of_virtual_attribute_on_model
+ repair_validations(Pet) do
+ Pet.send(:attr_accessor, :nickname)
+ Pet.validates_length_of(:name, minimum: 1)
+ Pet.validates_length_of(:nickname, minimum: 1)
+
+ pet = Pet.create!(name: 'Fancy Pants', nickname: 'Fancy')
+
+ assert pet.valid?
+
+ pet.nickname = ''
+
+ assert pet.invalid?
+ end
+ end
end
diff --git a/activerecord/test/cases/validations/presence_validation_test.rb b/activerecord/test/cases/validations/presence_validation_test.rb
index 6f8ad06ab6..691f10a635 100644
--- a/activerecord/test/cases/validations/presence_validation_test.rb
+++ b/activerecord/test/cases/validations/presence_validation_test.rb
@@ -80,4 +80,19 @@ class PresenceValidationTest < ActiveRecord::TestCase
assert man.valid?
end
end
+
+ def test_validates_presence_of_virtual_attribute_on_model
+ repair_validations(Interest) do
+ Interest.send(:attr_accessor, :abbreviation)
+ Interest.validates_presence_of(:topic)
+ Interest.validates_presence_of(:abbreviation)
+
+ interest = Interest.create!(topic: 'Thought Leadering', abbreviation: 'tl')
+ assert interest.valid?
+
+ interest.abbreviation = ''
+
+ assert interest.invalid?
+ end
+ end
end