New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reversible commands #8267

Merged
merged 16 commits into from Dec 21, 2012

Conversation

Projects
None yet
7 participants
@marcandre
Contributor

marcandre commented Nov 19, 2012

This pull request makes the following commands reversible:

  • drop_table
  • remove_column
  • remove_index

It also creates the reversible command drop_join_table.

Finally, remove_columns and remove_column are no longer aliases and the interface of remove_column is changed to accept only one column (along with new arguments that are used in case of reversal).

The PR builds on #8177 (itself building on #7627), but the need they all fulfill is independent. Each PR can be accepted/rejected independently. I hope all three can be accepted as they nicely complement each other and give powerful means to create reversible migrations. The ChangeLog remains to be updated, but I'll wait until confirmation that this will be merged.

Thanks

@marcandre marcandre referenced this pull request Nov 19, 2012

Closed

Migration revert #7627

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 19, 2012

Member

Already out of date somehow :/

Member

steveklabnik commented Nov 19, 2012

Already out of date somehow :/

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Nov 19, 2012

Contributor

Already out of date somehow :/

I've removed the entry in the Changelog (which was only for the #7627 anyways)

When there's a decision about these 3 PRs, I'll add an entry in the Changelog.

Contributor

marcandre commented Nov 19, 2012

Already out of date somehow :/

I've removed the entry in the Changelog (which was only for the #7627 anyways)

When there's a decision about these 3 PRs, I'll add an entry in the Changelog.

@rafaelfranca

View changes

