Merge branch '198326-migrate-commit-notes-mentions-to-db-table' into 'master'

Migrate mentions for commit notes to DB table

See merge request gitlab-org/gitlab!23859

changelogs/unreleased/198326-migrate-commit-notes-mentions-to-db-table.yml

+---
+title: Migrate mentions for commit notes to commit_user_mentions DB table
+merge_request: 23859
+author:
+type: changed

db/post_migrate/20200128133510_cleanup_empty_commit_user_mentions.rb

+# frozen_string_literal: true
+
+class CleanupEmptyCommitUserMentions < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+  BATCH_SIZE = 10_000
+
+  class CommitUserMention < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'commit_user_mentions'
+  end
+
+  def up
+    # cleanup commit user mentions with no actual mentions,
+    # re https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24586#note_285982468
+    CommitUserMention
+      .where(mentioned_users_ids: nil)
+      .where(mentioned_groups_ids: nil)
+      .where(mentioned_projects_ids: nil)
+      .each_batch(of: BATCH_SIZE) do |batch|
+      batch.delete_all
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20200128134110_migrate_commit_notes_mentions_to_db.rb

+# frozen_string_literal: true
+
+class MigrateCommitNotesMentionsToDb < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+  DELAY = 3.minutes.to_i
+  BATCH_SIZE = 1_000
+  MIGRATION = 'UserMentions::CreateResourceUserMention'
+
+  QUERY_CONDITIONS = "note LIKE '%@%'::text AND notes.noteable_type = 'Commit' AND commit_user_mentions.commit_id IS NULL"
+  JOIN = 'LEFT JOIN commit_user_mentions ON notes.id = commit_user_mentions.note_id'
+
+  class Note < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'notes'
+  end
+
+  def up
+    Note
+      .joins(JOIN)
+      .where(QUERY_CONDITIONS)
+      .each_batch(of: BATCH_SIZE) do |batch, index|
+      range = batch.pluck(Arel.sql('MIN(notes.id)'), Arel.sql('MAX(notes.id)')).first
+      migrate_in(index * DELAY, MIGRATION, ['Commit', JOIN, QUERY_CONDITIONS, true, *range])
+    end
+  end
+
+  def down
+    # no-op
+    # temporary index is to be dropped in a different migration in an upcoming release:
+    # https://gitlab.com/gitlab-org/gitlab/issues/196842
+  end
+end

ee/spec/migrations/migrate_epic_mentions_to_db_spec.rb

@@ -20,7 +20,7 @@ describe 'epic mentions migration', migration: false do
 
   # non-migrateable resources
   # this epic is already migrated, as it has a record in the epic_user_mentions table
