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

Notify A User they Have Pending Migrations #6665

Merged
merged 3 commits into from Jun 10, 2012

Conversation

Projects
None yet
7 participants
@schneems
Member

schneems commented Jun 7, 2012

Problem

After a member of a team adds commits new code and a migration, other developers might not notice the migration and will get NoMethodError, UnknownAttributeError, or other exceptions raised.

I've seen this behavior confuse beginner rails developers and seasoned devs. Usually they are capable of figuring out they need to run migrations, but the error is misleading, frustrating, and potentially time consuming.

Solution

raise error for pending migration

Can be configured by setting config.active_record.migration. Setting to :page_load will raise an error on each page refresh if there are migrations that are pending. Setting to :page_load is defaulted in development for new applications.

Setting config.active_record.migration to :app_start will raise an error when the app is initialized if there are pending migrations. Setting to :app_start is defaulted in test mode.

This check is turned off in production.

The check can be disabled by setting config.active_record.skip_migration_errors = true. This is useful when the error would conflict with fixing the problem such as having an error on :app_start and trying to run rake db:migrate which starts the app. A convenience rake task is also included to be run before :environment is loaded on crucial tasks.

Notes

This PR picks up where #4165 left off.

Screenshot of :page_load error: http://cl.ly/0Q0t3z1s3R2Z142p2o1B

Screenshot of :app_start error: http://cl.ly/1k0c1i2O0v122T3P2Y1n

Railties Tests are green: https://gist.github.com/2885779

AR Tests are green: https://gist.github.com/2885796

cc/ @jonleighton

add convenience methods for checking migrations
if a rails project needs to be migrated ActiveRecord::Migrator.needs_migration? will be true or false if the current version matches the last version.
@josevalim

View changes

activerecord/lib/active_record/core.rb Outdated
##
# :singleton-method:
# Specifies if migration pending check is skipped
config_attribute :skip_migration_errors , :global => true

This comment has been minimized.

@josevalim

josevalim Jun 7, 2012

Contributor

Is there a need for both skip and the migration_error equals to :none? They seem redundant, i would keep just migration_error.

@josevalim

View changes

activerecord/lib/active_record/railtie.rb Outdated
@@ -22,6 +22,9 @@ class Railtie < Rails::Railtie
config.app_middleware.insert_after "::ActionDispatch::Callbacks",
"ActiveRecord::ConnectionAdapters::ConnectionManagement"
config.app_middleware.insert_after "::ActionDispatch::Callbacks",

This comment has been minimized.

@josevalim

josevalim Jun 7, 2012

Contributor

We should not always add this middleware, we should add it just if the configuration is set to pending.

@josevalim

This comment has been minimized.

Contributor

josevalim commented Jun 7, 2012

I am still uneasy about some of these changes. The fact we are white-listing many rake tasks makes my spider-sense go crazy. Here is what I propose:

  1. Have a method like ActiveRecord::Migrator.check_pending_migrations! (or in ActiveRecord::Base);
  2. We could add this method by default in test/test_helper.rb. This way we don't need to whitelist anything, removing it is as easy as removing one line. RSpec can do the same;
  3. Then, all we need is a configuration to add the middleware, which will be set only in development and does not even need to belong to ActiveRecord::Base, it will solely be part of AR::Railtie.

Thanks a lot for your work!

@jonleighton

View changes

activerecord/lib/active_record/railties/databases.rake Outdated
step = ENV['STEP'] ? ENV['STEP'].to_i : 1
ActiveRecord::Migrator.forward(ActiveRecord::Migrator.migrations_paths, step)
db_namespace['_dump'].invoke
end
# desc 'Drops and recreates the database from db/schema.rb for the current environment and loads the seeds.'
task :reset => :environment do
task :reset => [:skip_migration_errors, :environmen] do

This comment has been minimized.

@jonleighton

jonleighton Jun 7, 2012

Member

typo o_O

This comment has been minimized.

@schneems

schneems Jun 7, 2012

Member

Good catch, seems like that should have been picked up by a test...

@jonleighton

This comment has been minimized.

Member

jonleighton commented Jun 7, 2012

I agree with @josevalim proposal for the test mode. Definitely seems cleaner :)

@schneems

This comment has been minimized.

Member

schneems commented Jun 7, 2012

@josevalim @jonleighton Seems good. I'll work on implementing proposed solution. Definitely less invasive. Thanks for taking a look.

@schneems

This comment has been minimized.

Member

schneems commented Jun 8, 2012

