make change_column_comment and change_table_comment invertible

We can revert migrations using `change_column_comment` or
`change_table_comment` at current master.
However, results are not what we expect: comments are remained in new
status.
This change tells previous comment to these methods in a way like
`change_column_default`.

activerecord/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   `change_column_comment` and `change_table_comment` are invertible only if
+    `to` and `from` options are specified.
+
+    *Yoshiyuki Kinjo*
+
 *   Don't call commit/rollback callbacks despite a record isn't saved.
 
     Fixes #29747.

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

@@ -1185,12 +1185,22 @@ def options_include_default?(options)
       end
 
       # Changes the comment for a table or removes it if +nil+.
-      def change_table_comment(table_name, comment)
+      #
+      # Passing a hash containing +:from+ and +:to+ will make this change
+      # reversible in migration:
+      #
+      #   change_table_comment(:posts, from: "old_comment", to: "new_comment")
+      def change_table_comment(table_name, comment_or_changes)
         raise NotImplementedError, "#{self.class} does not support changing table comments"
       end
 
       # Changes the comment for a column or removes it if +nil+.
-      def change_column_comment(table_name, column_name, comment)
+      #
+      # Passing a hash containing +:from+ and +:to+ will make this change
+      # reversible in migration:
+      #
+      #   change_column_comment(:posts, :state, from: "old_comment", to: "new_comment")
+      def change_column_comment(table_name, column_name, comment_or_changes)
         raise NotImplementedError, "#{self.class} does not support changing column comments"
       end
 
@@ -1374,6 +1384,7 @@ def extract_new_default_value(default_or_changes)
             default_or_changes
           end
         end
+        alias :extract_new_comment_value :extract_new_default_value
 
         def can_remove_index_by_name?(options)
           options.is_a?(Hash) && options.key?(:name) && options.except(:name, :algorithm).empty?

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -285,7 +285,8 @@ def table_comment(table_name) # :nodoc:
         SQL
       end
 
-      def change_table_comment(table_name, comment) #:nodoc:
+      def change_table_comment(table_name, comment_or_changes) # :nodoc:
+        comment = extract_new_comment_value(comment_or_changes)
         comment = "" if comment.nil?
         execute("ALTER TABLE #{quote_table_name(table_name)} COMMENT #{quote(comment)}")
       end
@@ -341,7 +342,8 @@ def change_column_null(table_name, column_name, null, default = nil) #:nodoc:
         change_column table_name, column_name, nil, null: null
       end
 
-      def change_column_comment(table_name, column_name, comment) #:nodoc:
+      def change_column_comment(table_name, column_name, comment_or_changes) # :nodoc:
+        comment = extract_new_comment_value(comment_or_changes)
         change_column table_name, column_name, nil, comment: comment
       end
 

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

@@ -418,14 +418,16 @@ def change_column_null(table_name, column_name, null, default = nil) #:nodoc:
         end
 
         # Adds comment for given table column or drops it if +comment+ is a +nil+
-        def change_column_comment(table_name, column_name, comment) # :nodoc:
+        def change_column_comment(table_name, column_name, comment_or_changes) # :nodoc:
           clear_cache!
+          comment = extract_new_comment_value(comment_or_changes)
           execute "COMMENT ON COLUMN #{quote_table_name(table_name)}.#{quote_column_name(column_name)} IS #{quote(comment)}"
         end
 
         # Adds comment for given table or drops it if +comment+ is a +nil+
-        def change_table_comment(table_name, comment) # :nodoc:
+        def change_table_comment(table_name, comment_or_changes) # :nodoc:
           clear_cache!
+          comment = extract_new_comment_value(comment_or_changes)
           execute "COMMENT ON TABLE #{quote_table_name(table_name)} IS #{quote(comment)}"
         end
 

activerecord/lib/active_record/migration/command_recorder.rb

@@ -14,6 +14,8 @@ class Migration
     # * change_column
     # * change_column_default (must supply a :from and :to option)
     # * change_column_null
+    # * change_column_comment (must supply a :from and :to option)
+    # * change_table_comment (must supply a :from and :to option)
     # * create_join_table
     # * create_table
     # * disable_extension
@@ -35,7 +37,8 @@ class CommandRecorder
         :change_column_default, :add_reference, :remove_reference, :transaction,
         :drop_join_table, :drop_table, :execute_block, :enable_extension, :disable_extension,
         :change_column, :execute, :remove_columns, :change_column_null,
-        :add_foreign_key, :remove_foreign_key
+        :add_foreign_key, :remove_foreign_key,
+        :change_column_comment, :change_table_comment
       ]
       include JoinTable
 
@@ -244,6 +247,26 @@ def invert_remove_foreign_key(args)
           [:add_foreign_key, reversed_args]
         end
 
+        def invert_change_column_comment(args)
+          table, column, options = *args
+
+          unless options && options.is_a?(Hash) && options.has_key?(:from) && options.has_key?(:to)
+            raise ActiveRecord::IrreversibleMigration, "change_column_comment is only reversible if given a :from and :to option."
+          end
+
+          [:change_column_comment, [table, column, from: options[:to], to: options[:from]]]
+        end
+
+        def invert_change_table_comment(args)
+          table, options = *args
+
+          unless options && options.is_a?(Hash) && options.has_key?(:from) && options.has_key?(:to)
+            raise ActiveRecord::IrreversibleMigration, "change_table_comment is only reversible if given a :from and :to option."
+          end
+
+          [:change_table_comment, [table, from: options[:to], to: options[:from]]]
+        end
+
         def respond_to_missing?(method, _)
           super || delegate.respond_to?(method)
         end

activerecord/lib/active_record/migration/compatibility.rb

@@ -27,6 +27,16 @@ module CommandRecorder
           def invert_transaction(args, &block)
             [:transaction, args, block]
           end
+
+          def invert_change_column_comment(args)
+            table_name, column_name, comment = args
+            [:change_column_comment, [table_name, column_name, from: comment, to: comment]]
+          end
+
+          def invert_change_table_comment(args)
+            table_name, comment = args
+            [:change_table_comment, [table_name, from: comment, to: comment]]
+          end
         end
 
         def create_table(table_name, **options)

activerecord/test/cases/invertible_migration_test.rb

@@ -103,6 +103,32 @@ def change
       end
     end
 
+    class ChangeColumnComment1 < SilentMigration
+      def change
+        create_table("horses") do |t|
+          t.column :name, :string, comment: "Sekitoba"
+        end
+      end
+    end
+
+    class ChangeColumnComment2 < SilentMigration
+      def change
+        change_column_comment :horses, :name, from: "Sekitoba", to: "Diomed"
+      end
+    end
+
+    class ChangeTableComment1 < SilentMigration
+      def change
+        create_table("horses", comment: "Sekitoba")
+      end
+    end
+
+    class ChangeTableComment2 < SilentMigration
+      def change
+        change_table_comment :horses, from: "Sekitoba", to: "Diomed"
+      end
+    end
+
     class DisableExtension1 < SilentMigration
       def change
         enable_extension "hstore"
@@ -290,6 +316,7 @@ def test_migrate_revert_transaction
     def test_migrate_revert_change_column_default
       migration1 = ChangeColumnDefault1.new
       migration1.migrate(:up)
+      Horse.reset_column_information
       assert_equal "Sekitoba", Horse.new.name
 
       migration2 = ChangeColumnDefault2.new
@@ -302,6 +329,38 @@ def test_migrate_revert_change_column_default
       assert_equal "Sekitoba", Horse.new.name
     end
 
+    if ActiveRecord::Base.connection.supports_comments?
+      def test_migrate_revert_change_column_comment
+        migration1 = ChangeColumnComment1.new
+        migration1.migrate(:up)
+        Horse.reset_column_information
+        assert_equal "Sekitoba", Horse.columns_hash["name"].comment
+
+        migration2 = ChangeColumnComment2.new
+        migration2.migrate(:up)
+        Horse.reset_column_information
+        assert_equal "Diomed", Horse.columns_hash["name"].comment
+
+        migration2.migrate(:down)
+        Horse.reset_column_information
+        assert_equal "Sekitoba", Horse.columns_hash["name"].comment
+      end
+
+      def test_migrate_revert_change_table_comment
+        connection = ActiveRecord::Base.connection
+        migration1 = ChangeTableComment1.new
+        migration1.migrate(:up)
+        assert_equal "Sekitoba", connection.table_comment("horses")
+
+        migration2 = ChangeTableComment2.new
+        migration2.migrate(:up)
+        assert_equal "Diomed", connection.table_comment("horses")
+
+        migration2.migrate(:down)
+        assert_equal "Sekitoba", connection.table_comment("horses")
+      end
+    end
+
     if current_adapter?(:PostgreSQLAdapter)
       def test_migrate_enable_and_disable_extension
         migration1 = InvertibleMigration.new

activerecord/test/cases/migration/command_recorder_test.rb

@@ -182,6 +182,40 @@ def test_invert_change_column_default_with_from_and_to_with_boolean
         assert_equal [:change_column_default, [:table, :column, from: false, to: true]], change
       end
 
+      if ActiveRecord::Base.connection.supports_comments?
+        def test_invert_change_column_comment
+          assert_raises(ActiveRecord::IrreversibleMigration) do
+            @recorder.inverse_of :change_column_comment, [:table, :column, "comment"]
+          end
+        end
+
+        def test_invert_change_column_comment_with_from_and_to
+          change = @recorder.inverse_of :change_column_comment, [:table, :column, from: "old_value", to: "new_value"]
+          assert_equal [:change_column_comment, [:table, :column, from: "new_value", to: "old_value"]], change
+        end
+
+        def test_invert_change_column_comment_with_from_and_to_with_nil
+          change = @recorder.inverse_of :change_column_comment, [:table, :column, from: nil, to: "new_value"]
+          assert_equal [:change_column_comment, [:table, :column, from: "new_value", to: nil]], change
+        end
+
+        def test_invert_change_table_comment
+          assert_raises(ActiveRecord::IrreversibleMigration) do
+            @recorder.inverse_of :change_column_comment, [:table, :column, "comment"]
+          end
+        end
+
+        def test_invert_change_table_comment_with_from_and_to
+          change = @recorder.inverse_of :change_table_comment, [:table, from: "old_value", to: "new_value"]
+          assert_equal [:change_table_comment, [:table, from: "new_value", to: "old_value"]], change
+        end
+
+        def test_invert_change_table_comment_with_from_and_to_with_nil
+          change = @recorder.inverse_of :change_table_comment, [:table, from: nil, to: "new_value"]
+          assert_equal [:change_table_comment, [:table, from: "new_value", to: nil]], change
+        end
+      end
+
       def test_invert_change_column_null
         add = @recorder.inverse_of :change_column_null, [:table, :column, true]
         assert_equal [:change_column_null, [:table, :column, false]], add

activerecord/test/cases/migration/compatibility_test.rb

@@ -220,6 +220,35 @@ def change
         end
       end
 
+      if ActiveRecord::Base.connection.supports_comments?
+        def test_change_column_comment_can_be_reverted
+          migration = Class.new(ActiveRecord::Migration[5.2]) {
+            def migrate(x)
+              revert do
+                change_column_comment(:testings, :foo, "comment")
+              end
+            end
+          }.new
+
+          ActiveRecord::Migrator.new(:up, [migration]).migrate
+          assert connection.column_exists?(:testings, :foo, comment: "comment")
+        end
+
+        def test_change_table_comment_can_be_reverted
+          migration = Class.new(ActiveRecord::Migration[5.2]) {
+            def migrate(x)
+              revert do
+                change_table_comment(:testings, "comment")
+              end
+            end
+          }.new
+
+          ActiveRecord::Migrator.new(:up, [migration]).migrate
+
+          assert_equal "comment", connection.table_comment("testings")
+        end
+      end
+
       if current_adapter?(:PostgreSQLAdapter)
         class Testing < ActiveRecord::Base
         end