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

IrreversibleMigration is not raised when a method not supported by reversible migrations is called in the change method of a migration #1857

Merged
merged 1 commit into from Jun 28, 2011

Conversation

Projects
None yet
3 participants
Member

vijaydev commented Jun 25, 2011

I came across this issue on @Gregg's Twitter timeline which he explained in this screencast: http://bit.ly/iu5ENC

Given a migration like this:

class AddPhone < ActiveRecord::Migration
def change
change_table :companies do |t|
t.string :phone
end
end
end

When rolling back this migration, we expect an IrreversibleMigration exception to be raised, because change_table is not supported by CommandRecorder and not reversible.

But what we get is an error complaining that the column name is a duplicate - SQLite3::SQLException: duplicate column name: phone: ALTER TABLE "companies" ADD "phone" varchar(255)

This happens because of the method_missing method in CommandRecorder which sends the change_table method to the delegate database connection, thereby causing the change method to actually run on the database, even when the migration is rolled back.

As a fix, I've changed method_missing to record the method. This causes the inverse method to raise IrreversibleMigration later when it finds that invert_change_table is not available.

I want to know if this is the right approach to solve this problem. And if my tests are sufficient. Let me know if anything is missing. Thanks!

@vijaydev vijaydev record unsupported methods in CommandRecorder instead of letting the …
…unsupported methods go through to the underlying db causing errors like duplicate columns to occur when rolling back migrations
c278a2c
Contributor

josevalim commented Jun 25, 2011

Member

vijaydev commented Jun 25, 2011

Thinking about my approach, we could even remove the class_eval that creates all the methods that are recordable and let method_missing handle the recording completely.

Or add change_table and any other missing methods to the class_eval code so that they are all recorded. In that case, if the method_missing is changed back to its original version, I'm not sure about the scenarios in which CommandRecorder should delegate to the database connection.

Contributor

josevalim commented Jun 25, 2011

My $0.02: In my opinion it makes total sense for change_table to work on change. We would just need to make it record commands and revert them as well. Unsure how much effort it would take.

Member

vijaydev commented Jun 25, 2011

Yeah, that'll be awesome! :)

@tenderlove tenderlove added a commit that referenced this pull request Jun 28, 2011

@tenderlove tenderlove Merge pull request #1857 from vijaydev/irreversible-migration
IrreversibleMigration is not raised when a method not supported by reversible migrations is called in the change method of a migration
941d5c1

@tenderlove tenderlove merged commit 941d5c1 into rails:master Jun 28, 2011

@vijaydev vijaydev added a commit to vijaydev/rails that referenced this pull request Jul 2, 2011

@vijaydev vijaydev Reversing the changes done in c278a2c while still resolving #1857.
The changes broke bulk migration tests and were fixed in 4d256bc;
however that brought back the issue of #1857 and so this commit goes
back to the original scenario and just adds change_table to the list
of methods which are to be recorded in the CommandRecorder. The
method_missing now delegates all calls to the underlying connection as
before.
15f35c0

@jonleighton jonleighton added a commit that referenced this pull request Jul 3, 2011

@vijaydev @jonleighton vijaydev + jonleighton Reversing the changes done in c278a2c while still resolving #1857.
The changes broke bulk migration tests and were fixed in 4d256bc;
however that brought back the issue of #1857 and so this commit goes
back to the original scenario and just adds change_table to the list
of methods which are to be recorded in the CommandRecorder. The
method_missing now delegates all calls to the underlying connection as
before.
d764378
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment