search

rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
968 stars 557 forks source link

Saving "Infinity" in a Float column #651

Open neongrau opened 6 years ago

neongrau commented 6 years ago

Noticed that when i tried to divide a string number which i converted to_f and then used as a divider, when the float is 0.0 ruby returns Infinity instead of a divison by zero erro. Which then the Adapter tries to persist like this:

e.g. 

value = "0"
record.some_float = 1000 / value.to_f
ActiveRecord::StatementInvalid: TinyTds::Error: Error converting data type nvarchar to float.
...
@0 = Infinity

Is this correct/intended behavior of the adapter?

This sure was only a bug in my apps code but i wondered why this ended up in the SQL.

metaskills commented 6 years ago

Interesting, what version of the adapter/rails? Also, can you tell me more about the schema (column type) under the hood for some_float?

neongrau commented 6 years ago

just a plain FLOAT(8) that came from a migration what was simply created by t.float :some_float

i googled a bit and it seems that while other DBs support NaN, INF and -INF like the IEEE standard describes. See https://www.codeproject.com/Tips/50340/MSSQL-doesn-t-completely-support-IEEE-floating

So i guess nothing reasonable could be done on the adapter part. Probably best to see the error like it now happens.

neongrau commented 6 years ago

and my adapter version is the current 5.1.6

metaskills commented 6 years ago

I do think this is an adapter but tho and we can improve the logic to rase an error when assigning. Here is our Float type.

https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/5-1-stable/lib/active_record/connection_adapters/sqlserver/type/float.rb

And here is the one we inherit from in ActiveModel.

https://github.com/rails/rails/blob/5-1-stable/activemodel/lib/active_model/type/float.rb

I think we should handle the Float::INFINITY differently than the base model and I'd be surprised if we were not the first adapter to do so.

thomasfedb commented 5 years ago

Might be best for the Type::Float to error out somehow before the statement is executed.