Add spec for when default is falsefix-migration-helper

When default is the value false, it should still copy the default over
into the new column.

3 files changed, 57 insertions, 38 deletions

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 7bc86bc4fac..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
-    undo_cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services, :boolean, null: false, default: false
+    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 d71e06e458d..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)
 
@@ -557,40 +557,30 @@ module Gitlab
         remove_column(table, old)
       end
 
-      # rubocop:disable Metrics/ParameterLists
-      def undo_cleanup_concurrent_column_rename(
-        table,
-        old,
-        new,
-        type,
-        limit: nil,
-        precision: nil,
-        scale: nil,
-        default: nil,
-        null: true)
-        # rubocop:enable Metrics/ParameterLists
-
+      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)
 
-        add_column(table, old, type,
-                   limit: limit,
-                   precision: precision,
-                   scale: scale)
+        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, default) unless default.nil?
+        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 null
+        change_column_null(table, old, false) unless new_column.null
 
         copy_indexes(table, new, old)
         copy_foreign_keys(table, new, old)
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 04f685aab37..cff4eb398bf 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -576,6 +576,25 @@ 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
 
@@ -610,13 +629,13 @@ describe Gitlab::Database::MigrationHelpers do
       it 'raises RuntimeError' do
         allow(model).to receive(:transaction_open?).and_return(true)
 
-        expect { model.undo_cleanup_concurrent_column_rename(:users, :old, :new, :integer) }
+        expect { model.undo_cleanup_concurrent_column_rename(:users, :old, :new) }
           .to raise_error(RuntimeError)
       end
     end
 
     context 'outside a transaction' do
-      let(:old_column) do
+      let(:new_column) do
         double(:column,
               type: :integer,
               limit: 8,
@@ -630,6 +649,7 @@ describe Gitlab::Database::MigrationHelpers do
 
       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
@@ -640,12 +660,12 @@ describe Gitlab::Database::MigrationHelpers do
 
         expect(model).to receive(:add_column)
           .with(:users, :old, :integer,
-              limit: old_column.limit,
-              precision: old_column.precision,
-              scale: old_column.scale)
+              limit: new_column.limit,
+              precision: new_column.precision,
+              scale: new_column.scale)
 
         expect(model).to receive(:change_column_default)
-          .with(:users, :old, old_column.default)
+          .with(:users, :old, new_column.default)
 
         expect(model).to receive(:update_column_in_batches)
 
@@ -654,17 +674,26 @@ describe Gitlab::Database::MigrationHelpers do
         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,
-          :integer,
-          limit: old_column.limit,
-          default: old_column.default,
-          null: old_column.null,
-          precision: old_column.precision,
-          scale: old_column.scale
-        )
+        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