Skip to content

Commit

Permalink
Conditionally convert the raw_value received by the numeric validator.
Browse files Browse the repository at this point in the history
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]
  • Loading branch information
repinel committed Jul 11, 2015
1 parent 14354f1 commit 7500dae
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 13 deletions.
23 changes: 11 additions & 12 deletions activemodel/lib/active_model/validations/numericality.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def check_validity!
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)
Expand All @@ -29,16 +29,14 @@ def validate_each(record, attr_name, value)

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|
Expand All @@ -64,14 +62,15 @@ def validate_each(record, attr_name, value)

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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,34 +71,69 @@ def test_validates_numericality_with_greater_than
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

invalid!([-9, 9], 'must be greater than or equal to 10')
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

invalid!([-10, 11] + INFINITY, 'must be equal to 10')
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

invalid!([10], 'must be less than 10')
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

invalid!([11], 'must be less than or equal to 10')
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

Expand Down Expand Up @@ -196,7 +231,7 @@ def invalid!(values, error = nil)

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

Expand Down

0 comments on commit 7500dae

Please sign in to comment.