Skip to content

Conversation

@dglaser
Copy link

@dglaser dglaser commented Nov 15, 2019

Included #710 changes from @serkaniyigun
Since rails 6.0 has been released, I changed the gem depency from rails ~>6.0 beta3 to ~> 6.0

Tests are failing:
TinyTds::Error: Cannot insert explicit value for identity column in table [tasks] when IDENTITY_INSERT is set to OFF

This could be as simple as setting IDENTITY_INSERT to ON/OFF. Doesn't help with activerecord tests that fail.
before do
connection.execute("SET IDENTITY_INSERT on [table] ON;")
...
after do
connection.execute("SET IDENTITY_INSERT [table] OFF;")
....

@dglaser dglaser requested a review from wpolicarpo February 3, 2020 19:35
@dglaser dglaser mentioned this pull request Feb 4, 2020
@ttilberg
Copy link

ttilberg commented Feb 18, 2020

While digging backwards for the identity issue, I'm finding that one potentially related issue is that ActiveRecord::ConnectionAdapters::SQLServerColumn doesn't seem to correctly report whether it is an identity column. I could see this kind of problem causing all sorts of interesting issues when you run the test suite.

=> #<ActiveRecord::ConnectionAdapters::SQLServerColumn:0x000055ae6b885ee0
 @collation=nil,
 @comment=nil,
 @default=nil,
 @default_function=nil,
 @name="id",
 @null=false,
 @sql_type_metadata=
  #<ActiveRecord::ConnectionAdapters::SQLServer::SqlTypeMetadata:0x000055ae6b886070
   @limit=8,
   @precision=nil,
   @scale=nil,
   @sql_type="bigint(8)",
   @sqlserver_options=
    {:sqlserver_options=>
      {:ordinal_position=>1, :is_primary=>true, :is_identity=>true}},
   @type=:integer>,
 @sqlserver_options={}>

Notice specifically that @sqlserver_options is an empty hash, but @sql_type_metadata knows it's an identity column deeper down. The check for identity_required? uses that @sqlserver_options ivar.

Looking at the file lib/active_record/connection_adapters/sqlserver_column.rb -- the initializer is explicitly setting @sqlserver_options to an empty hash, and seemingly never doing anything with it, however these accessors are relying on the data it's supposed to entail.

We can see this was recently introduced at dglaser@b4f558e to handle kwargs, but was never assigned in the body.

I submitted a simple PR to the branch you are attempting to merge.

@ttilberg
Copy link

In the test run I'm working on, I'm seeing a ton of errors related to

NoMethodError: undefined method `split' for nil:NilClass

Upon digging into the stacktraces, this seems to be getting caused by something quirky in Minitest, not necessarily the AR adapter. I left some information here: minitest/minitest#831

@dglaser
Copy link
Author

dglaser commented Feb 19, 2020

The PR you submitted is close to reverting the code I included from PR #710. I feel it would be cleaner to close this PR.

@ttilberg
Copy link

Yes, I think it might be a better starting place if the rest works as expected.

@dglaser dglaser closed this Feb 19, 2020
@dglaser dglaser deleted the 6-0-dev branch February 20, 2020 01:34
@dglaser dglaser mentioned this pull request Feb 25, 2020
@utkarsh2102
Copy link

@ttilberg, you might be interested in this: minitest/minitest#831 (comment) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants