From 1fe71ebd0486299de36d4ed551d38043ce8a419b Mon Sep 17 00:00:00 2001 From: Yoshiyuki Kinjo Date: Sun, 14 Apr 2019 20:31:05 +0900 Subject: 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 | 5 ++ .../abstract/schema_statements.rb | 15 +++++- .../connection_adapters/abstract_mysql_adapter.rb | 6 ++- .../postgresql/schema_statements.rb | 6 ++- .../active_record/migration/command_recorder.rb | 25 ++++++++- .../lib/active_record/migration/compatibility.rb | 10 ++++ .../test/cases/invertible_migration_test.rb | 59 ++++++++++++++++++++++ .../test/cases/migration/command_recorder_test.rb | 34 +++++++++++++ .../test/cases/migration/compatibility_test.rb | 29 +++++++++++ 9 files changed, 182 insertions(+), 7 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3b8767787f..7ae6326fc3 100644 --- a/activerecord/CHANGELOG.md +++ b/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. 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 8ba9943f75..7041d92586 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -1185,12 +1185,22 @@ module ActiveRecord 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 @@ module ActiveRecord 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? diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 2a2b234ef8..8b907759c6 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -285,7 +285,8 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index ec8af73d07..40c5e51d92 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -418,14 +418,16 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 8e7f596076..efed4b0e26 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -14,6 +14,8 @@ module ActiveRecord # * 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 @@ module ActiveRecord :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 @@ module ActiveRecord [: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 diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index abc939826b..ff91218696 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -27,6 +27,16 @@ module ActiveRecord 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) diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb index d68cc40107..7f67b945f0 100644 --- a/activerecord/test/cases/invertible_migration_test.rb +++ b/activerecord/test/cases/invertible_migration_test.rb @@ -103,6 +103,32 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb index 01f8628fc5..c9f3756b1f 100644 --- a/activerecord/test/cases/migration/command_recorder_test.rb +++ b/activerecord/test/cases/migration/command_recorder_test.rb @@ -182,6 +182,40 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb index eaf88bf698..726ccf925e 100644 --- a/activerecord/test/cases/migration/compatibility_test.rb +++ b/activerecord/test/cases/migration/compatibility_test.rb @@ -220,6 +220,35 @@ module ActiveRecord 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 -- cgit v1.2.3