Fix calculation of batch ranges for background migration created with…

… explicit ranges

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## master (unreleased)
 
+- Fix calculation of batch ranges for background migration created with explicit ranges
+
 ## 0.13.0 (2024-01-22)
 
 - Add ability to configure the path where generated background migrations will be placed

lib/online_migrations/background_migrations/migration.rb

@@ -197,10 +197,7 @@ def next_batch_range
 
         on_shard do
           # rubocop:disable Lint/UnreachableLoop
-          iterator.each_batch(of: batch_size, column: batch_column_name, start: next_min_value) do |relation, min_value, max_value|
-            if max_value.nil?
-              max_value = relation.pick(relation.arel_table[batch_column_name].maximum)
-            end
+          iterator.each_batch(of: batch_size, column: batch_column_name, start: next_min_value, finish: max_value) do |_relation, min_value, max_value|
             batch_range = [min_value, max_value]
 
             break

lib/online_migrations/batch_iterator.rb

@@ -69,7 +69,7 @@ def each_batch(of: 1000, column: relation.primary_key, start: nil, finish: nil,
         # efficient UPDATE queries, hence we get rid of it.
         batch_relation = batch_relation.except(:order)
 
-        last_id = (last_row && last_row[column]) || start_id
+        last_id = (last_row && last_row[column]) || finish
 
         # Retaining the results in the query cache would undermine the point of batching.
         batch_relation.uncached { yield batch_relation, start_id, last_id }

test/background_migrations/migration_runner_test.rb

@@ -220,6 +220,32 @@ def test_run_all_migration_jobs
       assert_no_admins
     end
 
+    def test_run_all_migration_jobs_on_empty_table
+      m = create_migration
+      run_all_migration_jobs(m)
+      assert_no_admins
+    end
+
+    def test_run_all_migration_jobs_on_empty_table_with_explicit_ranges
+      m = create_migration(min_value: 1, max_value: 100_000_000)
+      run_all_migration_jobs(m)
+      assert_no_admins
+    end
+
+    def test_run_all_migration_jobs_on_empty_relation
+      _user = User.create!(admin: false)
+      m = create_migration
+      run_all_migration_jobs(m)
+      assert_no_admins
+    end
+
+    def test_run_all_migration_jobs_on_empty_relation_with_explicit_ranges
+      _user = User.create!(admin: false)
+      m = create_migration(min_value: 1, max_value: 100_000_000)
+      run_all_migration_jobs(m)
+      assert_no_admins
+    end
+
     def test_run_all_migration_jobs_on_composite_migration
       on_each_shard { 2.times { Dog.create! } }
 

test/background_migrations/migration_test.rb

@@ -267,6 +267,16 @@ def test_next_batch_range_on_edges
       assert_equal [user3.id, user3.id], m.next_batch_range
     end
 
+    def test_next_batch_range_on_empty_table
+      m = create_migration(batch_size: 2, sub_batch_size: 1)
+      assert_equal [1, 1], m.next_batch_range
+    end
+
+    def test_next_batch_range_on_empty_table_and_explicit_ranges
+      m = create_migration(batch_size: 2, sub_batch_size: 1, min_value: 1000, max_value: 1_000_000)
+      assert_equal [1000, 1_000_000], m.next_batch_range
+    end
+
     def test_next_batch_range_on_composite_migration
       on_shard(:shard_one) { Dog.create!(id: 100) }
       on_shard(:shard_two) { Dog.create!(id: 1000) }