From a8a4518099cceec5826bb9ea5d9e3198c5a10698 Mon Sep 17 00:00:00 2001
From: Heinrich Lee Yu <heinrich@gitlab.com>
Date: Thu, 28 Feb 2019 19:15:30 +0800
Subject: [PATCH] Clean up `noteable_id` for notes on commits

This was incorrectly set by a bug in:
https://gitlab.com/gitlab-org/gitlab-ce/issues/54924

Also adds a `batch_size` option to `update_column_in_batches`
---
 ...ean_up_noteable_id_for_notes_on_commits.rb | 33 ++++++++++++++++++
 lib/gitlab/database/migration_helpers.rb      | 19 ++++++-----
 ...p_noteable_id_for_notes_on_commits_spec.rb | 34 +++++++++++++++++++
 3 files changed, 78 insertions(+), 8 deletions(-)
 create mode 100644 db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb
 create mode 100644 spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb

diff --git a/db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb b/db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb
new file mode 100644
index 00000000000..fcd63f42b0e
--- /dev/null
+++ b/db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CleanUpNoteableIdForNotesOnCommits < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  TEMP_INDEX_NAME = 'index_notes_on_commit_with_null_noteable_id'
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index_by_name(:notes, TEMP_INDEX_NAME)
+
+    add_concurrent_index(:notes, :id, where: "noteable_type = 'Commit' AND noteable_id IS NOT NULL", name: TEMP_INDEX_NAME)
+
+    # rubocop:disable Migration/UpdateLargeTable
+    update_column_in_batches(:notes, :noteable_id, nil, batch_size: 300) do |table, query|
+      query.where(
+        table[:noteable_type].eq('Commit').and(table[:noteable_id].not_eq(nil))
+      )
+    end
+
+    remove_concurrent_index_by_name(:notes, TEMP_INDEX_NAME)
+  end
+
+  def down
+  end
+end
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 3abd0600e9d..7f5eb1188fc 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -282,6 +282,7 @@ module Gitlab
       # Updates the value of a column in batches.
       #
       # This method updates the table in batches of 5% of the total row count.
+      # A `batch_size` option can also be passed to set this to a fixed number.
       # This method will continue updating rows until no rows remain.
       #
       # When given a block this method will yield two values to the block:
@@ -320,7 +321,7 @@ module Gitlab
       # make things _more_ complex).
       #
       # rubocop: disable Metrics/AbcSize
-      def update_column_in_batches(table, column, value)
+      def update_column_in_batches(table, column, value, batch_size: nil)
         if transaction_open?
           raise 'update_column_in_batches can not be run inside a transaction, ' \
             'you can disable transactions by calling disable_ddl_transaction! ' \
@@ -336,14 +337,16 @@ module Gitlab
 
         return if total == 0
 
-        # Update in batches of 5% until we run out of any rows to update.
-        batch_size = ((total / 100.0) * 5.0).ceil
-        max_size = 1000
+        if batch_size.nil?
+          # Update in batches of 5% until we run out of any rows to update.
+          batch_size = ((total / 100.0) * 5.0).ceil
+          max_size = 1000
 
-        # The upper limit is 1000 to ensure we don't lock too many rows. For
-        # example, for "merge_requests" even 1% of the table is around 35 000
-        # rows for GitLab.com.
-        batch_size = max_size if batch_size > max_size
+          # The upper limit is 1000 to ensure we don't lock too many rows. For
+          # example, for "merge_requests" even 1% of the table is around 35 000
+          # rows for GitLab.com.
+          batch_size = max_size if batch_size > max_size
+        end
 
         start_arel = table.project(table[:id]).order(table[:id].asc).take(1)
         start_arel = yield table, start_arel if block_given?
diff --git a/spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb b/spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb
new file mode 100644
index 00000000000..572b7dfd0c8
--- /dev/null
+++ b/spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190313092516_clean_up_noteable_id_for_notes_on_commits.rb')
+
+describe CleanUpNoteableIdForNotesOnCommits, :migration do
+  let(:notes) { table(:notes) }
+
+  before do
+    notes.create!(noteable_type: 'Commit', commit_id: '3d0a182204cece4857f81c6462720e0ad1af39c9', noteable_id: 3, note: 'Test')
+    notes.create!(noteable_type: 'Commit', commit_id: '3d0a182204cece4857f81c6462720e0ad1af39c9', noteable_id: 3, note: 'Test')
+    notes.create!(noteable_type: 'Commit', commit_id: '3d0a182204cece4857f81c6462720e0ad1af39c9', noteable_id: 3, note: 'Test')
+
+    notes.create!(noteable_type: 'Issue', noteable_id: 1, note: 'Test')
+    notes.create!(noteable_type: 'MergeRequest', noteable_id: 1, note: 'Test')
+    notes.create!(noteable_type: 'Snippet', noteable_id: 1, note: 'Test')
+  end
+
+  it 'clears noteable_id for notes on commits' do
+    expect { migrate! }.to change { dirty_notes_on_commits.count }.from(3).to(0)
+  end
+
+  it 'does not clear noteable_id for other notes' do
+    expect { migrate! }.not_to change { other_notes.count }
+  end
+
+  def dirty_notes_on_commits
+    notes.where(noteable_type: 'Commit').where('noteable_id IS NOT NULL')
+  end
+
+  def other_notes
+    notes.where("noteable_type != 'Commit' AND noteable_id IS NOT NULL")
+  end
+end