Merge branch 'fix-migration-helper' into 'master'

Add helpers to exactly undo cleanup_concurrent_column_rename

See merge request gitlab-org/gitlab-ce!32183

4 files changed, 148 insertions, 3 deletions

diff --git a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb
index ac65e8d745c..cce8942128c 100644
--- a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb
+++ b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb
@@ -12,6 +12,6 @@ class RenameAllowLocalRequestsFromHooksAndServicesApplicationSetting < ActiveRec
   end
 
   def down
-    cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services
+    undo_rename_column_concurrently :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services
   end
 end
diff --git a/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb
index 127e44254ac..cb86f843f9c 100644
--- a/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb
+++ b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb
@@ -12,6 +12,6 @@ class CleanupAllowLocalRequestsFromHooksAndServicesApplicationSettingRename < Ac
   end
 
   def down
-    rename_column_concurrently :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services
+    undo_cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services
   end
 end
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 8bc45f6e78c..57a413f8e04 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -470,7 +470,7 @@ module Gitlab
         # We set the default value _after_ adding the column so we don't end up
         # updating any existing data with the default value. This isn't
         # necessary since we copy over old values further down.
-        change_column_default(table, new, old_col.default) if old_col.default
+        change_column_default(table, new, old_col.default) unless old_col.default.nil?
 
         install_rename_triggers(table, old, new)
 
@@ -482,6 +482,16 @@ module Gitlab
         copy_foreign_keys(table, old, new)
       end
 
+      def undo_rename_column_concurrently(table, old, new)
+        trigger_name = rename_trigger_name(table, old, new)
+
+        check_trigger_permissions!(table)
+
+        remove_rename_triggers_for_postgresql(table, trigger_name)
+
+        remove_column(table, new)
+      end
+
       # Installs triggers in a table that keep a new column in sync with an old
       # one.
       #
@@ -547,6 +557,35 @@ module Gitlab
         remove_column(table, old)
       end
 
+      def undo_cleanup_concurrent_column_rename(table, old, new, type: nil)
+        if transaction_open?
+          raise 'undo_cleanup_concurrent_column_rename can not be run inside a transaction'
+        end
+
+        check_trigger_permissions!(table)
+
+        new_column = column_for(table, new)
+
+        add_column(table, old, type || new_column.type,
+                   limit: new_column.limit,
+                   precision: new_column.precision,
+                   scale: new_column.scale)
+
+        # We set the default value _after_ adding the column so we don't end up
+        # updating any existing data with the default value. This isn't
+        # necessary since we copy over old values further down.
+        change_column_default(table, old, new_column.default) unless new_column.default.nil?
+
+        install_rename_triggers(table, old, new)
+
+        update_column_in_batches(table, old, Arel::Table.new(table)[new])
+
+        change_column_null(table, old, false) unless new_column.null
+
+        copy_indexes(table, new, old)
+        copy_foreign_keys(table, new, old)
+      end
+
       # Changes the column type of a table using a background migration.
       #
       # Because this method uses a background migration it's more suitable for
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index fe44b09aa24..cff4eb398bf 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -576,6 +576,38 @@ describe Gitlab::Database::MigrationHelpers do
 
         model.rename_column_concurrently(:users, :old, :new)
       end
+
+      context 'when default is false' do
+        let(:old_column) do
+          double(:column,
+               type: :boolean,
+               limit: nil,
+               default: false,
+               null: false,
+               precision: nil,
+               scale: nil)
+        end
+
+        it 'copies the default to the new column' do
+          expect(model).to receive(:change_column_default)
+            .with(:users, :new, old_column.default)
+
+          model.rename_column_concurrently(:users, :old, :new)
+        end
+      end
+    end
+  end
+
+  describe '#undo_rename_column_concurrently' do
+    it 'reverses the operations of rename_column_concurrently' do
+      expect(model).to receive(:check_trigger_permissions!).with(:users)
+
+      expect(model).to receive(:remove_rename_triggers_for_postgresql)
+        .with(:users, /trigger_.{12}/)
+
+      expect(model).to receive(:remove_column).with(:users, :new)
+
+      model.undo_rename_column_concurrently(:users, :old, :new)
     end
   end
 
@@ -592,6 +624,80 @@ describe Gitlab::Database::MigrationHelpers do
     end
   end
 
+  describe '#undo_cleanup_concurrent_column_rename' do
+    context 'in a transaction' do
+      it 'raises RuntimeError' do
+        allow(model).to receive(:transaction_open?).and_return(true)
+
+        expect { model.undo_cleanup_concurrent_column_rename(:users, :old, :new) }
+          .to raise_error(RuntimeError)
+      end
+    end
+
+    context 'outside a transaction' do
+      let(:new_column) do
+        double(:column,
+              type: :integer,
+              limit: 8,
+              default: 0,
+              null: false,
+              precision: 5,
+              scale: 1)
+      end
+
+      let(:trigger_name) { model.rename_trigger_name(:users, :old, :new) }
+
+      before do
+        allow(model).to receive(:transaction_open?).and_return(false)
+        allow(model).to receive(:column_for).and_return(new_column)
+      end
+
+      it 'reverses the operations of cleanup_concurrent_column_rename' do
+        expect(model).to receive(:check_trigger_permissions!).with(:users)
+
+        expect(model).to receive(:install_rename_triggers_for_postgresql)
+          .with(trigger_name, '"users"', '"old"', '"new"')
+
+        expect(model).to receive(:add_column)
+          .with(:users, :old, :integer,
+              limit: new_column.limit,
+              precision: new_column.precision,
+              scale: new_column.scale)
+
+        expect(model).to receive(:change_column_default)
+          .with(:users, :old, new_column.default)
+
+        expect(model).to receive(:update_column_in_batches)
+
+        expect(model).to receive(:change_column_null).with(:users, :old, false)
+
+        expect(model).to receive(:copy_indexes).with(:users, :new, :old)
+        expect(model).to receive(:copy_foreign_keys).with(:users, :new, :old)
+
+        model.undo_cleanup_concurrent_column_rename(:users, :old, :new)
+      end
+
+      context 'when default is false' do
+        let(:new_column) do
+          double(:column,
+               type: :boolean,
+               limit: nil,
+               default: false,
+               null: false,
+               precision: nil,
+               scale: nil)
+        end
+
+        it 'copies the default to the old column' do
+          expect(model).to receive(:change_column_default)
+            .with(:users, :old, new_column.default)
+
+          model.undo_cleanup_concurrent_column_rename(:users, :old, :new)
+        end
+      end
+    end
+  end
+
   describe '#change_column_type_concurrently' do
     it 'changes the column type' do
       expect(model).to receive(:rename_column_concurrently)