Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReversibleMigration: remove_reference is not reversible. #604

Closed
TonyArra opened this issue Dec 27, 2021 · 4 comments · Fixed by #606
Closed

ReversibleMigration: remove_reference is not reversible. #604

TonyArra opened this issue Dec 27, 2021 · 4 comments · Fixed by #606

Comments

@TonyArra
Copy link
Contributor

TonyArra commented Dec 27, 2021

Behavior was introduced in: #592

Expected behavior

No warning for usage of remove_reference or remove_belongs_to in a migration.

As far as I can tell, remove_reference is reversible. Its inverse is available in ActiveRecord::Migration::CommandRecorder, and I'm able to rollback with it.

Actual behavior

Receive the following warning:

db/migrate/20210826075926_x:5:5: C: Rails/ReversibleMigration: remove_reference is not reversible.

Steps to reproduce the problem

Run rubocop on a migration that uses remove_reference or remove_belongs_to

RuboCop version

❯ rubocop -V
1.24.0 (using Parser 3.0.3.2, rubocop-ast 1.15.1, running on ruby 2.7.4 x86_64-darwin20)
  - rubocop-performance 1.12.0
  - rubocop-rails 2.13.0
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.6.0
@pirj
Copy link
Member

pirj commented Dec 27, 2021

Isn't it only reversible if it lists all the qualifiers as the add_reference has, like index: { unique: true, name: 'my_index' }, foreign_key: { to_table: :firms }? If one skips any of that in remove_reference, it would rollback to a slightly different state than it was before forward-running the migration. Unique index and fk table seem most problematic, and may break existing specs. They would have to create and migrate the DB from scrath on their local machine, or rollback until the migration with add_reference.
Is there a way to reliably tell if add_reference options were later used with remove_reference? It's hard with static analysis, you would have to read other migration files. I am uncertain if it's also possible in runtime, as CommandRecorder only dry runs one migration, and has no records about the one with the initial add_reference.

I'm in doubt about adding remove_reference to the allow-list.
On the one hand, we are not excessively cautious, even though we'll miss those incomplete remove_reference options.
On the other - if we be excessively strict, and suggest users to mark those statements with # rubocop:disable Rais/ReversibleMigration.
How do you suggest approaching this?

@TonyArra
Copy link
Contributor Author

TonyArra commented Dec 27, 2021

@pirj isn't the same thing true of all the other reversible migration statements?

For example, if I add a column with additional options, like default:

    add_column :comments, :position, :integer, default: 0

and then later remove that column without specifying those same options

    remove_column :comments, :position, :integer

Then it won't properly rollback to its previous state. It will be missing the default.

@pirj
Copy link
Member

pirj commented Dec 27, 2021

Fair enough 👍
Would you like to send a PR?

@TonyArra
Copy link
Contributor Author

@pirj sure, I can do it a few hours from now if you don't mind

TonyArra pushed a commit to TonyArra/rubocop-rails that referenced this issue Dec 27, 2021
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Dec 28, 2021
…ibleMigration`

This reverts commit 798b39e.

Remove `belongs_to` check from `Rails/ReversibleMigration`
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Dec 28, 2021
…ibleMigration`

This reverts commit 798b39e.

Remove `belongs_to` and `remove_reference` checks from `Rails/ReversibleMigration`.
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Dec 28, 2021
…ibleMigration`

This reverts commit 798b39e.

Remove `belongs_to` and `remove_reference` checks from `Rails/ReversibleMigration`.
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Dec 28, 2021
…ibleMigration`

This reverts commit 798b39e.

Remove `belongs_to` and `remove_reference` checks from `Rails/ReversibleMigration`.
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Dec 28, 2021
…ibleMigration`

This reverts commit 798b39e.

Remove `belongs_to` and `remove_reference` checks from `Rails/ReversibleMigration`.
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Dec 28, 2021
…ibleMigration`

This reverts commit 798b39e.

Remove `belongs_to` and `remove_reference` checks from `Rails/ReversibleMigration`.
@koic koic closed this as completed in #606 Jan 5, 2022
koic added a commit that referenced this issue Jan 5, 2022
…erence

[Fix #604] Remove `remove_reference` checks from `Rails/ReversibleMigration`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants