activerecord-sqlserver-adapter icon indicating copy to clipboard operation
activerecord-sqlserver-adapter copied to clipboard

Saving a float goes to the DB as int

Open erlingur opened this issue 7 years ago • 16 comments

I've got a model called Supply with a column called average_weight defined in a legacy database as numeric(38,2).

class Supply < ApplicationRecord
  attribute :average_weight, :float, default: 1
end

When I try to save a value as float into the table it gets rounded.

2.3.6 :001 > s = Supply.find(1107741)
2.3.6 :002 > s.average_weight
 => 253.0
2.3.6 :003 > s.average_weight = 253.67
 => 253.67
2.3.6 :004 > s.save
  SQL (2.9ms)  BEGIN TRANSACTION
  SQL (3.4ms)  EXEC sp_executesql N'UPDATE [supplies] SET [average_weight] = @0 WHERE [supplies].[id] = @1; SELECT @@ROWCOUNT AS AffectedRows', N'@0 int, @1 int', @0 = 253.67, @1 = 1107741  [["average_weight", nil], ["id", nil]]
  SQL (4.2ms)  COMMIT TRANSACTION
2.3.6 :005 > s2 = Supply.find(1107741)
2.3.6 :006 > s2.average_weight
 => 253.0

Here is the column definition as given by ActiveRecord::Base.connection.columns("supplies")

#<ActiveRecord::ConnectionAdapters::SQLServerColumn:0x00007fd7a7e53ce8 @sqlserver_options={:ordinal_position=>5, :is_primary=>false, :is_identity=>false}, @name="average_weight", @table_name="Supply", @sql_type_metadata=#<ActiveRecord::ConnectionAdapters::SQLServer::SqlTypeMetadata:0x00007fd7a7e53ea0 @sqlserver_options={:sqlserver_options=>{:ordinal_position=>5, :is_primary=>false, :is_identity=>false}}, @sql_type="numeric(38,2)", @type=:decimal, @limit=nil, @precision=38, @scale=2>, @null=true, @default=nil, @default_function=nil, @collation=nil, @comment=nil>

The table is actually just a view that does really nothing but rename columns and I write straight into it.

I'm running

  • Rails 5.1.4
  • tiny_tds 2.1.1
  • ActiveRecord SQL Server adapter version 5.1.6
  • SQL Server v12.0.2480.0 running on Windows Server 2012 R2
  • connecting from a Mac with Ruby 2.3.6p384 and FreeTDS 1.00.9

Any ideas? I'm sure it's something on my end. Maybe a configuration issue or something to update?

erlingur avatar Mar 13 '18 15:03 erlingur

So I figured out that if I remove the attribute :average_weight, :float, default: 1 line from the supply model it works correctly. So this is maybe a Rails 5 attributes API problem rather than SQLServer adapter problem. I'll look at this further and hopefully figure out where the problem lies.

erlingur avatar Mar 14 '18 10:03 erlingur

Thanks for posting. I've never tried to use the attribute API to re-type a column. In this case, guessing that you wanted to force it to a Float vs a BigDecimal which is the type TinyTDS casts too for that column type.

metaskills avatar Mar 14 '18 11:03 metaskills

Yeah. The interesting thing is that if I change the attribute line from float to decimal, i.e. attribute :average_weight, :decimal, default: 1 I get a tiny_tds errror:

