Merge pull request rails#50400 from adrianna-chang-shopify/ac-validate-migration-timestamps-2

Add `active_record.config.validate_migration_timestamps` config option.

Author: eileencodes

activerecord/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Add `active_record.config.validate_migration_timestamps` option for validating migration timestamps.
+
+    When set, validates that the timestamp prefix for a migration is no more than a day ahead of
+    the timestamp associated with the current time. This is designed to prevent migrations prefixes
+    from being hand-edited to future timestamps, which impacts migration generation and other
+    migration commands.
+
+    *Adrianna Chang*
+
 *   Properly synchronize `Mysql2Adapter#active?` and `TrilogyAdapter#active?`
 
     As well as `disconnect!` and `verify!`.

activerecord/lib/active_record.rb

@@ -380,6 +380,14 @@ def self.global_executor_concurrency # :nodoc:
   singleton_class.attr_accessor :timestamped_migrations
   self.timestamped_migrations = true
 
+  ##
+  # :singleton-method: validate_migration_timestamps
+  # Specify whether or not to validate migration timestamps. When set, an error
+  # will be raised if a timestamp is more than a day ahead of the timestamp
+  # associated with the current time. +timestamped_migrations+ must be set to true.
+  singleton_class.attr_accessor :validate_migration_timestamps
+  self.validate_migration_timestamps = false
+
   ##
   # :singleton-method: migration_strategy
   # Specify strategy to use for executing migrations.

activerecord/lib/active_record/migration.rb

@@ -131,6 +131,22 @@ def initialize(name = nil)
     end
   end
 
+  class InvalidMigrationTimestampError < MigrationError # :nodoc:
+    def initialize(version = nil, name = nil)
+      if version && name
+        super(<<~MSG)
+          Invalid timestamp #{version} for migration file: #{name}.
+          Timestamp must be in form YYYYMMDDHHMMSS, and less than #{(Time.now.utc + 1.day).strftime("%Y%m%d%H%M%S")}.
+        MSG
+      else
+        super(<<~MSG)
+          Invalid timestamp for migration.
+          Timestamp must be in form YYYYMMDDHHMMSS, and less than #{(Time.now.utc + 1.day).strftime("%Y%m%d%H%M%S")}.
+        MSG
+      end
+    end
+  end
+
   class PendingMigrationError < MigrationError # :nodoc:
     include ActiveSupport::ActionableError
 
@@ -494,7 +510,11 @@ def initialize
   #
   #    20080717013526_your_migration_name.rb
   #
-  # The prefix is a generation timestamp (in UTC).
+  # The prefix is a generation timestamp (in UTC). Timestamps should not be
+  # modified manually. To validate that migration timestamps adhere to the
+  # format Active Record expects, you can use the following configuration option:
+  #
+  #    config.active_record.validate_migration_timestamps = true
   #
   # If you'd prefer to use numeric prefixes, you can turn timestamped migrations
   # off by setting:
@@ -1316,6 +1336,9 @@ def migrations # :nodoc:
       migrations = migration_files.map do |file|
         version, name, scope = parse_migration_filename(file)
         raise IllegalMigrationNameError.new(file) unless version
+        if validate_timestamp? && !valid_migration_timestamp?(version)
+          raise InvalidMigrationTimestampError.new(version, name)
+        end
         version = version.to_i
         name = name.camelize
 
@@ -1331,6 +1354,9 @@ def migrations_status # :nodoc:
       file_list = migration_files.filter_map do |file|
         version, name, scope = parse_migration_filename(file)
         raise IllegalMigrationNameError.new(file) unless version
+        if validate_timestamp? && !valid_migration_timestamp?(version)
+          raise InvalidMigrationTimestampError.new(version, name)
+        end
         version = schema_migration.normalize_migration_number(version)
         status = db_list.delete(version) ? "up" : "down"
         [status, version, (name + scope).humanize]
@@ -1375,6 +1401,14 @@ def parse_migration_filename(filename)
         File.basename(filename).scan(Migration::MigrationFilenameRegexp).first
       end
 
