This project is archived and is in readonly mode.

#4803 ✓resolved
Jeff Dean

[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
  • Jeff Dean

    Jeff Dean June 9th, 2010 @ 06:02 AM

    • Tag changed from activecord, migrations to activecord, migrations, patch
  • Neeraj Singh
  • Paul Barry

    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

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

  • Paul Barry

    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

    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

    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>

Attachments

Referenced by

Pages