TinyTds::Error:
  Error converting data type nvarchar to int.
  /Users/eth/.rvm/gems/ruby-2.3.6@rsf/gems/activerecord-sqlserver-adapter-5.1.6/lib/active_record/connection_adapters/sqlserver/database_statements.rb:368:in `each'

And looking at the insert statement I see @0 int and then the value is @0 = N'254.67'

So I'm not sure what's going on here. But there is definitely something not playing very nicely with the Attributes API, at least in my environment.

erlingur avatar Mar 14 '18 11:03 erlingur

Could be more related to the view and perhaps the schema reflection for that table. But I dont see that in the column info you posted. It looked fine.

metaskills avatar Mar 14 '18 11:03 metaskills

I wonder why when I have a decimal attribute it get's converted to an nvarchar. Surely it shouldn't do that, right?

I think I have to just attach a debugger and step through this and find out what's going on.

erlingur avatar Mar 14 '18 11:03 erlingur

So when I have the line attribute :average_weight, :decimal in the model, the type of the attribute is set as ActiveModel::Type::Decimal which does not respond to the sqlserver_type method and instead of giving decimal(38,2) it gets set as int.

Removing the line, the type of the attribute is set as ActiveRecord::ConnectionAdapters::SQLServer::Type::Decimal and behaves as expected since it responds to sqlserver_type.

Any ideas?

erlingur avatar Mar 14 '18 14:03 erlingur

Hey i am not sure this is a bug, check out the docs for attribute: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/attributes.rb#L15-L18

it says it will overwrite the type. It looks like attribute :decimal is setting your atribute to ActiveModel::Type::Decimal which isn't being patched by

ActiveRecord::ConnectionAdapters::SQLServer::Type::Decimal, only ActiveRecord::Type::Decimal is ie here: https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/lib/active_record/connection_adapters/sqlserver/type.rb

ab320012 avatar Mar 14 '18 15:03 ab320012

Yeah, looks like you're right. So I guess the attributes API is incompatible with SQL server adapter?

erlingur avatar Mar 14 '18 15:03 erlingur

Oh... no, I would not say that. I would say that if you want to use the attribute API, you need to use our types vs the standard symbols that would go to the ActiveModel types. Doesn't the attribute API allow you to define the type via a class const? If so, can you use ours? I put them under ActiveRecord:: Type::SQLServer namespace.

metaskills avatar Mar 14 '18 15:03 metaskills

@metaskills, @erlingur I am not 100% sure if this will work or if there is an issue i am not seeing but what about registering types the way other adapters and AR do let me know what you think.

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/type.rb#L66-L77 https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L834-L852

ab320012 avatar Mar 14 '18 16:03 ab320012

I've created an initializer rails_sqlserver_types.rb that does:

ActiveRecord::Type.register(:sqls_decimal, ActiveRecord::Type::SQLServer::Decimal)
ActiveRecord::Type.register(:sqls_float, ActiveRecord::Type::SQLServer::Float

(I've not given the names of the types any thought, I'll think of better ones :))

Then, in the model I can do:

class Supply < ApplicationRecord
  attribute :average_weight, :sqls_float, default: 1.0
end

And this works! Decimal still gave me a weird result (it rounded the value), looking at it right now but this is a step in the right direction.

erlingur avatar Mar 14 '18 16:03 erlingur

If I use Decimal both precision and scale are nil here:

https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/ed8b1700e4404a529b59f55d6bd76397ee9df737/lib/active_record/connection_adapters/sqlserver/type/decimal.rb#L9

erlingur avatar Mar 14 '18 16:03 erlingur

@ab320012 I added

private
  ActiveRecord::Type.register(:decimal, ActiveRecord::Type::SQLServer::Decimal, adapter: :sqlserver)

To the bottom of sqlserver_adapter.rb like the Postgres adapter does and it works. Haven't run any tests or anything but it seems to be functional at least.

Maybe @metaskills could weigh in if this is something that could be included?

erlingur avatar Mar 14 '18 16:03 erlingur

@metaskills It may make sense to register all the types in the library here, let me know if neccessary I can send another PR https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/lib/active_record/connection_adapters/sqlserver/type.rb#L47

ab320012 avatar Mar 14 '18 16:03 ab320012

Any thoughts on this issue @metaskills? I could also send in a PR with this if you are interested? Would be nice to be compatible with the new attributes API so others don't have this issue as I did.

It's easily fixed by using an initializer but it's not really something I expected I would have to do and could trip others up as well.

erlingur avatar Mar 29 '18 15:03 erlingur

Hi, I see no problems in having the types registered like that. Anyone willing to open a PR?

wpolicarpo avatar May 15 '20 19:05 wpolicarpo

I think this is resolved, I don't see this anymore using Rails 7 at least.

erlingur avatar Dec 21 '22 14:12 erlingur