Migration revert #7280

Closed
wants to merge 1 commit into
from

8 participants

@marcandre

Reversible migrations are very cool.

Some commands are not reversible, for example the removal of a table or a column.

Also, one downside is that it is now more difficult to write the reverse of a migration if change is used. When up and down were used, one could simply swap the code around.

These commits introduce Migration#revert that makes it trivial to revert a past migration, in part or in whole, or do a reversible removal of a table/column.

Note that revert can even be called from legacy migrations using up & down and that it can revert legacy-style migrations too. For anyone changing their mind every second day, revert is fully nestable.

To have complete revertible capability, I would like to introduce a modified syntax for change_column that would allow it to be revertible; pull request upcoming when I get a chance...

Here's what the rdoc looks like:


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:

class FixupTLMigration < ActiveRecord::Migration
  def change
    revert TenderloveMigration

    create_table(:apples) do |t|
      t.string :variety
    end
  end
end

This command can be nested.

@carlosantoniodasilva
Ruby on Rails member
@steveklabnik
Ruby on Rails member

This will need a rebase.

@marcandre

Please rebase and squash

Done

@rafaelfranca
Ruby on Rails member

Am I crazy or all the @tenderlove comments are being removed?

@rafaelfranca
Ruby on Rails member

I didn't get this feature. In this case:

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

In the migration up the tables horse and apples will be create. In the migration down, do will the two table removed?

@marcandre

@rafaelfranca
1) Indeed, @tenderlove's comments are gone. Maybe he doesn't like the feature after all ;-)

2) In the migration up, the table horses gets destroyed (since it's in a revert block) and apples gets created. In down, the reverse happens, i.e. horses gets created (with the field content and remind_at) and apples is dropped. I think that's what I wrote, no?

@tenderlove
Ruby on Rails member

Sorry, someone was impersonating me. I haven't actually reviewed this at all. :(

@steveklabnik
Ruby on Rails member

@tenderlove you really gotta stop letting Gorby sit on your keyboard. ;)

@rafaelfranca
Ruby on Rails member

@marcandre sorry I was sleepy. Seems fine 👍

@rorra

Is this a feature to create reversible drop_table? I usually create independent up and down methods when dropping a table, so this feature will save me some lines of code :)

But why is the "revert" method introduced into the migrations?

What about just add the drop_table to the CommandRecorder and to update the CommandRecorder to revert commands with blocks? (I tried to do the udpate on the code but I wasn't able to do that)

So the syntax will look like:

class Migration1 < ActiveRecord::Migration
def change
create_table :users do |t|
t.string :name
end
end
end

class Migration2 < ActiveRecord::Migration
def change
drop_table :users do |t|
t.string :name
end
end
end

Its just that I don't understand what's the idea behind the revert method.

Thx

@marcandre

Is this a feature to create reversible drop_table

Yes. And a reversible remove_column and remove_index.

I understand that you are suggesting an approach where these commands would accept extra arguments & a block to be reversible.

drop_table could accept and ignore an extra options parameter and would also ignore a block and remove_index could be modified to have the same api as add_index.

For remove_column there is a conflict, though, as it accepts multiple column names. But there is remove_columns too, so that could be an acceptable incompatibility, especially for 4.0.

Note: There is no drop_join_table, so revert makes it easy to reverse create_join_table, but clearly drop_join_table could be added and would be reversible.

The plusses I see with revert are:

  • quicker to revert a bunch of changes:
    • just wrap with revert{} instead of modifying every create_table with drop_table and add_column with remove_column
    • the order is taken care of for you, no need to inverse the order of the lines.
  • easy to revert a whole migration (in your example revert Migration1 would do it)
  • drop_table :users do ... all fields here ... end may be a bit counterintuitive?

We could have both approaches: add revert and change the methods to be invertible. Maybe that would be best, anyone would have the choice of writing what they want.

@al2o3cr

One question about the version that passes class names: doesn't that require the migration file that defines that class to be manually loaded? Tried it against 3.2 and it didn't get automatically loaded...

@robin850
Ruby on Rails member

Does this pull request is going to be merge ? I think it's a very nice feature! 👍

@rafaelfranca
Ruby on Rails member

I'm fine with this feature. But I would rename revert to reverted I think is better to undertand what it does.

@marcandre

@al2o3cr Oh, indeed, Rails doesn't require all the migrations. So it would be nice to accept a string/symbol and load it for you, I guess, like revert :tenderlove_migration (or equivalently revert '012345678_tenderlove_migration'). I'll amend the request when I get a chance.

@rafaelfranca I proposed revert because the rest is imperative, def change, add_column, etc... I think the following read very well:

def change
   add_column :my_table, :whatever, :string
   revert { add_column :other_table, :whatever, :string }
end

Doesn't reverted sound like it will return something reverted instead of actually doing something?

In any case, we could also have both aliased :-)

@rafaelfranca
Ruby on Rails member

@marcandre I'm fine with the revert only.

@tenderlove do you have any objections?

@marcandre

@al2o3cr Looks difficult to do what I suggested; the loading of migrations is at a higher level than the processing of them. Unless someone says otherwise, I'd say that yeah, you have to require it (or more easily to require_relative '1234_migration_name').

I added a require_relative to the example in the doc.

@marcandre marcandre referenced this pull request Sep 13, 2012
Closed

Migration revert #7627

@marcandre

I can't seem to update this pull request (presumably because I had to recreate my rails fork in the meantime...), so created a new pull request.

BTW, anyone wanting this feature in Rails 3.x can add gem 'migration_revert'

@robin850
Ruby on Rails member

@marcandre : Nice ! Thanks for the gem for Rails 3.x !

@steveklabnik
Ruby on Rails member

@marcandre you would update it by force pushing your branch, but since you said that #7627 is the updated one, I'm just going to close this one.

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