Merge pull request #30714 from bogdanvlviv/fix-rails_db_migrate_VERSION

Fix `bin/rails db:migrate` with specified `VERSION`

activerecord/CHANGELOG.md

+*   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.
 

activerecord/lib/active_record/migration.rb

@@ -1224,7 +1224,7 @@ def ran?(migration)
 
       # 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)

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
 

activerecord/lib/active_record/tasks/database_tasks.rb

@@ -164,13 +164,12 @@ def drop_current(environment = env)
       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 @@ def migrate
         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

activerecord/test/cases/migrator_test.rb

@@ -66,6 +66,26 @@ def test_migrator_with_missing_version_numbers
       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

activerecord/test/cases/tasks/database_tasks_test.rb

@@ -357,8 +357,15 @@ def test_migrate_receives_correct_env_vars
       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 @@ def test_migrate_receives_correct_env_vars
       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 @@ class DatabaseTasksCollationTest < ActiveRecord::TestCase
     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
 

railties/test/application/rake/migrations_test.rb

@@ -37,9 +37,64 @@ class AMigration < ActiveRecord::Migration::Current
         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 @@ class AMigration < ActiveRecord::Migration::Current
 
         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 @@ class AMigration < ActiveRecord::Migration::Current
         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")