Merge branch '201852-epic-mentions-migration-error' into 'master'

Filter out notes with noteable_id pointing to inexistent records

See merge request gitlab-org/gitlab!24362

db/post_migrate/20191115115522_migrate_epic_notes_mentions_to_db.rb

@@ -14,7 +14,7 @@ class MigrateEpicNotesMentionsToDb < ActiveRecord::Migration[5.2]
   INDEX_NAME = 'epic_mentions_temp_index'
   INDEX_CONDITION = "note LIKE '%@%'::text AND notes.noteable_type = 'Epic'"
   QUERY_CONDITIONS = "#{INDEX_CONDITION} AND epic_user_mentions.epic_id IS NULL"
-  JOIN = 'LEFT JOIN epic_user_mentions ON notes.id = epic_user_mentions.note_id'
+  JOIN = 'INNER JOIN epics ON epics.id = notes.noteable_id LEFT JOIN epic_user_mentions ON notes.id = epic_user_mentions.note_id'
 
   class Note < ActiveRecord::Base
     include EachBatch

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

@@ -9,9 +9,8 @@ describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention do
 
   let(:users) { table(:users) }
   let(:namespaces) { table(:namespaces) }
-  let(:epics) { table(:epics) }
+  let(:projects) { table(:projects) }
   let(:notes) { table(:notes) }
-  let(:epic_user_mentions) { table(:epic_user_mentions) }
 
   let(:author) { users.create!(email: 'author@example.com', notification_email: 'author@example.com', name: 'author', username: 'author', projects_limit: 10, state: 'active') }
   let(:member) { users.create!(email: 'member@example.com', notification_email: 'member@example.com', name: 'member', username: 'member', projects_limit: 10, state: 'active') }
@@ -20,14 +19,15 @@ describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention do
   let(:skipped) { users.create!(email: 'skipped@example.com', notification_email: 'skipped@example.com', name: 'skipped', username: 'skipped', projects_limit: 10, state: 'active') }
 
   let(:mentioned_users) { [author, member, admin, john_doe, skipped] }
-  let(:user_mentions) { mentioned_users.map { |u| "@#{u.username}" }.join(' ') }
+  let(:mentioned_users_refs) { mentioned_users.map { |u| "@#{u.username}" }.join(' ') }
 
   let(:group) { namespaces.create!(name: 'test1', path: 'test1', runners_token: 'my-token1', project_creation_level: 1, visibility_level: 20, type: 'Group') }
   let(:inaccessible_group) { namespaces.create!(name: 'test2', path: 'test2', runners_token: 'my-token2', project_creation_level: 1, visibility_level: 0, type: 'Group') }
+  let(:project) { projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1', namespace_id: group.id, visibility_level: 0) }
 
   let(:mentioned_groups) { [group, inaccessible_group] }
   let(:group_mentions) { [group, inaccessible_group].map { |gr| "@#{gr.path}" }.join(' ') }
-  let(:description_mentions) { "description with mentions #{user_mentions} and #{group_mentions}" }
+  let(:description_mentions) { "description with mentions #{mentioned_users_refs} and #{group_mentions}" }
 
   before do
     # build personal namespaces and routes for users
@@ -41,62 +41,36 @@ describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention do
     end
   end
 
-  context 'mentions in epic description' do
-    let(:epic) do
-      epics.create!(iid: 1, group_id: group.id, author_id: author.id, title: "epic title @#{author.username}",
-                    title_html: "epic title  @#{author.username}", description: description_mentions)
-    end
-
-    it 'has correct no_quote_columns' do
-      expect(Gitlab::BackgroundMigration::UserMentions::Models::Epic.no_quote_columns).to match([:note_id, :epic_id])
-    end
-
-    it 'migrates mentions' do
-      join = MigrateEpicMentionsToDb::JOIN
-      conditions = MigrateEpicMentionsToDb::QUERY_CONDITIONS
+  describe 'epic mentions' do
+    let(:epics) { table(:epics) }
+    let(:epic_user_mentions) { table(:epic_user_mentions) }
 
-      expect do
-        subject.perform('Epic', join, conditions, false, epic.id, epic.id)
-      end.to change { epic_user_mentions.count }.by(1)
+    context 'mentions in epic description' do
+      let!(:epic) do
+        epics.create!(iid: 1, group_id: group.id, author_id: author.id, title: "epic title @#{author.username}",
+                      title_html: "epic title  @#{author.username}", description: description_mentions)
+      end
+      let!(:epic_without_mentions) do
+        epics.create!(iid: 2, group_id: group.id, author_id: author.id, title: "epic title}",
+                      title_html: "epic title", description: 'simple description')
+      end
 
-      epic_user_mention = epic_user_mentions.last
-      expect(epic_user_mention.mentioned_users_ids.sort).to eq(mentioned_users.pluck(:id).sort)
-      expect(epic_user_mention.mentioned_groups_ids.sort).to eq([group.id])
-      expect(epic_user_mention.mentioned_groups_ids.sort).not_to include(inaccessible_group.id)
-    end
+      let(:user_mentions) { epic_user_mentions }
+      let(:resource) { epic }
 
-    context 'mentions in epic notes' do
-      let(:epic_note) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions) }
-      let(:epic_note2) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: 'sample note') }
-      let(:system_epic_note) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions, system: true) }
+      it_behaves_like 'resource mentions migration', MigrateEpicMentionsToDb, Epic
 
