#create_table doesn't raise for columns with name equal to the PK #6378

Closed
elia opened this Issue May 18, 2012 · 10 comments

Comments

Projects
None yet
6 participants
Contributor

elia commented May 18, 2012

The following will create an integer PK with name slug and ignore the column definition:

create_table :posts, primary_key: :slug do |t|
  t.string :slug
  # ...
end

Probably it should pick the column type from the column definition or fail completely.

Contributor

KensoDev commented May 18, 2012

You can do it like that:

create_table :posts, {:id => false} do |t|
  t.string :slug
  t.text :title
  t.text :subtitle
  t.timestamps
end
execute "ALTER TABLE posts ADD PRIMARY KEY (slug);"

OR

create_table(:posts, :id => false) do |t|
      t.string :slug, :primary => true
end

Assuming you are using 3.x

Contributor

elia commented May 18, 2012

@KensoDev thanks, the second seems better.

I think the issue is still valid, since as it is rails say nothing and clearly does something different from what the user intended, at least it should raise a key "mismatch" exception.

Contributor

KensoDev commented May 18, 2012

I am not a Rails core commiter.
for what my opinion is worth, I think we can at least add a :primary_key_type option.
for the very least, we can add documentation for that.

Owner

jeremy commented May 18, 2012

+1 to raising an exception. Doing id: false then a separate column with primary: true is the way to go.

Contributor

elia commented May 18, 2012

@KensoDev @jeremy

I looked into the sources (1, 2) and I can't find any reference to the syntax above (repeated here):

create_table :posts, id: false do |t|
  t.string :slug, primary: true
end

What instead seems to be possible is to set :primary_key as the column type, as a consequence it can't be of type :string.

I'll keep working on the exception patch though.

Member

senny commented Oct 28, 2012

@elia just found this issue, is there a patch in the works?

Contributor

elia commented Oct 28, 2012

sadly I completely forgot about this (don't even remember on which project I found it 😊)

I can look at it after #7587 is closed, but if you please go forth!

Member

steveklabnik commented Oct 28, 2012

Does this rely on the changes in #7587? Why does it need to wait?

Contributor

elia commented Oct 28, 2012

@steveklabnik no, just my open source queue :D (sorry for the ref)

Member

senny commented Oct 28, 2012

@elia @steveklabnik I have a patch nearly completed. I have to look deeper into the sqlite3_adapter's copy_table method though. It seems that this method actually redefines the primary key, which breaks with the patch.

@senny senny added a commit to senny/rails that referenced this issue Oct 28, 2012

@senny senny refactor `SQLite3Adapter#copy_table` to prevent primary key redefinit…
…ions. #6378
b104157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment