From 90fe2a42f0b68a66e970169d38e91a0126de7d3e Mon Sep 17 00:00:00 2001 From: bogdanvlviv Date: Tue, 26 Sep 2017 00:10:17 +0300 Subject: Fix `bin/rails db:migrate` with specified `VERSION` Ensure that `bin/rails db:migrate` with specified `VERSION` reverts all migrations only if `VERSION` is `0`. Raise error if target migration doesn't exist. --- activerecord/CHANGELOG.md | 8 ++ activerecord/lib/active_record/migration.rb | 2 +- .../lib/active_record/railties/databases.rake | 19 ++- .../lib/active_record/tasks/database_tasks.rb | 15 +- activerecord/test/cases/migrator_test.rb | 20 +++ .../test/cases/tasks/database_tasks_test.rb | 149 +++++++++++++++++++- railties/test/application/rake/migrations_test.rb | 156 ++++++++++++++++++++- 7 files changed, 355 insertions(+), 14 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 4aa7ecfc7e..e6a41ec281 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,11 @@ +* Fix `bin/rails db:migrate` with specified `VERSION`. + `bin/rails db:migrate` with empty VERSION behaves as without `VERSION`. + Check a format of `VERSION`: Allow a migration version number + or name of a migration file. Raise error if format of `VERSION` is invalid. + Raise error if target migration doesn't exist. + + *bogdanvlviv* + * Fixed a bug where column orders for an index weren't written to db/schema.rb when using the sqlite adapter. diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 1485687053..d12a979a7f 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -1224,7 +1224,7 @@ module ActiveRecord # Return true if a valid version is not provided. def invalid_target? - !target && @target_version && @target_version > 0 + @target_version && @target_version != 0 && !target end def execute_migration_in_transaction(migration, direction) diff --git a/activerecord/lib/active_record/railties/databases.rake b/activerecord/lib/active_record/railties/databases.rake index 723272b4b2..3bca2982e0 100644 --- a/activerecord/lib/active_record/railties/databases.rake +++ b/activerecord/lib/active_record/railties/databases.rake @@ -97,16 +97,27 @@ db_namespace = namespace :db do task up: [:environment, :load_config] do raise "VERSION is required" if !ENV["VERSION"] || ENV["VERSION"].empty? - version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil - ActiveRecord::Migrator.run(:up, ActiveRecord::Tasks::DatabaseTasks.migrations_paths, version) + ActiveRecord::Tasks::DatabaseTasks.check_target_version + + ActiveRecord::Migrator.run( + :up, + ActiveRecord::Tasks::DatabaseTasks.migrations_paths, + ActiveRecord::Tasks::DatabaseTasks.target_version + ) db_namespace["_dump"].invoke end # desc 'Runs the "down" for a given migration VERSION.' task down: [:environment, :load_config] do raise "VERSION is required - To go down one migration, use db:rollback" if !ENV["VERSION"] || ENV["VERSION"].empty? - version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil - ActiveRecord::Migrator.run(:down, ActiveRecord::Tasks::DatabaseTasks.migrations_paths, version) + + ActiveRecord::Tasks::DatabaseTasks.check_target_version + + ActiveRecord::Migrator.run( + :down, + ActiveRecord::Tasks::DatabaseTasks.migrations_paths, + ActiveRecord::Tasks::DatabaseTasks.target_version + ) db_namespace["_dump"].invoke end diff --git a/activerecord/lib/active_record/tasks/database_tasks.rb b/activerecord/lib/active_record/tasks/database_tasks.rb index ff388ff1f6..4657e51e6d 100644 --- a/activerecord/lib/active_record/tasks/database_tasks.rb +++ b/activerecord/lib/active_record/tasks/database_tasks.rb @@ -164,13 +164,12 @@ module ActiveRecord end def migrate - raise "Empty VERSION provided" if ENV["VERSION"] && ENV["VERSION"].empty? + check_target_version verbose = ENV["VERBOSE"] ? ENV["VERBOSE"] != "false" : true - version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil scope = ENV["SCOPE"] verbose_was, Migration.verbose = Migration.verbose, verbose - Migrator.migrate(migrations_paths, version) do |migration| + Migrator.migrate(migrations_paths, target_version) do |migration| scope.blank? || scope == migration.scope end ActiveRecord::Base.clear_cache! @@ -178,6 +177,16 @@ module ActiveRecord Migration.verbose = verbose_was end + def check_target_version + if target_version && !(Migration::MigrationFilenameRegexp.match?(ENV["VERSION"]) || /\A\d+\z/.match?(ENV["VERSION"])) + raise "Invalid format of target version: `VERSION=#{ENV['VERSION']}`" + end + end + + def target_version + ENV["VERSION"].to_i if ENV["VERSION"] && !ENV["VERSION"].empty? + end + def charset_current(environment = env) charset ActiveRecord::Base.configurations[environment] end diff --git a/activerecord/test/cases/migrator_test.rb b/activerecord/test/cases/migrator_test.rb index ee10be119c..1047ba1367 100644 --- a/activerecord/test/cases/migrator_test.rb +++ b/activerecord/test/cases/migrator_test.rb @@ -66,6 +66,26 @@ class MigratorTest < ActiveRecord::TestCase list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)] ActiveRecord::Migrator.new(:up, list, 3).run end + + assert_raises(ActiveRecord::UnknownMigrationVersionError) do + list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)] + ActiveRecord::Migrator.new(:up, list, -1).run + end + + assert_raises(ActiveRecord::UnknownMigrationVersionError) do + list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)] + ActiveRecord::Migrator.new(:up, list, 0).run + end + + assert_raises(ActiveRecord::UnknownMigrationVersionError) do + list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)] + ActiveRecord::Migrator.new(:up, list, 3).migrate + end + + assert_raises(ActiveRecord::UnknownMigrationVersionError) do + list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)] + ActiveRecord::Migrator.new(:up, list, -1).migrate + end end def test_finds_migrations diff --git a/activerecord/test/cases/tasks/database_tasks_test.rb b/activerecord/test/cases/tasks/database_tasks_test.rb index 1495d2ab89..5a094ead42 100644 --- a/activerecord/test/cases/tasks/database_tasks_test.rb +++ b/activerecord/test/cases/tasks/database_tasks_test.rb @@ -357,8 +357,15 @@ module ActiveRecord ActiveRecord::Migration.expects(:verbose=).with(ActiveRecord::Migration.verbose) ActiveRecord::Tasks::DatabaseTasks.migrate + ENV["VERBOSE"] = "" + ENV["VERSION"] = "" + ActiveRecord::Migrator.expects(:migrate).with("custom/path", nil) + ActiveRecord::Migration.expects(:verbose=).with(true) + ActiveRecord::Migration.expects(:verbose=).with(ActiveRecord::Migration.verbose) + ActiveRecord::Tasks::DatabaseTasks.migrate + ENV["VERBOSE"] = "yes" - ENV["VERSION"] = "unknown" + ENV["VERSION"] = "0" ActiveRecord::Migrator.expects(:migrate).with("custom/path", 0) ActiveRecord::Migration.expects(:verbose=).with(true) ActiveRecord::Migration.expects(:verbose=).with(ActiveRecord::Migration.verbose) @@ -367,15 +374,47 @@ module ActiveRecord ENV["VERBOSE"], ENV["VERSION"] = verbose, version end - def test_migrate_raise_error_on_empty_version + def test_migrate_raise_error_on_invalid_version_format version = ENV["VERSION"] - ENV["VERSION"] = "" + + ENV["VERSION"] = "unknown" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "0.1.11" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1.1.11" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "0 " + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1." + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1_" e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } - assert_equal "Empty VERSION provided", e.message + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1_name" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } + assert_match(/Invalid format of target version/, e.message) ensure ENV["VERSION"] = version end + def test_migrate_raise_error_on_failed_check_target_version + ActiveRecord::Tasks::DatabaseTasks.stubs(:check_target_version).raises("foo") + + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate } + assert_equal "foo", e.message + end + def test_migrate_clears_schema_cache_afterward ActiveRecord::Base.expects(:clear_cache!) ActiveRecord::Tasks::DatabaseTasks.migrate @@ -444,6 +483,108 @@ module ActiveRecord end end + class DatabaseTaskTargetVersionTest < ActiveRecord::TestCase + def test_target_version_returns_nil_if_version_does_not_exist + version = ENV.delete("VERSION") + assert_nil ActiveRecord::Tasks::DatabaseTasks.target_version + ensure + ENV["VERSION"] = version + end + + def test_target_version_returns_nil_if_version_is_empty + version = ENV["VERSION"] + + ENV["VERSION"] = "" + assert_nil ActiveRecord::Tasks::DatabaseTasks.target_version + ensure + ENV["VERSION"] = version + end + + def test_target_version_returns_converted_to_integer_env_version_if_version_exists + version = ENV["VERSION"] + + ENV["VERSION"] = "0" + assert_equal ENV["VERSION"].to_i, ActiveRecord::Tasks::DatabaseTasks.target_version + + ENV["VERSION"] = "42" + assert_equal ENV["VERSION"].to_i, ActiveRecord::Tasks::DatabaseTasks.target_version + + ENV["VERSION"] = "042" + assert_equal ENV["VERSION"].to_i, ActiveRecord::Tasks::DatabaseTasks.target_version + ensure + ENV["VERSION"] = version + end + end + + class DatabaseTaskCheckTargetVersionTest < ActiveRecord::TestCase + def test_check_target_version_does_not_raise_error_on_empty_version + version = ENV["VERSION"] + ENV["VERSION"] = "" + assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + ensure + ENV["VERSION"] = version + end + + def test_check_target_version_does_not_raise_error_if_version_is_not_setted + version = ENV.delete("VERSION") + assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + ensure + ENV["VERSION"] = version + end + + def test_check_target_version_raises_error_on_invalid_version_format + version = ENV["VERSION"] + + ENV["VERSION"] = "unknown" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "0.1.11" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1.1.11" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "0 " + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1." + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1_" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + assert_match(/Invalid format of target version/, e.message) + + ENV["VERSION"] = "1_name" + e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + assert_match(/Invalid format of target version/, e.message) + ensure + ENV["VERSION"] = version + end + + def test_check_target_version_does_not_raise_error_on_valid_version_format + version = ENV["VERSION"] + + ENV["VERSION"] = "0" + assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + + ENV["VERSION"] = "1" + assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + + ENV["VERSION"] = "001" + assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + + ENV["VERSION"] = "001_name.rb" + assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version } + ensure + ENV["VERSION"] = version + end + end + class DatabaseTasksStructureDumpTest < ActiveRecord::TestCase include DatabaseTasksSetupper diff --git a/railties/test/application/rake/migrations_test.rb b/railties/test/application/rake/migrations_test.rb index 33f2b7038c..788f9160d6 100644 --- a/railties/test/application/rake/migrations_test.rb +++ b/railties/test/application/rake/migrations_test.rb @@ -37,9 +37,64 @@ module ApplicationTests assert_match(/AMigration: reverted/, output) end + test "migrate with specified VERSION in different formats" do + app_file "db/migrate/01_one_migration.rb", <<-MIGRATION + class OneMigration < ActiveRecord::Migration::Current + end + MIGRATION + + app_file "db/migrate/02_two_migration.rb", <<-MIGRATION + class TwoMigration < ActiveRecord::Migration::Current + end + MIGRATION + + app_file "db/migrate/03_three_migration.rb", <<-MIGRATION + class ThreeMigration < ActiveRecord::Migration::Current + end + MIGRATION + + rails "db:migrate" + + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/up\s+002\s+Two migration/, output) + assert_match(/up\s+003\s+Three migration/, output) + + rails "db:migrate", "VERSION=01_one_migration.rb" + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/down\s+002\s+Two migration/, output) + assert_match(/down\s+003\s+Three migration/, output) + + rails "db:migrate", "VERSION=3" + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/up\s+002\s+Two migration/, output) + assert_match(/up\s+003\s+Three migration/, output) + + rails "db:migrate", "VERSION=001" + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/down\s+002\s+Two migration/, output) + assert_match(/down\s+003\s+Three migration/, output) + end + test "migration with empty version" do - output = rails("db:migrate", "VERSION=", allow_failure: true) - assert_match(/Empty VERSION provided/, output) + app_file "db/migrate/01_one_migration.rb", <<-MIGRATION + class OneMigration < ActiveRecord::Migration::Current + end + MIGRATION + + app_file "db/migrate/02_two_migration.rb", <<-MIGRATION + class TwoMigration < ActiveRecord::Migration::Current + end + MIGRATION + + rails("db:migrate", "VERSION=") + + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/up\s+002\s+Two migration/, output) output = rails("db:migrate:redo", "VERSION=", allow_failure: true) assert_match(/Empty VERSION provided/, output) @@ -55,6 +110,34 @@ module ApplicationTests output = rails("db:migrate:down", allow_failure: true) assert_match(/VERSION is required - To go down one migration, use db:rollback/, output) + + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/up\s+002\s+Two migration/, output) + end + + test "migration with 0 version" do + app_file "db/migrate/01_one_migration.rb", <<-MIGRATION + class OneMigration < ActiveRecord::Migration::Current + end + MIGRATION + + app_file "db/migrate/02_two_migration.rb", <<-MIGRATION + class TwoMigration < ActiveRecord::Migration::Current + end + MIGRATION + + rails "db:migrate" + + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/up\s+002\s+Two migration/, output) + + rails "db:migrate", "VERSION=0" + + output = rails("db:migrate:status") + assert_match(/down\s+001\s+One migration/, output) + assert_match(/down\s+002\s+Two migration/, output) end test "model and migration generator with change syntax" do @@ -192,6 +275,75 @@ module ApplicationTests end end + test "raise error on any move when target migration does not exist" do + app_file "db/migrate/01_one_migration.rb", <<-MIGRATION + class OneMigration < ActiveRecord::Migration::Current + end + MIGRATION + + app_file "db/migrate/02_two_migration.rb", <<-MIGRATION + class TwoMigration < ActiveRecord::Migration::Current + end + MIGRATION + + rails "db:migrate" + + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/up\s+002\s+Two migration/, output) + + output = rails("db:migrate", "VERSION=3", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/ActiveRecord::UnknownMigrationVersionError:/, output) + assert_match(/No migration with version number 3/, output) + + output = rails("db:migrate:status") + assert_match(/up\s+001\s+One migration/, output) + assert_match(/up\s+002\s+Two migration/, output) + end + + test "raise error on any move when VERSION has invalid format" do + output = rails("db:migrate", "VERSION=unknown", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate", "VERSION=0.1.11", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate", "VERSION=1.1.11", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate", "VERSION='0 '", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate", "VERSION=1.", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate", "VERSION=1_", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate", "VERSION=1_name", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate:redo", "VERSION=unknown", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate:up", "VERSION=unknown", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + + output = rails("db:migrate:down", "VERSION=unknown", allow_failure: true) + assert_match(/rails aborted!/, output) + assert_match(/Invalid format of target version/, output) + end + test "migration status after rollback and redo without timestamps" do add_to_config("config.active_record.timestamped_migrations = false") -- cgit v1.2.3