-      before do
-        epic_note.becomes(Note).save!
-        epic_note2.becomes(Note).save!
-        system_epic_note.becomes(Note).save!
+      it 'has correct no_quote_columns' do
+        expect(Gitlab::BackgroundMigration::UserMentions::Models::Epic.no_quote_columns).to match([:note_id, :epic_id])
       end
 
-      it 'migrates mentions from note' do
-        join = MigrateEpicNotesMentionsToDb::JOIN
-        conditions = MigrateEpicNotesMentionsToDb::QUERY_CONDITIONS
-
-        expect do
-          subject.perform('Epic', join, conditions, true, Note.minimum(:id), Note.maximum(:id))
-        end.to change { epic_user_mentions.where(note_id: epic_note.id).count }.by(1)
-
-        # check that the epic_user_mention for regular note is created
-        epic_user_mention = epic_user_mentions.first
-        expect(epic_user_mention.becomes(EpicUserMention).note.system).to be false
-        expect(epic_user_mention.mentioned_users_ids.sort).to eq(users.pluck(:id).sort)
-        expect(epic_user_mention.mentioned_groups_ids.sort).to eq([group.id])
-        expect(epic_user_mention.mentioned_groups_ids.sort).not_to include(inaccessible_group.id)
-
-        # check that the epic_user_mention for system note is created
-        epic_user_mention = epic_user_mentions.second
-        expect(epic_user_mention.becomes(EpicUserMention).note.system).to be true
-        expect(epic_user_mention.mentioned_users_ids.sort).to eq(users.pluck(:id).sort)
-        expect(epic_user_mention.mentioned_groups_ids.sort).to eq([group.id])
-        expect(epic_user_mention.mentioned_groups_ids.sort).not_to include(inaccessible_group.id)
+      context 'mentions in epic notes' do
+        let(:note1) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions) }
+        let(:note2) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: 'sample note') }
+        let(:note3) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions, system: true) }
+        let!(:note4) { notes.create!(noteable_id: epics.maximum(:id) + 10, noteable_type: 'Epic', author_id: author.id, note: description_mentions, project_id: project.id) }
+
+        it_behaves_like 'resource notes mentions migration', MigrateEpicNotesMentionsToDb, Epic
       end
     end
   end

ee/spec/migrations/migrate_epic_mentions_to_db_spec.rb

@@ -10,28 +10,13 @@ describe MigrateEpicMentionsToDb, :migration do
 
   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, type: 'Group') }
