Incorrect infinity and NaN conversion to Ruby values in postgresql adapter #13334

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@gregolsen
Contributor

Active Record successfully stores Ruby Infinity value in a Postgresql DB column of double precision type.
However it fails to convert it back to Ruby Float::INFINITY when the record is fetched from DB.
The issue can be illustrated with the code below:
While the value is correctly stored as 'Infinity' when it is accessed through AR instance - it's converted to 0.

It looks like the problem is in the type cast which is simply to_f for the float values.
Thus - in case 'Infinity' or 'NaN' in database it converts it to 0.

update - all below is fixed
The naive fix in this PR is rather to start the discussion and clarify is this really an issue or I'm just missing something.
For example after applying this fix Point.create starts throwing an exception Infinity (FloatDomainError) - that's because OID::Float#type_cast is called with Float::INFINITY as an argument.

require 'active_record'
require 'pg'

ActiveRecord::Base.establish_connection(
  database: 'infinity',
  adapter: 'postgresql',
  username: 'postgres'
)

ActiveRecord::Base.connection.instance_eval do
  create_table :points, force: true do |t|
    t.column :value, :float
  end
end

class Point < ActiveRecord::Base; end
infinity = 1.0/0
Point.create(value: infinity)
point = Point.first
p Point.connection.execute("select value from points limit 1").to_a #=> [{"value"=>"Infinity"}]
p point.read_attribute_before_type_cast('value') #=> "Infinity"
p point.value #=> 0.0 when it should be Float::INFINITY
@pftg
Contributor
pftg commented Dec 16, 2013

Please add changelog entry into this PR.

@egilburg egilburg commented on an outdated diff Dec 16, 2013
...b/active_record/connection_adapters/postgresql/oid.rb
@@ -211,8 +211,13 @@ def type_cast(value)
class Float < Type
def type_cast(value)
return if value.nil?
-
- value.to_f
+ case value
+ when 'Infinity' then ::Float::INFINITY
@egilburg
egilburg Dec 16, 2013 Contributor

This is likely a hotspot area. Perhaps extract these strings into constants to avoid extra object allocation.

@gregolsen
Contributor

Changelog entry added, commits squashed

@senny
Member
senny commented Mar 31, 2014

This looks reasonable. Can you push a rebased version?

@gregolsen
Contributor

@senny I've rebased this PR and fixed the specs. However one spec MemCacheStoreTest#test_deserializes_unloaded_class has failed due to memcache server timeout which has nothing to do with the changes introduced by this PR I hope :-)

@senny
Member
senny commented Apr 2, 2014

@gregolsen yea that happens from time to time, will restart the job for you.

@senny senny commented on the diff Apr 2, 2014
...b/active_record/connection_adapters/postgresql/oid.rb
@@ -255,8 +255,15 @@ def type; :float end
def type_cast(value)
return if value.nil?
+ return value unless ::String === value
@senny
senny Apr 2, 2014 Member

what call prompted this guard? Do we have a test to make sure we don't get regressions?

@gregolsen
gregolsen Apr 2, 2014 Contributor

Tests were failing previously but now it looks fine without this guard.

@senny senny commented on an outdated diff Apr 2, 2014
activerecord/CHANGELOG.md
@@ -1,3 +1,18 @@
+* Fix `PostgreSQLAdapter::OID::Float#type_cast` to convert Infinity and
+ NaN PostgreSQL values into a native Ruby `Float::INFINITY` and `Float::NAN`
+
+ Example:
+
+ # Before
+ Point.create(value: 1.0/0)
+ p Point.last.value #=> 0.0
@senny
senny Apr 2, 2014 Member

no need to have p in front of the expression. The comment # => signals that we are talking about the evaluated value of the expression on the left. Nitpick: can you include a space after # so that it looks like # =>?

@senny senny commented on an outdated diff Apr 2, 2014
activerecord/CHANGELOG.md
@@ -1,3 +1,18 @@
+* Fix `PostgreSQLAdapter::OID::Float#type_cast` to convert Infinity and
+ NaN PostgreSQL values into a native Ruby `Float::INFINITY` and `Float::NAN`
+
+ Example:
+
+ # Before
+ Point.create(value: 1.0/0)
+ p Point.last.value #=> 0.0
+
+ # After
+ Point.create(value: 1.0/0)
+ p Point.last.value #=> Infinity
@senny
senny Apr 2, 2014 Member

same as above

@senny senny commented on the diff Apr 2, 2014
...b/active_record/connection_adapters/postgresql/oid.rb
- value.to_f
+ case value
+ when 'Infinity' then ::Float::INFINITY
+ when '-Infinity' then -::Float::INFINITY
+ when 'NaN' then ::Float::NAN
+ else
+ value.to_f
+ end
@senny
senny Apr 2, 2014 Member

talked this over with @jeremy . He suggested to create a type map to perform the conversion:

TYPE_CASTS.fetch(value) { value.to_f };  TYPE_CASTS = { nil => nil, ‘Infinity’ => Float::INFINITY, … }
@senny
senny Apr 2, 2014 Member

@gregolsen after further discussion we think folding the guard into the case statement is better looking than the type map. Please strike my previous comment and include the guard in the case. Sorry for the roundtrip 😊

@gregolsen
gregolsen Apr 2, 2014 Contributor

no problem 😃

@senny senny closed this in 4320b77 May 12, 2014
@senny
Member
senny commented May 12, 2014

@gregolsen Thanks 💛 . I applied a rebased and slightly modified version of the patch.

@gregolsen
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment