Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Migration revert #7627

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

marcandre commented Sep 13, 2012

Same as #7280 with doc updated and rebased.

Had to create a new pull request as previous one did not update (presumably because I had to recreate my fork in the meantime...)

Owner

rafaelfranca commented Sep 13, 2012

Seems fine to me. I just want to know if @tenderlove or @jonleighton have any objections.

@steveklabnik steveklabnik referenced this pull request Sep 15, 2012

Closed

Migration revert #7280

Owner

tenderlove commented Sep 16, 2012

The code seems fine. At first, the example totally confused me because you write revert { create_table ... } to drop the table. When I read this code, it reads as "Do the opposite thing of creating a table", which seems confusing. I understand the feature, I just wish the code was easier to understand. Maybe drop_table should take a block?

@frodsan frodsan commented on an outdated diff Oct 26, 2012

activerecord/lib/active_record/migration.rb
+ # 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.
+ #
@frodsan

frodsan Oct 26, 2012

Contributor

✂️ this line.

@frodsan frodsan commented on an outdated diff Oct 26, 2012

activerecord/lib/active_record/migration.rb
+ # 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?
@frodsan

frodsan Oct 26, 2012

Contributor

Please, use 1.9 hash syntax. We're encouraging people to write new/changed code in 1.9 syntax. Thanks.

@frodsan frodsan commented on an outdated diff Oct 26, 2012

...ecord/lib/active_record/migration/command_recorder.rb
def initialize(delegate = nil)
@commands = []
@delegate = delegate
+ @reverting = false
+ end
+
+ def revert
@frodsan

frodsan Oct 26, 2012

Contributor

Can you add docs here?

@frodsan frodsan commented on an outdated diff Oct 26, 2012

...ecord/lib/active_record/migration/command_recorder.rb
end
# record +command+. +command+ should be a method name and arguments.
# For example:
#
# recorder.record(:method_name, [:arg1, :arg2])
- def record(*command)
- @commands << command
+ #
@frodsan

frodsan Oct 26, 2012

Contributor

✂️ this line

@frodsan frodsan commented on the diff Oct 26, 2012

...ecord/lib/active_record/migration/command_recorder.rb
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
@frodsan

frodsan Oct 26, 2012

Contributor

fix punctuation.

@frodsan frodsan commented on an outdated diff Oct 26, 2012

...ecord/lib/active_record/migration/command_recorder.rb
#
- # recorder.record(:rename_table, [:old, :new])
- # recorder.inverse # => [:rename_table, [:new, :old]]
+ # recorder.inverse_of(:rename_table, [:old, :new])
+ # # => [:rename_table, [:new, :old]]
@frodsan

frodsan Oct 26, 2012

Contributor

✂️ whitespace before #.

@frodsan frodsan commented on an outdated diff Oct 26, 2012

...ecord/lib/active_record/migration/command_recorder.rb
#
# 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+.
+ #
@frodsan

frodsan Oct 26, 2012

Contributor

✂️ this line.

Contributor

marcandre commented Oct 26, 2012

Updated according to comments of @frodsan

@frodsan frodsan commented on an outdated diff Oct 26, 2012

...erecord/test/cases/migration/command_recorder_test.rb
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}]
@frodsan

frodsan Oct 26, 2012

Contributor

1.9 hash syntax please :)

@frodsan frodsan commented on an outdated diff Oct 26, 2012

...erecord/test/cases/migration/command_recorder_test.rb
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}]
@frodsan

frodsan Oct 26, 2012

Contributor

ditto.

@frodsan frodsan commented on an outdated diff Oct 26, 2012

...erecord/test/cases/migration/command_recorder_test.rb
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"}]
@frodsan

frodsan Oct 26, 2012

Contributor

ditto.

The code seems fine. At first, the example totally confused me because you write revert { create_table ... } to drop the table. When I read this code, it reads as "Do the opposite thing of creating a table", which seems confusing.

This was my issue looking at the code as well. My best guess is that this is defining changes that can be rolled back via db:migrate:down vs. things that cannot be rolled back?

If that is the case, I would argue that the approach should be opposite. In other words, Rails should assume that everything can be reverted unless otherwise specified. e.g.:

def change
  irreversible do
    create_table(:horses) { ... }
  end

  create_table(:cows) { ... }
end

My other issue is with the name revert. It sounds like it will be reverting at the moment the method is called, as opposed to saying this can be reverted in the future. If the functionality remains as is, I would recommend changing the name to something like revertible or reversible.

Contributor

marcandre commented Oct 26, 2012

