rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
975 stars 563 forks source link

Rails test are failing on fixture loading #672

Closed juice closed 4 years ago

juice commented 5 years ago

Hello, we tried to migrate the current master into our project, but our tests are failing.

I created a tiny test project with one table and one field.

execute: rails test

https://github.com/juice/mssql-test

It looks like the fixtures cannot be inserted into the table.

I added all options from our project, like: ActiveRecord::ConnectionAdapters::SQLServerAdapter.use_output_inserted = false

But it doesn't matter, simple tests are failing anyway. It works with Rails 5.1 and previous versions of the adapter.

Here the same tests in a working branch with Rails 5.1: https://github.com/juice/mssql-test/tree/working

wpolicarpo commented 5 years ago

I couldn't reproduce the error locally even with your test project (thanks for that, btw).

Can you tell us which versions you have for SQL Server, tiny_tds and freetds?

Also, what's the compatibility level of your database?

juice commented 5 years ago

The master branch in NOT failing in your case? Interesting.

FreeTDS: 1.00.109 TinyTDS: 2.1.2 MSSQL Server 2012 (110) (11.0.6607)

juice commented 5 years ago

@wpolicarpo Btw, the adapter is working, just the tests are failing.

In Rails 5.1 the SET IDENTITY_INSERT is set to ON before a fixture insert is executed. In Rails 5.2 the execution is done with the ` Backquote.

Rails 5.1

   (0.1ms)  BEGIN
  Fixture Delete (1.6ms)  DELETE FROM [StoragePolicy]; SELECT @@ROWCOUNT AS AffectedRows
  SQL (0.6ms)  SET IDENTITY_INSERT [StoragePolicy] ON
  Fixture Insert (1.6ms)  INSERT INTO [StoragePolicy] ([sp_ID], [sp_sl_ID], [sp_c_ID], [sp_Name], [sp_Description], [sp_CopyToArchive], [sp_CopyToArchiveAfterDays], [sp_RemoveFromWorkspace], [sp_RemoveFromWorkspaceAfterDays], [sp_ShortName], [sp_IsExplicitContentPolicy]) VALUES (1, 1, 1, N'Test', N'Test Test Test', 1, 20, 1, 20, N'test', 0)
  SQL (0.5ms)  SET IDENTITY_INSERT [StoragePolicy] OFF

Rails 5.2

TypeError: can't quote ActiveRecord::ConnectionAdapters::SQLServer::Type::Data

ActiveRecord::StatementInvalid:         ActiveRecord::StatementInvalid: TinyTds::Error: Incorrect syntax near '`'.: INSERT INTO `StoragePolicy` (`sp_ID`, `sp_sl_ID`, `sp_c_ID`, `sp_Name`, `sp_Description`, `sp_CopyToArchive`, `sp_CopyToArchiveAfterDays`, `sp_RemoveFromWorkspace`, `sp_RemoveFromWorkspaceAfterDays`, `sp_ShortName`, `sp_IsExplicitContentPolicy`) VALUES (1, 1, 1, 'Test', 'Test Test Test', TRUE, 20, TRUE, 20, 'test', FALSE), (2, 1, 1, 'Test', 'Test Test Test', TRUE, 20, TRUE, 20, 'test', FALSE)

SQL (0.6ms)  BEGIN TRANSACTION
  Fixtures Load (1.0ms)  DELETE FROM [StoragePolicy]
  Fixtures Load (1.3ms)  INSERT INTO `StoragePolicy` (`sp_ID`, `sp_sl_ID`, `sp_c_ID`, `sp_Name`, `sp_Description`, `sp_CopyToArchive`, `sp_CopyToArchiveAfterDays`, `sp_RemoveFromWorkspace`, `sp_RemoveFromWorkspaceAfterDays`, `sp_ShortName`, `sp_IsExplicitContentPolicy`) VALUES (1, 1, 1, 'Test', 'Test Test Test', TRUE, 20, TRUE, 20, 'test', FALSE), (2, 1, 1, 'Test', 'Test Test Test', TRUE, 20, TRUE, 20, 'test', FALSE)
  SQL (0.7ms)  IF @@TRANCOUNT > 0 ROLLBACK TRANSACTION
   (3.2ms)  SELECT s.name, o.name
FROM sys.foreign_keys i
INNER JOIN sys.objects o ON i.parent_object_id = o.OBJECT_ID
INNER JOIN sys.schemas s ON o.schema_id = s.schema_id
wpolicarpo commented 5 years ago

I cloned your demo project and ran the tests against a 2016 (130) database and all worked fine.

Can you tell us the types for those *_ID columns there?

You can use rails itself if you want: StoragePolicy.columns_hash.

juice commented 5 years ago

Ok, the just managed to try the sample project on a 2016 Server:

MSSQL Server 2016 (130) 13.0.5201

The message is now:

Error: MyModelsControllerTest#test_should_create_my_model: TypeError: can't quote ActiveRecord::ConnectionAdapters::SQLServer::Type::Data

You are in the master branch and you executed, "rails db:migrate RAILS_ENV=test" and "rails test"?

This is my output for MyModel.columes_hash:

{"id"=>#<ActiveRecord::ConnectionAdapters::SQLServerColumn:0x00007fda4ac7adb8 @sqlserver_options={:ordinal_position=>1, :is_primary=>true, :is_identity=>true}, @name="id", @table_name="dbo.my_models", @sql_type_metadata=#<ActiveRecord::ConnectionAdapters::SQLServer::SqlTypeMetadata:0x00007fda4ac7b218 @sqlserver_options={:sqlserver_options=>{:ordinal_position=>1, :is_primary=>true, :is_identity=>true}}, @sql_type="bigint(8)", @type=:integer, @limit=8, @precision=nil, @scale=nil>, @null=false, @default=nil, @default_function=nil, @collation=nil, @comment=nil>, "name"=>#<ActiveRecord::ConnectionAdapters::SQLServerColumn:0x00007fda4ac6f080 @sqlserver_options={:ordinal_position=>2, :is_primary=>false, :is_identity=>false}, @name="name", @table_name="dbo.my_models", @sql_type_metadata=#<ActiveRecord::ConnectionAdapters::SQLServer::SqlTypeMetadata:0x00007fda4ac6f698 @sqlserver_options={:sqlserver_options=>{:ordinal_position=>2, :is_primary=>false, :is_identity=>false}}, @sql_type="nvarchar(4000)", @type=:string, @limit=4000, @precision=nil, @scale=nil>, @null=true, @default=nil, @default_function=nil, @collation="Latin1_General_CI_AS", @comment=nil>, "created_at"=>#<ActiveRecord::ConnectionAdapters::SQLServerColumn:0x00007fda4ac83a08 @sqlserver_options={:ordinal_position=>3, :is_primary=>false, :is_identity=>false}, @name="created_at", @table_name="dbo.my_models", @sql_type_metadata=#<ActiveRecord::ConnectionAdapters::SQLServer::SqlTypeMetadata:0x00007fda4ac83f58 @sqlserver_options={:sqlserver_options=>{:ordinal_position=>3, :is_primary=>false, :is_identity=>false}}, @sql_type="datetime", @type=:datetime, @limit=nil, @precision=nil, @scale=nil>, @null=false, @default=nil, @default_function=nil, @collation=nil, @comment=nil>, "updated_at"=>#<ActiveRecord::ConnectionAdapters::SQLServerColumn:0x00007fda4ac83058 @sqlserver_options={:ordinal_position=>4, :is_primary=>false, :is_identity=>false}, @name="updated_at", @table_name="dbo.my_models", @sql_type_metadata=#<ActiveRecord::ConnectionAdapters::SQLServer::SqlTypeMetadata:0x00007fda4ac83710 @sqlserver_options={:sqlserver_options=>{:ordinal_position=>4, :is_primary=>false, :is_identity=>false}}, @sql_type="datetime", @type=:datetime, @limit=nil, @precision=nil, @scale=nil>, @null=false, @default=nil, @default_function=nil, @collation=nil, @comment=nil>}

wpolicarpo commented 5 years ago

@juice why do you have both sqlite3 and activerecord-sqlserver-adapter gems on your Gemfile?

Does that work if you remove sqlite3?

juice commented 5 years ago

@wpolicarpo Is there an option to disable the backquotes syntax? "TinyTds::Error: Incorrect syntax near '`'", isn't this the problem?

We use multiple db connections, i tried to get as close as possible to the original project, thats why there is another db connection. I'm looking forward to Rails 6.0 multiple Database connection feature.

What has changed between rails 5.1 and rails 5.2 in the sqlserver adapter so that tests are now failing?

wpolicarpo commented 5 years ago

@juice first of all, I don't think Rails supports (or will support) the presence of multiple adapters within the same application even with Rails 6 multiple db connection feature, but I might be wrong.

Your tests are failing because Rails changed the way fixtures are loaded with the addition of Arel::InsertManager, which fallbacks to the connection attached to ActiveRecord::Base if none is passed as seen here and here. And because your main database connection is SQLite, InsertManager is trying to build the SQL to insert fixtures using that adapter instead of SQL Server's one.

If the SQL Server adapter was the main connection, this problem would not be happening, but I bet you would have more issues other than these.

Given that, I would suggest:

  1. Check with Rails team if what you want to do is possible (having multiple adapters)
  2. If so, open an issue there explaining what's happening so they can fix the issue (I might help with a PR if they decide it is a bug)
juice commented 5 years ago

@wpolicarpo I have a backtrace here, i hope this helps and thanks for the links to the source. It would be great to fix the broken Fixture code in this Adapter. Everything works together as expected and with Rails 6.0 it will finally backed into the Rails Core. So setting up multiple DB will be a bit easier.

The Adapter is playing nicely with other adapters, but as you mentioned the Problem is in the "insert_fixtures_set" method. After calling "insert_fixtures_set" from the activerecord-sqlserver-adapter, it is falling back to the active_record method "build_fixture_sql". And at this point the data from the SQLServer is mishandled.

Understandably, "TypeError: can't quote ActiveRecord::ConnectionAdapters::SQLServer::Type::Data" will be raised.

BTW, our original Setup is not, sqlite3, it is MySQL in some projects and PostgreSQL in other projects.

TypeError: can't quote ActiveRecord::ConnectionAdapters::SQLServer::Type::Data
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/connection_adapters/abstract/quoting.rb:179:in `_quote'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/connection_adapters/abstract/quoting.rb:18:in `quote'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:799:in `quote'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:181:in `block (2 levels) in visit_Arel_Nodes_ValuesList'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:176:in `each'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:176:in `each_with_index'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:176:in `block in visit_Arel_Nodes_ValuesList'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:173:in `each'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:173:in `each_with_index'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:173:in `visit_Arel_Nodes_ValuesList'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/reduce.rb:15:in `visit'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:815:in `maybe_visit'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/to_sql.rb:134:in `visit_Arel_Nodes_InsertStatement'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/reduce.rb:15:in `visit'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/visitors/reduce.rb:8:in `accept'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/connection_adapters/determine_if_preparable_visitor.rb:10:in `accept'
 .rvm/gems/ruby-2.6.0/gems/arel-9.0.0/lib/arel/tree_manager.rb:22:in `to_sql'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/connection_adapters/abstract/database_statements.rb:454:in `build_fixture_sql'
 .rvm/gems/ruby-2.6.0/bundler/gems/activerecord-sqlserver-adapter-0dd5e98dadc0/lib/active_record/connection_adapters/sqlserver/database_statements.rb:88:in `block (2 levels) in insert_fixtures_set'
 .rvm/gems/ruby-2.6.0/bundler/gems/activerecord-sqlserver-adapter-0dd5e98dadc0/lib/active_record/connection_adapters/sqlserver/database_statements.rb:87:in `each'
 .rvm/gems/ruby-2.6.0/bundler/gems/activerecord-sqlserver-adapter-0dd5e98dadc0/lib/active_record/connection_adapters/sqlserver/database_statements.rb:87:in `each_slice'
 .rvm/gems/ruby-2.6.0/bundler/gems/activerecord-sqlserver-adapter-0dd5e98dadc0/lib/active_record/connection_adapters/sqlserver/database_statements.rb:87:in `block in insert_fixtures_set'
 .rvm/gems/ruby-2.6.0/bundler/gems/activerecord-sqlserver-adapter-0dd5e98dadc0/lib/active_record/connection_adapters/sqlserver/database_statements.rb:86:in `each'
 .rvm/gems/ruby-2.6.0/bundler/gems/activerecord-sqlserver-adapter-0dd5e98dadc0/lib/active_record/connection_adapters/sqlserver/database_statements.rb:86:in `insert_fixtures_set'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/fixtures.rb:566:in `block in create_fixtures'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/fixtures.rb:558:in `each'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/fixtures.rb:558:in `create_fixtures'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/fixtures.rb:1031:in `load_fixtures'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/fixtures.rb:968:in `setup_fixtures'
 .rvm/gems/ruby-2.6.0/gems/activerecord-5.2.2/lib/active_record/fixtures.rb:860:in `before_setup'
 .rvm/gems/ruby-2.6.0/gems/activesupport-5.2.2/lib/active_support/testing/setup_and_teardown.rb:40:in `before_setup'
 .rvm/gems/ruby-2.6.0/gems/actionpack-5.2.2/lib/action_dispatch/testing/integration.rb:314:in `before_setup'
 .rvm/gems/ruby-2.6.0/gems/railties-5.2.2/lib/rails/test_help.rb:46:in `before_setup'
wpolicarpo commented 5 years ago

@juice I don't think the issue is in the adapter as I explained before. Your setup is very peculiar and I don't know if Rails would support it out of the box (even Rails 6).

I made a few tests (manually monkey patching ActiveRecord) and managed to make the tests pass, but it has nothing to do with the adapter itself. I understand that it was working on Rails 5.1, but it does not guarantee it was expected or by design.

I'm happy to help with a PR to Rails, but I need to be sure this is intentionally supported by them. Can you open an issue there linking back this thread and we can discuss with them what needs to be done?

metaskills commented 5 years ago

FWIW, I am a little surprised Rails does not find the right constant. It could that 5.2 has a regression. Maybe check rails/rails to be sure but I would expect it to find your right model constant. A few thoughts:

HTH

juice commented 5 years ago

@metaskills Thanks for your help. Sadly, both tips didn't change the behavior.

juice commented 5 years ago

@wpolicarpo @metaskills The next major version Rails 6.0 is now in beta. So we are counting weeks here. I'm sure you guys are working already on a compatible version for Rails 6.0.

Right now the MSSQL Adapter is in a broken state, loading fixtures in tests with multi adapters is not working.

As highlighted by the Rails Maintainers, one of the biggest points on the Roadmap is multi database connection and easier multi database adapter support.

https://github.com/rails/rails/pull/34052

Is there a way to make sure that the original behavior is working again by adding a test?

wpolicarpo commented 4 years ago

@juice version 6.0.0.rc1 is out.

Would you be able to test it and let us know if the problem still persists?

juice commented 4 years ago

@wpolicarpo I just did an upgrade to Rails 6.0 and the tests are now working as intended.

Thanks to everyone who helped with the Rails 6.0 PRs.