Skip to content

Commit

Permalink
Add active_record.config.validate_migration_timestamps config option.
Browse files Browse the repository at this point in the history
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.

It is turned off by default, but will be turned on for applications starting in Rails 7.2.
  • Loading branch information
adrianna-chang-shopify committed Dec 19, 2023
1 parent 18b8f37 commit efaf4fc
Show file tree
Hide file tree
Showing 11 changed files with 169 additions and 8 deletions.
9 changes: 9 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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*

* Add support for generated columns in SQLite3 adapter

Generated columns (both stored and dynamic) are supported since version 3.31.0 of SQLite.
Expand Down
8 changes: 8 additions & 0 deletions activerecord/lib/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,14 @@ 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 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:
# Specify strategy to use for executing migrations.
Expand Down
36 changes: 35 additions & 1 deletion activerecord/lib/active_record/migration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.squish)
Invalid timestamp #{version} for migration file: #{name}.\nTimestamp must be in form YYYYMMDDHHMMSS,
and less than #{(Time.now.utc + 1.day).strftime("%Y%m%d%H%M%S")}.
MSG
else
super(<<-MSG.squish)
Invalid timestamp for migration.\nTimestamp 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

Expand Down Expand Up @@ -493,7 +509,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:
Expand Down Expand Up @@ -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

Expand All @@ -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]
Expand Down Expand Up @@ -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)

Expand Down
60 changes: 58 additions & 2 deletions activerecord/test/cases/migration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)

Expand All @@ -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 = []
Expand Down
18 changes: 18 additions & 0 deletions guides/source/configuring.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -1047,6 +1051,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 a SQL query produces a warning. The following options are available:
Expand Down
4 changes: 4 additions & 0 deletions railties/lib/rails/application/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions railties/test/application/loading_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,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
Expand Down
19 changes: 14 additions & 5 deletions railties/test/application/rake/migrations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand All @@ -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
Expand Down
3 changes: 3 additions & 0 deletions railties/test/application/rake/multi_dbs_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit efaf4fc

Please sign in to comment.