From 4705ba82dbf303b5eb84c46d1c7112a75d3273e5 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 3 Oct 2019 21:49:41 -0700 Subject: [PATCH] Use FileUpdateChecker for Migration::CheckPending Migration::CheckPending is a rack middleware normally used in development to raise an exception if there pending migrations. This commit replaces the existing caching, which avoided checking all migrations if the newest migration (by version number) hadn't changed. Instead, we now use FileUpdateChecker, which has two advantages: it can detect new migrations which aren't the highest version, and it is faster. --- activerecord/lib/active_record/migration.rb | 47 ++++++----- activerecord/lib/active_record/railtie.rb | 5 +- .../migration/pending_migrations_test.rb | 78 +++++++++++++++++-- 3 files changed, 97 insertions(+), 33 deletions(-) diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 222e803a3e..52aa1b265f 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -3,6 +3,7 @@ require "benchmark" require "set" require "zlib" +require "active_support/core_ext/enumerable" require "active_support/core_ext/module/attribute_accessors" require "active_support/actionable_error" @@ -553,21 +554,37 @@ module ActiveRecord # 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) + def initialize(app, file_watcher: ActiveSupport::FileUpdateChecker) @app = app - @last_check = 0 + @needs_check = true + @mutex = Mutex.new + @file_watcher = file_watcher end def call(env) - mtime = ActiveRecord::Base.connection.migration_context.last_migration.mtime.to_i - if @last_check < mtime - ActiveRecord::Migration.check_pending!(connection) - @last_check = mtime + @mutex.synchronize do + @watcher ||= build_watcher do + @needs_check = true + ActiveRecord::Migration.check_pending!(connection) + @needs_check = false + end + + if @needs_check + @watcher.execute + else + @watcher.execute_if_updated + end end + @app.call(env) end private + def build_watcher(&block) + paths = Array(connection.migration_context.migrations_paths) + @file_watcher.new([], paths.index_with(["rb"]), &block) + end + def connection ActiveRecord::Base.connection end @@ -993,10 +1010,6 @@ module ActiveRecord File.basename(filename) end - def mtime - File.mtime filename - end - delegate :migrate, :announce, :write, :disable_ddl_transaction, to: :migration private @@ -1010,16 +1023,6 @@ module ActiveRecord end end - class NullMigration < MigrationProxy #:nodoc: - def initialize - super(nil, 0, nil, nil) - end - - def mtime - 0 - end - end - class MigrationContext #:nodoc: attr_reader :migrations_paths, :schema_migration @@ -1098,10 +1101,6 @@ module ActiveRecord migrations.any? end - def last_migration #:nodoc: - migrations.last || NullMigration.new - end - def migrations migrations = migration_files.map do |file| version, name, scope = parse_migration_filename(file) diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index f06a98bd98..c0998e8bc6 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -82,10 +82,11 @@ module ActiveRecord ActiveSupport.on_load(:active_record) { LogSubscriber.backtrace_cleaner = ::Rails.backtrace_cleaner } end - initializer "active_record.migration_error" do + 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 + ActiveRecord::Migration::CheckPending, + file_watcher: app.config.file_watcher end end diff --git a/activerecord/test/cases/migration/pending_migrations_test.rb b/activerecord/test/cases/migration/pending_migrations_test.rb index 119bfd372a..dd6f88493f 100644 --- a/activerecord/test/cases/migration/pending_migrations_test.rb +++ b/activerecord/test/cases/migration/pending_migrations_test.rb @@ -7,34 +7,98 @@ module ActiveRecord if current_adapter?(:SQLite3Adapter) && !in_memory_db? class PendingMigrationsTest < ActiveRecord::TestCase setup do + @migration_dir = Dir.mktmpdir("activerecord-migrations-") + file = ActiveRecord::Base.connection.raw_connection.filename - @conn = ActiveRecord::Base.establish_connection adapter: "sqlite3", database: ":memory:", migrations_paths: MIGRATIONS_ROOT + "/valid" + @conn = ActiveRecord::Base.establish_connection adapter: "sqlite3", database: ":memory:", migrations_paths: @migration_dir source_db = SQLite3::Database.new file dest_db = ActiveRecord::Base.connection.raw_connection backup = SQLite3::Backup.new(dest_db, "main", source_db, "main") backup.step(-1) backup.finish + + ActiveRecord::Base.connection.drop_table "schema_migrations", if_exists: true + + @app = Minitest::Mock.new end teardown do @conn.release_connection if @conn ActiveRecord::Base.establish_connection :arunit + FileUtils.rm_rf(@migration_dir) + end + + def run_migrations + migrator = Base.connection.migration_context + capture(:stdout) { migrator.migrate } + end + + def create_migration(number, name) + filename = "#{number}_#{name.underscore}.rb" + File.write(File.join(@migration_dir, filename), <<~RUBY) + class #{name.classify} < ActiveRecord::Migration::Current + end + RUBY end def test_errors_if_pending - ActiveRecord::Base.connection.drop_table "schema_migrations", if_exists: true + create_migration "01", "create_foo" assert_raises ActiveRecord::PendingMigrationError do - CheckPending.new(Proc.new { }).call({}) + CheckPending.new(@app).call({}) + end + + # Continues failing + assert_raises ActiveRecord::PendingMigrationError do + CheckPending.new(@app).call({}) end end def test_checks_if_supported - ActiveRecord::SchemaMigration.create_table - migrator = Base.connection.migration_context - capture(:stdout) { migrator.migrate } + run_migrations - assert_nil CheckPending.new(Proc.new { }).call({}) + check_pending = CheckPending.new(@app) + + @app.expect :call, nil, [{}] + check_pending.call({}) + @app.verify + + # With cached result + @app.expect :call, nil, [{}] + check_pending.call({}) + @app.verify + end + + def test_okay_with_no_migrations + check_pending = CheckPending.new(@app) + + @app.expect :call, nil, [{}] + check_pending.call({}) + @app.verify + + # With cached result + @app.expect :call, nil, [{}] + check_pending.call({}) + @app.verify + end + + # Regression test for https://github.com/rails/rails/pull/29759 + def test_understands_migrations_created_out_of_order + # With a prior file before even initialization + create_migration "05", "create_bar" + run_migrations + + check_pending = CheckPending.new(@app) + + @app.expect :call, nil, [{}] + check_pending.call({}) + @app.verify + + # It understands the new migration created at 01 + create_migration "01", "create_foo" + assert_raises ActiveRecord::PendingMigrationError do + check_pending.call({}) + end end end end