@mhuggins I think you are confused. revert will indeed be reverting, at the moment the method is called, whatever is inside the given block. Unless the change method was called in a migrate:down, in which case everything not inside the block of the revert will be reverted and things in the revert block will be executed as is.

I'm probably not being entirely clear, because I'm not sure what the confusion is. If it's clear that def change; create_table :foo; end can actually drop a table (when called from migrate:down), then it should be clear that def change; revert { create_table :foo }; end can actually drop a table (when not called from migrate:down).

Contributor

marcandre commented Oct 26, 2012

Updated more hash syntax as per @frodsan.

I am definitely confused then, since drop_table :foo is much simple than revert { create_table :foo }.

Contributor

marcandre commented Oct 26, 2012

I am definitely confused then, since drop_table :foo is much simple than revert { create_table :foo }.

It is simpler, except that:

  • drop_table is not revertible
  • if you have a bunch of create_table, rename_column, etc..., in a migration and you later want to undo that migration because you changed your mind, it's easier to enclose the commands in a revert {} than to modify each command and reverse their order.

So let's say I have three migration files:

000000001_create_users.rb:

def change
  create_table :users do |t|
    t.string :username, null: false
    t.string :encrypted_password, null: false
  end
end

000000002_add_name_to_users.rb:

def change
  add_column :users, :first_name, :string
  add_column :users, :last_name, :string
end

000000003_remove_users.rb:

def change
  revert do
    create_table :users do
      t.string :username, null: false
      t.string :encrypted_password, null: false
      t.string :first_name
      t.string :last_name
    end
  end
end

It seems like the third migration has to know the state of the table as a result of prior migrations. Is that right?

Contributor

marcandre commented Oct 27, 2012

@mhuggins If you have your two migrations, and then decide you want to revert them, you can either copy-paste the table creation from your db/schema.rb (which gives you the 3rd migration as you wrote it), or copy-paste from both migration files:

def change
  revert do
    # Copied straight from your first migration:
    create_table :users do |t|
      t.string :username, null: false
      t.string :encrypted_password, null: false
    end
    # Copied straight from your second migration:
    add_column :users, :first_name, :string
    add_column :users, :last_name, :string
  end
end

In either case, this migration can be ran forward or backwards.

Hopefully that answers your question; I'm not sure exactly what you meant by "knowing the state".

I understand better now. Thanks for the explanation.

However, since other migration actions (create_table, add_column, remove_column, add_index, remove_index) can be reverted without the needing to be wrapped in a revert block, why not just change drop_table itself to be revertible similar to the other methods? It seems awkward to require a revert block for one DB action and none others.

I would propose something like this instead:

def change
  # Copied straight from your first migration:
  drop_table :users do |t|
    t.string :username, null: false
    t.string :encrypted_password, null: false
    t.string :first_name
    t.string :last_name
  end
end

The block can be optional (i.e.: drop_table :users would still work, it just would not be revertible). But by supplying the block, you can now revert that action just like any other action, plus the syntax is more in line with other migration methods.

Contributor

marcandre commented Oct 27, 2012

@mhuggins I believe I already answered that. In short: we probably should have a revertable drop_table & al, in addition to revert.

@marcandre marcandre referenced this pull request Nov 11, 2012

Closed

Migration reversible #8177

Contributor

marcandre commented Nov 12, 2012

Modified the code slightly to record block when inverting methods. For this PR, this has no consequence, but will be needed if transaction is made reversible, like in #8177, or if drop_table et al. ever accept a block.

@marcandre marcandre referenced this pull request Nov 19, 2012

Merged

Reversible commands #8267

Contributor

marcandre commented Nov 19, 2012

Maybe drop_table should take a block?

Created #8267 for drop_table to accept a block, etc...

@tenderlove & al.: I'd really appreciate feedback on all 3 PR. Thanks

Owner

rafaelfranca commented Nov 19, 2012

This one seems good to me but it will need documentation, like updating the migrations guides.

Contributor

marcandre commented Nov 29, 2012

@rafaelfranca Agreed. I will gladly update the migration guides, as well as produce a sensible ChangeLog entry, but I'd like to know first which feature requests are accepted to make a coherent document (I'm not sure how the decision process works!). SO, are is this feature request, as well as #8177 officially "accepted"? What about #8267 (in particular, is the api change to remove_column ok)?

I will also update the generators to use change when possible.

Contributor

marcandre commented Dec 10, 2012

Contributor

marcandre commented Dec 19, 2012

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

@marcandre marcandre closed this Dec 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment