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

Add a #populate method to migrations #31082

Merged
merged 5 commits into from Nov 14, 2017

Conversation

Projects
None yet
7 participants
@pedantic-git
Contributor

pedantic-git commented Nov 7, 2017

This is a feature suggestion, and my first time contributing to the core, so I'm very open to comments about whether this is a good idea, etc.

I often find myself writing migrations that look something like this:

class AddPublishedToPosts < ActiveRecord::Migration[5.2]
  def change
    add_column :posts, :published, :boolean, default: false
    reversible do |dir|
      dir.up { Post.update_all(published: true) }
    end
  end
end

That is, I'm using one half of a #reversible block to prepopulate the existing records with appropriate values for the new column. (In the example above, we assume all existing posts are already published, but new posts are unpublished by default.)

It doesn't seem very Railsy because it's not really a reversible operation - it's an operation that only happens on the way up and is irrelevant on the way down because the column ceases to exist.

So this PR adds a new #populate method for this use case which simplifies the above migration slightly to:

class AddPublishedToPosts < ActiveRecord::Migration[5.2]
  def change
    add_column :posts, :published, :boolean, default: false
    populate { Post.update_all(published: true) }
  end
end

Thoughts?

@rails-bot

This comment has been minimized.

rails-bot commented Nov 7, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (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.

@pedantic-git

This comment has been minimized.

Contributor

pedantic-git commented Nov 7, 2017

Looks like the MySQL iterations of the test are failing for some reason I can't quite discern. I suspect it's an error in how I've coded the test, rather than an error in the functionality.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 7, 2017

Thank you for the pull request. Although the code is good I'm not too much inclined to advertising doing data migrations in the same migrations as the structure migration. Data migrations are problematic and I'd not do them in the same time I'd doing a structure change. If anything does wrong for example, when using PostgreSQL, your entire migration will be reverted. Other problem is if only part of the records are updated, the migration will not be finished and you will have to edit an existing migration to make it work again.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 7, 2017

@pedantic-git

This comment has been minimized.

Contributor

pedantic-git commented Nov 7, 2017

@rafaelfranca That's an interesting point, although of course in my common use case it's fine because this is primarily used for populating a column that has been created in the same migration (so a rollback will just erase the whole column).

Is there a more conventional way to do data migrations? As far as I knew, there's only one kind of migration available in Active Record so I use it for both schema and data.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 7, 2017

Yeah, right now active record don't have any other way to do it. My problem is not with doing it in the same migration as the structure change is being made. At Shopify for example we wrote a new framework to do data migrations to avoid this kind of problem that I mentioned but before this new framework we avoided to do data migrations in the same migration as a structure migration.

@jeremy

This comment has been minimized.

Member

jeremy commented Nov 7, 2017

IMO the reversible block is an appropriate level of ceremony to capture this data migration. It's clear what's happening and why. Introducing a populate method encourages data migrations without a complete story to substantiate it. Alternatively, we could introduce a shortcut "on-up-only" method to simplify the reversible block, e.g. up { … } and down { … } that do def up(&b) reversible { |dir| dir.up(&b) } end.

@pedantic-git

This comment has been minimized.

Contributor

pedantic-git commented Nov 7, 2017

I'm certainly happy to call it up. I just felt like an up-only reversible is way too much boilerplate when you're just doing a one-line data migration.

@matthewd

This comment has been minimized.

Member

matthewd commented Nov 7, 2017

I fully support doing data migrations in migrations -- if you're rearranging tables & columns that currently have data in them, it is The Right Thing for you to carry their existing data with them as needed, and to correspondingly back-fill new fields, such that they reflect the most reasonable approximation of the state the DB would end up in if the user performed those same operations against the post-migration version of the app. (Or, semi-equivalently, such that they behave in the way most consistent with their pre-migration form -- as in the published: true example.)

To my view, the fact that your entire migration will be reverted if something goes wrong is a feature not a problem: unless you have an inordinate amount of data [in which case you probably have a more complex migration story anyway], for the average app, it's ideal that the schema and data are never out of sync.

For me the danger is in any encouragement for people to use migrations to seed, which is very different from back-filling a column, but also sounds a lot like "populate". (The distinction is in whether they need to be performed on an empty database, as you'll get from db:schema:load.)

I do agree that the full reversible block is quite a mouthful for a seemingly simple concept: it's strictly true that it's a reversible operation and the down just happens to be a no-op, but that's not how any human would describe it.

I like @jeremy's suggestion of an up (or even up_only?)... though I do wonder 1) if we introduce both up and down, will people use those even when they have both?, and 2) would that be a bad thing?

@pedantic-git

This comment has been minimized.

Contributor

pedantic-git commented Nov 8, 2017

@matthewd Thanks for saying this! I was going to say the same thing - I want the whole thing to roll back if it fails, so putting the schema and data in the same migration is essential for me.

It seems like people would prefer this was called up rather than populate? I'm not sure there's a use case for a separate down method.

I don't think we can use the name up without some metaprogramming because it's already the name of the equivalent of #change that only runs on the way up. Am I wrong? Is there another similar name we can use?

# def change
# add_column :posts, :published, :boolean, default: false
# populate do
# Post.update_all(published: true)

This comment has been minimized.

@yskkin

yskkin Nov 10, 2017

Contributor

I think using model directly in migration is fragile since future modification on validation or callback may break this.
If a succeeding migration do drop_table :posts and app/models/post.rb is deleted, Post even does not exist.

How about defining model in place?

class AddPublishedToPosts < ActiveRecord::Migration[5.3]
  class Post < ActiveRecord::Base; end
  def change
    ....

This comment has been minimized.

@pedantic-git

pedantic-git Nov 10, 2017

Contributor

That's a good point. Maybe the example and test should use execute instead? That could be easily done with:

execute "update posts set published = 'true'"

This comment has been minimized.

@yskkin

yskkin Nov 11, 2017

Contributor

👍

@pedantic-git

This comment has been minimized.

Contributor

pedantic-git commented Nov 13, 2017

Thanks for all your comments so far! Following the feedback above, I've renamed the method to #up_only, and changed the example and test to use #execute.

@rafaelfranca

Can you add a CHANGELOG entry?

@pedantic-git

This comment has been minimized.

Contributor

pedantic-git commented Nov 14, 2017

@rafaelfranca Sure! Done.

pedantic-git and others added some commits Nov 14, 2017

@rafaelfranca rafaelfranca merged commit df82237 into rails:master Nov 14, 2017

0 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment