Merge branch 'osw-use-cached-highlighted-content-for-discussions' into 'master'

Use persisted diff data instead fetching Git on discussions

Closes #44052

See merge request gitlab-org/gitlab-ce!18657

app/models/diff_note.rb

@@ -54,7 +54,20 @@ class DiffNote < Note
   end
 
   def diff_file
-    @diff_file ||= self.original_position.diff_file(self.project.repository)
+    @diff_file ||=
+      begin
+        if created_at_diff?(noteable.diff_refs)
+          # We're able to use the already persisted diffs (Postgres) if we're
+          # presenting a "current version" of the MR discussion diff.
+          # So no need to make an extra Gitaly diff request for it.
+          # As an extra benefit, the returned `diff_file` already
+          # has `highlighted_diff_lines` data set from Redis on
+          # `Diff::FileCollection::MergeRequestDiff`.
+          noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first
+        else
+          original_position.diff_file(self.project.repository)
+        end
+      end
   end
 
   def diff_line

changelogs/unreleased/osw-use-cached-highlighted-content-for-discussions.yml

+---
+title: Use persisted diff data instead fetching Git on discussions
+merge_request:
+author:
+type: performance

lib/gitlab/diff/file_collection/base.rb

@@ -36,6 +36,8 @@ module Gitlab
         private
 
         def decorate_diff!(diff)
+          return diff if diff.is_a?(File)
+
           Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs)
         end
       end

spec/models/diff_note_spec.rb

@@ -85,12 +85,35 @@ describe DiffNote do
   end
 
   describe "#diff_file" do
-    it "returns the correct diff file" do
-      diff_file = subject.diff_file
+    context 'when the discussion was created in the diff' do
+      it 'returns correct diff file' do
+        diff_file = subject.diff_file
 
-      expect(diff_file.old_path).to eq(position.old_path)
-      expect(diff_file.new_path).to eq(position.new_path)
-      expect(diff_file.diff_refs).to eq(position.diff_refs)
+        expect(diff_file.old_path).to eq(position.old_path)
+        expect(diff_file.new_path).to eq(position.new_path)
+        expect(diff_file.diff_refs).to eq(position.diff_refs)
+      end
+    end
+
+    context 'when discussion is outdated or not created in the diff' do
+      let(:diff_refs) { project.commit(sample_commit.id).diff_refs }
+      let(:position) do
+        Gitlab::Diff::Position.new(
+          old_path: "files/ruby/popen.rb",
+          new_path: "files/ruby/popen.rb",
+          old_line: nil,
+          new_line: 14,
+          diff_refs: diff_refs
+        )
+      end
+
+      it 'returns the correct diff file' do
+        diff_file = subject.diff_file
+
+        expect(diff_file.old_path).to eq(position.old_path)
+        expect(diff_file.new_path).to eq(position.new_path)
+        expect(diff_file.diff_refs).to eq(position.diff_refs)
+      end
     end
   end