Merge branch 'fix/gb/improve-updating-column-in-batches-helper' into 'master'

Improve `update_column_in_batches` migration helper

Closes #34064

See merge request !12350

db/migrate/20160615191922_set_missing_stage_on_ci_builds.rb

@@ -2,6 +2,8 @@
 class SetMissingStageOnCiBuilds < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:ci_builds, :stage, :test) do |table, query|
       query.where(table[:stage].eq(nil))

db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb

@@ -5,6 +5,8 @@ class DropAndReaddHasExternalWikiInProjects < ActiveRecord::Migration
   # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:projects, :has_external_wiki, nil) do |table, query|
       query.where(table[:has_external_wiki].not_eq(nil))

db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb

@@ -4,6 +4,8 @@ class SetConfidentialIssuesEventsOnWebhooks < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:web_hooks, :confidential_issues_events, true) do |table, query|
       query.where(table[:issues_events].eq(true))

db/migrate/20160919144305_add_type_to_labels.rb

@@ -5,6 +5,8 @@ class AddTypeToLabels < ActiveRecord::Migration
   DOWNTIME = true
   DOWNTIME_REASON = 'Labels will not work as expected until this migration is complete.'
 
+  disable_ddl_transaction!
+
   def change
     add_column :labels, :type, :string
 

db/migrate/20161018124658_make_project_owners_masters.rb

@@ -4,6 +4,8 @@ class MakeProjectOwnersMasters < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:members, :access_level, 40) do |table, query|
       query.where(table[:access_level].eq(50).and(table[:source_type].eq('Project')))

db/migrate/20161227192806_rename_slack_and_mattermost_notification_services.rb

@@ -4,6 +4,8 @@ class RenameSlackAndMattermostNotificationServices < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:services, :type, 'SlackService') do |table, query|
       query.where(table[:type].eq('SlackNotificationService'))

db/post_migrate/20170309171644_reset_relative_position_for_issue.rb

@@ -4,6 +4,8 @@ class ResetRelativePositionForIssue < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:issues, :relative_position, nil) do |table, query|
       query.where(table[:relative_position].not_eq(nil))
@@ -11,5 +13,6 @@ class ResetRelativePositionForIssue < ActiveRecord::Migration
   end
 
   def down
+    # noop
   end
 end

db/post_migrate/20170317162059_update_upload_paths_to_system.rb

@@ -7,6 +7,8 @@ class UpdateUploadPathsToSystem < ActiveRecord::Migration
   DOWNTIME = false
   AFFECTED_MODELS = %w(User Project Note Namespace Appearance)
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], base_directory, new_upload_dir)) do |_table, query|
       query.where(uploads_to_switch_to_new_path)

db/post_migrate/20170406142253_migrate_user_project_view.rb

@@ -7,6 +7,8 @@ class MigrateUserProjectView < ActiveRecord::Migration
   # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:users, :project_view, 2) do |table, query|
       query.where(table[:project_view].eq(0))

db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb

@@ -3,6 +3,8 @@ class AddHeadPipelineForEachMergeRequest < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     disable_statement_timeout
 

lib/gitlab/database/migration_helpers.rb

@@ -222,6 +222,12 @@ module Gitlab
       #
       # rubocop: disable Metrics/AbcSize
       def update_column_in_batches(table, column, value)
+        if transaction_open?
+          raise 'update_column_in_batches can not be run inside a transaction, ' \
+            'you can disable transactions by calling disable_ddl_transaction! ' \
+            'in the body of your migration class'
+        end
+
         table = Arel::Table.new(table)
 
         count_arel = table.project(Arel.star.count.as('count'))

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -262,39 +262,53 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
   end
 
   describe '#update_column_in_batches' do
-    before do
-      create_list(:empty_project, 5)
-    end
+    context 'when running outside of a transaction' do
+      before do
+        expect(model).to receive(:transaction_open?).and_return(false)
 
-    it 'updates all the rows in a table' do
-      model.update_column_in_batches(:projects, :import_error, 'foo')
+        create_list(:empty_project, 5)
+      end
 
-      expect(Project.where(import_error: 'foo').count).to eq(5)
-    end
+      it 'updates all the rows in a table' do
+        model.update_column_in_batches(:projects, :import_error, 'foo')
 
