Merge pull request #36589 from yskkin/reversible_remove_columns

Reversible remove_columns

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

@@ -597,10 +597,15 @@ def add_column(table_name, column_name, type, **options)
       #
       #   remove_columns(:suppliers, :qualification, :experience)
       #
+      # +type+ and other column options can be passed to make migration reversible.
+      #
+      #    remove_columns(:suppliers, :qualification, :experience, type: :string, null: false)
       def remove_columns(table_name, *column_names)
         raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") if column_names.empty?
+        column_options = column_names.extract_options!
+        type = column_options.delete(:type)
         column_names.each do |column_name|
-          remove_column(table_name, column_name)
+          remove_column(table_name, column_name, type, column_options)
         end
       end
 

activerecord/lib/active_record/migration/command_recorder.rb

@@ -73,7 +73,12 @@ def revert
       #   recorder.record(:method_name, [:arg1, :arg2])
       def record(*command, &block)
         if @reverting
-          @commands << inverse_of(*command, &block)
+          inverse_command = inverse_of(*command, &block)
+          if inverse_command[0].is_a?(Array)
+            @commands = @commands.concat(inverse_command)
+          else
+            @commands << inverse_command
+          end
         else
           @commands << (command << block)
         end
@@ -84,6 +89,11 @@ def record(*command, &block)
       #   recorder.inverse_of(:rename_table, [:old, :new])
       #   # => [:rename_table, [:new, :old]]
       #
+      # If the inverse of a command requires several commands, returns array of commands.
+      #
+      #   recorder.inverse_of(:remove_columns, [:some_table, :foo, :bar, type: :string])
+      #   # => [[:add_column, :some_table, :foo, :string], [:add_column, :some_table, :bar, :string]]
+      #
       # This method will raise an +IrreversibleMigration+ exception if it cannot
       # invert the +command+.
       def inverse_of(command, args, &block)
@@ -168,6 +178,18 @@ def invert_remove_column(args)
           super
         end
 
+        def invert_remove_columns(args)
+          unless args[-1].is_a?(Hash) && args[-1].has_key?(:type)
+            raise ActiveRecord::IrreversibleMigration, "remove_columns is only reversible if given a type."
+          end
+
+          column_options = args.extract_options!
+          type = column_options.delete(:type)
+          args[1..-1].map do |arg|
+            [:add_column, [args[0], arg, type, column_options]]
+          end
+        end
+
         def invert_rename_index(args)
           [:rename_index, [args.first] + args.last(2).reverse]
         end

activerecord/test/cases/invertible_migration_test.rb

@@ -22,6 +22,15 @@ def change
       end
     end
 
+    class InvertibleChangeTableMigration < SilentMigration
+      def change
+        change_table("horses") do |t|
+          t.column :name, :string
+          t.remove :remind_at, type: :datetime
+        end
+      end
+    end
+
     class InvertibleTransactionMigration < InvertibleMigration
       def change
         transaction do
@@ -264,6 +273,15 @@ def test_migrate_revert
       assert_not migration.connection.table_exists?("horses")
     end
 
+    def test_migrate_revert_change_table
+      InvertibleMigration.new.migrate :up
+      migration = InvertibleChangeTableMigration.new
+      migration.migrate :up
+      assert_not migration.connection.column_exists?(:horses, :remind_at)
+      migration.migrate :down
+      assert migration.connection.column_exists?(:horses, :remind_at)
+    end
+
     def test_migrate_revert_by_part
       InvertibleMigration.new.migrate :up
       received = []

activerecord/test/cases/migration/change_table_test.rb

@@ -295,6 +295,13 @@ def test_remove_drops_multiple_columns
         end
       end
 
+      def test_remove_drops_multiple_columns_when_column_options_are_given
+        with_change_table do |t|
+          @connection.expect :remove_columns, nil, [:delete_me, :bar, :baz, type: :string, null: false]
+          t.remove :bar, :baz, type: :string, null: false
+        end
+      end
+
       def test_remove_index_removes_index_with_options
         with_change_table do |t|
           @connection.expect :remove_index, nil, [:delete_me, :bar, { unique: true }]