Use UPSERT when storing user mentions

Using UPSERT saves us a query, avoids cases where we get
RecordNotUnique, and removes the use of SAVEPOINT which can cause PG
performance issues.

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,12 @@ module CacheMarkdownField
     # We can only store mentions if the mentionable is a database object
     return unless self.is_a?(ApplicationRecord)
 
+    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 = {}
@@ -167,24 +173,10 @@ module CacheMarkdownField
     references[:mentioned_groups_ids] = refs.mentioned_group_ids.presence
     references[:mentioned_projects_ids] = refs.mentioned_project_ids.presence
 
-    # One retry is enough as next time `model_user_mention` should return the existing mention record,
-    # that threw the `ActiveRecord::RecordNotUnique` exception in first place.
-    self.class.safe_ensure_unique(retries: 1) do
-      user_mention = model_user_mention
-
-      # 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
-      break if user_mention.blank?
-
-      user_mention.mentioned_users_ids = references[:mentioned_users_ids]
-      user_mention.mentioned_groups_ids = references[:mentioned_groups_ids]
-      user_mention.mentioned_projects_ids = references[:mentioned_projects_ids]
-
-      if user_mention.has_mentions?
-        user_mention.save!
+    if references.compact.any?
+      user_mention_class.upsert(references.merge(identifier), unique_by: identifier.compact.keys)
     else
-        user_mention.destroy!
-      end
+      user_mention_class.delete_by(identifier)
     end
 
     true

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,15 +214,8 @@ module Mentionable
     {}
   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
-  # a description attribute.
-  #
-  # Using this method followed by a call to *save* may result in *ActiveRecord::RecordNotUnique* exception
-  # in a multi-threaded environment. Make sure to use it within a *safe_ensure_unique* block.
-  def model_user_mention
-    user_mentions.where(note_id: nil).first_or_initialize
+  def user_mention_association
+    association(:user_mentions).reflection
   end
 end
 

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,16 +585,24 @@ class Note < ApplicationRecord
     cache_key_items.join(':')
   end
 
-  private
+  override :user_mention_class
+  def user_mention_class
+    return if noteable.blank?
 
-  # Using this method followed by a call to *save* may result in *ActiveRecord::RecordNotUnique* exception
-  # in a multi-threaded environment. Make sure to use it within a *safe_ensure_unique* block.
-  def model_user_mention
-    return if user_mentions.is_a?(ActiveRecord::NullRelation)
+    noteable.user_mention_class
+  end
 
-    user_mentions.first_or_initialize
+  override :user_mention_identifier
+  def user_mention_identifier
+    return if noteable.blank?
+
+    noteable.user_mention_identifier.merge({
+      note_id: id
+    })
   end
 
+  private
+
   def system_note_viewable_by?(user)
     return true unless system_note_metadata
 

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

@@ -278,7 +278,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 +302,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