-    it 'updates boolean values correctly' do
-      model.update_column_in_batches(:projects, :archived, true)
+        expect(Project.where(import_error: 'foo').count).to eq(5)
+      end
 
-      expect(Project.where(archived: true).count).to eq(5)
-    end
+      it 'updates boolean values correctly' do
+        model.update_column_in_batches(:projects, :archived, true)
+
+        expect(Project.where(archived: true).count).to eq(5)
+      end
+
+      context 'when a block is supplied' do
+        it 'yields an Arel table and query object to the supplied block' do
+          first_id = Project.first.id
 
-    context 'when a block is supplied' do
-      it 'yields an Arel table and query object to the supplied block' do
-        first_id = Project.first.id
+          model.update_column_in_batches(:projects, :archived, true) do |t, query|
+            query.where(t[:id].eq(first_id))
+          end
 
-        model.update_column_in_batches(:projects, :archived, true) do |t, query|
-          query.where(t[:id].eq(first_id))
+          expect(Project.where(archived: true).count).to eq(1)
         end
+      end
+
+      context 'when the value is Arel.sql (Arel::Nodes::SqlLiteral)' do
+        it 'updates the value as a SQL expression' do
+          model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1'))
 
-        expect(Project.where(archived: true).count).to eq(1)
+          expect(Project.sum(:star_count)).to eq(2 * Project.count)
+        end
       end
     end
 
-    context 'when the value is Arel.sql (Arel::Nodes::SqlLiteral)' do
-      it 'updates the value as a SQL expression' do
-        model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1'))
+    context 'when running inside the transaction' do
+      it 'raises RuntimeError' do
+        expect(model).to receive(:transaction_open?).and_return(true)
 
-        expect(Project.sum(:star_count)).to eq(2 * Project.count)
+        expect do
+          model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1'))
+        end.to raise_error(RuntimeError)
       end
     end
   end
@@ -303,7 +317,9 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
     context 'outside of a transaction' do
       context 'when a column limit is not set' do
         before do
-          expect(model).to receive(:transaction_open?).and_return(false)
+          expect(model).to receive(:transaction_open?)
+            .and_return(false)
+            .at_least(:once)
 
           expect(model).to receive(:transaction).and_yield
 
@@ -810,7 +826,11 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
       let!(:user) { create(:user, name: 'Kathy Alice Aliceson') }
 
       it 'replaces the correct part of the string' do
-        model.update_column_in_batches(:users, :name, model.replace_sql(Arel::Table.new(:users)[:name], 'Alice', 'Eve'))
+        allow(model).to receive(:transaction_open?).and_return(false)
+        query = model.replace_sql(Arel::Table.new(:users)[:name], 'Alice', 'Eve')
+
+        model.update_column_in_batches(:users, :name, query)
+
         expect(user.reload.name).to eq('Kathy Eve Aliceson')
       end
     end

spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :truncate do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
 

spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces, :truncate do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
 

spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :truncate do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
 

spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb

@@ -13,7 +13,7 @@ shared_examples 'renames child namespaces' do |type|
   end
 end
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1 do
+describe Gitlab::Database::RenameReservedPathsMigration::V1, :truncate do
   let(:subject) { FakeRenameReservedPathMigrationV1.new }
 
   before do

spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb

 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170508170547_add_head_pipeline_for_each_merge_request.rb')
 
-describe AddHeadPipelineForEachMergeRequest do
+describe AddHeadPipelineForEachMergeRequest, :truncate do
   let(:migration) { described_class.new }
 
   let!(:project) { create(:empty_project) }

spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170324160416_migrate_user_activities_to_users_last_activity_on.rb')
 
-describe MigrateUserActivitiesToUsersLastActivityOn, :redis do
+describe MigrateUserActivitiesToUsersLastActivityOn, :redis, :truncate do
   let(:migration) { described_class.new }
   let!(:user_active_1) { create(:user) }
   let!(:user_active_2) { create(:user) }

spec/migrations/migrate_user_project_view_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170406142253_migrate_user_project_view.rb')
 
-describe MigrateUserProjectView do
+describe MigrateUserProjectView, :truncate do
   let(:migration) { described_class.new }
   let!(:user) { create(:user) }