+      def validate_timestamp?
+        ActiveRecord.timestamped_migrations && ActiveRecord.validate_migration_timestamps
+      end
+
+      def valid_migration_timestamp?(version)
+        version.to_i < (Time.now.utc + 1.day).strftime("%Y%m%d%H%M%S").to_i
+      end
+
       def move(direction, steps)
         migrator = Migrator.new(direction, migrations, schema_migration, internal_metadata)
 

activerecord/test/cases/migration_test.rb

@@ -1807,6 +1807,8 @@ def setup
       @verbose_was, ActiveRecord::Migration.verbose = ActiveRecord::Migration.verbose, false
       @schema_migration = ActiveRecord::Base.connection.schema_migration
       @internal_metadata = ActiveRecord::Base.connection.internal_metadata
+      @active_record_validate_timestamps_was = ActiveRecord.validate_migration_timestamps
+      ActiveRecord.validate_migration_timestamps = true
       ActiveRecord::Base.connection.schema_cache.clear!
 
       @migrations_path = MIGRATIONS_ROOT + "/temp"
@@ -1816,15 +1818,69 @@ def setup
     def teardown
       @schema_migration.create_table
       @schema_migration.delete_all_versions
+      ActiveRecord.validate_migration_timestamps = @active_record_validate_timestamps_was
       ActiveRecord::Migration.verbose = @verbose_was
     end
 
+    def test_migration_raises_if_timestamp_greater_than_14_digits
+      with_temp_migration_files(["201801010101010000_test_migration.rb"]) do
+        error = assert_raises(ActiveRecord::InvalidMigrationTimestampError) do
+          @migrator.up(201801010101010000)
+        end
+        assert_match(/Invalid timestamp 201801010101010000 for migration file: test_migration/, error.message)
+      end
+    end
+
+    def test_migration_raises_if_timestamp_is_future_date
+      timestamp = (Time.now.utc + 1.month).strftime("%Y%m%d%H%M%S").to_i
+      with_temp_migration_files(["#{timestamp}_test_migration.rb"]) do
+        error = assert_raises(ActiveRecord::InvalidMigrationTimestampError) do
+          @migrator.up(timestamp)
+        end
+        assert_match(/Invalid timestamp #{timestamp} for migration file: test_migration/, error.message)
+      end
+    end
+
+    def test_migration_succeeds_if_timestamp_is_less_than_one_day_in_the_future
+      timestamp = (Time.now.utc + 1.minute).strftime("%Y%m%d%H%M%S").to_i
+      with_temp_migration_files(["#{timestamp}_test_migration.rb"]) do
+        @migrator.up(timestamp)
+        assert_equal timestamp, @migrator.current_version
+      end
+    end
+
+    def test_migration_succeeds_despite_future_timestamp_if_validate_timestamps_is_false
+      validate_migration_timestamps_was = ActiveRecord.validate_migration_timestamps
+      ActiveRecord.validate_migration_timestamps = false
+
+      timestamp = (Time.now.utc + 1.month).strftime("%Y%m%d%H%M%S").to_i
+      with_temp_migration_files(["#{timestamp}_test_migration.rb"]) do
+        @migrator.up(timestamp)
+        assert_equal timestamp, @migrator.current_version
+      end
+    ensure
+      ActiveRecord.validate_migration_timestamps = validate_migration_timestamps_was
+    end
+
+    def test_migration_succeeds_despite_future_timestamp_if_timestamped_migrations_is_false
+      timestamped_migrations_was = ActiveRecord.timestamped_migrations
+      ActiveRecord.timestamped_migrations = false
+
+      timestamp = (Time.now.utc + 1.month).strftime("%Y%m%d%H%M%S").to_i
+      with_temp_migration_files(["#{timestamp}_test_migration.rb"]) do
+        @migrator.up(timestamp)
+        assert_equal timestamp, @migrator.current_version
+      end
+    ensure
+      ActiveRecord.timestamped_migrations = timestamped_migrations_was
+    end
+
     def test_copied_migrations_at_timestamp_boundary_are_valid
       migrations_path_source = MIGRATIONS_ROOT + "/temp_source"
       migrations_path_dest = MIGRATIONS_ROOT + "/temp_dest"
       migrations = ["20180101010101_test_migration.rb", "20180101010102_test_migration_two.rb", "20180101010103_test_migration_three.rb"]
 
-      with_temp_migration_files(migrations_path_source, migrations) do
+      with_temp_migration_files(migrations, migrations_path_source) do
         travel_to(Time.utc(2023, 12, 1, 10, 10, 59)) do
           ActiveRecord::Migration.copy(migrations_path_dest, temp: migrations_path_source)
 
@@ -1847,7 +1903,7 @@ def test_copied_migrations_at_timestamp_boundary_are_valid
     end
 
     private
-      def with_temp_migration_files(migrations_dir, filenames)
+      def with_temp_migration_files(filenames, migrations_dir = @migrations_path)
         Dir.mkdir(migrations_dir) unless Dir.exist?(migrations_dir)
 
         paths = []

guides/source/configuring.md

@@ -58,6 +58,10 @@ NOTE: If you need to apply configuration directly to a class, use a [lazy load h
 
 Below are the default values associated with each target version. In cases of conflicting values, newer versions take precedence over older versions.
 
+#### Default Values for Target Version 7.2
+
+- [`config.active_record.validate_migration_timestamps`](#config-active-record-validate-migration-timestamps): `true`
+
 #### Default Values for Target Version 7.1
 
 - [`config.action_dispatch.debug_exception_log_level`](#config-action-dispatch-debug-exception-log-level): `:error`
@@ -1048,6 +1052,20 @@ Specifies if an error should be raised if the order of a query is ignored during
 
 Controls whether migrations are numbered with serial integers or with timestamps. The default is `true`, to use timestamps, which are preferred if there are multiple developers working on the same application.
 
+#### `config.active_record.validate_migration_timestamps`
+
+Controls whether to validate migration timestamps. When set, an error will be raised if the
+timestamp prefix for a migration is more than a day ahead of the timestamp associated with the
+current time. This is done to prevent forward-dating of migration files, which can impact migration
+generation and other migration commands. `config.active_record.timestamped_migrations` must be set to `true`.
+
+The default value depends on the `config.load_defaults` target version:
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `false`              |
+| 7.2                   | `true`               |
+
 #### `config.active_record.db_warnings_action`
 
 Controls the action to be taken when an SQL query produces a warning. The following options are available:

railties/lib/rails/application/configuration.rb

@@ -323,6 +323,10 @@ def load_defaults(target_version)
           end
         when "7.2"
           load_defaults "7.1"
+
+          if respond_to?(:active_record)
+            active_record.validate_migration_timestamps = true
+          end
         else
           raise "Unknown version #{target_version.to_s.inspect}"
         end

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_2.rb.tt

@@ -8,3 +8,14 @@
 #
 # Read the Guide for Upgrading Ruby on Rails for more info on each option.
 # https://guides.rubyonrails.org/upgrading_ruby_on_rails.html
+
+###
+# Enable validation of migration timestamps. When set, an ActiveRecord::InvalidMigrationTimestampError
+# will be raised if the timestamp prefix for a migration is more than a day ahead of the timestamp
+# associated with the current time. This is done to prevent forward-dating of migration files, which can
+# impact migration generation and other migration commands.
+#
+# Applications with existing timestamped migrations that do not adhere to the
+# expected format can disable validation by setting this config to `false`.
+#++
+# Rails.application.config.active_record.validate_migration_timestamps = true

railties/test/application/loading_test.rb

@@ -389,6 +389,7 @@ class User
   test "columns migrations also trigger reloading" do
     add_to_config <<-RUBY
       config.enable_reloading = true
+      config.active_record.timestamped_migrations = false
     RUBY
 
     app_file "config/routes.rb", <<-RUBY

railties/test/application/rake/migrations_test.rb

@@ -8,6 +8,7 @@ class RakeMigrationsTest < ActiveSupport::TestCase
       def setup
         build_app
         FileUtils.rm_rf("#{app_path}/config/environments")
+        add_to_config("config.active_record.timestamped_migrations = false")
       end
 
       def teardown
@@ -17,7 +18,7 @@ def teardown
       test "running migrations with given scope" do
         rails "generate", "model", "user", "username:string", "password:string"
 
-        app_file "db/migrate/01_a_migration.bukkits.rb", <<-MIGRATION
+        app_file "db/migrate/02_a_migration.bukkits.rb", <<-MIGRATION
           class AMigration < ActiveRecord::Migration::Current
           end
         MIGRATION
@@ -200,6 +201,8 @@ class TwoMigration < ActiveRecord::Migration::Current
       end
 
       test "migration status" do
+        remove_from_config("config.active_record.timestamped_migrations = false")
+
         rails "generate", "model", "user", "username:string", "password:string"
         rails "generate", "migration", "add_email_to_users", "email:string"
         rails "db:migrate"
@@ -217,7 +220,7 @@ class TwoMigration < ActiveRecord::Migration::Current
       end
 
       test "migration status without timestamps" do
-        add_to_config("config.active_record.timestamped_migrations = false")
+        remove_from_config("config.active_record.timestamped_migrations = false")
 
         rails "generate", "model", "user", "username:string", "password:string"
         rails "generate", "migration", "add_email_to_users", "email:string"
@@ -236,6 +239,8 @@ class TwoMigration < ActiveRecord::Migration::Current
       end
 
       test "migration status after rollback and redo" do
+        remove_from_config("config.active_record.timestamped_migrations = false")
+
         rails "generate", "model", "user", "username:string", "password:string"
         rails "generate", "migration", "add_email_to_users", "email:string"
         rails "db:migrate"
@@ -259,6 +264,8 @@ class TwoMigration < ActiveRecord::Migration::Current
       end
 
       test "migration status after rollback and forward" do
+        remove_from_config("config.active_record.timestamped_migrations = false")
+
         rails "generate", "model", "user", "username:string", "password:string"
         rails "generate", "migration", "add_email_to_users", "email:string"
         rails "db:migrate"
@@ -283,6 +290,8 @@ class TwoMigration < ActiveRecord::Migration::Current
 
       test "raise error on any move when current migration does not exist" do
         Dir.chdir(app_path) do
+          remove_from_config("config.active_record.timestamped_migrations = false")
+
           rails "generate", "model", "user", "username:string", "password:string"
           rails "generate", "migration", "add_email_to_users", "email:string"
           rails "db:migrate"
@@ -382,8 +391,6 @@ class TwoMigration < ActiveRecord::Migration::Current
       end
 
       test "migration status after rollback and redo without timestamps" do
-        add_to_config("config.active_record.timestamped_migrations = false")
-
         rails "generate", "model", "user", "username:string", "password:string"
         rails "generate", "migration", "add_email_to_users", "email:string"
         rails "db:migrate"
@@ -497,6 +504,8 @@ class TwoMigration < ActiveRecord::Migration::Current
 
       test "migration status migrated file is deleted" do
         Dir.chdir(app_path) do
+          remove_from_config("config.active_record.timestamped_migrations = false")
+
           rails "generate", "model", "user", "username:string", "password:string"
           rails "generate", "migration", "add_email_to_users", "email:string"
           rails "db:migrate"
@@ -523,7 +532,7 @@ class ApplicationRecord < ActiveRecord::Base
 
           rails("db:migrate")
 
-          app_file "db/migrate/01_a_migration.bukkits.rb", <<-MIGRATION
+          app_file "db/migrate/02_a_migration.bukkits.rb", <<-MIGRATION
             class AMigration < ActiveRecord::Migration::Current
               def change
                 User.first

railties/test/application/rake/multi_dbs_test.rb

@@ -10,6 +10,7 @@ class RakeMultiDbsTest < ActiveSupport::TestCase
       def setup
         build_app(multi_db: true)
         FileUtils.rm_rf("#{app_path}/config/environments")
+        add_to_config("config.active_record.timestamped_migrations = false")
       end
 
       def teardown
@@ -779,11 +780,13 @@ class TwoMigration < ActiveRecord::Migration::Current
       end
 
       test "db:migrate:status works on all databases" do
+        remove_from_config("config.active_record.timestamped_migrations = false")
         require "#{app_path}/config/environment"
         db_migrate_and_migrate_status
       end
 
       test "db:migrate:status:namespace works" do
+        remove_from_config("config.active_record.timestamped_migrations = false")
         require "#{app_path}/config/environment"
         ActiveRecord::Base.configurations.configs_for(env_name: Rails.env).each do |db_config|
           db_migrate_namespaced db_config.name