Fix `transaction` reverting for migrations
[fatkodima & David Verhasselt]
This commit is contained in:
parent
6556898884
commit
bdd8d58987
|
@ -1,3 +1,10 @@
|
|||
* Fix `transaction` reverting for migrations.
|
||||
|
||||
Before: Commands inside a `transaction` in a reverted migration ran uninverted.
|
||||
Now: This change fixes that by reverting commands inside `transaction` block.
|
||||
|
||||
*fatkodima*, *David Verhasselt*
|
||||
|
||||
* Raise an error instead of scanning the filesystem root when `fixture_path` is blank.
|
||||
|
||||
*Gannon McGibbon*, *Max Albrecht*
|
||||
|
|
|
@ -678,15 +678,13 @@ module ActiveRecord
|
|||
if connection.respond_to? :revert
|
||||
connection.revert { yield }
|
||||
else
|
||||
recorder = CommandRecorder.new(connection)
|
||||
recorder = command_recorder
|
||||
@connection = recorder
|
||||
suppress_messages do
|
||||
connection.revert { yield }
|
||||
end
|
||||
@connection = recorder.delegate
|
||||
recorder.commands.each do |cmd, args, block|
|
||||
send(cmd, *args, &block)
|
||||
end
|
||||
recorder.replay(self)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -962,6 +960,10 @@ module ActiveRecord
|
|||
yield
|
||||
end
|
||||
end
|
||||
|
||||
def command_recorder
|
||||
CommandRecorder.new(connection)
|
||||
end
|
||||
end
|
||||
|
||||
# MigrationProxy is used to defer loading of the actual migration classes
|
||||
|
|
|
@ -108,11 +108,17 @@ module ActiveRecord
|
|||
yield delegate.update_table_definition(table_name, self)
|
||||
end
|
||||
|
||||
def replay(migration)
|
||||
commands.each do |cmd, args, block|
|
||||
migration.send(cmd, *args, &block)
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
module StraightReversions # :nodoc:
|
||||
private
|
||||
{ transaction: :transaction,
|
||||
{
|
||||
execute_block: :execute_block,
|
||||
create_table: :drop_table,
|
||||
create_join_table: :drop_join_table,
|
||||
|
@ -133,6 +139,17 @@ module ActiveRecord
|
|||
|
||||
include StraightReversions
|
||||
|
||||
def invert_transaction(args)
|
||||
sub_recorder = CommandRecorder.new(delegate)
|
||||
sub_recorder.revert { yield }
|
||||
|
||||
invertions_proc = proc {
|
||||
sub_recorder.replay(self)
|
||||
}
|
||||
|
||||
[:transaction, args, invertions_proc]
|
||||
end
|
||||
|
||||
def invert_drop_table(args, &block)
|
||||
if args.size == 1 && block == nil
|
||||
raise ActiveRecord::IrreversibleMigration, "To avoid mistakes, drop_table is only reversible if given options or a block (can be empty)."
|
||||
|
|
|
@ -16,6 +16,21 @@ module ActiveRecord
|
|||
V6_0 = Current
|
||||
|
||||
class V5_2 < V6_0
|
||||
module CommandRecorder
|
||||
def invert_transaction(args, &block)
|
||||
[:transaction, args, block]
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def command_recorder
|
||||
recorder = super
|
||||
class << recorder
|
||||
prepend CommandRecorder
|
||||
end
|
||||
recorder
|
||||
end
|
||||
end
|
||||
|
||||
class V5_1 < V5_2
|
||||
|
|
|
@ -22,6 +22,14 @@ module ActiveRecord
|
|||
end
|
||||
end
|
||||
|
||||
class InvertibleTransactionMigration < InvertibleMigration
|
||||
def change
|
||||
transaction do
|
||||
super
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
class InvertibleRevertMigration < SilentMigration
|
||||
def change
|
||||
revert do
|
||||
|
@ -271,6 +279,14 @@ module ActiveRecord
|
|||
assert_not revert.connection.table_exists?("horses")
|
||||
end
|
||||
|
||||
def test_migrate_revert_transaction
|
||||
migration = InvertibleTransactionMigration.new
|
||||
migration.migrate :up
|
||||
assert migration.connection.table_exists?("horses")
|
||||
migration.migrate :down
|
||||
assert_not migration.connection.table_exists?("horses")
|
||||
end
|
||||
|
||||
def test_migrate_revert_change_column_default
|
||||
migration1 = ChangeColumnDefault1.new
|
||||
migration1.migrate(:up)
|
||||
|
|
|
@ -360,6 +360,16 @@ module ActiveRecord
|
|||
@recorder.inverse_of :remove_foreign_key, [:dogs]
|
||||
end
|
||||
end
|
||||
|
||||
def test_invert_transaction_with_irreversible_inside_is_irreversible
|
||||
assert_raises(ActiveRecord::IrreversibleMigration) do
|
||||
@recorder.revert do
|
||||
@recorder.transaction do
|
||||
@recorder.execute "some sql"
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -127,6 +127,20 @@ module ActiveRecord
|
|||
assert_match(/LegacyMigration < ActiveRecord::Migration\[4\.2\]/, e.message)
|
||||
end
|
||||
|
||||
def test_legacy_migrations_not_raise_exception_on_reverting_transaction
|
||||
migration = Class.new(ActiveRecord::Migration[5.2]) {
|
||||
def change
|
||||
transaction do
|
||||
execute "select 1"
|
||||
end
|
||||
end
|
||||
}.new
|
||||
|
||||
assert_nothing_raised do
|
||||
migration.migrate(:down)
|
||||
end
|
||||
end
|
||||
|
||||
if current_adapter?(:PostgreSQLAdapter)
|
||||
class Testing < ActiveRecord::Base
|
||||
end
|
||||
|
|
Loading…
Reference in New Issue