Setting integer column to NaN throws FloatDomainError (NaN) #8757

Closed
trisweb opened this Issue Jan 4, 2013 · 2 comments

Comments

Projects
None yet
2 participants
Contributor

trisweb commented Jan 4, 2013

This has been a change since 3.2.8 introduced by 96a13fc — in the same vein as, and almost fixed by 3525a9b (pulls #8718 / #8734)

(As a side note, prior to 96a13fc, there was a similar issue as pointed out in #8718: NaN is truthy, so it was being interpreted as 1 in integer columns. This was also scarily incorrect, but was addressed when this issue began)

The issue now (after 96a13fc, and still existing after 3525a9b) is that attepting to set an integer column = NaN throws FloatDomainError (NaN)

Can be easily reproduced by:

record.integer_column_name = Float::NAN

I encounter the issue when using RubyAMF to receive uninitialized records from a Flex app. NaN is the default value for uninitialized Numbers in Flex/Flash, so it's very common in that case. The error is raised when RubyAMF internals set the attributes hash containing NaN values for integer columns.

Reported over at rubyamf/rubyamf#17 also to bring the issue to their attention with more detail on the rubyamf exception there.

I think it's proper for Rails to handle this value cleanly. It was almost fixed in 3525a9b since respond_to?(:to_i) is checked, but unfortunately NaN does respond to to_i, so the issue still occurs.

I think the fix is to check for .nan? in value_to_integer, and if nan? return nil instead, or rescue the error and return nil. So Column.value_to_integer might use value.to_i rescue nil instead of just value.to_i

However, this has the consequence of treating Infinity and -Infinity as nil, which may or may not be desired. Should probably decide how we want those treated and make value_to_integer more robust with expected inputs.

Posting issue for guidance from @rafaelfranca and/or @jstirk before continuing since they have more experience with this code, but I am happy to make a pull!

@trisweb trisweb added a commit to trisweb/rubyamf that referenced this issue Jan 4, 2013

@trisweb trisweb Hotfix for Rails bug with NaN integer column values
See rails/rails#8757 - After the 'rescue' truthiness check was removed from integer column casting in 3.2.9 (which itself was a bug causing incorrect default values from Flash NaNs), this bug appeared raising an exception when NaN.to_i is called.

The temporary fix is to convert all NaN to nil which is correctly interpreted by Rails. This is what they end up being anyway, since the database adapter will convert NaN to NULL in all cases I know of.
101f881
Owner

rafaelfranca commented Jan 4, 2013

Well. I think the best would remove the check from to_i, call to_i directly and recue with nil. It is ugly but it is best that check for all the possible cases that can raise a exception.

Contributor

trisweb commented Jan 6, 2013

Agreed, thanks. I'll open a pull request.

@rafaelfranca rafaelfranca added a commit that referenced this issue 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

@ChrisHooks ChrisHooks pushed a commit to econsultancy/rails that referenced this issue 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