Merge pull request #37395 from jhawthorn/check_pending_file_watcher3

Use FileUpdateChecker for Migration::CheckPending

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