Fixes #28359

Add stronger assertions to rake migration tasks to make sure the user is providing a numeric VERSION
An empty string was getting converted to version = 0. This would in turn pass the presence check.

Address linting warning

Add test for rake task and refactor code to meet expectations
In particular passing VERSION=0 should not raise an error.

Addressed Comments for PR #28485. Trimmed empty lines + change of wording for error message

Adjust test for change of wording in error message

Change condition to follow rails idioms

3 files changed, 16 insertions, 3 deletions

diff --git a/activerecord/lib/active_record/railties/databases.rake b/activerecord/lib/active_record/railties/databases.rake
index 1c7206aca4..f3e2df8786 100644
--- a/activerecord/lib/active_record/railties/databases.rake
+++ b/activerecord/lib/active_record/railties/databases.rake
@@ -77,6 +77,8 @@ db_namespace = namespace :db do
   namespace :migrate do
     # desc  'Rollbacks the database one migration and re migrate up (options: STEP=x, VERSION=x).'
     task redo: [:environment, :load_config] do
+      raise "Empty VERSION provided" if ENV["VERSION"] && ENV["VERSION"].empty?
+
       if ENV["VERSION"]
         db_namespace["migrate:down"].invoke
         db_namespace["migrate:up"].invoke
@@ -91,16 +93,17 @@ db_namespace = namespace :db do
 
     # desc 'Runs the "up" for a given migration VERSION.'
     task up: [:environment, :load_config] do
+      raise "VERSION is required" if ENV["VERSION"] && ENV["VERSION"].empty?
+
       version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil
-      raise "VERSION is required" unless version
       ActiveRecord::Migrator.run(:up, ActiveRecord::Tasks::DatabaseTasks.migrations_paths, 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
-      raise "VERSION is required - To go down one migration, run db:rollback" unless version
       ActiveRecord::Migrator.run(:down, ActiveRecord::Tasks::DatabaseTasks.migrations_paths, 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 82604a915f..46fa8a70a3 100644
--- a/activerecord/lib/active_record/tasks/database_tasks.rb
+++ b/activerecord/lib/active_record/tasks/database_tasks.rb
@@ -162,9 +162,11 @@ module ActiveRecord
       end
 
       def migrate
+        raise "Empty VERSION provided" if ENV["VERSION"] && ENV["VERSION"].empty?
+
         verbose = ENV["VERBOSE"] ? ENV["VERBOSE"] == "true" : true
         version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil
-        scope   = ENV["SCOPE"]
+        scope = ENV["SCOPE"]
         verbose_was, Migration.verbose = Migration.verbose, verbose
         Migrator.migrate(migrations_paths, version) do |migration|
           scope.blank? || scope == migration.scope
diff --git a/activerecord/test/cases/tasks/database_tasks_test.rb b/activerecord/test/cases/tasks/database_tasks_test.rb
index 5653fd83fd..baa41a3a47 100644
--- a/activerecord/test/cases/tasks/database_tasks_test.rb
+++ b/activerecord/test/cases/tasks/database_tasks_test.rb
@@ -350,6 +350,14 @@ module ActiveRecord
       ENV["VERBOSE"], ENV["VERSION"] = verbose, version
     end
 
+    def test_migrate_raise_error_on_empty_version
+      version = ENV["VERSION"]
+      ENV["VERSION"] = ""
+      assert_raise(RuntimeError, "Empty VERSION provided") { ActiveRecord::Tasks::DatabaseTasks.migrate }
+    ensure
+      ENV["VERSION"] = version
+    end
+
     def test_migrate_clears_schema_cache_afterward
       ActiveRecord::Base.expects(:clear_cache!)
       ActiveRecord::Tasks::DatabaseTasks.migrate