remove_column should not be included in BulkChangeTable cop

[connorshea]

Rubocop warns about multiple uses of remove_column in the same migration, saying it should be changed to use a change_table, bulk: true. However, change_table has no equivalent means of removing a column in a way which can be reversed. So when you try to fix the warning, a new warning for a non-reversible migration shows up.

---

Expected behavior

I would expect that multiple uses of remove_column would not cause a warning from Rails/BulkChangeTable, because remove_column can't be converted to change_table in a way that's reversible.

Actual behavior

db/migrate/20190817225925_replace_before_after_values_with_differences_column.rb:4:5: C: Rails/BulkChangeTable: You can use change_table :game_purchase_events, bulk: true to combine alter queries.
    remove_column :game_purchase_events, :before_value, :text
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Or, if you change it to use change_table, bulk: true:

db/migrate/20190817225925_replace_before_after_values_with_differences_column.rb:6:7: C: Rails/ReversibleMigration: change_table(with remove) is not reversible.
      t.remove :after_value, :text
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Steps to reproduce the problem

1. Create a migration like so:

class AddAndRemoveColumnsToTable < ActiveRecord::Migration[6.0]
  def change
    remove_column :table_name, :before_value, :text
    remove_column :table_name, :after_value, :text
    add_column :table_name, :differences, :jsonb
  end
end

2. Enable the Rails/BulkChangeTable and Rails/ReversibleMigration cops
3. Run bundle exec rubocop, see that there are violations for Rails/BulkChangeTable.
4. Change your migration to look like this instead:

class AddAndRemoveColumnsToTable < ActiveRecord::Migration[6.0]
  def change
    change_table :table_name, bulk: true do |t|
      t.remove :before_value
      t.remove :after_value
      t.jsonb :differences
    end
  end
end

5. Run Rubocop again
6. Note that now there's a lint error for Rails/ReversibleMigration (see the docs), t.remove is an alias of remove_columns and it doesn't seem like there's any way to specify a column type. So it's not possible (as far as I can tell) to reversibly use change_table, bulk: true when removing columns.

RuboCop version

0.74.0

[wata727]

Hi @connorshea, I'm an author of this cop.

If you focus only on suppressing warnings, there is a way to define up and down methods respectively. This is a completely reversible migration.

class AddAndRemoveColumnsToTable < ActiveRecord::Migration[6.0]
  def up
    change_table :table_name, bulk: true do |t|
      t.remove :before_value
      t.remove :after_value
      t.jsonb :differences
    end
  end

  def down
    change_table :table_name, bulk: true do |t|
      t.text :before_value
      t.text :after_value
      t.remove :differences
    end
  end
end

But if you feel this way is redundant, I think you should not follow the Rails/BulkChangeTable cop. The bulk is an option and you should use the appropriate option for your situation.

Initially, I added this cop for telling us about how to use grouping alter statements, but I understand the opinion that it's a bit too assertive.

[connorshea]

Fair enough, thanks for clarifying 👍

[eugeneius]

Note that remove_columns / t.remove will be reversible in Rails 6.1 via rails/rails#36589.

[Tao-Galasse]

Hi @wata727 ! Now that Rails 6.1 released, I feel like this issue could be re-evaluated. Should a line like t.remove :my_string, type: :string in a bulk migration raise a lint error ?

[koic]

@wata727 Can you sort out the current situation and work on this?

[wata727]

I believe this issue is already ready to be closed. The remove_column is a bulk changeable transformation and should be included in the BulkChangeTable cop. As a result of the fix, the ReversibleMigration cop may add a new offense, but it's avoidable and it's not the responsibility of this cop.

[wata727]

By the way, this list of combinable transformations seems a bit outdated. Perhaps we can add the list below:

• change_default
• change_null (PostgreSQL only)
• blob
• tinyblob (MySQL only)
• mediumblob (MySQL only)
• longblob (MySQL only)
• tinytext (MySQL only)
• mediumtext (MySQL only)
• longtext (MySQL only)
• unsigned_integer (MySQL only)
• unsigned_bigint (MySQL only)
• unsigned_float (MySQL only)
• unsigned_decimal (MySQL only)
• bigserial (PostgreSQL only)
• bit (PostgreSQL only)
• bit_varying (PostgreSQL only)
• cidr (PostgreSQL only)
• citext (PostgreSQL only)
• daterange (PostgreSQL only)
• hstore (PostgreSQL only)
• inet (PostgreSQL only)
• interval (PostgreSQL only)
• int4range (PostgreSQL only)
• int8range (PostgreSQL only)
• jsonb (PostgreSQL only)
• ltree (PostgreSQL only)
• macaddr (PostgreSQL only)
• money (PostgreSQL only)
• numrange (PostgreSQL only)
• oid (PostgreSQL only)
• point (PostgreSQL only)
• line (PostgreSQL only)
• lseg (PostgreSQL only)
• box (PostgreSQL only)
• path (PostgreSQL only)
• polygon (PostgreSQL only)
• circle (PostgreSQL only)
• serial (PostgreSQL only)
• tsrange (PostgreSQL only)
• tstzrange (PostgreSQL only)
• tsvector (PostgreSQL only)
• uuid (PostgreSQL only)
• xml (PostgreSQL only)
• timestamptz (PostgreSQL only)
• enum (PostgreSQL only)

References

• ALTER operations
  • https://github.com/rails/rails/blob/v7.0.4.3/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb#L569-L612
  • https://github.com/rails/rails/blob/v7.0.4.3/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L1611-L1634
  • https://github.com/rails/rails/blob/v7.0.4.3/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
• column methods
  • https://github.com/rails/rails/blob/v7.0.4.3/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb#L261-L265
  • https://github.com/rails/rails/blob/v7.0.4.3/activerecord/lib/active_record/connection_adapters/mysql/schema_definitions.rb#L54-L56
  • https://github.com/rails/rails/blob/v7.0.4.3/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb#L185-L188

[koic]

@wata727 Superb! I'll close this issue. OTOH, If you'd like, please open a PR as the results of your investigation if necessary. Thank you!