Merge branch '226924-capture-diff-note-positions-draft-notes-publish' into 'master'

Reduce N+1 Gitaly queries when publishing multiple draft notes

See merge request gitlab-org/gitlab!68045

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