Merge branch...

Merge branch '223766-modify-cleanupconcurrentschemachange-with-check-for-columns-existing' into 'master'

Modify schema change helper with guard clause

See merge request gitlab-org/gitlab!35355

lib/gitlab/background_migration/cleanup_concurrent_schema_change.rb

@@ -2,7 +2,7 @@
 
 module Gitlab
   module BackgroundMigration
-    # Base class for cleaning up concurrent schema changes.
+    # Base class for background migration for rename/type changes.
     class CleanupConcurrentSchemaChange
       include Database::MigrationHelpers
 
@@ -10,7 +10,7 @@ module Gitlab
       # old_column - The name of the old (to drop) column.
       # new_column - The name of the new column.
       def perform(table, old_column, new_column)
-        return unless column_exists?(table, new_column)
+        return unless column_exists?(table, new_column) && column_exists?(table, old_column)
 
         rows_to_migrate = define_model_for(table)
           .where(new_column => nil)
@@ -28,6 +28,10 @@ module Gitlab
         end
       end
 
+      def cleanup_concurrent_schema_change(_table, _old_column, _new_column)
+        raise NotImplementedError
+      end
+
       # These methods are necessary so we can re-use the migration helpers in
       # this class.
       def connection

spec/lib/gitlab/background_migration/cleanup_concurrent_schema_change_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::CleanupConcurrentSchemaChange do
+  describe '#perform' do
+    it 'new column does not exist' do
+      expect(subject).to receive(:column_exists?).with(:issues, :closed_at_timestamp).and_return(false)
+      expect(subject).not_to receive(:column_exists?).with(:issues, :closed_at)
+      expect(subject).not_to receive(:define_model_for)
+
+      expect(subject.perform(:issues, :closed_at, :closed_at_timestamp)).to be_nil
+    end
+
+    it 'old column does not exist' do
+      expect(subject).to receive(:column_exists?).with(:issues, :closed_at_timestamp).and_return(true)
+      expect(subject).to receive(:column_exists?).with(:issues, :closed_at).and_return(false)
+      expect(subject).not_to receive(:define_model_for)
+
+      expect(subject.perform(:issues, :closed_at, :closed_at_timestamp)).to be_nil
+    end
+
+    it 'has both old and new columns' do
+      expect(subject).to receive(:column_exists?).twice.and_return(true)
+
+      expect { subject.perform('issues', :closed_at, :created_at) }.to raise_error(NotImplementedError)
+    end
+  end
+end