Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Notify A User they Have Pending Migrations #6665

Merged
merged 3 commits into from

7 participants

Richard Schneeman José Valim Jon Leighton Rafael Mendonça França Olivier Lacan Maximilian Schulz bacrossland
Richard Schneeman
Collaborator

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

Richard Schneeman schneems 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.
e5b3986
activerecord/lib/active_record/core.rb
@@ -85,6 +85,20 @@ module Core
# be added to the model instead.
config_attribute :dependent_restrict_raises, :global => true
self.dependent_restrict_raises = true
+
+ ##
+ # :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
+ self.migration_error = :none
+
+ ##
+ # :singleton-method:
+ # Specifies if migration pending check is skipped
+ config_attribute :skip_migration_errors , :global => true
José Valim Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/lib/active_record/railtie.rb
@@ -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",
José Valim Owner

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

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

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!

activerecord/lib/active_record/railties/databases.rake
((14 lines not shown))
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
Jon Leighton Collaborator

typo o_O

Richard Schneeman Collaborator
schneems added a note

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

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

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

Richard Schneeman
Collaborator

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

Richard Schneeman
Collaborator

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.

activerecord/lib/active_record/migration.rb
((18 lines not shown))
class << self
attr_accessor :delegate # :nodoc:
end
+ def self.check_pending!
+ raise ActiveRecord::PendingMigrationError if ActiveRecord::Migrator::needs_migration?
José Valim Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/lib/active_record/core.rb
@@ -85,6 +85,14 @@ module Core
# be added to the model instead.
config_attribute :dependent_restrict_raises, :global => true
self.dependent_restrict_raises = true
+
+ ##
+ # :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
José Valim Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/lib/active_record/railtie.rb
@@ -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
José Valim Owner

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.

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

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

schneems added some commits
Richard Schneeman schneems 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.
96f19f6
Richard Schneeman schneems test errors for pending migrations
App should raise error on page_load
d741a4c
Richard Schneeman
Collaborator

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.

José Valim josevalim merged commit 4845c06 into from
Richard Schneeman
Collaborator

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

Rafael Mendonça França

@josevalim already added the changelog entries at 03f2249

Richard Schneeman
Collaborator

thanks, you guys rock!

Olivier Lacan

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.

Maximilian Schulz

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

Olivier Lacan

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

Richard Schneeman
Collaborator

@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.

Olivier Lacan

Running through Pow. Which may be the cause.

Rafael Mendonça França

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

Richard Schneeman schneems referenced this pull request from a commit in schneems/rails
Richard Schneeman schneems Don't log on pending migration check
Conversation from: #6665 cc/ @rafaelfranca
0fbdddd
bacrossland

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

Paul Morganthall slothbear referenced this pull request in blowmage/minitest-rails
Closed

Should test_helper.rb include Rails 4 migration check? #110

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 3, 2012
  1. Richard Schneeman

    add convenience methods for checking migrations

    schneems authored
    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.
Commits on Jun 9, 2012
  1. Richard Schneeman

    raise error for pending migration

    schneems authored
    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.
  2. Richard Schneeman

    test errors for pending migrations

    schneems authored
    App should raise error on page_load
This page is out of date. Refresh to see the latest.
32 activerecord/lib/active_record/migration.rb
View
@@ -32,6 +32,12 @@ def initialize(name)
end
end
+ class PendingMigrationError < ActiveRecordError#:nodoc:
+ def initialize
+ super("Migrations are pending run 'bundle exec rake db:migrate RAILS_ENV=#{ENV['RAILS_ENV']}' to resolve the issue")
+ end
+ end
+
# = Active Record Migrations
#
# Migrations can manage the evolution of a schema used by several physical
@@ -326,10 +332,28 @@ def initialize(name)
class Migration
autoload :CommandRecorder, 'active_record/migration/command_recorder'
+
+ # This class is used to verify that all migrations have been run before
+ # loading a web page if config.active_record.migration_error is set to :page_load
+ class CheckPending
+ def initialize(app)
+ @app = app
+ end
+
+ def call(env)
+ ActiveRecord::Migration.check_pending!
+ status, headers, body = @app.call(env)
+ end
+ end
+
class << self
attr_accessor :delegate # :nodoc:
end
+ def self.check_pending!
+ raise ActiveRecord::PendingMigrationError if ActiveRecord::Migrator::needs_migrations?
+ end
+
def self.method_missing(name, *args, &block) # :nodoc:
(delegate || superclass.delegate).send(name, *args, &block)
end
@@ -605,6 +629,14 @@ def current_version
end
end
+ def needs_migrations?
+ current_version < last_version
+ end
+
+ def last_version
+ migrations(migrations_paths).last.try(:version)||0
+ end
+
def proper_table_name(name)
# Use the Active Record objects own table_name, or pre/suffix from ActiveRecord::Base if name is a symbol/string
name.table_name rescue "#{ActiveRecord::Base.table_name_prefix}#{name}#{ActiveRecord::Base.table_name_suffix}"
7 activerecord/lib/active_record/railtie.rb
View
@@ -59,6 +59,13 @@ class Railtie < Rails::Railtie
ActiveSupport.on_load(:active_record) { self.logger ||= ::Rails.logger }
end
+ initializer "active_record.migration_error" do |app|
+ if config.active_record.delete(:migration_error) == :page_load
+ config.app_middleware.insert_after "::ActionDispatch::Callbacks",
+ "ActiveRecord::Migration::CheckPending"
+ end
+ end
+
initializer "active_record.set_configs" do |app|
ActiveSupport.on_load(:active_record) do
if app.config.active_record.delete(:whitelist_attributes)
15 activerecord/test/cases/migration_test.rb
View
@@ -56,6 +56,21 @@ def teardown
Person.reset_column_information
end
+ def test_migrator_versions
+ migrations_path = MIGRATIONS_ROOT + "/valid"
+ ActiveRecord::Migrator.migrations_paths = migrations_path
+
+ ActiveRecord::Migrator.up(migrations_path)
+ assert_equal 3, ActiveRecord::Migrator.current_version
+ assert_equal 3, ActiveRecord::Migrator.last_version
+ assert_equal false, ActiveRecord::Migrator.needs_migrations?
+
+ ActiveRecord::Migrator.down(MIGRATIONS_ROOT + "/valid")
+ assert_equal 0, ActiveRecord::Migrator.current_version
+ assert_equal 3, ActiveRecord::Migrator.last_version
+ assert_equal true, ActiveRecord::Migrator.needs_migrations?
+ end
+
def test_create_table_with_force_true_does_not_drop_nonexisting_table
if Person.connection.table_exists?(:testings2)
Person.connection.drop_table :testings2
3  railties/lib/rails/generators/rails/app/templates/config/environments/development.rb.tt
View
@@ -26,6 +26,9 @@
# Log the query plan for queries taking more than this (works
# with SQLite, MySQL, and PostgreSQL).
config.active_record.auto_explain_threshold_in_seconds = 0.5
+
+ # Raise an error on page load if there are pending migrations
+ config.active_record.migration_error = :page_load
<%- end -%>
<%- unless options.skip_sprockets? -%>
2  railties/lib/rails/generators/rails/app/templates/test/test_helper.rb
View
@@ -4,6 +4,8 @@
class ActiveSupport::TestCase
<% unless options[:skip_active_record] -%>
+ ActiveRecord::Migration.check_pending!
+
# Setup all fixtures in test/fixtures/*.(yml|csv) for all tests in alphabetical order.
#
# Note: You'll currently still have to declare fixtures explicitly in integration tests
15 railties/test/application/configuration_test.rb
View
@@ -41,6 +41,21 @@ def teardown
FileUtils.rm_rf(new_app) if File.directory?(new_app)
end
+ test "a renders exception on pending migration" do
+ add_to_config <<-RUBY
+ config.active_record.migration_error = :page_load
+ config.consider_all_requests_local = true
+ config.action_dispatch.show_exceptions = true
+ RUBY
+
+ require "#{app_path}/config/environment"
+ ActiveRecord::Migrator.stubs(:needs_migrations?).returns(true)
+
+ get "/foo"
+ assert_equal 500, last_response.status
+ assert_match "ActiveRecord::PendingMigrationError", last_response.body
+ end
+
test "multiple queue construction is possible" do
require 'rails'
require "#{app_path}/config/environment"
Something went wrong with that request. Please try again.