Merge branch 'pb-store-bare-class-name-for-batched-migrations' into 'master'

Store bare class name for batched migrations

See merge request gitlab-org/gitlab!56036

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 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