Skip to content
Browse files

Fix error when assigning NaN to an integer column

Also covers any non-castable case by returning nil, which
is in-line with the intention of the former implementation,
but covers the odd cases which respond to to_i but raise
an error when it's called, such as NaN, Infinity and -Infinity.

Fixes #8757
  • Loading branch information...
1 parent 85afc11 commit 807e176aba7804cad8d630d04f3b771011be4fe6 @trisweb trisweb committed Jan 6, 2013
View
9 activerecord/CHANGELOG.md
@@ -4,10 +4,11 @@
*Rob Worley*
-* Fix undefined method `to_i` when calling `new` on a scope that uses an Array.
- Fixes #8718, #8734.
+* Fix undefined method `to_i` when calling `new` on a scope that uses an
+ Array; Fix FloatDomainError when setting integer column to NaN.
+ Fixes #8718, #8734, #8757.
- *Jason Stirk*
+ *Jason Stirk + Tristan Harward*
* Rename `update_attributes` to `update`, keep `update_attributes` as an alias for `update` method.
This is a soft-deprecation for `update_attributes`, although it will still work without any
@@ -17,7 +18,7 @@
*Amparo Luna + Guillermo Iguaran*
* `after_commit` and `after_rollback` now validate the `:on` option and raise an `ArgumentError`
- if it is not one of `:create`, `:destroy` or ``:update`
+ if it is not one of `:create`, `:destroy` or `:update`
*Pascal Friederich*
View
6 activerecord/lib/active_record/connection_adapters/column.rb
@@ -206,11 +206,7 @@ def value_to_integer(value)
when TrueClass, FalseClass
value ? 1 : 0
else
- if value.respond_to?(:to_i)
- value.to_i
- else
- nil
- end
+ value.to_i rescue nil
@sobrinho
sobrinho added a note Jan 13, 2013

I'm afraid that makes difficulty to debug custom to_i implementations.

class Classification
  def to_i
    if something?
      raise InvalidArgument
    else
      1
    end
  end
end

classification = Classification.new(params[:classification])

post = Post.new
post.classification = classification
#=> nil

Obviously this a contrived example but points what I mean.

Since Float::INFINITY and Float::NAN raises FloatDomainError exception when to_i is called, makes more sense to rescue only from this exception.

Something like:

def value_to_integer(value)
  case value
  when TrueClass, FalseClass
    value ? 1 : 0
  else
    begin
      value.to_i if value.respond_to?(:to_i)
    rescue FloatDomainError
      nil
    end
  end
end

But I'm not sure if FloatDomainError is raised in other situations.

Makes sense?

@rafaelfranca
Ruby on Rails member
rafaelfranca added a note Jan 14, 2013

It can make sense, but there may be a lot of edge cases and I don't want to change this method in every edge case that can appear.

This implementation is exactly what is implemented before but without the type cast to 1 or 0 if the value is not a boolean.

I don't think is worth to change, but If you really want to change this implementation to make easier to debug, I would appreciate if you can take a look on more edge cases (Complex, Rational, etc.)

@sobrinho
sobrinho added a note Jan 14, 2013

I don't know any edge case with Rational and documentation does not mention anything:

Rational(3, 1).to_i
#=> 3

Rational(1, 10).to_i
#=> 0

The C code about Rational#to_i does not have anything obvious at first glance.

I'm not familiar with Complex, so I have no idea what it represents in math (I do not remember anything related to complex numbers from my school days, I read the documentation but still nothing).

Calling Complex#to_i didn't worked for some values:

Complex(1).to_i
#=> 1

Complex(2, 3).to_i
#=> RangeError: can't convert 2+3i into Integer

Complex.polar(2, 3).to_i
#=> RangeError: can't convert -1.9799849932008908+0.2822400161197344i into Integer

The documentation says that RangeError will be raised if is not possible to convert but I don't know what kind of value can be converted or not.

I don't have any custom to_i implementation on my systems so it's not a big deal for me, but I had bad times on rails applications to find the root cause due to rescues not specifying the exception classes.

If you say that's ok, I'm happy :)

@rafaelfranca
Ruby on Rails member
rafaelfranca added a note Jan 14, 2013
@sobrinho
sobrinho added a note Jan 14, 2013

:ok_hand:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
end
View
6 activerecord/test/cases/column_test.rb
@@ -57,6 +57,12 @@ def test_type_cast_object_without_to_i_to_integer
assert_nil column.type_cast(Object.new)
end
+ def test_type_cast_nan_and_infinity_to_integer
+ column = Column.new("field", nil, "integer")
+ assert_nil column.type_cast(Float::NAN)
+ assert_nil column.type_cast(1.0/0.0)
+ end
+
def test_type_cast_time
column = Column.new("field", nil, "time")
assert_equal nil, column.type_cast('')

0 comments on commit 807e176

Please sign in to comment.
Something went wrong with that request. Please try again.