Store only bare class names for batched migrations

When storing `job_class_name` and `batch_class_name` values in the
database, save only the bare class name, rather than the fully qualified
name. The enclosing module will be dynamically attached to the class
name in the application, when the class is requested.

changelogs/unreleased/pb-store-bare-class-name-for-batched-migrations.yml

+---
+title: Change the default batch_class_name for batched_background_migrations to the
+  unqualified class name
+merge_request: 56036
+author:
+type: changed

db/migrate/20210308190413_change_batched_background_migrations_batch_class_name_default.rb

+# frozen_string_literal: true
+
+class ChangeBatchedBackgroundMigrationsBatchClassNameDefault < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    change_column_default :batched_background_migrations, :batch_class_name,
+      from: 'Gitlab::Database::BackgroundMigration::PrimaryKeyBatchingStrategy', to: 'PrimaryKeyBatchingStrategy'
+  end
+end

db/schema_migrations/20210308190413

+cc131cf37f2af8f0f58c7fa6e5055e88a3b2ed413862c155b0d18383aba06058
\ No newline at end of file

db/structure.sql

@@ -9876,7 +9876,7 @@ CREATE TABLE batched_background_migrations (
     "interval" smallint NOT NULL,
     status smallint DEFAULT 0 NOT NULL,
     job_class_name text NOT NULL,
-    batch_class_name text DEFAULT 'Gitlab::Database::BackgroundMigration::PrimaryKeyBatchingStrategy'::text NOT NULL,
+    batch_class_name text DEFAULT 'PrimaryKeyBatchingStrategy'::text NOT NULL,
     table_name text NOT NULL,
     column_name text NOT NULL,
     job_arguments jsonb DEFAULT '"[]"'::jsonb NOT NULL,

lib/gitlab/database/background_migration/primary_key_batching_strategy.rb → lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy.rb

 # frozen_string_literal: true
 
 module Gitlab
-  module Database
-    module BackgroundMigration
+  module BackgroundMigration
+    module BatchingStrategies
+      # Generic batching class for use with a BatchedBackgroundMigration.
+      # Batches over the given table and column combination, returning the MIN() and MAX()
+      # values for the next batch as an array.
+      #
+      # If no more batches exist in the table, returns nil.
       class PrimaryKeyBatchingStrategy
         include Gitlab::Database::DynamicModelHelpers
 
+        # Finds and returns the next batch in the table.
+        #
+        # table_name - The table to batch over
+        # column_name - The column to batch over
+        # batch_min_value - The minimum value which the next batch will start at
+        # batch_size - The size of the next batch
         def next_batch(table_name, column_name, batch_min_value:, batch_size:)
           model_class = define_batchable_model(table_name)
 

lib/gitlab/database/background_migration/batched_migration.rb

@@ -4,6 +4,9 @@ module Gitlab
   module Database
     module BackgroundMigration
       class BatchedMigration < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
+        JOB_CLASS_MODULE = 'Gitlab::BackgroundMigration'
+        BATCH_CLASS_MODULE = "#{JOB_CLASS_MODULE}::BatchingStrategies".freeze
+
         self.table_name = :batched_background_migrations
 
         has_many :batched_jobs, foreign_key: :batched_background_migration_id
@@ -20,10 +23,6 @@ module Gitlab
           finished: 3
         }
 
-        def self.remove_toplevel_prefix(name)
-          name&.sub(/\A::/, '')
-        end
-
         def interval_elapsed?
           last_job.nil? || last_job.created_at <= Time.current - interval
         end
@@ -37,19 +36,19 @@ module Gitlab
         end
 
         def job_class
-          job_class_name.constantize
+          "#{JOB_CLASS_MODULE}::#{job_class_name}".constantize
         end
 
         def batch_class
-          batch_class_name.constantize
+          "#{BATCH_CLASS_MODULE}::#{batch_class_name}".constantize
         end
 
         def job_class_name=(class_name)
-          write_attribute(:job_class_name, self.class.remove_toplevel_prefix(class_name))
+          write_attribute(:job_class_name, class_name.demodulize)
         end
 
         def batch_class_name=(class_name)
-          write_attribute(:batch_class_name, self.class.remove_toplevel_prefix(class_name))
+          write_attribute(:batch_class_name, class_name.demodulize)
         end
       end
     end

spec/factories/gitlab/database/background_migration/batched_migrations.rb

@@ -6,7 +6,7 @@ FactoryBot.define do
     batch_size { 5 }
     sub_batch_size { 1 }
     interval { 2.minutes }
-    job_class_name { 'Gitlab::BackgroundMigration::CopyColumnUsingBackgroundMigrationJob' }
+    job_class_name { 'CopyColumnUsingBackgroundMigrationJob' }
     table_name { :events }
     column_name { :id }
   end

spec/lib/gitlab/database/background_migration/primary_key_batching_strategy_spec.rb → spec/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy_spec.rb

@@ -2,41 +2,42 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::Database::BackgroundMigration::PrimaryKeyBatchingStrategy, '#next_batch' do
+RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchingStrategy, '#next_batch' do
   let(:batching_strategy) { described_class.new }
+  let(:namespaces) { table(:namespaces) }
 
-  let_it_be(:event1) { create(:event) }
-  let_it_be(:event2) { create(:event) }
-  let_it_be(:event3) { create(:event) }
-  let_it_be(:event4) { create(:event) }
+  let!(:namespace1) { namespaces.create!(name: 'batchtest1', path: 'batch-test1') }
+  let!(:namespace2) { namespaces.create!(name: 'batchtest2', path: 'batch-test2') }
+  let!(:namespace3) { namespaces.create!(name: 'batchtest3', path: 'batch-test3') }
+  let!(:namespace4) { namespaces.create!(name: 'batchtest4', path: 'batch-test4') }
 
   context 'when starting on the first batch' do
     it 'returns the bounds of the next batch' do
-      batch_bounds = batching_strategy.next_batch(:events, :id, batch_min_value: event1.id, batch_size: 3)
+      batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace1.id, batch_size: 3)
 
-      expect(batch_bounds).to eq([event1.id, event3.id])
+      expect(batch_bounds).to eq([namespace1.id, namespace3.id])
     end
   end
 
   context 'when additional batches remain' do
     it 'returns the bounds of the next batch' do
-      batch_bounds = batching_strategy.next_batch(:events, :id, batch_min_value: event2.id, batch_size: 3)
+      batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace2.id, batch_size: 3)
 
-      expect(batch_bounds).to eq([event2.id, event4.id])
+      expect(batch_bounds).to eq([namespace2.id, namespace4.id])
     end
   end
 
   context 'when on the final batch' do
     it 'returns the bounds of the next batch' do
-      batch_bounds = batching_strategy.next_batch(:events, :id, batch_min_value: event4.id, batch_size: 3)
+      batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id, batch_size: 3)
 
-      expect(batch_bounds).to eq([event4.id, event4.id])
+      expect(batch_bounds).to eq([namespace4.id, namespace4.id])
     end
   end
 
   context 'when no additional batches remain' do
     it 'returns nil' do
-      batch_bounds = batching_strategy.next_batch(:events, :id, batch_min_value: event4.id + 1, batch_size: 1)
+      batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id + 1, batch_size: 1)
 
       expect(batch_bounds).to be_nil
     end

spec/lib/gitlab/database/background_migration/batched_migration_spec.rb

@@ -122,7 +122,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m
   end
 
   describe '#batch_class' do
-    let(:batch_class) { Gitlab::Database::BackgroundMigration::PrimaryKeyBatchingStrategy}
+    let(:batch_class) { Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchingStrategy}
     let(:batched_migration) { build(:batched_background_migration) }
 
     it 'returns the class of the batch strategy for the migration' do
@@ -130,31 +130,31 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m
     end
   end
 
-  shared_examples_for 'an attr_writer that normalizes assigned class names' do |attribute_name|
+  shared_examples_for 'an attr_writer that demodulizes assigned class names' do |attribute_name|
     let(:batched_migration) { build(:batched_background_migration) }
 
-    context 'when the toplevel namespace prefix exists' do
-      it 'removes the leading prefix' do
+    context 'when a module name exists' do
+      it 'removes the module name' do
         batched_migration.public_send(:"#{attribute_name}=", '::Foo::Bar')
 
-        expect(batched_migration[attribute_name]).to eq('Foo::Bar')
+        expect(batched_migration[attribute_name]).to eq('Bar')
       end
     end
 
-    context 'when the toplevel namespace prefix does not exist' do
+    context 'when a module name does not exist' do
       it 'does not change the given class name' do
-        batched_migration.public_send(:"#{attribute_name}=", '::Foo::Bar')
+        batched_migration.public_send(:"#{attribute_name}=", 'Bar')
 
-        expect(batched_migration[attribute_name]).to eq('Foo::Bar')
+        expect(batched_migration[attribute_name]).to eq('Bar')
       end
     end
   end
 
   describe '#job_class_name=' do
-    it_behaves_like 'an attr_writer that normalizes assigned class names', :job_class_name
+    it_behaves_like 'an attr_writer that demodulizes assigned class names', :job_class_name
   end
 
   describe '#batch_class_name=' do
-    it_behaves_like 'an attr_writer that normalizes assigned class names', :batch_class_name
+    it_behaves_like 'an attr_writer that demodulizes assigned class names', :batch_class_name
   end
 end