Made the suggested modifications. Middleware only loads when the config is set to :page_load. Removed the error :app_start configuration option and added a migration check to the default test_helper.rb. Moved and refactored middleware test.

Let me know what you think when you get a chance. Have a happy weekend.

@josevalim

View changes

activerecord/lib/active_record/migration.rb Outdated
class << self
attr_accessor :delegate # :nodoc:
end
def self.check_pending!
raise ActiveRecord::PendingMigrationError if ActiveRecord::Migrator::needs_migration?

This comment has been minimized.

@josevalim

josevalim Jun 9, 2012

Contributor

Please use ActiveRecord::Migrator.needs_migrations? to follow Rails code base conventions.

@josevalim

View changes

activerecord/lib/active_record/core.rb Outdated
# :singleton-method:
# Specify if a migration error should be raised when migrations pending
# on :app_start or :page_load
config_attribute :migration_error , :global => true

This comment has been minimized.

@josevalim

josevalim Jun 9, 2012

Contributor

Since this is used just for the middleware, this configuration should not be here anymore.

@josevalim

View changes

activerecord/lib/active_record/railtie.rb Outdated
@@ -110,6 +110,13 @@ class Railtie < Rails::Railtie
config.watchable_files.concat ["#{app.root}/db/schema.rb", "#{app.root}/db/structure.sql"]
end
initializer "active_record.migration_error" do |app|
if ActiveRecord::Base.migration_error == :page_load

This comment has been minimized.

@josevalim

josevalim Jun 9, 2012

Contributor

This should now be something like:

if config.active_record.delete(:migration_error) == :page_load

And it should be moved up, at least before this initializer.

@josevalim

This comment has been minimized.

Contributor

josevalim commented Jun 9, 2012

Thanks, we are almost there! I have added a couple more comments. :)

schneems added some commits Jun 6, 2012

raise error for pending migration
can be configured by setting config.active_record.migration. Setting to :page_load will raise an error on each page refresh if there are migrations that are pending. Setting to :page_load is defaulted in development for new applications.
test errors for pending migrations
App should raise error on page_load
@schneems

This comment has been minimized.

Member

schneems commented Jun 9, 2012

Changed ActiveRecord::Migrator.needs_migration? => ActiveRecord::Migrator.needs_migrations?. Removed all code in AR core. Moved the initializer before setting config variables in railties. Deleting the config var before in the initializer. Re-ran tests... Still green.

josevalim added a commit that referenced this pull request Jun 10, 2012

@josevalim josevalim merged commit 4845c06 into rails:master Jun 10, 2012

@schneems

This comment has been minimized.

Member

schneems commented Jun 11, 2012

Thanks for merging that in, under what circumstances should a changelog entry be added for a change? Upgrading devs will hopefully want to add that config to their dev environment and test_helper

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jun 11, 2012

@josevalim already added the changelog entries at 03f2249

@schneems

This comment has been minimized.

Member

schneems commented Jun 11, 2012

thanks, you guys rock!

@olivierlacan

This comment has been minimized.

Contributor

olivierlacan commented Jun 23, 2012

It's hard to understate how impactful I believe this seemingly small feature will be. Both seasoned devs and beginners routinely get caught pants down by the problem it solves.

Congratulations to @schneems and everyone involved for pushing this through.

@namxam

This comment has been minimized.

namxam commented Oct 3, 2012

I just stumbled upon this pull request. Great work @schneems !

@olivierlacan

This comment has been minimized.

Contributor

olivierlacan commented Oct 24, 2012

Not sure it's worth opening a new issue for, but this is what I see on the latest rails/rails in development:

Migrations are pending run 'rake db:migrate RAILS_ENV=' to resolve the issue

Seems like ENV['RAILS_ENV'] isn't defined.

https://github.com/schneems/rails/blob/d741a4c6f863778c5ebf04b21f6c3292091c13a7/activerecord/lib/active_record/migration.rb#L37

@schneems

This comment has been minimized.

Member

schneems commented Oct 24, 2012

@olivierlacan What command are you running to get this error? rails c, rails s, rails c -e production ? Please make a new issue, and mention my name in it? I'll take a look when I get some time.

@olivierlacan

This comment has been minimized.

Contributor

olivierlacan commented Oct 24, 2012

Running through Pow. Which may be the cause.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Oct 27, 2012

This is printing an annoying SQL log message in development. We need to fix it.

schneems added a commit to schneems/rails that referenced this pull request Oct 28, 2012

@bacrossland

This comment has been minimized.

bacrossland commented Oct 31, 2012

+1 on this. Thanks schneems for putting this in and everyone for working to improve it.

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