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.

6 files changed, 201 insertions, 12 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