-  let!(:epic1) { epics.create!(iid: 1, title: "title1", title_html: 'title1', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
-  let!(:epic2) { epics.create!(iid: 2, title: "title2", title_html: 'title2', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
-  let!(:epic3) { epics.create!(iid: 3, title: "title3", title_html: 'title3', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
+  let!(:resource1) { epics.create!(iid: 1, title: "title1", title_html: 'title1', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
+  let!(:resource2) { epics.create!(iid: 2, title: "title2", title_html: 'title2', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
+  let!(:resource3) { epics.create!(iid: 3, title: "title3", title_html: 'title3', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
 
   before do
     stub_const("#{described_class.name}::BATCH_SIZE", 1)
   end
 
-  it 'schedules epic mentions migrations' do
-    Sidekiq::Testing.fake! do
-      Timecop.freeze do
-        migrate!
-
-        migration = described_class::MIGRATION
-        join = described_class::JOIN
-        conditions = described_class::QUERY_CONDITIONS
-
-        expect(migration).to be_scheduled_delayed_migration(2.minutes, 'Epic', join, conditions, false, epic1.id, epic1.id)
-        expect(migration).to be_scheduled_delayed_migration(4.minutes, 'Epic', join, conditions, false, epic2.id, epic2.id)
-        expect(migration).to be_scheduled_delayed_migration(6.minutes, 'Epic', join, conditions, false, epic3.id, epic3.id)
-        expect(BackgroundMigrationWorker.jobs.size).to eq 3
-      end
-    end
-  end
+  it_behaves_like 'schedules resource mentions migration', Epic, false
 end

ee/spec/migrations/migrate_epic_notes_mentions_to_db_spec.rb

@@ -12,28 +12,13 @@ describe MigrateEpicNotesMentionsToDb, :migration do
   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, type: 'Group') }
   let(:epic) { epics.create!(iid: 1, title: "title", title_html: 'title', description: 'epic description', group_id: group.id, author_id: user.id) }
-  let!(:note1) { notes.create!(note: 'note1 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
-  let!(:note2) { notes.create!(note: 'note2 for @root to check', noteable_id: epic.id, noteable_type: 'Epic', system: true) }
-  let!(:note3) { notes.create!(note: 'note3 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
+  let!(:resource1) { notes.create!(note: 'note1 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
+  let!(:resource2) { notes.create!(note: 'note2 for @root to check', noteable_id: epic.id, noteable_type: 'Epic', system: true) }
+  let!(:resource3) { notes.create!(note: 'note3 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
 
   before do
     stub_const("#{described_class.name}::BATCH_SIZE", 1)
   end
 
-  it 'schedules epic mentions migrations' do
-    Sidekiq::Testing.fake! do
-      Timecop.freeze do
-        migrate!
-
-        migration = described_class::MIGRATION
-        join = described_class::JOIN
-        conditions = described_class::QUERY_CONDITIONS
-
-        expect(migration).to be_scheduled_delayed_migration(2.minutes, 'Epic', join, conditions, true, note1.id, note1.id)
-        expect(migration).to be_scheduled_delayed_migration(4.minutes, 'Epic', join, conditions, true, note2.id, note2.id)
-        expect(migration).to be_scheduled_delayed_migration(6.minutes, 'Epic', join, conditions, true, note3.id, note3.id)
-        expect(BackgroundMigrationWorker.jobs.size).to eq 3
-      end
-    end
-  end
+  it_behaves_like 'schedules resource mentions migration', Epic, true
 end

lib/gitlab/background_migration/user_mentions/create_resource_user_mention.rb

@@ -13,7 +13,7 @@ module Gitlab
 
         def perform(resource_model, join, conditions, with_notes, start_id, end_id)
           resource_model = "#{ISOLATION_MODULE}::#{resource_model}".constantize if resource_model.is_a?(String)
-          model = with_notes ? "#{ISOLATION_MODULE}::Note".constantize : resource_model
+          model = with_notes ? Gitlab::BackgroundMigration::UserMentions::Models::Note : resource_model
           resource_user_mention_model = resource_model.user_mention_model
 
           records = model.joins(join).where(conditions).where(id: start_id..end_id)
@@ -21,7 +21,7 @@ module Gitlab
           records.in_groups_of(BULK_INSERT_SIZE, false).each do |records|
             mentions = []
             records.each do |record|
-              mentions << record.build_mention_values
+              mentions << record.build_mention_values(resource_user_mention_model.resource_foreign_key)
             end
 
             Gitlab::Database.bulk_insert(

lib/gitlab/background_migration/user_mentions/models/concerns/isolated_mentionable.rb

@@ -65,11 +65,11 @@ module Gitlab
             false
           end
 
-          def build_mention_values
+          def build_mention_values(resource_foreign_key)
             refs = all_references(author)
 
             {
-              "#{self.user_mention_model.resource_foreign_key}": user_mention_resource_id,
+              "#{resource_foreign_key}": user_mention_resource_id,
               note_id: user_mention_note_id,
               mentioned_users_ids: array_to_sql(refs.mentioned_users.pluck(:id)),
               mentioned_projects_ids: array_to_sql(refs.mentioned_projects.pluck(:id)),

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

@@ -19,10 +19,6 @@ module Gitlab
           belongs_to :noteable, polymorphic: true
           belongs_to :project
 
-          def user_mention_model
-            "#{CreateResourceUserMention::ISOLATION_MODULE}::#{noteable.class}".constantize.user_mention_model
-          end
-
           def for_personal_snippet?
             noteable.class.name == 'PersonalSnippet'
           end

spec/support/shared_examples/lib/gitlab/background_migration/mentions_migration_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'resource mentions migration' do |migration_class, resource_class|
+  it 'migrates resource mentions' do
+    join = migration_class::JOIN
+    conditions = migration_class::QUERY_CONDITIONS
+
+    expect do
+      subject.perform(resource_class.name, join, conditions, false, resource_class.minimum(:id), resource_class.maximum(:id))
+    end.to change { user_mentions.count }.by(1)
+
+    user_mention = user_mentions.last
+    expect(user_mention.mentioned_users_ids.sort).to eq(mentioned_users.pluck(:id).sort)
+    expect(user_mention.mentioned_groups_ids.sort).to eq([group.id])
+    expect(user_mention.mentioned_groups_ids.sort).not_to include(inaccessible_group.id)
+
+    # check that performing the same job twice does not fail and does not change counts
+    expect do
+      subject.perform(resource_class.name, join, conditions, false, resource_class.minimum(:id), resource_class.maximum(:id))
+    end.to change { user_mentions.count }.by(0)
+  end
+end
+
+shared_examples 'resource notes mentions migration' do |migration_class, resource_class|
+  before do
+    note1.becomes(Note).save!
+    note2.becomes(Note).save!
+    note3.becomes(Note).save!
+    # note4.becomes(Note).save(validate: false)
+  end
+
+  it 'migrates mentions from note' do
+    join = migration_class::JOIN
+    conditions = migration_class::QUERY_CONDITIONS
+
+    # there are 4 notes for each noteable_type, but one does not have mentions and
+    # another one's noteable_id points to an inexistent resource
+    expect(notes.where(noteable_type: resource_class.to_s).count).to eq 4
+
+    expect do
+      subject.perform(resource_class.name, join, conditions, true, Note.minimum(:id), Note.maximum(:id))
+    end.to change { user_mentions.count }.by(2)
+
+    # check that the user_mention for regular note is created
+    user_mention = user_mentions.first
+    expect(Note.find(user_mention.note_id).system).to be false
+    expect(user_mention.mentioned_users_ids.sort).to eq(users.pluck(:id).sort)
+    expect(user_mention.mentioned_groups_ids.sort).to eq([group.id])
+    expect(user_mention.mentioned_groups_ids.sort).not_to include(inaccessible_group.id)
+
+    # check that the user_mention for system note is created
+    user_mention = user_mentions.second
+    expect(Note.find(user_mention.note_id).system).to be true
+    expect(user_mention.mentioned_users_ids.sort).to eq(users.pluck(:id).sort)
+    expect(user_mention.mentioned_groups_ids.sort).to eq([group.id])
+    expect(user_mention.mentioned_groups_ids.sort).not_to include(inaccessible_group.id)
+
+    # check that performing the same job twice does not fail and does not change counts
+    expect do
+      subject.perform(resource_class.name, join, conditions, true, Note.minimum(:id), Note.maximum(:id))
+    end.to change { user_mentions.count }.by(0)
+  end
+end
+
+shared_examples 'schedules resource mentions migration' do |resource_class, is_for_notes|
+  it 'schedules background migrations' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        migration = described_class::MIGRATION
+        join = described_class::JOIN
+        conditions = described_class::QUERY_CONDITIONS
+
+        expect(migration).to be_scheduled_delayed_migration(2.minutes, resource_class.name, join, conditions, is_for_notes, resource1.id, resource1.id)
+        expect(migration).to be_scheduled_delayed_migration(4.minutes, resource_class.name, join, conditions, is_for_notes, resource2.id, resource2.id)
+        expect(migration).to be_scheduled_delayed_migration(6.minutes, resource_class.name, join, conditions, is_for_notes, resource3.id, resource3.id)
+        expect(BackgroundMigrationWorker.jobs.size).to eq 3
+      end
+    end
+  end
+end