From 7500daec69499e4f2da2fc06cd816c754cf59504 Mon Sep 17 00:00:00 2001 From: Roque Pinel Date: Sat, 11 Jul 2015 14:40:14 -0400 Subject: Conditionally convert the raw_value received by the numeric validator. This fixes the issue where you may be comparing (using a numeric validator such as `greater_than`) numbers of a specific Numeric type such as `BigDecimal`. Previous behavior took the numeric value to be validated and unconditionally converted to Float. For example, due to floating point precision, this can cause issues when comparing a Float to a BigDecimal. Consider the following: ``` validates :sub_total, numericality: { greater_than: BigDecimal('97.18') } ``` If the `:sub_total` value BigDecimal.new('97.18') was validated against the above, the following would be valid since `:sub_total` is converted to a Float regardless of its original type. The result therefore becomes Kernel.Float(97.18) > BigDecimal.new('97.18') The above illustrated behavior is corrected with this patch by conditionally converting the value to validate to float. Use the post-type-cast version of the attribute to validate numericality [Roque Pinel & Trevor Wistaff] --- .../lib/active_model/validations/numericality.rb | 23 +++++++------- .../validations/numericality_validation_test.rb | 37 +++++++++++++++++++++- 2 files changed, 47 insertions(+), 13 deletions(-) (limited to 'activemodel') diff --git a/activemodel/lib/active_model/validations/numericality.rb b/activemodel/lib/active_model/validations/numericality.rb index 4ba4e3e8f7..9c1e8b4ba7 100644 --- a/activemodel/lib/active_model/validations/numericality.rb +++ b/activemodel/lib/active_model/validations/numericality.rb @@ -20,7 +20,7 @@ module ActiveModel def validate_each(record, attr_name, value) before_type_cast = :"#{attr_name}_before_type_cast" - raw_value = record.send(before_type_cast) if record.respond_to?(before_type_cast) + raw_value = record.send(before_type_cast) if record.respond_to?(before_type_cast) && record.send(before_type_cast) != value raw_value ||= value if record_attribute_changed_in_place?(record, attr_name) @@ -29,16 +29,14 @@ module ActiveModel return if options[:allow_nil] && raw_value.nil? - unless value = parse_raw_value_as_a_number(raw_value) + unless is_number?(raw_value) record.errors.add(attr_name, :not_a_number, filtered_options(raw_value)) return end - if allow_only_integer?(record) - unless value = parse_raw_value_as_an_integer(raw_value) - record.errors.add(attr_name, :not_an_integer, filtered_options(raw_value)) - return - end + if allow_only_integer?(record) && !is_integer?(raw_value) + record.errors.add(attr_name, :not_an_integer, filtered_options(raw_value)) + return end options.slice(*CHECKS.keys).each do |option, option_value| @@ -64,14 +62,15 @@ module ActiveModel protected - def parse_raw_value_as_a_number(raw_value) - Kernel.Float(raw_value) if raw_value !~ /\A0[xX]/ + def is_number?(raw_value) + parsed_value = Kernel.Float(raw_value) if raw_value !~ /\A0[xX]/ + !parsed_value.nil? rescue ArgumentError, TypeError - nil + false end - def parse_raw_value_as_an_integer(raw_value) - raw_value.to_i if raw_value.to_s =~ /\A[+-]?\d+\z/ + def is_integer?(raw_value) + /\A[+-]?\d+\z/ === raw_value.to_s end def filtered_options(value) diff --git a/activemodel/test/cases/validations/numericality_validation_test.rb b/activemodel/test/cases/validations/numericality_validation_test.rb index 05432abaff..4db6c828f8 100644 --- a/activemodel/test/cases/validations/numericality_validation_test.rb +++ b/activemodel/test/cases/validations/numericality_validation_test.rb @@ -71,6 +71,13 @@ class NumericalityValidationTest < ActiveModel::TestCase valid!([11]) end + def test_validates_numericality_with_greater_than_using_differing_numeric_types + Topic.validates_numericality_of :approved, greater_than: BigDecimal.new('97.18') + + invalid!([-97.18, BigDecimal.new('97.18'), BigDecimal('-97.18')], 'must be greater than 97.18') + valid!([97.18, 98, BigDecimal.new('98')]) # Notice the 97.18 as a float is greater than 97.18 as a BigDecimal due to floating point precision + end + def test_validates_numericality_with_greater_than_or_equal Topic.validates_numericality_of :approved, greater_than_or_equal_to: 10 @@ -78,6 +85,13 @@ class NumericalityValidationTest < ActiveModel::TestCase valid!([10]) end + def test_validates_numericality_with_greater_than_or_equal_using_differing_numeric_types + Topic.validates_numericality_of :approved, greater_than_or_equal_to: BigDecimal.new('97.18') + + invalid!([-97.18, 97.17, 97, BigDecimal.new('97.17'), BigDecimal.new('-97.18')], 'must be greater than or equal to 97.18') + valid!([97.18, 98, BigDecimal.new('97.19')]) + end + def test_validates_numericality_with_equal_to Topic.validates_numericality_of :approved, equal_to: 10 @@ -85,6 +99,13 @@ class NumericalityValidationTest < ActiveModel::TestCase valid!([10]) end + def test_validates_numericality_with_equal_to_using_differing_numeric_types + Topic.validates_numericality_of :approved, equal_to: BigDecimal.new('97.18') + + invalid!([-97.18, 97.18], 'must be equal to 97.18') + valid!([BigDecimal.new('97.18')]) + end + def test_validates_numericality_with_less_than Topic.validates_numericality_of :approved, less_than: 10 @@ -92,6 +113,13 @@ class NumericalityValidationTest < ActiveModel::TestCase valid!([-9, 9]) end + def test_validates_numericality_with_less_than_using_differing_numeric_types + Topic.validates_numericality_of :approved, less_than: BigDecimal.new('97.18') + + invalid!([97.18, BigDecimal.new('97.18')], 'must be less than 97.18') + valid!([-97.0, 97.0, -97, 97, BigDecimal.new('-97'), BigDecimal.new('97')]) + end + def test_validates_numericality_with_less_than_or_equal_to Topic.validates_numericality_of :approved, less_than_or_equal_to: 10 @@ -99,6 +127,13 @@ class NumericalityValidationTest < ActiveModel::TestCase valid!([-10, 10]) end + def test_validates_numericality_with_less_than_or_equal_to_using_differing_numeric_types + Topic.validates_numericality_of :approved, less_than_or_equal_to: BigDecimal.new('97.18') + + invalid!([97.18, 98], 'must be less than or equal to 97.18') + valid!([-97.18, BigDecimal.new('-97.18'), BigDecimal.new('97.18')]) + end + def test_validates_numericality_with_odd Topic.validates_numericality_of :approved, odd: true @@ -196,7 +231,7 @@ class NumericalityValidationTest < ActiveModel::TestCase def valid!(values) with_each_topic_approved_value(values) do |topic, value| - assert topic.valid?, "#{value.inspect} not accepted as a number" + assert topic.valid?, "#{value.inspect} not accepted as a number with validation error: #{topic.errors[:approved].first}" end end -- cgit v1.2.3