Show outdated Hide outdated ...cord/lib/active_record/connection_adapters/abstract/schema_statements.rb
def remove_column(table_name, *column_names)
columns_for_remove(table_name, *column_names).each {|column_name| execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{column_name}" }
def remove_columns(table_name, *column_names)
raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.empty?

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 19, 2012

Member

Should not be remove_columns here?

@rafaelfranca

rafaelfranca Nov 19, 2012

Member

Should not be remove_columns here?

This comment has been minimized.

@marcandre

marcandre Nov 19, 2012

Contributor

Good eye 🙇. Fixed

@marcandre

marcandre Nov 19, 2012

Contributor

Good eye 🙇. Fixed

@marcandre marcandre referenced this pull request Dec 19, 2012

Closed

Migration reversible #8177

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 19, 2012

Contributor

I've updated this PR with these additional changes:

  • The migration generator now always generate change method
  • Guides: Migration and 4.0 releate notes updated to reflect these changes (see last commit)
  • Not directly related, but I simplified change_table (see second commit)
  • Made change_table reversible (as long as its block doesn't call remove, change or change_default)
Contributor

marcandre commented Dec 19, 2012

I've updated this PR with these additional changes:

  • The migration generator now always generate change method
  • Guides: Migration and 4.0 releate notes updated to reflect these changes (see last commit)
  • Not directly related, but I simplified change_table (see second commit)
  • Made change_table reversible (as long as its block doesn't call remove, change or change_default)
@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Dec 19, 2012

Member

@marcandre can you squash the commits, and I'll merge this in?

Member

tenderlove commented Dec 19, 2012

@marcandre can you squash the commits, and I'll merge this in?

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 19, 2012

Contributor

@tenderlove Which commits should I merge? They should all stand by themselves, and many are not related at all to others

Contributor

marcandre commented Dec 19, 2012

@tenderlove Which commits should I merge? They should all stand by themselves, and many are not related at all to others

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 20, 2012

Member

@marcandre we generally squash all commits so that it's easy to backport/revert.

Member

steveklabnik commented Dec 20, 2012

@marcandre we generally squash all commits so that it's easy to backport/revert.

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 20, 2012

Contributor

@steveklabnik @tenderlove With all due respect, this is a terrible rationale. Not only is backporting/reverting rare, but git revert and git cherry-pick accept ranges of commits.

Squashing is lossy, the commits I'm submitting are atomic and make it easier to understand what's going on, for example if anyone does a git blame at some point.

Finally, if you decide to revert part of this (e.g. make drop_table revertible), you won't be able to if the commits are squashed. Similarly, the first commits are simple code improvements made because I noticed things while doing my changes.

These commits total over 1000 line changes in total, I don't see a problem in them being split in a bunch of atomic commits.

If you want me to open 13 different pull requests, I will, or maybe the commit messages should refer to this PR, but I won't squash these commits. Of course, you guys are free to do it yourself if you feel that's the way it should be.

Contributor

marcandre commented Dec 20, 2012

@steveklabnik @tenderlove With all due respect, this is a terrible rationale. Not only is backporting/reverting rare, but git revert and git cherry-pick accept ranges of commits.

Squashing is lossy, the commits I'm submitting are atomic and make it easier to understand what's going on, for example if anyone does a git blame at some point.

Finally, if you decide to revert part of this (e.g. make drop_table revertible), you won't be able to if the commits are squashed. Similarly, the first commits are simple code improvements made because I noticed things while doing my changes.

These commits total over 1000 line changes in total, I don't see a problem in them being split in a bunch of atomic commits.

If you want me to open 13 different pull requests, I will, or maybe the commit messages should refer to this PR, but I won't squash these commits. Of course, you guys are free to do it yourself if you feel that's the way it should be.

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 21, 2012

Contributor

Updated commit logs with this PR number (except the first three which aren't directly related).

Had forgotten to update the ChangeLog, so this PR will probably conflict soon.

Contributor

marcandre commented Dec 21, 2012

Updated commit logs with this PR number (except the first three which aren't directly related).

Had forgotten to update the ChangeLog, so this PR will probably conflict soon.

tenderlove added a commit that referenced this pull request Dec 21, 2012

@tenderlove tenderlove merged commit 68e91da into rails:master Dec 21, 2012

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 21, 2012

Member

I'm getting these errors when running with mysql2:

  1) Error:
test_migrate_revert_by_part(ActiveRecord::InvertibleMigrationTest):
ActiveRecord::StatementInvalid: Mysql2::Error: SAVEPOINT active_record_1 does not exist: ROLLBACK TO SAVEPOINT active_record_1

(there are a bunch of apparently related errors and travis build page for mysql/mysql2 doesn't seem to be loading)

Also, travis reported a failing test in railties. And another fix I already commited in 68e91da...4da76d7.

@marcandre would you mind taking a look at the mysql failures? I think I'll just be able to continue working on it tomorrow, so if you can, that'd be very welcome. Thanks.

I'm getting these errors when running with mysql2:

  1) Error:
test_migrate_revert_by_part(ActiveRecord::InvertibleMigrationTest):
ActiveRecord::StatementInvalid: Mysql2::Error: SAVEPOINT active_record_1 does not exist: ROLLBACK TO SAVEPOINT active_record_1

(there are a bunch of apparently related errors and travis build page for mysql/mysql2 doesn't seem to be loading)

Also, travis reported a failing test in railties. And another fix I already commited in 68e91da...4da76d7.

@marcandre would you mind taking a look at the mysql failures? I think I'll just be able to continue working on it tomorrow, so if you can, that'd be very welcome. Thanks.

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 22, 2012

Contributor

@carlosantoniodasilva Thanks for the fixes. The railties error is trivial (string vs symbol), but I'm not sure what's up with the mysql errors. Probably related to using transaction in the migration?

I'll be able to check that first thing tomorrow.

Contributor

marcandre commented Dec 22, 2012

@carlosantoniodasilva Thanks for the fixes. The railties error is trivial (string vs symbol), but I'm not sure what's up with the mysql errors. Probably related to using transaction in the migration?

I'll be able to check that first thing tomorrow.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 22, 2012

Member

@marcandre it seems so, somehow related to the reversible command (that has a transaction block on it). If I skip that test I sent before, everything seems to be fine.

@marcandre it seems so, somehow related to the reversible command (that has a transaction block on it). If I skip that test I sent before, everything seems to be fine.

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 22, 2012

Contributor

@carlosantoniodasilva Right. I still can't imagine why it causes a problem on mysql, but I'll just not use transaction at all. Actually I can probably use send something :tap instead. Anyways, I'll run the test when I'm home tomorrow and send a PR.

Contributor

marcandre commented Dec 22, 2012

@carlosantoniodasilva Right. I still can't imagine why it causes a problem on mysql, but I'll just not use transaction at all. Actually I can probably use send something :tap instead. Anyways, I'll run the test when I'm home tomorrow and send a PR.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 22, 2012

Member

@marcandre thanks, let me know if you find anything or need any help.

@marcandre thanks, let me know if you find anything or need any help.

@josevalim

This comment has been minimized.

Show comment
Hide comment
Contributor

josevalim commented Dec 22, 2012

marcandre added a commit to marcandre/rails that referenced this pull request Dec 23, 2012

Fixes for PR [#8267]
* Fix Migration#reversible by not using `transaction`.

* Adapt mysql adapter to updated api for remove_column

* Update test after aedcd68
@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 23, 2012

Contributor

@josevalim Should all be fixed by [#8588]

Contributor

marcandre commented Dec 23, 2012

@josevalim Should all be fixed by [#8588]

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 29, 2013

Support `remove_columns`
`def remove_columns(table_name, *column_names)` is defined
in ActiveRecord::ConnectionAdapters::SchemaStatements, which does not
require Oracle enhanced specific implementation.

`def remove_column(table_name, column_name, type = nil, options = {})`
ignores `type` and `options` arguments to avoid ORA-00904 errors

See rails/rails#8267 for Rails change.
This commit should not backport into release14 branch.

@yahonda yahonda referenced this pull request Nov 29, 2013

Merged

Support `remove_columns` #377

@seanlinsley

This comment has been minimized.

Show comment
Hide comment
@seanlinsley

seanlinsley Dec 15, 2014

Contributor

remove_index is reversible when you provide the :column option:

def change
  remove_index :users, column: :email
end

But when providing the column in the normal way,

def change
  remove_index :users, :email
end

you get this error message:

remove_index is only reversible if given a :column option.

Is this by design, or should I create a ticket / PR for it?

Contributor

seanlinsley commented Dec 15, 2014

remove_index is reversible when you provide the :column option:

def change
  remove_index :users, column: :email
end

But when providing the column in the normal way,

def change
  remove_index :users, :email
end

you get this error message:

remove_index is only reversible if given a :column option.

Is this by design, or should I create a ticket / PR for it?

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Dec 15, 2014

Contributor

@seanlinsley Definitely not by design!

Issue or PR would be good (I don't have time now), please cc me though 😄

Contributor

marcandre commented Dec 15, 2014

@seanlinsley Definitely not by design!

Issue or PR would be good (I don't have time now), please cc me though 😄

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