This project is archived and is in readonly mode.
![Jeff Dean Jeff Dean](/images/avatar-large.gif)
[PATCH] remove_column should raise an ArgumentError when no columns are passed
Reported by Jeff Dean | June 9th, 2010 @ 05:57 AM
Currently in migrations you can write the following:
remove_column :foo
This will execute without errors, but will make no change to your database. I think this is surprising behavior - I expect my migration methods fail if proper arguments are not passed. I can't think of any case in which having a migration method that fails silently is desirable.
I've attached a patch that will raise an argument error if you don't pass any columns to remove_column. It includes a simple test there and it passes against mysql, postgres and sqlite3.
Comments and changes to this ticket
-
Jeff Dean June 9th, 2010 @ 06:00 AM
- no changes were found...
-
Jeff Dean June 9th, 2010 @ 06:02 AM
- Tag changed from activecord, migrations to activecord, migrations, patch
-
-
Paul Barry June 12th, 2010 @ 03:30 AM
I've looked at this one for a while and I'm totally baffled as to why is doesn't just raise a MySQL exception anyway. If you try to drop a column that doesn't exist using the db shell, you get an error:
$ mysql -u rails -D activerecord_unittest Welcome to the MySQL monitor. Commands end with ; or \g. Your MySQL connection id is 78 Server version: 5.1.42-log Source distribution Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. mysql> alter table people drop funny; ERROR 1091 (42000): Can't DROP 'funny'; check that column/key exists
Seems like the thing you would want to have happen is just for that error to bubble up. I agree that trying to remove a column doesn't exist should raise an error, but that should just happen naturally, we shouldn't have to have an explicit ArgumentError check for that. It's important that drop table, change column, etc. behave the same way as well.
So it seems to me that the right fix is to prevent the actual MySQL error from being swallowed, but I haven't figured out why that's happening
-
Jeff Dean June 12th, 2010 @ 03:43 AM
I agree that the db errors should be consistent, and I believe they are. However, when you execute
remove_column(:name)
it's never getting to mysql at all because because internally it's iterating over the column names. If they are empty, it never enters the loop, so MySQL never gets any commands. Here's the code:def remove_column(table_name, *column_names) remove_column(:people, :first_name)") if column_names.empty? column_names.flatten.each do |column_name| execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{quote_column_name(column_name)}" end end
Notice how if column_names is empty, nothing happens.
There was a time when
remove_column
only took a single column, and it would raise an ArgumentError if you only passed it a single parameter:def remove_column(table_name, column_name) #... end remove_column(:table_name)
If you only passed a single argument, it would throw and error. Now
remove_column
now has some sugary syntax and it allows multiple columns with*columns
:remove_column(:table_name, :column1, :column2)
When that change was made, it broke the original behavior of requiring a second parameter. So I see this as a regression. The method can't possibly work when you pass it a single parameter, so I think and ArgumentError is appropriate, and restores the original behavior of the method.
drop_table
andchange_column
don't happen to have the same bug. -
Paul Barry June 12th, 2010 @ 04:11 AM
Jeff,
You're right, I completely was looking at the wrong thing. Raising an ArgumentError here completely makes sense if column_names is empty, which is what this patch does, so +1!
-
Repository June 23rd, 2010 @ 05:25 AM
- State changed from new to resolved
(from [da93d69bcb8c507a16503f883f67597338b5edeb]) remove_column should raise an ArgumentError when no columns are passed [#4803 state:resolved]
Signed-off-by: Michael Koziarski michael@koziarski.com
http://github.com/rails/rails/commit/da93d69bcb8c507a16503f883f6759... -
Repository June 23rd, 2010 @ 05:25 AM
(from [e639536ea80e94f5d72493267c8aec21d305cf74]) remove_column should raise an ArgumentError when no columns are passed [#4803 state:resolved]
Signed-off-by: Michael Koziarski michael@koziarski.com
http://github.com/rails/rails/commit/e639536ea80e94f5d72493267c8aec...
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Tags
Referenced by
-
4803 [PATCH] remove_column should raise an ArgumentError when no columns are passed (from [da93d69bcb8c507a16503f883f67597338b5edeb]) remove_...
-
4803 [PATCH] remove_column should raise an ArgumentError when no columns are passed (from [e639536ea80e94f5d72493267c8aec21d305cf74]) remove_...