Merge branch '14202-speed-up-diff-note-active' into 'master'

Speedup DiffNote#active? on discussions

See merge request !5597

CHANGELOG

@@ -40,6 +40,7 @@ v 8.11.0 (unreleased)
   - Multiple trigger variables show in separate lines (Katarzyna Kobierska Ula Budziszewska)
   - Profile requests when a header is passed
   - Avoid calculation of line_code and position for _line partial when showing diff notes on discussion tab.
+  - Speedup DiffNote#active? on discussions, preloading noteables and avoid touching git repository to return diff_refs when possible
   - Add commit stats in commit api. !5517 (dixpac)
   - Add CI configuration button on project page
   - Make error pages responsive (Takuya Noguchi)

app/controllers/projects/merge_requests_controller.rb

@@ -378,6 +378,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       fresh.
       discussions
 
+    preload_noteable_for_regular_notes(@discussions.flat_map(&:notes))
+
     # This is not executed lazily
     @notes = Banzai::NoteRenderer.render(
       @discussions.flat_map(&:notes),

app/helpers/notes_helper.rb

@@ -92,6 +92,10 @@ module NotesHelper
     project.team.max_member_access_for_user_ids(user_ids)
   end
 
+  def preload_noteable_for_regular_notes(notes)
+    ActiveRecord::Associations::Preloader.new.preload(notes.select { |note| !note.for_commit? }, :noteable)
+  end
+
   def note_max_access_for_user(note)
     note.project.team.human_max_access(note.author_id)
   end

app/models/diff_note.rb

@@ -67,7 +67,7 @@ class DiffNote < Note
     return false unless supported?
     return true if for_commit?
 
-    diff_refs ||= self.noteable.diff_refs
+    diff_refs ||= noteable_diff_refs
 
     self.position.diff_refs == diff_refs
   end
@@ -78,6 +78,14 @@ class DiffNote < Note
     !self.for_merge_request? || self.noteable.support_new_diff_notes?
   end
 
+  def noteable_diff_refs
+    if noteable.respond_to?(:diff_sha_refs)
+      noteable.diff_sha_refs
+    else
+      noteable.diff_refs
+    end
+  end
+
   def set_original_position
     self.original_position = self.position.dup
   end
@@ -96,7 +104,7 @@ class DiffNote < Note
       self.project,
       nil,
       old_diff_refs: self.position.diff_refs,
-      new_diff_refs: self.noteable.diff_refs,
+      new_diff_refs: noteable_diff_refs,
       paths: self.position.paths
     ).execute(self)
   end

app/models/discussion.rb

@@ -49,6 +49,12 @@ class Discussion
     self.noteable == target && !diff_discussion?
   end
 
+  def active?
+    return @active if defined?(@active)
+
+    @active = first_note.active?
+  end
+
   def expanded?
     !diff_discussion? || active?
   end

app/models/merge_request.rb

@@ -255,6 +255,19 @@ class MergeRequest < ActiveRecord::Base
     )
   end
 
+  # Return diff_refs instance trying to not touch the git repository
+  def diff_sha_refs
+    if merge_request_diff && merge_request_diff.diff_refs_by_sha?
+      return Gitlab::Diff::DiffRefs.new(
+        base_sha:  merge_request_diff.base_commit_sha,
+        start_sha: merge_request_diff.start_commit_sha,
+        head_sha:  merge_request_diff.head_commit_sha
+      )
+    else
+      diff_refs
+    end
+  end
+
   def validate_branches
     if target_project == source_project && target_branch == source_branch
       errors.add :branch_conflict, "You can not use same project/branch for source and target"
@@ -659,7 +672,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def support_new_diff_notes?
-    diff_refs && diff_refs.complete?
+    diff_sha_refs && diff_sha_refs.complete?
   end
 
   def update_diff_notes_positions(old_diff_refs:, new_diff_refs:)

app/models/merge_request_diff.rb

@@ -82,6 +82,10 @@ class MergeRequestDiff < ActiveRecord::Base
     project.commit(self.head_commit_sha)
   end
 
+  def diff_refs_by_sha?
+    base_commit_sha? && head_commit_sha? && start_commit_sha?
+  end
+
   def compare
     @compare ||=
       begin

spec/models/diff_note_spec.rb

@@ -119,7 +119,7 @@ describe DiffNote, models: true do
 
       context "when the merge request's diff refs don't match that of the diff note" do
         before do
-          allow(subject.noteable).to receive(:diff_refs).and_return(commit.diff_refs)
+          allow(subject.noteable).to receive(:diff_sha_refs).and_return(commit.diff_refs)
         end
 
         it "returns false" do
@@ -168,7 +168,7 @@ describe DiffNote, models: true do
 
         context "when the note is outdated" do
           before do
-            allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
+            allow(merge_request).to receive(:diff_sha_refs).and_return(commit.diff_refs)
           end
 
           it "uses the DiffPositionUpdateService" do

spec/models/merge_request_spec.rb

@@ -686,4 +686,28 @@ describe MergeRequest, models: true do
       subject.reload_diff
     end
   end
+
+  describe "#diff_sha_refs" do
+    context "with diffs" do
+      subject { create(:merge_request, :with_diffs) }
+
+      it "does not touch the repository" do
+        subject # Instantiate the object
+
+        expect_any_instance_of(Repository).not_to receive(:commit)
+
+        subject.diff_sha_refs
+      end
+
+      it "returns expected diff_refs" do
+        expected_diff_refs = Gitlab::Diff::DiffRefs.new(
+          base_sha:  subject.merge_request_diff.base_commit_sha,
+          start_sha: subject.merge_request_diff.start_commit_sha,
+          head_sha:  subject.merge_request_diff.head_commit_sha
+        )
+
+        expect(subject.diff_sha_refs).to eq(expected_diff_refs)
+      end
+    end
+  end
 end