From 95ed7e7809a9f5939ffb8dc9232e7739c39d9778 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 28 Jan 2020 10:19:22 -0500 Subject: [PATCH] Add support for `if_exists/if_not_exists` on `remove_column/add_column` 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. --- activerecord/CHANGELOG.md | 24 +++ .../abstract/schema_statements.rb | 16 ++ activerecord/test/cases/migration_test.rb | 174 ++++++++++++++++++ 3 files changed, 214 insertions(+) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 88838b6923..a63579a66b 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -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 + 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. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 2b07401147..4e22a84dd8 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -536,6 +536,9 @@ def drop_table(table_name, **options) # column will have the same collation as the table. # * :comment - # Specifies the comment for the column. This option is ignored by some backends. + # * :if_not_exists - + # 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) + execute "ALTER TABLE #{quote_table_name(table_name)} #{remove_column_for_alter(table_name, column_name, type, **options)}" end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 227f2dcd2b..7338a2f8e4 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -169,6 +169,180 @@ def test_create_table_with_force_true_does_not_drop_nonexisting_table Person.connection.drop_table :testings2, if_exists: true end + def test_remove_column_with_if_not_exists_not_set + migration_a = Class.new(ActiveRecord::Migration::Current) { + def version; 100 end + def migrate(x) + add_column "people", "last_name", :string + end + }.new + + migration_b = Class.new(ActiveRecord::Migration::Current) { + def version; 101 end + def migrate(x) + remove_column "people", "last_name" + end + }.new + + migration_c = Class.new(ActiveRecord::Migration::Current) { + def version; 102 end + def migrate(x) + remove_column "people", "last_name" + end + }.new + + ActiveRecord::Migrator.new(:up, [migration_a], @schema_migration, 100).migrate + assert_column Person, :last_name, "migration_a should have added the last_name column on people" + + ActiveRecord::Migrator.new(:up, [migration_b], @schema_migration, 101).migrate + assert_no_column Person, :last_name, "migration_b should have dropped the last_name column on people" + + migrator = ActiveRecord::Migrator.new(:up, [migration_c], @schema_migration, 102) + + if current_adapter?(:SQLite3Adapter) + assert_nothing_raised do + migrator.migrate + end + else + error = assert_raises do + migrator.migrate + end + + if current_adapter?(:Mysql2Adapter) + if ActiveRecord::Base.connection.mariadb? + assert_match(/Can't DROP COLUMN `last_name`; check that it exists/, error.message) + else + assert_match(/check that column\/key exists/, error.message) + end + elsif + assert_match(/column \"last_name\" of relation \"people\" does not exist/, error.message) + end + end + ensure + Person.reset_column_information + end + + def test_remove_column_with_if_exists_set + migration_a = Class.new(ActiveRecord::Migration::Current) { + def version; 100 end + def migrate(x) + add_column "people", "last_name", :string + end + }.new + + migration_b = Class.new(ActiveRecord::Migration::Current) { + def version; 101 end + def migrate(x) + remove_column "people", "last_name" + end + }.new + + migration_c = Class.new(ActiveRecord::Migration::Current) { + def version; 102 end + def migrate(x) + remove_column "people", "last_name", if_exists: true + end + }.new + + ActiveRecord::Migrator.new(:up, [migration_a], @schema_migration, 100).migrate + assert_column Person, :last_name, "migration_a should have added the last_name column on people" + + ActiveRecord::Migrator.new(:up, [migration_b], @schema_migration, 101).migrate + assert_no_column Person, :last_name, "migration_b should have dropped the last_name column on people" + + migrator = ActiveRecord::Migrator.new(:up, [migration_c], @schema_migration, 102) + + assert_nothing_raised do + migrator.migrate + end + ensure + Person.reset_column_information + end + + def test_add_column_with_if_not_exists_not_set + migration_a = Class.new(ActiveRecord::Migration::Current) { + def version; 100 end + def migrate(x) + add_column "people", "last_name", :string + end + }.new + + migration_b = Class.new(ActiveRecord::Migration::Current) { + def version; 101 end + def migrate(x) + add_column "people", "last_name", :string + end + }.new + + ActiveRecord::Migrator.new(:up, [migration_a], @schema_migration, 100).migrate + assert_column Person, :last_name, "migration_a should have created the last_name column on people" + + assert_raises do + ActiveRecord::Migrator.new(:up, [migration_b], @schema_migration, 101).migrate + end + ensure + Person.reset_column_information + if Person.column_names.include?("last_name") + Person.connection.remove_column("people", "last_name") + end + end + + def test_add_column_with_if_not_exists_set_to_true + migration_a = Class.new(ActiveRecord::Migration::Current) { + def version; 100 end + def migrate(x) + add_column "people", "last_name", :string + end + }.new + + migration_b = Class.new(ActiveRecord::Migration::Current) { + def version; 101 end + def migrate(x) + add_column "people", "last_name", :string, if_not_exists: true + end + }.new + + ActiveRecord::Migrator.new(:up, [migration_a], @schema_migration, 100).migrate + assert_column Person, :last_name, "migration_a should have created the last_name column on people" + + assert_nothing_raised do + ActiveRecord::Migrator.new(:up, [migration_b], @schema_migration, 101).migrate + end + ensure + Person.reset_column_information + if Person.column_names.include?("last_name") + Person.connection.remove_column("people", "last_name") + end + end + + def test_add_column_with_if_not_exists_set_to_true_still_raises_if_type_is_different + migration_a = Class.new(ActiveRecord::Migration::Current) { + def version; 100 end + def migrate(x) + add_column "people", "last_name", :string + end + }.new + + migration_b = Class.new(ActiveRecord::Migration::Current) { + def version; 101 end + def migrate(x) + add_column "people", "last_name", :boolean, if_not_exists: true + end + }.new + + ActiveRecord::Migrator.new(:up, [migration_a], @schema_migration, 100).migrate + assert_column Person, :last_name, "migration_a should have created the last_name column on people" + + assert_raises do + ActiveRecord::Migrator.new(:up, [migration_b], @schema_migration, 101).migrate + end + ensure + Person.reset_column_information + if Person.column_names.include?("last_name") + Person.connection.remove_column("people", "last_name") + end + end + def test_migration_instance_has_connection migration = Class.new(ActiveRecord::Migration::Current).new assert_equal ActiveRecord::Base.connection, migration.connection -- GitLab