[PATCH] remove_column should raise an ArgumentError when no columns are passed

+1

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

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 and change_column don't happen to have the same bug.

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!

(from [e639536ea80e94f5d72493267c8aec21d305cf74]) remove_column should raise an ArgumentError when no columns are passed [#4803 [PATCH] remove_column should raise an ArgumentError when no columns are passed state:resolved]

Signed-off-by: Michael Koziarski michael@koziarski.com
http://github.com/rails/rails/commit/e639536ea80e94f5d72493267c8aec...