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.

Author: eileencodes

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.

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.
       # * <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)
+
         execute "ALTER TABLE #{quote_table_name(table_name)} #{remove_column_for_alter(table_name, column_name, type, **options)}"
       end
 

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