Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

create_table raises an ArgumentError when the primary key is redefined. #8054

Merged
merged 2 commits into from Oct 28, 2012

Conversation

Projects
None yet
4 participants
Member

senny commented Oct 28, 2012

This patch adds error handling to create_table when the primary_key is being redefined.

I added a separate refactoring commit for the SQLite3Adapter#copy_table method. This method redefined the primary key multiple times, wich of course raised exceptions with the patch. I looked over the tests for copy_table and made some additions to lock the current behavior down before refactoring (I think it would be good to glance over to make sure I didn't forget anything).

Also there might be a better description for the error message, just leave a comment if you got a better text.

This is a fix for #6378

Member

senny commented Oct 28, 2012

@jeremy @rafaelfranca can you take a look?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Oct 28, 2012

...record/test/cases/adapters/sqlite3/copy_table_test.rb
@@ -43,7 +48,9 @@ def test_copy_table_with_index
end
def test_copy_table_without_primary_key
- test_copy_table('developers_projects', 'programmers_projects')
+ test_copy_table('developers_projects', 'programmers_projects') do
+ assert_equal nil, @connection.primary_key('programmers_projects')
@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2012

Owner

You should be able to use assert_nil.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Oct 28, 2012

activerecord/test/cases/migration/change_schema_test.rb
@@ -132,6 +132,14 @@ def test_create_table_with_primary_key_prefix_as_table_name
assert_equal %w(foo testingid), connection.columns(:testings).map(&:name).sort
end
+ def test_create_table_raises_when_redefining_primary_key_column
+ assert_raise(ArgumentError, "you can't redefine the primary key column 'id'. To define a custom primary key, pass :id => false to create_table.") do
+ connection.create_table :testings do |t|
+ t.column :id, :string
+ end
+ end
+ end
@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2012

Owner

I think it'd be interesting to also test with a custom pk name, wdyt?

@senny

senny Oct 28, 2012

Member

I'll add a test using the :primary_key option for create_table

I think it looks good 👍, thanks!

Member

senny commented Oct 28, 2012

@carlosantoniodasilva thanks for the inputs. Updated the PR with the requested changes.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Oct 28, 2012

...record/test/cases/adapters/sqlite3/copy_table_test.rb
@@ -32,6 +32,11 @@ def test_copy_table_renaming_column
end
end
+ def test_copy_table_allows_to_pass_options_to_create_table
+ @connection.create_table('blocker_table')
+ test_copy_table('customers', 'blocker_table', :force => true)
@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2012

Owner

Sorry, just noticed this could be a 1.9 hash style :)

@senny thanks. I just noticed one thing about the hash style, and that made me wonder whether the example should show a hash like { id: false } instead. Wdyt?

Member

senny commented Oct 28, 2012

@carlosantoniodasilva changed 1.8 syntax to 1.9. what example do you mean? The one about passing the options to create_table? Currently copy_table will always copy the primary key (I think that was the case before too) so :id => false will have no impact.

@rafaelfranca rafaelfranca and 2 others commented on an outdated diff Oct 28, 2012

...rd/connection_adapters/abstract/schema_definitions.rb
@@ -234,6 +234,10 @@ def column(name, type, options = {})
name = name.to_s
type = type.to_sym
+ if primary_key_column_name == name
+ raise ArgumentError, "you can't redefine the primary key column '#{name}'. To define a custom primary key, pass :id => false to create_table."
@rafaelfranca

rafaelfranca Oct 28, 2012

Owner

I think @carlosantoniodasilva is talking about this :id => false

@senny

senny Oct 28, 2012

Member

Ah sure 😄

@frodsan

frodsan Oct 28, 2012

Contributor

1.9 hash syntax ❤️

@senny I meant the text To define a custom primary key, pass :id => false to create_table., but I think it might be unnecessary.

Some other thing that just came to my mind is that giving a message to assert_raise does not work anymore with minitest, you have to do something like this.

Member

senny commented Oct 28, 2012

@carlosantoniodasilva @rafaelfranca example now has 1.9 syntax. I also changed the assert_raise call as you suggested.

@senny great, thanks!

carlosantoniodasilva added a commit that referenced this pull request Oct 28, 2012

Merge pull request #8054 from senny/6378_create_table_raises_when_def…
…ining_pk_column

create_table raises an ArgumentError when the primary key is redefined.

@carlosantoniodasilva carlosantoniodasilva merged commit ed80dd7 into rails:master Oct 28, 2012

Member

senny commented Oct 28, 2012

thanks for the instant review feedback. (really have to get used to ruby 1.9's hash syntax ;)

No problem, we're still getting used too :)

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