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

@@ -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 <tt>config.active_record.migration_error</tt> 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)

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
 

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