ActiveRecord >=3.1 / setting primary_key to an column defined in the db as auto-increment but not primary key #3987

Closed
kjg opened this Issue Dec 14, 2011 · 11 comments

Projects

None yet

2 participants

@kjg
Contributor
kjg commented Dec 14, 2011

When using a legacy database or when writing to a view, if a column is set to auto-increment, but the schema doesn't define it as a "primary key", then active record fails to properly set the primary_key column in the schema_cache early enough.

It looks like by calling #create, the schema_cache's colums are filled before AR:Base#columns gets called. #columns being the method that sets the primary_key in the cache

I've included a failing test as well as a proposed fix in this gist.

In my opinion #columns is probably not the place to be putting the primary_key in the cache though since clearly the column definitions in the cache are filled before #columns is called. A better fix would be to either 1. find a better time earlier in the query life cycle to cache the primary_key as set by AR, or 2. cache the column definitions later in the query cycle.

@kjg
Contributor
kjg commented Dec 14, 2011

FYI: This issue was caused by this patch's attempt to fix this issue

@jonleighton jonleighton was assigned Dec 14, 2011
@jonleighton jonleighton added a commit that closed this issue Dec 15, 2011
@jonleighton jonleighton Fix #3987.
Conflicts:

	activerecord/lib/active_record/attribute_methods/primary_key.rb
	activerecord/test/cases/primary_keys_test.rb
df932c4
@SweeD SweeD pushed a commit to mafolz/rails that referenced this issue Dec 15, 2011
@jonleighton jonleighton Fix #3987. 8dba32f
@kjg
Contributor
kjg commented Dec 15, 2011

Just curious. Why didn't you remove the similar code from #columns?

@jonleighton
Member

Because that would bring back bug #2807.

@kjg
Contributor
kjg commented Dec 15, 2011

How so? Doesn't the if connected? fix that?

@jonleighton
Member

PDI

@kjg
Contributor
kjg commented Dec 15, 2011
-- $ git diff
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 9215a68..511292a 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -767,10 +767,6 @@ module ActiveRecord #:nodoc:

       # Returns an array of column objects for the table associated with this class.
       def columns
-        if defined?(@primary_key)
-          connection.schema_cache.primary_keys[table_name] ||= primary_key
-        end
-
         connection.schema_cache.columns[table_name]
       end

-- $ ruby -Itest test/cases/primary_keys_test.rb -n test_set_primary_key_with_no_connection
Using sqlite3 with Identity Map off
Loaded suite test/cases/primary_keys_test
Started
.
Finished in 0.006121 seconds.

1 tests, 2 assertions, 0 failures, 0 errors, 0 skips

Test run options: --seed 36004 --name "test_set_primary_key_with_no_connection"
@jonleighton
Member

Yes but now when you call Model.columns none of the items will have primary == true. The test doesn't show this though.

@kjg
Contributor
kjg commented Dec 15, 2011

Okay, if thats true, then this current issue will still exist if the Model is loaded when the db is not connected, then the db connects, then queries are run.

@jonleighton
Member

that's what the if defined?(@primary_key) ... in columns is for - in case you set the primary key before a connection existed. that's why we can't delete it.

@kjg
Contributor
kjg commented Dec 15, 2011

Yes, but that means this bug, bug 3987 isn't fixed. I'll write up a new script to prove it to you.

@kjg
Contributor
kjg commented Dec 16, 2011

Your changes this morning on master fixes this bug there. However 3-1-stable still fails with this new test:

## must run activerecord's `rake postgresql:build_databases`
require 'active_record'

ActiveRecord::Base.establish_connection(:adapter => 'postgresql', :database => 'activerecord_unittest')

ActiveRecord::Schema.define do
  execute <<_SQL
    CREATE SEQUENCE foo_seq;
    CREATE TABLE posts(
      foo int NOT NULL DEFAULT nextval( 'foo_seq'::regclass )
    );
_SQL
end

connection = ActiveRecord::Base.remove_connection

class Post < ActiveRecord::Base
  set_primary_key :foo
end

ActiveRecord::Base.establish_connection(connection)

begin
  p Post.create
  p Post.primary_key
ensure
  ActiveRecord::Base.connection.execute <<_SQL
    DROP TABLE posts;
    DROP SEQUENCE foo_seq;
_SQL
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment