Reduce N+1 Gitaly queries when publishing multiple draft notes

We are currently calling
`Discussions::CaptureDiffNotePositionService` for each draft note
being published and results to N+1 Gitaly queries. This is because
we create an instance of that service in `Notes::CreateService`
for each draft note being published and that service is being
  called in that service.

In this fix, we are adding the capability to skip that call so we
can call it within the `DraftNotes::PublishService` instead and
only use a single instance of that service.

Changelog: performance

app/services/draft_notes/publish_service.rb

@@ -32,26 +32,28 @@ module DraftNotes
 
       review = Review.create!(author: current_user, merge_request: merge_request, project: project)
 
-      draft_notes.map do |draft_note|
+      created_notes = draft_notes.map do |draft_note|
         draft_note.review = review
-        create_note_from_draft(draft_note)
+        create_note_from_draft(draft_note, skip_capture_diff_note_position: true)
       end
-      draft_notes.delete_all
 
+      capture_diff_note_positions(created_notes)
+      draft_notes.delete_all
       set_reviewed
-
       notification_service.async.new_review(review)
       MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request)
     end
 
-    def create_note_from_draft(draft)
+    def create_note_from_draft(draft, skip_capture_diff_note_position: false)
       # Make sure the diff file is unfolded in order to find the correct line
       # codes.
       draft.diff_file&.unfold_diff_lines(draft.original_position)
 
-      note = Notes::CreateService.new(draft.project, draft.author, draft.publish_params).execute
-      set_discussion_resolve_status(note, draft)
+      note = Notes::CreateService.new(draft.project, draft.author, draft.publish_params).execute(
+        skip_capture_diff_note_position: skip_capture_diff_note_position
+      )
 
+      set_discussion_resolve_status(note, draft)
       note
     end
 
@@ -70,5 +72,19 @@ module DraftNotes
     def set_reviewed
       ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user).execute(merge_request)
     end
+
+    def capture_diff_note_positions(notes)
+      paths = notes.flat_map do |note|
+        note.diff_file&.paths if note.diff_note?
+      end
+
+      return if paths.empty?
+
+      capture_service = Discussions::CaptureDiffNotePositionService.new(merge_request, paths.compact)
+
+      notes.each do |note|
+        capture_service.execute(note.discussion) if note.diff_note? && note.start_of_discussion?
+      end
+    end
   end
 end

app/services/notes/create_service.rb

@@ -4,7 +4,7 @@ module Notes
   class CreateService < ::Notes::BaseService
     include IncidentManagement::UsageData
 
-    def execute
+    def execute(skip_capture_diff_note_position: false)
       note = Notes::BuildService.new(project, current_user, params.except(:merge_request_diff_head_sha)).execute
 
       # n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37440
@@ -34,7 +34,7 @@ module Notes
           end
         end
 
-        when_saved(note) if note_saved
+        when_saved(note, skip_capture_diff_note_position: skip_capture_diff_note_position) if note_saved
       end
 
       note
@@ -68,14 +68,14 @@ module Notes
       end
     end
 
-    def when_saved(note)
+    def when_saved(note, skip_capture_diff_note_position: false)
       todo_service.new_note(note, current_user)
       clear_noteable_diffs_cache(note)
       Suggestions::CreateService.new(note).execute
       increment_usage_counter(note)
       track_event(note, current_user)
 
-      if note.for_merge_request? && note.diff_note? && note.start_of_discussion?
+      if !skip_capture_diff_note_position && note.for_merge_request? && note.diff_note? && note.start_of_discussion?
         Discussions::CaptureDiffNotePositionService.new(note.noteable, note.diff_file&.paths).execute(note.discussion)
       end
     end

spec/services/draft_notes/publish_service_spec.rb

@@ -66,8 +66,8 @@ RSpec.describe DraftNotes::PublishService do
     let(:commit_id) { nil }
 
     before do
-      create(:draft_note, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position)
-      create(:draft_note, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position)
+      create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position)
+      create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position)
     end
 
     context 'when review fails to create' do
@@ -127,6 +127,30 @@ RSpec.describe DraftNotes::PublishService do
       publish
     end
 
+    context 'capturing diff notes positions' do
+      before do
+        # Need to execute this to ensure that we'll be able to test creation of
+        # DiffNotePosition records as that only happens when the `MergeRequest#merge_ref_head`
+        # is present. This service creates that for the specified merge request.
+        MergeRequests::MergeToRefService.new(project: project, current_user: user).execute(merge_request)
+      end
+
+      it 'creates diff_note_positions for diff notes' do
+        publish
+
+        notes = merge_request.notes.order(id: :asc)
+        expect(notes.first.diff_note_positions).to be_any
+        expect(notes.last.diff_note_positions).to be_any
+      end
+
+      it 'does not requests a lot from Gitaly', :request_store do
+        # NOTE: This should be reduced as we work on reducing Gitaly calls.
+        # Gitaly requests shouldn't go above this threshold as much as possible
+        # as it may add more to the Gitaly N+1 issue we are experiencing.
+        expect { publish }.to change { Gitlab::GitalyClient.get_request_count }.by(11)
+      end
+    end
+
     context 'commit_id is set' do
       let(:commit_id) { commit.id }
 

spec/services/notes/create_service_spec.rb

@@ -185,6 +185,14 @@ RSpec.describe Notes::CreateService do
               expect(note.note_diff_file).to be_present
               expect(note.diff_note_positions).to be_present
             end
+
+            context 'when skip_capture_diff_note_position execute option is set to true' do
+              it 'does not execute Discussions::CaptureDiffNotePositionService' do
+                expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new)
+
+                described_class.new(project_with_repo, user, new_opts).execute(skip_capture_diff_note_position: true)
+              end
+            end
           end
 
           context 'when DiffNote is a reply' do