Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

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

  • no changes were found...

  • remove-column-raises-argument-error.diff 2.8 KB

• You flagged this item as spam.
  

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

  • Tag changed from “activecord, migrations” to “activecord, migrations, patch”
• 

  Neeraj Singh June 9th, 2010 @ 06:10 AM

  +1

• You flagged this item as spam.
  

  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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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...