rails-sqlserver / activerecord-sqlserver-adapter

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

Bug in column type definition: TINYINT() should be TINYINT #157

Closed pivotal-tabernash closed 12 years ago

pivotal-tabernash commented 12 years ago

Hi,

updating to 3.1.5 from 3.0.18 our code started breaking with tinyint columns causing invalid syntax:

EXEC sp_executesql N'INSERT INTO [MyTable] ([Column1]) VALUES (@0); SELECT CAST(SCOPE_IDENTITY() AS bigint) AS Ident', N'@0 tinyint()', @0 = 2

should be

EXEC sp_executesql N'INSERT INTO [MyTable] ([Column1]) VALUES (@0); SELECT CAST(SCOPE_IDENTITY() AS bigint) AS Ident', N'@0 tinyint', @0 = 2

We have a potential fix for this issue, but do not currently have the dev setup to run the tests.

Our fix is here: https://github.com/pivotal-tabernash/activerecord-sqlserver-adapter/commit/6be061a748b45192534065a790ee70388a82fd48

-- David and Stephan

metaskills commented 12 years ago

I need to understand what the problem is and how to reproduce with a test. I also have a problem since the current tests for tinyint pass [1] and [2] and your fix is in the wrong spot. Please look at these existing tests.

[1] https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/test/cases/specific_schema_test_sqlserver.rb#L117

[2] https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/test/cases/column_test_sqlserver.rb#L323

Now, first thing is to find out why when I run the tests on both 2005, 2008, I do not see what you see. Case in point, if I use some code from one of those tests above and put it in the ScratchTestSqlserver file like so.

require 'cases/sqlserver_helper'
class ScratchTestSqlserver < ActiveRecord::TestCase
  should 'pass' do
    t1 = SqlServerEdgeSchema.create! :tinyint => 1
    t255 = SqlServerEdgeSchema.create! :tinyint => 255
  end
end

Then run it, I get the following in my log. Which is just fine.

SQL (1.2ms)  EXEC sp_executesql N'INSERT INTO [sql_server_edge_schemas] ([bigint], [description], [guid], [guid_newid], [guid_newseqid], [tinyint]) VALUES (@0, @1, @2, @3, @4, @5); SELECT CAST(SCOPE_IDENTITY() AS bigint) AS Ident', N'@0 bigint, @1 nvarchar(255), @2 uniqueidentifier, @3 uniqueidentifier, @4 uniqueidentifier, @5 tinyint', @0 = NULL, @1 = NULL, @2 = NULL, @3 = NULL, @4 = NULL, @5 = 1  [["bigint", nil], ["description", nil], ["guid", nil], ["guid_newid", nil], ["guid_newseqid", nil], ["tinyint", 1]]

SQL (1.4ms)  EXEC sp_executesql N'INSERT INTO [sql_server_edge_schemas] ([bigint], [description], [guid], [guid_newid], [guid_newseqid], [tinyint]) VALUES (@0, @1, @2, @3, @4, @5); SELECT CAST(SCOPE_IDENTITY() AS bigint) AS Ident', N'@0 bigint, @1 nvarchar(255), @2 uniqueidentifier, @3 uniqueidentifier, @4 uniqueidentifier, @5 tinyint', @0 = NULL, @1 = NULL, @2 = NULL, @3 = NULL, @4 = NULL, @5 = 255  [["bigint", nil], ["description", nil], ["guid", nil], ["guid_newid", nil], ["guid_newseqid", nil], ["tinyint", 255]]

So after looking thru that, it is easy to see what is potentially happening. Your sql_type from the server is coming back as tinyint(). To check I am right on that, in the console type this MyModel.columns_hash['Column1'].sql_tpe and show me what it outputs. Please use the name of your model and column tho please. Per the tests above you supposed to get back tinyint(1). The core of this issue is why not? I would like to understand that before proposing a fix. To that end, it may help to know if the tests pass for you too. The setup for testing has never been more easy. Just create a DB, assign a proper user/role. Set a few ENV vars and run a rake command. It is all outline in the RUNNING_UNIT_TEST.md file. Please make an effort.

Once we go from there, a possible fix could be from changing this method or something else.

def sql_type_for_statement
  if is_integer? || is_real?
    sql_type.sub(/\(\d+\)/,'')
  else
    sql_type
  end
end

But we need to understand the issue and put in a test so regressions will not happen. I'd like to understand why your tinyint comes back without a number in it. Then the real fix could be patching the regexp above.

pivotal-tabernash commented 12 years ago

Your assumption about the column types was correct: we get tinyint() as a column type.

We come across this issue working with a client database in which we have little capabilities and which we did not create from our code. No idea how to create such a situation from ruby code.

metaskills commented 12 years ago

Well, is there anything that you can tell me about it. Like is it a view, etc. You must be able to get some client DBA to help you get some answers from a DB perspective?

pivotal-tabernash commented 12 years ago

The source of this column is not a view, it's a table where the column in question is the primary key. Here is a screenshot of the column's properties in SQLServer:

https://docs.google.com/drawings/d/1jdAFIwIfRLDEDLTNM3U8MvSjPV_38W3rveVXNE2irw0/edit

metaskills commented 12 years ago

That commit made a tinyint column as a primary key which properties were identical to your screen shot and work just find. Perhaps this bug is only in the 3-1-stable behavior, but I can confirm it works just fine.

Altonymous commented 12 years ago

I'm seeing something similar in my setup... It's on bigint instead of tinyint

SQL Server 2008 FreeTDS, 0.9.1 TinyTDS, 0.5.1 Rails, 3.1.3 activerecord-sqserver-adapter, 3.1.6

If I understand what I've read correctly, in order to resolve this issue, I'll need to upgrade rails to get the latest version of this gem?

metaskills commented 12 years ago

Please comment in #175, this issue is closed.