-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add support for if_exists/if_not_exists
on remove_column/add_column
#38352
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,27 @@ | ||
* Adds support for `if_not_exists` to `add_column` and `if_exists` to `remove_column. | ||
|
||
Applications can set their migrations to ignore exceptions raised when adding a column that already exists or when removing a column that does not exist. | ||
|
||
Example Usage: | ||
|
||
``` | ||
class AddColumnTitle < ActiveRecord::Migration[6.1] | ||
def change | ||
add_column :posts, :title, :string, if_not_exists: true | ||
end | ||
end | ||
``` | ||
|
||
``` | ||
class RemoveColumnTitle < ActiveRecord::Migration[6.1] | ||
def change | ||
remove_column :posts, :title, if_exists: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :posts, :title, :string, skip_when_migrated: true
remove_column :posts, :title, skip_when_migrated: true I don't care so much that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah doh, I tried searching for |
||
end | ||
end | ||
``` | ||
|
||
*Eileen M. Uchitelle* | ||
|
||
* Regexp-escape table name for MS SQL | ||
|
||
Add `Regexp.escape` to one method in ActiveRecord, so that table names with regular expression characters in them work as expected. Since MS SQL Server uses "[" and "]" to quote table and column names, and those characters are regular expression characters, methods like `pluck` and `select` fail in certain cases when used with the MS SQL Server adapter. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -536,6 +536,9 @@ def drop_table(table_name, **options) | |
# column will have the same collation as the table. | ||
# * <tt>:comment</tt> - | ||
# Specifies the comment for the column. This option is ignored by some backends. | ||
# * <tt>:if_not_exists</tt> - | ||
# Specifies if the column already exists to not try to re-add it. This will avoid | ||
# duplicate column errors. | ||
# | ||
# Note: The precision is the total number of significant digits, | ||
# and the scale is the number of digits that can be stored following | ||
|
@@ -587,7 +590,12 @@ def drop_table(table_name, **options) | |
# # Defines a column with a database-specific type. | ||
# add_column(:shapes, :triangle, 'polygon') | ||
# # ALTER TABLE "shapes" ADD "triangle" polygon | ||
# | ||
# # Ignores the method call if the column exists | ||
# add_column(:shapes, :triangle, 'polygon', if_not_exists: true) | ||
def add_column(table_name, column_name, type, **options) | ||
return if options[:if_not_exists] == true && column_exists?(table_name, column_name, type) | ||
|
||
at = create_alter_table table_name | ||
at.add_column(column_name, type, **options) | ||
execute schema_creation.accept at | ||
|
@@ -616,7 +624,15 @@ def remove_columns(table_name, *column_names, **options) | |
# to provide these in a migration's +change+ method so it can be reverted. | ||
# In that case, +type+ and +options+ will be used by #add_column. | ||
# Indexes on the column are automatically removed. | ||
# | ||
# If the options provided include an +if_exists+ key, it will be used to check if the | ||
# column does not exist. This will silently ignore the migration rather than raising | ||
# if the column was already used. | ||
# | ||
# remove_column(:suppliers, :qualification, if_exists: true) | ||
def remove_column(table_name, column_name, type = nil, **options) | ||
return if options[:if_exists] == true && !column_exists?(table_name, column_name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we pass the type into the exists check here too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this and tried it out and there's an issue with passing type. For However, in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically doesn't matter what you pass for type, remove column will always remove the column because type is ignored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, perfect 👌 |
||
|
||
execute "ALTER TABLE #{quote_table_name(table_name)} #{remove_column_for_alter(table_name, column_name, type, **options)}" | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 astring
? 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woot 🙌