Merge pull request rails#50231 from rails/revert-50205-ac-validate-migration-timestamps

eileencodes · web-flow · commit b3b230c08e5f · 2023-12-01T12:46:02.000-05:00
Revert "Add config for validating migration timestamps"
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,10 +1,3 @@
-*   Add `active_record.config.validate_migration_timestamps` option for validating migration timestamps.
-
-    When set, validates that the timestamp prefix for a migration is in the form YYYYMMDDHHMMSS.
-    This is designed to prevent migration timestamps from being modified by hand.
-
-    *Adrianna Chang*
-
 *   When using a `DATABASE_URL`, allow for a configuration to map the protocol in the URL to a specific database
     adapter. This allows decoupling the adapter the application chooses to use from the database connection details
     set in the deployment environment.
diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb
@@ -370,14 +370,6 @@ def self.global_executor_concurrency # :nodoc:
   singleton_class.attr_accessor :timestamped_migrations
   self.timestamped_migrations = true
 
-  ##
-  # :singleton-method:
-  # Specify whether or not to validate migration timestamps. When set, an error
-  # will be raised if a timestamp is not a valid timestamp in the form
-  # YYYYMMDDHHMMSS. +timestamped_migrations+ must be set to true.
-  singleton_class.attr_accessor :validate_migration_timestamps
-  self.validate_migration_timestamps = false
-
   ##
   # :singleton-method:
   # Specify strategy to use for executing migrations.
diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
@@ -131,16 +131,6 @@ def initialize(name = nil)
     end
   end
 
-  class InvalidMigrationTimestampError < MigrationError # :nodoc:
-    def initialize(version = nil, name = nil)
-      if version && name
-        super("Invalid timestamp #{version} for migration file: #{name}.\nTimestamp should be in form YYYYMMDDHHMMSS.")
-      else
-        super("Invalid timestamp for migration. Timestamp should be in form YYYYMMDDHHMMSS.")
-      end
-    end
-  end
-
   class PendingMigrationError < MigrationError # :nodoc:
     include ActiveSupport::ActionableError
 
@@ -503,11 +493,7 @@ def initialize
   #
   #    20080717013526_your_migration_name.rb
   #
-  # 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
+  # The prefix is a generation timestamp (in UTC).
   #
   # If you'd prefer to use numeric prefixes, you can turn timestamped migrations
   # off by setting:
@@ -1330,9 +1316,6 @@ 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
 
@@ -1348,9 +1331,6 @@ 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]
@@ -1395,22 +1375,6 @@ 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)
-        timestamp = "#{version} UTC"
-        timestamp_format = "%Y%m%d%H%M%S %Z"
-        time = Time.strptime(timestamp, timestamp_format)
-
-        # Time.strptime will wrap if a day between 1-31 is specified, even if the day doesn't exist
-        # for the given month. Comparing against the original timestamp guards against this.
-        time.strftime(timestamp_format) == timestamp
-      rescue ArgumentError
-        false
-      end
-
       def move(direction, steps)
         migrator = Migrator.new(direction, migrations, schema_migration, internal_metadata)
 
diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb
@@ -1801,103 +1801,75 @@ def test_check_pending_with_stdlib_logger
   def test_unknown_migration_version_should_raise_an_argument_error
     assert_raise(ArgumentError) { ActiveRecord::Migration[1.0] }
   end
-end
-
-class MigrationValidationTest < ActiveRecord::TestCase
-  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"
-    @migrator = ActiveRecord::MigrationContext.new(@migrations_path, @schema_migration, @internal_metadata)
-  end
 
-  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
+  class MigrationValidationTest < ActiveRecord::TestCase
+    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
+      ActiveRecord::Base.connection.schema_cache.clear!
 
-  def test_migration_raises_if_timestamp_not_14_digits
-    with_temp_migration_file(@migrations_path, "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)
+      @migrations_path = MIGRATIONS_ROOT + "/temp"
+      @migrator = ActiveRecord::MigrationContext.new(@migrations_path, @schema_migration, @internal_metadata)
     end
 
-    with_temp_migration_file(@migrations_path, "201801010_test_migration.rb") do
-      error = assert_raises(ActiveRecord::InvalidMigrationTimestampError) do
-        @migrator.up(201801010)
-      end
-      assert_match(/Invalid timestamp 201801010 for migration file: test_migration/, error.message)
+    def teardown
+      @schema_migration.create_table
+      @schema_migration.delete_all_versions
+      ActiveRecord::Migration.verbose = @verbose_was
     end
-  end
 
-  def test_migration_raises_if_timestamp_is_invalid_date
-    # 2023-15-26 is an invalid date
-    with_temp_migration_file(@migrations_path, "20231526101010_test_migration.rb") do
-      error = assert_raises(ActiveRecord::InvalidMigrationTimestampError) do
-        @migrator.up(20231526101010)
-      end
-      assert_match(/Invalid timestamp 20231526101010 for migration file: test_migration/, error.message)
-    end
-  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
+        travel_to(Time.utc(2023, 12, 1, 10, 10, 59)) do
+          ActiveRecord::Migration.copy(migrations_path_dest, temp: migrations_path_source)
+
+          assert File.exist?(migrations_path_dest + "/20231201101059_test_migration.temp.rb")
+          assert File.exist?(migrations_path_dest + "/20231201101060_test_migration_two.temp.rb")
+          assert File.exist?(migrations_path_dest + "/20231201101061_test_migration_three.temp.rb")
+
+          migrator = ActiveRecord::MigrationContext.new(migrations_path_dest, @schema_migration, @internal_metadata)
+          migrator.up(20231201101059)
+          migrator.up(20231201101060)
+          migrator.up(20231201101061)
 
-  def test_migration_raises_if_timestamp_is_invalid_day_month_combo
-    # 2023-02-31 is an invalid date; 02 and 31 are valid month / days individually,
-    # but February 31 does not exist.
-    with_temp_migration_file(@migrations_path, "20230231101010_test_migration.rb") do
-      error = assert_raises(ActiveRecord::InvalidMigrationTimestampError) do
-        @migrator.up(20230231101010)
+          assert_equal 20231201101061, migrator.current_version
+          assert_not migrator.needs_migration?
+        end
       end
-      assert_match(/Invalid timestamp 20230231101010 for migration file: test_migration/, error.message)
+    ensure
+      File.delete(*Dir[migrations_path_dest + "/*.rb"])
+      Dir.rmdir(migrations_path_dest) if Dir.exist?(migrations_path_dest)
     end
-  end
 
-  def test_migration_succeeds_despite_invalid_timestamp_if_validate_timestamps_is_false
-    validate_migration_timestamps_was = ActiveRecord.validate_migration_timestamps
-    ActiveRecord.validate_migration_timestamps = false
-    with_temp_migration_file(@migrations_path, "201801010_test_migration.rb") do
-      @migrator.up(201801010)
-      assert_equal 201801010, @migrator.current_version
-    end
-  ensure
-    ActiveRecord.validate_migration_timestamps = validate_migration_timestamps_was
-  end
+    private
+      def with_temp_migration_files(migrations_dir, filenames)
+        Dir.mkdir(migrations_dir) unless Dir.exist?(migrations_dir)
 
-  def test_migration_succeeds_despite_invalid_timestamp_if_timestamped_migrations_is_false
-    timestamped_migrations_was = ActiveRecord.timestamped_migrations
-    ActiveRecord.timestamped_migrations = false
-    with_temp_migration_file(@migrations_path, "201801010_test_migration.rb") do
-      @migrator.up(201801010)
-      assert_equal 201801010, @migrator.current_version
-    end
-  ensure
-    ActiveRecord.timestamped_migrations = timestamped_migrations_was
-  end
+        paths = []
+        filenames.each do |filename|
+          path = File.join(migrations_dir, filename)
+          paths << path
 
-  private
-    def with_temp_migration_file(migrations_dir, filename)
-      Dir.mkdir(migrations_dir) unless Dir.exist?(migrations_dir)
-      path = File.join(migrations_dir, filename)
+          migration_class = filename.match(ActiveRecord::Migration::MigrationFilenameRegexp)[2].camelize
 
-      File.open(path, "w+") do |file|
-        file << <<-MIGRATION
-      class TestMigration < ActiveRecord::Migration::Current
-        def change; end
-      end
-        MIGRATION
-      end
+          File.open(path, "w+") do |file|
+            file << <<~MIGRATION
+          class #{migration_class} < ActiveRecord::Migration::Current
+            def change; end
+          end
+            MIGRATION
+          end
+        end
 
-      yield
-    ensure
-      File.delete(path) if File.exist?(path)
-      Dir.rmdir(migrations_dir) if Dir.exist?(migrations_dir)
-    end
+        yield
+      ensure
+        paths.each { |path| File.delete(path) if File.exist?(path) }
+        Dir.rmdir(migrations_dir) if Dir.exist?(migrations_dir)
+      end
+  end
 end
diff --git a/guides/source/configuring.md b/guides/source/configuring.md
@@ -58,10 +58,6 @@ 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`
@@ -1051,17 +1047,6 @@ 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 a timestamp is not a valid timestamp in the form YYYYMMDDHHMMSS. `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 a SQL query produces a warning. The following options are available:
diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb
@@ -323,10 +323,6 @@ 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
diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_2.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_2.rb.tt
@@ -8,13 +8,3 @@
 #
 # 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. Migration timestamps must be
-# in the form YYYYMMDDHHMMSS, or an ActiveRecord::InvalidMigrationTimestampError
-# will be raised.
-#
-# 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
diff --git a/railties/test/application/loading_test.rb b/railties/test/application/loading_test.rb
@@ -319,7 +319,6 @@ 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
diff --git a/railties/test/application/rake/migrations_test.rb b/railties/test/application/rake/migrations_test.rb
diff --git a/railties/test/application/rake/multi_dbs_test.rb b/railties/test/application/rake/multi_dbs_test.rb
diff --git a/railties/test/railties/engine_test.rb b/railties/test/railties/engine_test.rb
