Fix AddClearanceToUsers down method (remove_index) #931
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Spone
changed the title
|
Thank you for opening this PR @Spone! I tried to reproduce this and failed. I set up a test rails app with
This completed successfully: ~/Code/thoughtbot/clearance_test main*
❯ bin/rails db:migrate
== 20210314172157 CreateUsers: migrating ======================================
-- create_table(:users)
-> 0.0016s
-- add_index(:users, :email)
-> 0.0010s
-- add_index(:users, :remember_token)
-> 0.0006s
== 20210314172157 CreateUsers: migrated (0.0032s) =============================
~/Code/thoughtbot/clearance_test main*
❯ bin/rails db:rollback
== 20210314172157 CreateUsers: reverting ======================================
-- remove_index(:users, :remember_token)
-> 0.0020s
-- remove_index(:users, :email)
-> 0.0009s
-- drop_table(:users)
-> 0.0007s
== 20210314172157 CreateUsers: reverted (0.0061s) =============================
~/Code/thoughtbot/clearance_test main*
❯ bin/rails db:migrate
== 20210314172157 CreateUsers: migrating ======================================
-- create_table(:users)
-> 0.0016s
-- add_index(:users, :email)
-> 0.0008s
-- add_index(:users, :remember_token)
-> 0.0007s
== 20210314172157 CreateUsers: migrated (0.0032s) =============================Could you share more details about your configuration to help me reproduce this issue? Thanks! |
|
@Spone Which database and Rails version are you using? I think that before rails 4 removing the column would not necessarily remove the index |
|
Hi, sorry for the delay. I'm using Postgres with Rails 6.1.3.1. My I managed to reproduce the issue on a fresh Rails install. Create this migration: class CreateUsers < ActiveRecord::Migration[6.1]
def change
enable_extension 'pgcrypto' unless extension_enabled?('pgcrypto')
create_table :users, id: :uuid, default: 'gen_random_uuid()' do |t|
t.string :name
t.string :email
t.timestamps
end
end
endThen run: |
|
After investigating this it seems that the cause of the issue is that your CreateUsers migration does not add an index to the email column, so the migration that clearance creates adds it on up but it's then not removed on down. The other index we add is remember_token, but that one doesn't cause an issue because we are also adding/removing the column. I think your proposed changes would fix this, but I want to take some time to make sure there won't be any side effects to this before merging it. |
|
Merging this. Thanks for your contribution @Spone! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the
self.downmethod of theAddClearanceToUsersmigration only removes the columns, but not the indexes.So if you migrate and then rollback, you can no longer migrate:
This fix adds the
remove_indexcalls to theself.downmethod.