Merge branch '338729-use-upsert-for-storing-mentions' into 'master'

Use UPSERT when storing user mentions

See merge request gitlab-org/gitlab!68433

app/models/alert_management/alert.rb

@@ -9,6 +9,7 @@ module AlertManagement
     include ShaAttribute
     include Sortable
     include Noteable
+    include Mentionable
     include Gitlab::SQL::Pattern
     include Presentable
     include Gitlab::Utils::StrongMemoize

app/models/commit.rb

@@ -3,6 +3,7 @@
 class Commit
   extend ActiveModel::Naming
   extend Gitlab::Cache::RequestCache
+  extend Gitlab::Utils::Override
 
   include ActiveModel::Conversion
   include Noteable
@@ -327,7 +328,7 @@ class Commit
   end
 
   def user_mentions
-    CommitUserMention.where(commit_id: self.id)
+    user_mention_class.where(commit_id: self.id)
   end
 
   def discussion_notes
@@ -554,6 +555,19 @@ class Commit
     Ability.allowed?(user, :read_commit, self)
   end
 
+  override :user_mention_class
+  def user_mention_class
+    CommitUserMention
+  end
+
+  override :user_mention_identifier
+  def user_mention_identifier
+    {
+      commit_id: id,
+      note_id: nil
+    }
+  end
+
   private
 
   def expire_note_etag_cache_for_related_mrs

app/models/concerns/cache_markdown_field.rb

@@ -160,6 +160,8 @@ module CacheMarkdownField
     # We can only store mentions if the mentionable is a database object
     return unless self.is_a?(ApplicationRecord)
 
+    return store_mentions_without_subtransaction! if Feature.enabled?(:store_mentions_without_subtransaction, default_enabled: :yaml)
+
     refs = all_references(self.author)
 
     references = {}
@@ -190,6 +192,29 @@ module CacheMarkdownField
     true
   end
 
+  def store_mentions_without_subtransaction!
+    identifier = user_mention_identifier
+
+    # this may happen due to notes polymorphism, so noteable_id may point to a record
+    # that no longer exists as we cannot have FK on noteable_id
+    return if identifier.blank?
+
+    refs = all_references(self.author)
+
+    references = {}
+    references[:mentioned_users_ids] = refs.mentioned_user_ids.presence
+    references[:mentioned_groups_ids] = refs.mentioned_group_ids.presence
+    references[:mentioned_projects_ids] = refs.mentioned_project_ids.presence
+
+    if references.compact.any?
+      user_mention_class.upsert(references.merge(identifier), unique_by: identifier.compact.keys)
+    else
+      user_mention_class.delete_by(identifier)
+    end
+
+    true
+  end
+
   def mentionable_attributes_changed?(changes = saved_changes)
     return false unless is_a?(Mentionable)
 

app/models/concerns/mentionable.rb

@@ -161,6 +161,21 @@ module Mentionable
     create_cross_references!(author)
   end
 
+  def user_mention_class
+    user_mention_association.klass
+  end
+
+  # Identifier for the user mention that is parsed from model description rather then its related notes.
+  # Models that have a description attribute like Issue, MergeRequest, Epic, Snippet may have such a user mention.
+  # Other mentionable models like DesignManagement::Design, will never have such record as those do not have
+  # a description attribute.
+  def user_mention_identifier
+    {
+      user_mention_association.foreign_key => id,
+      note_id: nil
+    }
+  end
+
   private
 
   def extracted_mentionables(refs)
@@ -199,6 +214,10 @@ module Mentionable
     {}
   end
 
+  def user_mention_association
+    association(:user_mentions).reflection
+  end
+
   # User mention that is parsed from model description rather then its related notes.
   # Models that have a description attribute like Issue, MergeRequest, Epic, Snippet may have such a user mention.
   # Other mentionable models like Commit, DesignManagement::Design, will never have such record as those do not have

app/models/note.rb

@@ -5,6 +5,8 @@
 # A note of this type is never resolvable.
 class Note < ApplicationRecord
   extend ActiveModel::Naming
+  extend Gitlab::Utils::Override
+
   include Gitlab::Utils::StrongMemoize
   include Participable
   include Mentionable
@@ -583,6 +585,22 @@ class Note < ApplicationRecord
     cache_key_items.join(':')
   end
 
+  override :user_mention_class
+  def user_mention_class
+    return if noteable.blank?
+
+    noteable.user_mention_class
+  end
+
+  override :user_mention_identifier
+  def user_mention_identifier
+    return if noteable.blank?
+
+    noteable.user_mention_identifier.merge({
+      note_id: id
+    })
+  end
+
   private
 
   # Using this method followed by a call to *save* may result in *ActiveRecord::RecordNotUnique* exception

config/feature_flags/development/store_mentions_without_subtransaction.yml

+---
+name: store_mentions_without_subtransaction
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68433
+rollout_issue_url: 
+milestone: '14.3'
+type: development
+group: group::project management
+default_enabled: false

db/migrate/20210818055357_add_unique_commit_design_user_mention_indexes.rb

+# frozen_string_literal: true
+
+class AddUniqueCommitDesignUserMentionIndexes < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  COMMIT_INDEX_NAME = 'commit_id_and_note_id_index'
+  DESIGN_INDEX_NAME = 'design_user_mentions_on_design_id_and_note_id_index'
+
+  COMMIT_UNIQUE_INDEX_NAME = 'commit_user_mentions_on_commit_id_and_note_id_unique_index'
+  DESIGN_UNIQUE_INDEX_NAME = 'design_user_mentions_on_design_id_and_note_id_unique_index'
+
+  def up
+    add_concurrent_index :commit_user_mentions, [:commit_id, :note_id], unique: true, name: COMMIT_UNIQUE_INDEX_NAME
+    add_concurrent_index :design_user_mentions, [:design_id, :note_id], unique: true, name: DESIGN_UNIQUE_INDEX_NAME
+
+    remove_concurrent_index_by_name :commit_user_mentions, COMMIT_INDEX_NAME
+    remove_concurrent_index_by_name :design_user_mentions, DESIGN_INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index :design_user_mentions, [:design_id, :note_id], name: DESIGN_INDEX_NAME
+    add_concurrent_index :commit_user_mentions, [:commit_id, :note_id], name: COMMIT_INDEX_NAME
+
+    remove_concurrent_index_by_name :design_user_mentions, DESIGN_UNIQUE_INDEX_NAME
+    remove_concurrent_index_by_name :commit_user_mentions, COMMIT_UNIQUE_INDEX_NAME
+  end
+end

db/schema_migrations/20210818055357

+1092a16d742b08ef2ef5f74bdaa92bb5f9cedbdb1161ab71abe501c39b164689
\ No newline at end of file

db/structure.sql

@@ -22895,7 +22895,7 @@ CREATE INDEX ci_builds_gitlab_monitor_metrics ON ci_builds USING btree (status,
 
 CREATE INDEX code_owner_approval_required ON protected_branches USING btree (project_id, code_owner_approval_required) WHERE (code_owner_approval_required = true);
 
-CREATE INDEX commit_id_and_note_id_index ON commit_user_mentions USING btree (commit_id, note_id);
+CREATE UNIQUE INDEX commit_user_mentions_on_commit_id_and_note_id_unique_index ON commit_user_mentions USING btree (commit_id, note_id);
 
 CREATE INDEX composer_cache_files_index_on_deleted_at ON packages_composer_cache_files USING btree (delete_at, id);
 
@@ -22905,7 +22905,7 @@ CREATE UNIQUE INDEX dast_site_profiles_builds_on_ci_build_id ON dast_site_profil
 
 CREATE UNIQUE INDEX design_management_designs_versions_uniqueness ON design_management_designs_versions USING btree (design_id, version_id);
 
-CREATE INDEX design_user_mentions_on_design_id_and_note_id_index ON design_user_mentions USING btree (design_id, note_id);
+CREATE UNIQUE INDEX design_user_mentions_on_design_id_and_note_id_unique_index ON design_user_mentions USING btree (design_id, note_id);
 
 CREATE UNIQUE INDEX epic_user_mentions_on_epic_id_and_note_id_index ON epic_user_mentions USING btree (epic_id, note_id);
 

ee/app/models/ee/vulnerability.rb

@@ -10,6 +10,7 @@ module EE
       include ::Redactable
       include ::StripAttribute
       include ::Noteable
+      include ::Mentionable
       include ::Awardable
       include ::Referable
       include ::Presentable

spec/services/issues/update_service_spec.rb

@@ -517,7 +517,7 @@ RSpec.describe Issues::UpdateService, :mailer do
           update_issue(description: "- [x] Task 1 #{user.to_reference}\n- [ ] Task 2 #{user.to_reference}")
         end
 
-        expect(recorded.count).to eq(baseline.count - 1)
+        expect(recorded.count).to eq(baseline.count)
         expect(recorded.cached_count).to eq(0)
       end
     end

spec/support/shared_examples/models/mentionable_shared_examples.rb

@@ -207,7 +207,7 @@ RSpec.shared_examples 'an editable mentionable' do
 end
 
 RSpec.shared_examples 'mentions in description' do |mentionable_type|
-  describe 'when storing user mentions' do
+  shared_examples 'when storing user mentions' do
     before do
       mentionable.store_mentions!
     end
@@ -238,10 +238,26 @@ RSpec.shared_examples 'mentions in description' do |mentionable_type|
       end
     end
   end
+
+  context 'when store_mentions_without_subtransaction is enabled' do
+    before do
+      stub_feature_flags(store_mentions_without_subtransaction: true)
+    end
+
+    it_behaves_like 'when storing user mentions'
+  end
+
+  context 'when store_mentions_without_subtransaction is disabled' do
+    before do
+      stub_feature_flags(store_mentions_without_subtransaction: false)
+    end
+
+    it_behaves_like 'when storing user mentions'
+  end
 end
 
 RSpec.shared_examples 'mentions in notes' do |mentionable_type|
-  context 'when mentionable notes contain mentions' do
+  shared_examples 'when mentionable notes contain mentions' do
     let(:user) { create(:user) }
     let(:user2) { create(:user) }
     let(:group) { create(:group) }
@@ -261,6 +277,22 @@ RSpec.shared_examples 'mentions in notes' do |mentionable_type|
       expect(mentionable.referenced_groups(user)).to eq [group]
     end
   end
+
+  context 'when store_mentions_without_subtransaction is enabled' do
+    before do
+      stub_feature_flags(store_mentions_without_subtransaction: true)
+    end
+
+    it_behaves_like 'when mentionable notes contain mentions'
+  end
+
+  context 'when store_mentions_without_subtransaction is disabled' do
+    before do
+      stub_feature_flags(store_mentions_without_subtransaction: false)
+    end
+
+    it_behaves_like 'when mentionable notes contain mentions'
+  end
 end
 
 RSpec.shared_examples 'load mentions from DB' do |mentionable_type|
@@ -278,7 +310,7 @@ RSpec.shared_examples 'load mentions from DB' do |mentionable_type|
 
     context 'when stored user mention contains ids of inexistent records' do
       before do
-        user_mention = note.send(:model_user_mention)
+        user_mention = note.user_mentions.first
         mention_ids = {
           mentioned_users_ids: user_mention.mentioned_users_ids.to_a << non_existing_record_id,
           mentioned_projects_ids: user_mention.mentioned_projects_ids.to_a << non_existing_record_id,
@@ -302,7 +334,7 @@ RSpec.shared_examples 'load mentions from DB' do |mentionable_type|
       let(:group_member) { create(:group_member, user: create(:user), group: private_group) }
 
       before do
-        user_mention = note.send(:model_user_mention)
+        user_mention = note.user_mentions.first
         mention_ids = {
           mentioned_projects_ids: user_mention.mentioned_projects_ids.to_a << private_project.id,
           mentioned_groups_ids: user_mention.mentioned_groups_ids.to_a << private_group.id