Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Migration reversible #8177

Closed
wants to merge 4 commits into from

3 participants

@marcandre

The commit of this PR builds on those of #7627, but the need they both fulfill is independent. Both functionalities can be accepted/rejected independently.

It is currently impossible to create a change migration that has a some data processing that rely on schema changes. Indeed, when migrating down, the schema modifying methods will be recorded and only played back (inverted) after the data processing took place.

This PR aims at providing a mechanism to process data at the right moment and conditionally of the direction. Please see the documented example in marcandre@migrati

@steveklabnik
Collaborator

This does not merge properly, and will need rebased.

@marcandre

@steveklabnik I've rebase both #7627 and this PR, but this won't last because changes to the Changelog (present in #7267) break the merge. Or am I missing something?

Ideally, #7627 will be accepted and merged, this would simplify looking at this PR.

@marcandre marcandre referenced this pull request
Closed

Migration revert #7627

@marcandre

I should have mentioned that this PR also makes transaction{} invertible as its own inverse.

AFAIK, transaction add nothing in a migration (which is always inside one anyways), but this makes it easy to implement reversible.

@marcandre marcandre referenced this pull request
Merged

Reversible commands #8267

@steveklabnik
Collaborator

this won't last because changes to the Changelog

Ah yes, damn Changelogs.

@rafaelfranca

Seems good to me, but need the documentation too.

@steveklabnik
Collaborator

@marcandre can you squash these commits? Since it seems you have the docs @rafaelfranca asked for, I think we can merge after that.

@marcandre

@tenderlove told me all 3 proposals were acceptable, so I'm closing this in favor of #8267

@marcandre marcandre closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
162 activerecord/lib/active_record/migration.rb
@@ -373,22 +373,129 @@ def initialize(name = self.class.name, version = nil)
@name = name
@version = version
@connection = nil
- @reverting = false
end
# instantiate the delegate object after initialize is defined
self.verbose = true
self.delegate = new
- def revert
- @reverting = true
- yield
- ensure
- @reverting = false
+ # Reverses the migration commands for the given block and
+ # the given migrations.
+ #
+ # The following migration will remove the table 'horses'
+ # and create the table 'apples' on the way up, and the reverse
+ # on the way down.
+ #
+ # class FixTLMigration < ActiveRecord::Migration
+ # def change
+ # revert do
+ # create_table(:horses) do |t|
+ # t.text :content
+ # t.datetime :remind_at
+ # end
+ # end
+ # create_table(:apples) do |t|
+ # t.string :variety
+ # end
+ # end
+ # end
+ #
+ # Or equivalently, if +TenderloveMigration+ is defined as in the
+ # documentation for Migration:
+ #
+ # require_relative '2012121212_tenderlove_migration'
+ #
+ # class FixupTLMigration < ActiveRecord::Migration
+ # def change
+ # revert TenderloveMigration
+ #
+ # create_table(:apples) do |t|
+ # t.string :variety
+ # end
+ # end
+ # end
+ #
+ # This command can be nested.
+ def revert(*migration_classes)
+ run(*migration_classes.reverse, revert: true) unless migration_classes.empty?
+ if block_given?
+ if @connection.respond_to? :revert
+ @connection.revert { yield }
+ else
+ recorder = CommandRecorder.new(@connection)
+ @connection = recorder
+ suppress_messages do
+ @connection.revert { yield }
+ end
+ @connection = recorder.delegate
+ recorder.commands.each do |cmd, args, block|
+ send(cmd, *args, &block)
+ end
+ end
+ end
end
def reverting?
- @reverting
+ @connection.respond_to?(:reverting) && @connection.reverting
+ end
+
+ class ReversibleBlockHelper < Struct.new(:reverting)
+ def up
+ yield unless reverting
+ end
+
+ def down
+ yield if reverting
+ end
+ end
+
+ # Used to specify an operation that can be run in one direction or another.
+ # Call the methods +up+ and +down+ of the yielded object to run a block
+ # only in one given direction.
+ # The whole block will be called in the right order within the migration.
+ #
+ # In the following example, the looping on users will always be done
+ # when the three columns 'first_name', 'last_name' and 'full_name' exist,
+ # even when migrating down:
+ #
+ # class SplitNameMigration < ActiveRecord::Migration
+ # def change
+ # add_column :users, :first_name, :string
+ # add_column :users, :last_name, :string
+ #
+ # reversible do |dir|
+ # User.reset_column_information
+ # User.all.each do |u|
+ # dir.up { u.first_name, u.last_name = u.full_name.split(' ') }
+ # dir.down { u.full_name = "#{u.first_name} #{u.last_name}" }
+ # u.save
+ # end
+ # end
+ #
+ # revert { add_column :users, :full_name, :string }
+ # end
+ # end
+ def reversible
+ helper = ReversibleBlockHelper.new(reverting?)
+ transaction{ yield helper }
+ end
+
+ # Runs the given migration classes.
+ # Last argument can specify options:
+ # - :direction (default is :up)
+ # - :revert (default is false)
+ def run(*migration_classes)
+ opts = migration_classes.extract_options!
+ dir = opts[:direction] || :up
+ dir = (dir == :down ? :up : :down) if opts[:revert]
+ if reverting?
+ # If in revert and going :up, say, we want to execute :down without reverting, so
+ revert { run(*migration_classes, direction: dir, revert: true) }
+ else
+ migration_classes.each do |migration_class|
+ migration_class.new.exec_migration(@connection, dir)
+ end
+ end
end
def up
@@ -414,29 +521,9 @@ def migrate(direction)
time = nil
ActiveRecord::Base.connection_pool.with_connection do |conn|
- @connection = conn
- if respond_to?(:change)
- if direction == :down
- recorder = CommandRecorder.new(@connection)
- suppress_messages do
- @connection = recorder
- change
- end
- @connection = conn
- time = Benchmark.measure {
- self.revert {
- recorder.inverse.each do |cmd, args|
- send(cmd, *args)
- end
- }
- }
- else
- time = Benchmark.measure { change }
- end
- else
- time = Benchmark.measure { send(direction) }
+ time = Benchmark.measure do
+ exec_migration(conn, direction)
end
- @connection = nil
end
case direction
@@ -445,6 +532,21 @@ def migrate(direction)
end
end
+ def exec_migration(conn, direction)
+ @connection = conn
+ if respond_to?(:change)
+ if direction == :down
+ revert { change }
+ else
+ change
+ end
+ else
+ send(direction)
+ end
+ ensure
+ @connection = nil
+ end
+
def write(text="")
puts(text) if verbose
end
@@ -483,7 +585,7 @@ def method_missing(method, *arguments, &block)
arg_list = arguments.map{ |a| a.inspect } * ', '
say_with_time "#{method}(#{arg_list})" do
- unless reverting?
+ unless @connection.respond_to? :revert
unless arguments.empty? || method == :execute
arguments[0] = Migrator.proper_table_name(arguments.first)
arguments[1] = Migrator.proper_table_name(arguments.second) if method == :rename_table
View
59 activerecord/lib/active_record/migration/command_recorder.rb
@@ -16,46 +16,65 @@ class Migration
class CommandRecorder
include JoinTable
- attr_accessor :commands, :delegate
+ attr_accessor :commands, :delegate, :reverting
def initialize(delegate = nil)
@commands = []
@delegate = delegate
+ @reverting = false
+ end
+
+ # While executing the given block, the recorded will be in reverting mode.
+ # All commands recorded will end up being recorded reverted
+ # and in reverse order.
+ # For example:
+ #
+ # recorder.revert{ recorder.record(:rename_table, [:old, :new]) }
+ # # same effect as recorder.record(:rename_table, [:new, :old])
+ def revert
+ @reverting = !@reverting
+ previous = @commands
+ @commands = []
+ yield
+ ensure
+ @commands = previous.concat(@commands.reverse)
+ @reverting = !@reverting
end
# record +command+. +command+ should be a method name and arguments.
# For example:
#
# recorder.record(:method_name, [:arg1, :arg2])
- def record(*command)
- @commands << command
+ def record(*command, &block)
+ if @reverting
+ @commands << inverse_of(*command, &block)
+ else
+ @commands << (command << block)
+ end
end
- # Returns a list that represents commands that are the inverse of the
- # commands stored in +commands+. For example:
+ # Returns the inverse of the given command. For example:
#
- # recorder.record(:rename_table, [:old, :new])
- # recorder.inverse # => [:rename_table, [:new, :old]]
+ # recorder.inverse_of(:rename_table, [:old, :new])
+ # # => [:rename_table, [:new, :old]]
#
# This method will raise an +IrreversibleMigration+ exception if it cannot
- # invert the +commands+.
- def inverse
- @commands.reverse.map { |name, args|
- method = :"invert_#{name}"
- raise IrreversibleMigration unless respond_to?(method, true)
- send(method, args)
- }
+ # invert the +command+.
+ def inverse_of(command, args, &block)
+ method = :"invert_#{command}"
+ raise IrreversibleMigration unless respond_to?(method, true)
+ send(method, args, &block)
end
def respond_to?(*args) # :nodoc:
super || delegate.respond_to?(*args)
end
- [:create_table, :create_join_table, :change_table, :rename_table, :add_column, :remove_column, :rename_index, :rename_column, :add_index, :remove_index, :add_timestamps, :remove_timestamps, :change_column, :change_column_default, :add_reference, :remove_reference].each do |method|
+ [:create_table, :create_join_table, :change_table, :rename_table, :add_column, :remove_column, :rename_index, :rename_column, :add_index, :remove_index, :add_timestamps, :remove_timestamps, :change_column, :change_column_default, :add_reference, :remove_reference, :transaction].each do |method|
class_eval <<-EOV, __FILE__, __LINE__ + 1
- def #{method}(*args) # def create_table(*args)
- record(:"#{method}", args) # record(:create_table, args)
- end # end
+ def #{method}(*args, &block) # def create_table(*args, &block)
+ record(:"#{method}", args, &block) # record(:create_table, args, &block)
+ end # end
EOV
end
alias :add_belongs_to :add_reference
@@ -63,6 +82,10 @@ def #{method}(*args) # def create_table(*args)
private
+ def invert_transaction(args, &block)
+ [:transaction, args, block]
+ end
+
def invert_create_table(args)
[:drop_table, [args.first]]
end
View
125 activerecord/test/cases/invertible_migration_test.rb
@@ -17,6 +17,37 @@ def change
end
end
+ class InvertibleRevertMigration < SilentMigration
+ def change
+ revert do
+ create_table("horses") do |t|
+ t.column :content, :text
+ t.column :remind_at, :datetime
+ end
+ end
+ end
+ end
+
+ class InvertibleByPartsMigration < SilentMigration
+ attr_writer :test
+ def change
+ create_table("new_horses") do |t|
+ t.column :breed, :string
+ end
+ reversible do |dir|
+ @test.yield :both
+ dir.up { @test.yield :up }
+ dir.down { @test.yield :down }
+ end
+ revert do
+ create_table("horses") do |t|
+ t.column :content, :text
+ t.column :remind_at, :datetime
+ end
+ end
+ end
+ end
+
class NonInvertibleMigration < SilentMigration
def change
create_table("horses") do |t|
@@ -40,6 +71,23 @@ def self.down
end
end
+ class RevertWholeMigration < SilentMigration
+ def initialize(name = self.class.name, version = nil, migration)
+ @migration = migration
+ super(name, version)
+ end
+
+ def change
+ revert @migration
+ end
+ end
+
+ class NestedRevertWholeMigration < RevertWholeMigration
+ def change
+ revert { super }
+ end
+ end
+
def teardown
if ActiveRecord::Base.connection.table_exists?("horses")
ActiveRecord::Base.connection.drop_table("horses")
@@ -67,6 +115,83 @@ def test_migrate_down
assert !migration.connection.table_exists?("horses")
end
+ def test_migrate_revert
+ migration = InvertibleMigration.new
+ revert = InvertibleRevertMigration.new
+ migration.migrate :up
+ revert.migrate :up
+ assert !migration.connection.table_exists?("horses")
+ revert.migrate :down
+ assert migration.connection.table_exists?("horses")
+ migration.migrate :down
+ assert !migration.connection.table_exists?("horses")
+ end
+
+ def test_migrate_revert_by_part
+ InvertibleMigration.new.migrate :up
+ received = []
+ migration = InvertibleByPartsMigration.new
+ migration.test = ->(dir){
+ assert migration.connection.table_exists?("horses")
+ assert migration.connection.table_exists?("new_horses")
+ received << dir
+ }
+ migration.migrate :up
+ assert_equal [:both, :up], received
+ assert !migration.connection.table_exists?("horses")
+ assert migration.connection.table_exists?("new_horses")
+ migration.migrate :down
+ assert_equal [:both, :up, :both, :down], received
+ assert migration.connection.table_exists?("horses")
+ assert !migration.connection.table_exists?("new_horses")
+ end
+
+ def test_migrate_revert_whole_migration
+ migration = InvertibleMigration.new
+ [LegacyMigration, InvertibleMigration].each do |klass|
+ revert = RevertWholeMigration.new(klass)
+ migration.migrate :up
+ revert.migrate :up
+ assert !migration.connection.table_exists?("horses")
+ revert.migrate :down
+ assert migration.connection.table_exists?("horses")
+ migration.migrate :down
+ assert !migration.connection.table_exists?("horses")
+ end
+ end
+
+ def test_migrate_nested_revert_whole_migration
+ revert = NestedRevertWholeMigration.new(InvertibleRevertMigration)
+ revert.migrate :down
+ assert revert.connection.table_exists?("horses")
+ revert.migrate :up
+ assert !revert.connection.table_exists?("horses")
+ end
+
+ def test_revert_order
+ block = Proc.new{|t| t.string :name }
+ recorder = ActiveRecord::Migration::CommandRecorder.new(ActiveRecord::Base.connection)
+ recorder.instance_eval do
+ create_table("apples", &block)
+ revert do
+ create_table("bananas", &block)
+ revert do
+ create_table("clementines")
+ create_table("dates")
+ end
+ create_table("elderberries")
+ end
+ revert do
+ create_table("figs")
+ create_table("grapes")
+ end
+ end
+ assert_equal [[:create_table, ["apples"], block], [:drop_table, ["elderberries"]],
+ [:create_table, ["clementines"], nil], [:create_table, ["dates"], nil],
+ [:drop_table, ["bananas"]], [:drop_table, ["grapes"]],
+ [:drop_table, ["figs"]]], recorder.commands
+ end
+
def test_legacy_up
LegacyMigration.migrate :up
assert ActiveRecord::Base.connection.table_exists?("horses"), "horses should exist"
View
99 activerecord/test/cases/migration/command_recorder_test.rb
@@ -26,7 +26,7 @@ def create_table(name); end
}.new)
assert recorder.respond_to?(:create_table), 'respond_to? create_table'
recorder.send(:create_table, :horses)
- assert_equal [[:create_table, [:horses]]], recorder.commands
+ assert_equal [[:create_table, [:horses], nil]], recorder.commands
end
def test_unknown_commands_delegate
@@ -35,9 +35,8 @@ def test_unknown_commands_delegate
end
def test_unknown_commands_raise_exception_if_they_cannot_delegate
- @recorder.record :execute, ['some sql']
assert_raises(ActiveRecord::IrreversibleMigration) do
- @recorder.inverse
+ @recorder.inverse_of :execute, ['some sql']
end
end
@@ -46,120 +45,124 @@ def test_record
assert_equal 1, @recorder.commands.length
end
- def test_inverse
- @recorder.record :create_table, [:system_settings]
- assert_equal 1, @recorder.inverse.length
-
- @recorder.record :rename_table, [:old, :new]
- assert_equal 2, @recorder.inverse.length
+ def test_inverted_commands_are_reversed
+ @recorder.revert do
+ @recorder.record :create_table, [:hello]
+ @recorder.record :create_table, [:world]
+ end
+ tables = @recorder.commands.map(&:last)
+ assert_equal [[:world], [:hello]], tables
end
- def test_inverted_commands_are_reveresed
- @recorder.record :create_table, [:hello]
- @recorder.record :create_table, [:world]
- tables = @recorder.inverse.map(&:last)
- assert_equal [[:world], [:hello]], tables
+ def test_revert_order
+ block = Proc.new{|t| t.string :name }
+ @recorder.instance_eval do
+ create_table("apples", &block)
+ revert do
+ create_table("bananas", &block)
+ revert do
+ create_table("clementines")
+ create_table("dates")
+ end
+ create_table("elderberries")
+ end
+ revert do
+ create_table("figs")
+ create_table("grapes")
+ end
+ end
+ assert_equal [[:create_table, ["apples"], block], [:drop_table, ["elderberries"]],
+ [:create_table, ["clementines"], nil], [:create_table, ["dates"], nil],
+ [:drop_table, ["bananas"]], [:drop_table, ["grapes"]],
+ [:drop_table, ["figs"]]], @recorder.commands
end
+
def test_invert_create_table
- @recorder.record :create_table, [:system_settings]
- drop_table = @recorder.inverse.first
+ @recorder.revert do
+ @recorder.record :create_table, [:system_settings]
+ end
+ drop_table = @recorder.commands.first
assert_equal [:drop_table, [:system_settings]], drop_table
end
def test_invert_create_table_with_options
- @recorder.record :create_table, [:people_reminders, {:id => false}]
- drop_table = @recorder.inverse.first
+ drop_table = @recorder.inverse_of :create_table, [:people_reminders, id: false]
assert_equal [:drop_table, [:people_reminders]], drop_table
end
def test_invert_create_join_table
- @recorder.record :create_join_table, [:musics, :artists]
- drop_table = @recorder.inverse.first
+ drop_table = @recorder.inverse_of :create_join_table, [:musics, :artists]
assert_equal [:drop_table, [:artists_musics]], drop_table
end
def test_invert_create_join_table_with_table_name
- @recorder.record :create_join_table, [:musics, :artists, {:table_name => :catalog}]
- drop_table = @recorder.inverse.first
+ drop_table = @recorder.inverse_of :create_join_table, [:musics, :artists, table_name: :catalog]
assert_equal [:drop_table, [:catalog]], drop_table
end
def test_invert_rename_table
- @recorder.record :rename_table, [:old, :new]
- rename = @recorder.inverse.first
+ rename = @recorder.inverse_of :rename_table, [:old, :new]
assert_equal [:rename_table, [:new, :old]], rename
end
def test_invert_add_column
- @recorder.record :add_column, [:table, :column, :type, {}]
- remove = @recorder.inverse.first
+ remove = @recorder.inverse_of :add_column, [:table, :column, :type, {}]
assert_equal [:remove_column, [:table, :column]], remove
end
def test_invert_rename_column
- @recorder.record :rename_column, [:table, :old, :new]
- rename = @recorder.inverse.first
+ rename = @recorder.inverse_of :rename_column, [:table, :old, :new]
assert_equal [:rename_column, [:table, :new, :old]], rename
end
def test_invert_add_index
- @recorder.record :add_index, [:table, [:one, :two], {:options => true}]
- remove = @recorder.inverse.first
+ remove = @recorder.inverse_of :add_index, [:table, [:one, :two], options: true]
assert_equal [:remove_index, [:table, {:column => [:one, :two]}]], remove
end
def test_invert_add_index_with_name
- @recorder.record :add_index, [:table, [:one, :two], {:name => "new_index"}]
- remove = @recorder.inverse.first
+ remove = @recorder.inverse_of :add_index, [:table, [:one, :two], name: "new_index"]
assert_equal [:remove_index, [:table, {:name => "new_index"}]], remove
end
def test_invert_add_index_with_no_options
- @recorder.record :add_index, [:table, [:one, :two]]
- remove = @recorder.inverse.first
+ remove = @recorder.inverse_of :add_index, [:table, [:one, :two]]
assert_equal [:remove_index, [:table, {:column => [:one, :two]}]], remove
end
def test_invert_rename_index
- @recorder.record :rename_index, [:table, :old, :new]
- rename = @recorder.inverse.first
+ rename = @recorder.inverse_of :rename_index, [:table, :old, :new]
assert_equal [:rename_index, [:table, :new, :old]], rename
end
def test_invert_add_timestamps
- @recorder.record :add_timestamps, [:table]
- remove = @recorder.inverse.first
+ remove = @recorder.inverse_of :add_timestamps, [:table]
assert_equal [:remove_timestamps, [:table]], remove
end
def test_invert_remove_timestamps
- @recorder.record :remove_timestamps, [:table]
- add = @recorder.inverse.first
+ add = @recorder.inverse_of :remove_timestamps, [:table]
assert_equal [:add_timestamps, [:table]], add
end
def test_invert_add_reference
- @recorder.record :add_reference, [:table, :taggable, { polymorphic: true }]
- remove = @recorder.inverse.first
+ remove = @recorder.inverse_of :add_reference, [:table, :taggable, { polymorphic: true }]
assert_equal [:remove_reference, [:table, :taggable, { polymorphic: true }]], remove
end
def test_invert_add_belongs_to_alias
- @recorder.record :add_belongs_to, [:table, :user]
- remove = @recorder.inverse.first
+ remove = @recorder.inverse_of :add_belongs_to, [:table, :user]
assert_equal [:remove_reference, [:table, :user]], remove
end
def test_invert_remove_reference
- @recorder.record :remove_reference, [:table, :taggable, { polymorphic: true }]
- add = @recorder.inverse.first
+ add = @recorder.inverse_of :remove_reference, [:table, :taggable, { polymorphic: true }]
assert_equal [:add_reference, [:table, :taggable, { polymorphic: true }]], add
end
def test_invert_remove_belongs_to_alias
- @recorder.record :remove_belongs_to, [:table, :user]
- add = @recorder.inverse.first
+ add = @recorder.inverse_of :remove_belongs_to, [:table, :user]
assert_equal [:add_reference, [:table, :user]], add
end
end
Something went wrong with that request. Please try again.