Rails/BulkChangeTable and remove_column

[6temes]

This code would trigger Rails/BulkChangeTable

class ChangeACoupleOfColumnsInThing < ActiveRecord::Migration[5.2]
  def change
    remove_column :things, :column_one, :boolean
    add_column    :things, :column_two, :string, null: false, default: ''
  end
end

The problem is that this:

class ChangeACoupleOfColumnsInThing < ActiveRecord::Migration[5.2]
  def change
    change_table :sellers, bulk: true do |t|
      t.remove :column_one, :boolean
      t.column :column_two, :string, null: false, default: ''
    end
  end
end

Will raise RailsReversibleMigration.

Version:

$ rubocop -V
0.57.2

[wata727]

Maybe you can avoid this offense by writing as follows:

class ChangeACoupleOfColumnsInThing < ActiveRecord::Migration[5.2]
  def change
    reversible do |dir|
      change_table :things, bulk: true do |t|
        dir.up do
          t.remove :column_one
          t.column :column_two, :string, null: false, default: ''
        end

        dir.down do
          t.boolean :column_one # , null: ..., default: ...
          t.remove :column_two
        end
      end
    end
  end
end

But if the time to execute the original two ALTER queries is small enough, this change will be nonsense... Unfortunately, Rails/BulkChangeTable cop can't know the time to execute queries.

Depending on the actual environment, I recommend that you select how to write.

[6temes]

I don't understand how this Cop can improve my code, though.

[wata727]

FYI https://aaronlasseigne.com/2012/06/05/faster-activerecord-migrations-using-change-table-bulk/

[6temes]

んー

So this cop can potentially make some migrations faster for large databases, while introducing some awkwardness in other cases.

I don't know if, with this current behavior, this cop should be activated by default. But it is obviously not to me to choose.

[bbatsov]

We're in the process of moving all Rails-related functionality to a standalone library (gem) named rubocop-rails. Please, migrate this issue to its issue tracker https://github.com/rubocop-hq/rubocop-rails/issues/

[louim]

@koic, would you mind moving this issue to the rubocop/rails tracker? I stumbled on this while updating and I think it's an issue that needs to be addressed. Also see: https://github.com/rubocop-hq/rubocop-rails/pull/48#issuecomment-500739488

I can also open a new issue there, but I would prefer the original issue with the context be moved if you don't mind.

[koic]

@louim Thanks for the mention. I transferred this issue to rubocop-hq/rubocop-rails repo.

[louim]

One way to handle the case could be like it was done in https://github.com/rubocop-hq/rubocop-rails/pull/48 and remove the remove_column from the combinable methods.

[thisismydesign]

Same goes for change_column. I think most projects don't benefit from this and where it actually matters migrations are handled with care. I like @louim's proposal. All methods which are not reversible and lead to more / more complex code should be removed, or the rule should be turned off by default.

[maddog666]

Add type

  def change
    change_table :sellers, bulk: true do |t|
      t.remove :column_one, type: :boolean
      t.column :column_two, :string, null: false, default: ''
    end
  end