rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
972 stars 558 forks source link

Saving a float goes to the DB as int #643

Closed erlingur closed 1 year ago

erlingur commented 6 years ago

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

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

erlingur commented 6 years ago

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.

metaskills commented 6 years ago

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.

erlingur commented 6 years ago

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.

metaskills commented 6 years ago

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.

erlingur commented 6 years ago

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 commented 6 years ago

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?

ab320012 commented 6 years ago

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

erlingur commented 6 years ago

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

metaskills commented 6 years ago

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.

ab320012 commented 6 years ago

@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

erlingur commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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?

ab320012 commented 6 years ago

@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

erlingur commented 6 years ago

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.

wpolicarpo commented 4 years ago

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

erlingur commented 1 year ago

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