From 603e93a4a3e5eba332dd6d2fed5f46ed8c3ffea6 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Wed, 8 May 2024 14:39:39 +1000
Subject: [PATCH] Reduce N+1 queries when publishing draft notes
Use noteable instead to avoid reloading it multiple times
---
app/models/draft_note.rb | 2 +-
app/services/notes/build_service.rb | 6 +++++-
spec/services/draft_notes/publish_service_spec.rb | 13 ++++++-------
3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/app/models/draft_note.rb b/app/models/draft_note.rb
index b2848bf2cff2f..6f5c7436f06e5 100644
--- a/app/models/draft_note.rb
+++ b/app/models/draft_note.rb
@@ -5,7 +5,7 @@ class DraftNote < ApplicationRecord
include Sortable
include ShaAttribute
- PUBLISH_ATTRS = %i[noteable_id noteable_type type note internal].freeze
+ PUBLISH_ATTRS = %i[noteable type note internal].freeze
DIFF_ATTRS = %i[position original_position change_position commit_id].freeze
sha_attribute :commit_id
diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb
index 91993700e255b..96df14aa5746b 100644
--- a/app/services/notes/build_service.rb
+++ b/app/services/notes/build_service.rb
@@ -20,7 +20,11 @@ def execute
discussion = discussion.convert_to_discussion! if discussion.can_convert_to_discussion?
- params.merge!(discussion.reply_attributes)
+ reply_attributes = discussion.reply_attributes
+ # NOTE: Avoid overriding noteable if it already exists so that we don't have to reload noteable.
+ reply_attributes = reply_attributes.except(:noteable_id, :noteable_type) if params[:noteable]
+
+ params.merge!(reply_attributes)
end
# The `confidential` param for notes is deprecated with 15.3
diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb
index ba4c579d391df..3822752b90ba6 100644
--- a/spec/services/draft_notes/publish_service_spec.rb
+++ b/spec/services/draft_notes/publish_service_spec.rb
@@ -224,19 +224,17 @@ def publish(draft: nil)
end
end
- context 'with many draft notes', :use_sql_query_cache do
+ context 'with many draft notes', :use_sql_query_cache, :request_store do
let(:merge_request) { create(:merge_request) }
it 'reduce N+1 queries' do
- create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'note 1')
- create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'note 2')
- create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'note 3')
- create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'note 4')
- create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'note 5')
+ 5.times do
+ create(:draft_note_on_discussion, merge_request: merge_request, author: user, note: 'some note')
+ end
recorder = ActiveRecord::QueryRecorder.new(skip_cached: false) { publish }
- expect(recorder.count).not_to be > 186
+ expect(recorder.count).not_to be > 105
end
end
@@ -303,6 +301,7 @@ def update_file(file_path, new_content)
refresh = MergeRequests::RefreshService.new(project: project, current_user: user)
refresh.execute(oldrev, newrev, merge_request.source_branch_ref)
+ merge_request.reload
expect { publish(draft: draft) }.to change { Suggestion.count }.by(1)
.and change { DiffNote.count }.from(0).to(1)
--
GitLab