Fix error raised when assigning NaN to an integer column #8781

Merged
merged 1 commit into from Jan 6, 2013

Projects

None yet

3 participants

@trisweb
Contributor
trisweb commented Jan 6, 2013

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

Those are the only ones I know of, but it's possible other objects could raise errors on to_i, which is the reason for the generic rescue. (Note that this generic rescue was also the behavior pre-3.2.8, except that the value was interpreted as a boolean instead—fixed in 96a13fc—after this pull it will be more correctly and consistently interpreted as nil).

Fixes #8757 (see that issue for full description)

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Jan 6, 2013
activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Fix FloatDomainError when setting integer column to NaN, or anything that
+ responds to to_i but raises an exception on calling it. Fixes #8757.
@rafaelfranca
rafaelfranca Jan 6, 2013 Member

I'd amend the issue number and you name in the changelog entry for the previous fix, since it was not released yet.

@trisweb
trisweb Jan 6, 2013 Contributor

Sounds good, excellent. I'll amend the commit.

@rafaelfranca
Member

Very good. Just a minor comment.

@trisweb trisweb 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
807e176
@trisweb
Contributor
trisweb commented Jan 6, 2013

All set. @rafaelfranca thanks for the review and advice on the little issue.

@rafaelfranca rafaelfranca merged commit 9ee65aa into rails:master Jan 6, 2013
@rafaelfranca
Member

@trisweb Thank YOU for the report and fix.

@rafaelfranca rafaelfranca added a commit that referenced this pull request Jan 6, 2013
@trisweb @rafaelfranca trisweb + rafaelfranca 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

Backport of #8781

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/test/cases/column_test.rb
c147dd7
@trisweb trisweb deleted the trisweb:tristan_8757_integer_column_nan_error_fix branch Jan 7, 2013
@sobrinho

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?

Member

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.)

Contributor

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 :)

Contributor

👌

@ChrisHooks ChrisHooks pushed a commit to econsultancy/rails that referenced this pull request Apr 10, 2013
@trisweb trisweb + Chris Hooks 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

Backport of #8781

Cherry Pick of c147dd7
83c3475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment