Add support for `if_exists/if_not_exists` on `remove_column/add_column` #38352

eileencodes · 2020-01-30T13:50:58Z

This PR adds support for if_exists on remove_column and
if_not_exists on add_column to support silently ignoring migrations
if the remove tries to remove a non-existent column or an add tries to
add an already existing column.

We (GitHub) have custom monkey-patched support for these features and
would like to upstream this behavior.

This matches the same behavior that is supported for create_table and
drop_table. The behavior for sqlite is different from mysql/postgres
and sqlite for remove column and that is reflected in the tests.

cc/ @rafaelfranca @tenderlove @jhawthorn

kaspth

Cool, my comments are more if there's some extra stuff we want to look into.

kaspth · 2020-01-30T14:05:55Z

activerecord/CHANGELOG.md

+    ```
+    class AddColumnTitle < ActiveRecord::Migration[6.1]
+      def change
+        add_column :posts, :title, :string, if_not_exists: true


Should we do something in the case where title exists, but it happens to be a text, then rolling back the existing column's removed and then migrating again, the column would now be a string? Though I suppose that's what you want and later migrations is the newest source of truth. Though it could be potentially confusing and happen silently.

Good call. Turns out column_exists? takes a type. I've updated this to still raise if you pass a different type and added a test for that case.

kaspth · 2020-01-30T14:12:47Z

activerecord/CHANGELOG.md

+    ```
+    class RemoveColumnTitle < ActiveRecord::Migration[6.1]
+      def change
+        remove_column :posts, :title, if_exists: true


It's nice that both these options correspond to their SQL counterparts, but the sorta tailing if yet just an option reads odd to me. I find that I have to read both add_column and remove_column multiple times and sort of migrate/rollback the migration in my head. I wonder if there's something like this could work:

add_column :posts, :title, :string, skip_when_migrated: true remove_column :posts, :title, skip_when_migrated: true

I don't care so much that add_column requires if_not_exists while if_exists is only for remove_column, it's more about describing what happens in case the migration is needless. Maybe only_when_required: true or only_when_needed: true. Maybe suppress_error: true as another option.

I'm following the existing if_exists/if_not_exists that is used by create and drop table and I don't think they should be different because that's really confusing for users.

I don't love if_exists / if_not_exists but I definitely don't want create and drop to have one set of options and add/remove column to have totally different options. We could change the create/drop table but that creates a whole deprecation cycle.

Ah doh, I tried searching for if_not_exists on api.rubyonrails.org and came back blank. Sorry about the derailing, all good from here 🙏

This PR adds support for `if_exists` on `remove_column` and `if_not_exists` on `add_column` to support silently ignoring migrations if the remove tries to remove a non-existent column or an add tries to add an already existing column. We (GitHub) have custom monkey-patched support for these features and would like to upstream this behavior. This matches the same behavior that is supported for `create_table` and `drop_table`. The behavior for sqlite is different from mysql/postgres and sqlite for remove column and that is reflected in the tests.

kaspth · 2020-01-30T14:54:52Z

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

      def remove_column(table_name, column_name, type = nil, **options)
+        return if options[:if_exists] == true && !column_exists?(table_name, column_name)


Should we pass the type into the exists check here too?

I thought about this and tried it out and there's an issue with passing type. For add_column the type is required, you have to pass it so we can use that to check if the column_exists?.

However, in remove_column it's allowed but completely ignored by the actual column dropper. For remove_column since type is ignored by the execute we don't need to pass type there because the column will be removed regardless of type so we don't want to protect on nil or type. Does that make sense? I feel like I'm rambling...

Basically doesn't matter what you pass for type, remove column will always remove the column because type is ignored.

Got it, perfect 👌

This PR allows for passing `if_exists` options to the `remove_index` method so that we can ignore already removed indexes. This work follows column `if/if_not_exists` from rails#38352 and `:if_not_exists` on `add_index` from rails#38555. We've found this useful at GitHub, there are migrations where we don't want to raise if an index was already removed. This will allow us to remove a monkey patch on `remove_index`. I considered raising after the `index_name_for_remove` method is called but that method will raise if the index doesn't exist before we get to execute. I have a commit that refactors this but after much consideration this change is cleaner and more straightforward than other ways of implementing this. This change also adds a little extra validation to the `add_index` test. Fix `nodoc` on edited methods.

slavadev · 2020-07-21T11:19:56Z

Hey,
I'm sorry for a late comment to a merged PR 😅 , but will it work properly with a rollback of the migration?

For example, I have:

def change
  add_column :posts, :title, :string, if_not_exists: true
end

During the initial migration, it will check if the column exists and creates a column.
But during the rollback, it will also check if the column exists(and it exists) and do nothing even though it should be deleted.

Most probably I'm missing something, but I can ask 🙂
Thanks!

eileencodes · 2020-07-21T12:35:04Z

During the initial migration, it will check if the column exists and creates a column. But during the rollback, it will also check if the column exists(and it exists) and do nothing even though it should be deleted.

If you're using this you should define up and down because you'll likely want down to also consider whether your column exists.

rails-bot bot added the activerecord label Jan 30, 2020

eileencodes added this to the 6.1.0 milestone Jan 30, 2020

eileencodes force-pushed the add-support-for-if-exists branch 2 times, most recently from 455ae9d to d93578c Compare January 30, 2020 14:01

kaspth approved these changes Jan 30, 2020

View reviewed changes

eileencodes force-pushed the add-support-for-if-exists branch from d93578c to 95ed7e7 Compare January 30, 2020 14:51

kaspth reviewed Jan 30, 2020

View reviewed changes

eileencodes merged commit c280ae7 into rails:master Jan 30, 2020

eileencodes deleted the add-support-for-if-exists branch January 30, 2020 17:42

eileencodes mentioned this pull request Apr 16, 2020

Add if_exists option to remove_index #38967

Merged

srt32 mentioned this pull request Jan 13, 2021

Update add_column with if_not_exists? to properly handle int column type #41108

Closed

robertomiranda mentioned this pull request Jun 27, 2021

Add support for if_exists/if_not_exists on remove_foreign_key/add_foreign_key #42616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `if_exists/if_not_exists` on `remove_column/add_column` #38352

Add support for `if_exists/if_not_exists` on `remove_column/add_column` #38352

eileencodes commented Jan 30, 2020

kaspth left a comment

kaspth Jan 30, 2020

eileencodes Jan 30, 2020

kaspth Jan 30, 2020

kaspth Jan 30, 2020

eileencodes Jan 30, 2020

eileencodes Jan 30, 2020

kaspth Jan 30, 2020

kaspth Jan 30, 2020

eileencodes Jan 30, 2020

eileencodes Jan 30, 2020

kaspth Jan 31, 2020

slavadev commented Jul 21, 2020

eileencodes commented Jul 21, 2020

		def remove_column(table_name, column_name, type = nil, **options)
		return if options[:if_exists] == true && !column_exists?(table_name, column_name)

Add support for if_exists/if_not_exists on remove_column/add_column #38352

Add support for if_exists/if_not_exists on remove_column/add_column #38352

Conversation

eileencodes commented Jan 30, 2020

kaspth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavadev commented Jul 21, 2020

eileencodes commented Jul 21, 2020

Add support for `if_exists/if_not_exists` on `remove_column/add_column` #38352

Add support for `if_exists/if_not_exists` on `remove_column/add_column` #38352