-  let!(:resource4) { epics.create!(iid: 4, title: "title3", title_html: 'title3', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
+  let!(:resource4) { epics.create!(iid: 4, title: "title4", title_html: 'title4', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
   let!(:user_mention) { epic_user_mentions.create!(epic_id: resource4.id, mentioned_users_ids: [1]) }
   # this epic has no mentions so should be filtered out
   let!(:resource5) { epics.create!(iid: 5, title: "title5", title_html: 'title5', description: 'epic description with no mention', group_id: group.id, author_id: user.id) }

ee/spec/migrations/migrate_epic_notes_mentions_to_db_spec.rb

@@ -24,7 +24,7 @@ describe 'epic notes mentions migration', migration: false do
   # this note is already migrated, as it has a record in the epic_user_mentions table
   let!(:resource4) { notes.create!(note: 'note3 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
   let!(:user_mention) { epic_user_mentions.create!(epic_id: epic.id, note_id: resource4.id, mentioned_users_ids: [1]) }
-  # this note points to an innexistent noteable record
+  # this note points to an inexistent noteable record
   let!(:resource5) { notes.create!(note: 'note3 for @root to check', noteable_id: epics.maximum(:id) + 10, noteable_type: 'Epic') }
 
   describe MigrateEpicNotesMentionsToDb, :migration do

lib/gitlab/background_migration/user_mentions/models/commit.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    module UserMentions
+      module Models
+        class Commit
+          include Concerns::IsolatedMentionable
+          include Concerns::MentionableMigrationMethods
+
+          def self.user_mention_model
+            Gitlab::BackgroundMigration::UserMentions::Models::CommitUserMention
+          end
+
+          def user_mention_model
+            self.class.user_mention_model
+          end
+
+          def user_mention_resource_id
+            id
+          end
+
+          def user_mention_note_id
+            'NULL'
+          end
+
+          def self.no_quote_columns
+            [:note_id]
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/background_migration/user_mentions/models/commit_user_mention.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    module UserMentions
+      module Models
+        class CommitUserMention < ActiveRecord::Base
+          self.table_name = 'commit_user_mentions'
+
+          def self.resource_foreign_key
+            :commit_id
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/background_migration/user_mentions/models/note.rb

@@ -20,7 +20,7 @@ module Gitlab
           belongs_to :project
 
           def for_personal_snippet?
-            noteable.class.name == 'PersonalSnippet'
+            noteable && noteable.class.name == 'PersonalSnippet'
           end
 
           def for_project_noteable?
@@ -32,7 +32,7 @@ module Gitlab
           end
 
           def for_epic?
-            noteable.class.name == 'Epic'
+            noteable && noteable_type == 'Epic'
           end
 
           def user_mention_resource_id
@@ -43,6 +43,14 @@ module Gitlab
             id
           end
 
+          def noteable
+            super unless for_commit?
+          end
+
+          def for_commit?
+            noteable_type == "Commit"
+          end
+
           private
 
           def mentionable_params
@@ -52,6 +60,8 @@ module Gitlab
           end
 
           def banzai_context_params
+            return {} unless noteable
+
             { group: noteable.group, label_url_method: :group_epics_url }
           end
         end

spec/lib/gitlab/background_migration/user_mentions/create_resource_user_mention_spec.rb

 # frozen_string_literal: true
 
 require 'spec_helper'
+require './db/post_migrate/20200128134110_migrate_commit_notes_mentions_to_db'
 require './db/post_migrate/20200211155539_migrate_merge_request_mentions_to_db'
 
 describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention, schema: 20200211155539 do
@@ -73,11 +74,36 @@ describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention, s
 
       it_behaves_like 'resource mentions migration', MigrateMergeRequestMentionsToDb, MergeRequest
     end
+
+    context 'migrate commit mentions' do
+      let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '', 'group/project') }
+      let(:commit) { Commit.new(RepoHelpers.sample_commit, project.becomes(Project)) }
+      let(:commit_user_mentions) { table(:commit_user_mentions) }
+
+      let!(:note1) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: author.id, note: description_mentions) }
+      let!(:note2) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: author.id, note: 'sample note') }
+      let!(:note3) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: author.id, note: description_mentions, system: true) }
+
+      # this not does not have actual mentions
+      let!(:note4) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: author.id, note: 'note for an email@somesite.com and some other random @ ref' ) }
+      # this should have pointed to an innexisted commit record in a commits table
+      # but because commit is not an AR we'll just make it so that it does not have mentions
+      let!(:note5) { notes.create!(commit_id: 'abc', noteable_type: 'Commit', project_id: project.id, author_id: author.id, note: 'note for an email@somesite.com and some other random @ ref') }
+
+      let(:user_mentions) { commit_user_mentions }
+      let(:resource) { commit }
+
+      it_behaves_like 'resource notes mentions migration', MigrateCommitNotesMentionsToDb, Commit
+    end
   end
 
   context 'checks no_quote_columns' do
     it 'has correct no_quote_columns' do
       expect(Gitlab::BackgroundMigration::UserMentions::Models::MergeRequest.no_quote_columns).to match([:note_id, :merge_request_id])
     end
+
+    it 'commit has correct no_quote_columns' do
+      expect(Gitlab::BackgroundMigration::UserMentions::Models::Commit.no_quote_columns).to match([:note_id])
+    end
   end
 end

spec/migrations/cleanup_empty_commit_user_mentions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200128133510_cleanup_empty_commit_user_mentions')
+
+describe CleanupEmptyCommitUserMentions, :migration, :sidekiq do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:notes) { table(:notes) }
+
+  let(:user) { users.create!(name: 'root', email: 'root@example.com', username: 'root', projects_limit: 0) }
+  let(:group) { namespaces.create!(name: 'group1', path: 'group1', owner_id: user.id) }
+  let(:project) { projects.create!(name: 'gitlab1', path: 'gitlab1', namespace_id: group.id, visibility_level: 0) }
+
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '', 'group/project') }
+  let(:commit) { Commit.new(RepoHelpers.sample_commit, project.becomes(Project)) }
+  let(:commit_user_mentions) { table(:commit_user_mentions) }
+
+  let!(:resource1) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: user.id, note: 'note1 for @root to check') }
+  let!(:resource2) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: user.id, note: 'note1 for @root to check') }
+  let!(:resource3) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: user.id, note: 'note1 for @root to check', system: true) }
+
+  # this note is already migrated, as it has a record in the commit_user_mentions table
+  let!(:resource4) { notes.create!(note: 'note3 for @root to check', commit_id: commit.id, noteable_type: 'Commit') }
+  let!(:user_mention) { commit_user_mentions.create!(commit_id: commit.id, note_id: resource4.id, mentioned_users_ids: [1]) }
+
+  # these should get cleanup, by the migration
+  let!(:blank_commit_user_mention1) { commit_user_mentions.create!(commit_id: commit.id, note_id: resource1.id)}
+  let!(:blank_commit_user_mention2) { commit_user_mentions.create!(commit_id: commit.id, note_id: resource2.id)}
+  let!(:blank_commit_user_mention3) { commit_user_mentions.create!(commit_id: commit.id, note_id: resource3.id)}
+
+  it 'cleanups blank user mentions' do
+    expect { migrate! }.to change { commit_user_mentions.count }.by(-3)
+  end
+end

spec/migrations/migrate_commit_notes_mentions_to_db_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200128134110_migrate_commit_notes_mentions_to_db')
+
+describe MigrateCommitNotesMentionsToDb, :migration, :sidekiq do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:notes) { table(:notes) }
+
+  let(:user) { users.create!(name: 'root', email: 'root@example.com', username: 'root', projects_limit: 0) }
+  let(:group) { namespaces.create!(name: 'group1', path: 'group1', owner_id: user.id) }
+  let(:project) { projects.create!(name: 'gitlab1', path: 'gitlab1', namespace_id: group.id, visibility_level: 0) }
+
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '', 'group/project') }
+  let(:commit) { Commit.new(RepoHelpers.sample_commit, project.becomes(Project)) }
+  let(:commit_user_mentions) { table(:commit_user_mentions) }
+
+  let!(:resource1) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: user.id, note: 'note1 for @root to check') }
+  let!(:resource2) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: user.id, note: 'note1 for @root to check') }
+  let!(:resource3) { notes.create!(commit_id: commit.id, noteable_type: 'Commit', project_id: project.id, author_id: user.id, note: 'note1 for @root to check', system: true) }
+
+  # non-migrateable resources
+  # this note is already migrated, as it has a record in the commit_user_mentions table
+  let!(:resource4) { notes.create!(note: 'note3 for @root to check', commit_id: commit.id, noteable_type: 'Commit') }
+  let!(:user_mention) { commit_user_mentions.create!(commit_id: commit.id, note_id: resource4.id, mentioned_users_ids: [1]) }
+  # this should have pointed to an inexistent commit record in a commits table
+  # but because commit is not an AR, we'll just make it so that the note does not have mentions, i.e. no `@` char.
+  let!(:resource5) { notes.create!(note: 'note3 to check', commit_id: 'abc', noteable_type: 'Commit') }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+  end
+
+  it_behaves_like 'schedules resource mentions migration', Commit, true
+end