Skip to content
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

Fix transaction reverting for migrations #31604

Merged
merged 1 commit into from Sep 27, 2018

Conversation

fatkodima
Copy link
Contributor

Commands than run inside a transaction block in a reverted migration ran uninverted, as is.
So, e.g. reverting this a bit contrived migration will fail:

class CreateComments < ActiveRecord::Migration[5.2]
  def change
    transaction do
      create_table :posts do |t|
        t.string :title
        t.text :body

        t.timestamps
      end
    end
  end
end

But, intuitively, it should be reverted and table should be removed.

This change fixes that problems with uninverted transaction blocks.

Originates from outdated #22141 and addresses only new migrations.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Before: Commands run inside a `transaction` in a reverted migration ran uninverted.
Now: This change fixes that by reverting commands inside `transaction` block.

*fatkodima*, *dv*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the original PR dv goes by "David Verhasselt" in the CHANGELOG so can you update that for consistecy?

Also can you update your commit to add the following so that he gets the commit credit in the contributors app too?

commit message text

[fatkodima & David Verhasselt]

@fatkodima fatkodima force-pushed the reverting-transaction branch 6 times, most recently from 0e84f08 to d662ee8 Compare January 10, 2018 13:10
@fatkodima
Copy link
Contributor Author

@eileencodes thank you for review. Fixed your notes.
Is there anything that I should do?

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still relevant in Rails 6 (0a4c2d4). Please rebase and move your compatibility changes to V5_2.

@@ -108,11 +108,17 @@ def change_table(table_name, options = {}) # :nodoc:
yield delegate.update_table_definition(table_name, self)
end

def replay(migration)
commands.each do |cmd, args, block|
migration.send(cmd, *args, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to use #public_send here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, no. For example, this method

is private.

@@ -16,6 +16,12 @@ def self.find(version)
V5_2 = Current

class V5_1 < V5_2
module CommandRecorder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should move these changes to the V5_2 class now that V6_0 is current.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

[fatkodima & David Verhasselt]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants