Primary Key is only pulling identity field, not actually primary key

[kenglishhi]

I found a bug in the primary key detection code. It appears this
code is only looking at for a column that is an identiy. However, some
of my tables have primary keys which are not also identity.
I forked the repo and made my patch here:
http://github.com/kenglishhi/activerecord-sqlserver-adapter/

Kevin

[metaskills]

OK a few things will need to be done before we can act on this.

1. Follow the code style for the patch, no comment, line space between methods, 2 space indentation, etc.

2. Please create a failing test that this patch fixes. Patches without a test are not likely to get added.

3. Make sure you run the unit tests – which you would do anyway if you added a new regression test for this patch. There is a file called RUNNING_UNIT_TESTS that covers the break down. If you need any help, let us know.

[metaskills]

Can you resubmit?

[bosko]

Actually proposed patch will not detect primary column since 'primary' attribute for column is not correctly set. Better way of detecting primary key for a table is by using following query:

SELECT A.TABLE_NAME, A.CONSTRAINT_NAME, B.COLUMN_NAME
FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS A, INFORMATION_SCHEMA.CONSTRAINT_COLUMN_USAGE B
WHERE CONSTRAINT_TYPE = 'PRIMARY KEY' AND A.CONSTRAINT_NAME = B.CONSTRAINT_NAME AND A.TABLE_NAME='<table_name>';

This will return column name which is set as primary key.

[metaskills]

Can you submit a patch, preferably one with a test in it? Also, we have things like SCHEMA_NAME() in our key methods for database statements now too. They were done in these commits.

05dda4f
9d5295b

Even if you can not run the tests easily, and just write the code, I can QA and fine tune if needed. Just prefer to get more details on this to help me with time constraints. If you do this within the next week. I can make sure it get's into the 3.1 release which is fast approaching!

[bosko]

Please look at the https://github.com/bosko/activerecord-sqlserver-adapter/commit/ab7a6ca7574881afdb7cc94dd6f8871d92acc287. I couldn't make tests since I have problems running existing ones. Can you review this patch and if you think it is acceptable I will try to make tests as soon as I manage to run existing ones first.

[metaskills]

A few thoughts.

1. Can you give me a simple table fixture to test against in sqlserver_specific_schema.rb? Maybe a basic table with a natural primary key.

2. Ideally we want to get some sort of :primary attribute back from the sql issued in the #column_definitions method. We do not want to do another SQL call if we can get it in one go. So can #fetch_primary_key_info go and we change #column_definitions to get what we need? I suggest that we replace the bad :is_identity part with something for :primary

3. Let's chagne the #columns method to let the sqlserver_options hold the primary. Then let the SQLServerColumn init method hook up the ivar for the #primary reader.

sqlserver_options = ci.except(:name,:default_value,:type,:null,:primary).merge(:database_year=>database_year)

def initialize(name, default, sql_type = nil, null = true, sqlserver_options = {})
  @sqlserver_options = sqlserver_options.symbolize_keys
  super(name, default, sql_type, null)
+ @primary = @sqlserver_options[:primary]
end

3. So then we remove the this method.

def is_identity?
@sqlserver_options[:is_identity]
end

4. Then change the #primary_key method to look like this.

   columns(table_name).detect(&:primary)

[bosko]

I think I've fixed all four items. New commit is at https://github.com/bosko/activerecord-sqlserver-adapter/commit/e8e7152d9a8525509545851e7b25acbcc48dcf70. Let me know if this is what you were targeting at and if anything else is needed to be changed.

[metaskills]

First, thanks for the patches, etc. Been really helpful. But this feature is a bit tricky to solve all around. I've been working with this diff and only have two real issues. https://gist.github.com/1011868

1. It is slow. The primary key constraint check is damn slow. Ideas how to speed it up?

2. There is technically a bug in ActiveRecord's connection_pool.rb that memoizes #table_exists? lookups but uses the #tables on the connection. This means that real tables that could be views and more importantly other schemes' tables can not be found and hence the new way to find the #primary/#is_identity will not work for tables in other schemas. I'll get that patched and it should not affect what you need tho.

If you can get some speed squeezed out of that new case, we can get this done.

[metaskills]

OK, I have master looking good. Can you confirm? Should be test'able under 3.1.0.rc4 too. Many things in this that make schemas work better.

[bosko]

It is working great now. Thanks!