Merge branch '37682-fix-diff-file-creation' into 'master'

Fix diff file creation

See merge request gitlab-org/gitlab!21034

app/models/diff_note.rb

@@ -23,6 +23,12 @@ class DiffNote < Note
 
   before_validation :set_line_code, if: :on_text?, unless: :importing?
   after_save :keep_around_commits, unless: :importing?
+
+  NoteDiffFileCreationError = Class.new(StandardError)
+
+  DIFF_LINE_NOT_FOUND_MESSAGE = "Failed to find diff line for: %{file_path}, old_line: %{old_line}, new_line: %{new_line}"
+  DIFF_FILE_NOT_FOUND_MESSAGE = "Failed to find diff file"
+
   after_commit :create_diff_file, on: :create
 
   def discussion_class(*)
@@ -33,7 +39,16 @@ class DiffNote < Note
     return unless should_create_diff_file?
 
     diff_file = fetch_diff_file
+    raise NoteDiffFileCreationError, DIFF_FILE_NOT_FOUND_MESSAGE unless diff_file
+
     diff_line = diff_file.line_for_position(self.original_position)
+    unless diff_line
+      raise NoteDiffFileCreationError, DIFF_LINE_NOT_FOUND_MESSAGE % {
+          file_path: diff_file.file_path,
+          old_line: original_position.old_line,
+          new_line: original_position.new_line
+      }
+    end
 
     creation_params = diff_file.diff.to_hash
       .except(:too_large)
@@ -110,19 +125,20 @@ class DiffNote < Note
   def fetch_diff_file
     return note_diff_file.raw_diff_file if note_diff_file
 
-    file =
-      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(original_position.diff_options).diff_files.first
-      else
-        original_position.diff_file(repository)
-      end
+    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`.
+      file = noteable.diffs(original_position.diff_options).diff_files.first
+      # if line is not found in persisted diffs, fallback and retrieve file from repository using gitaly
+      # This is required because of https://gitlab.com/gitlab-org/gitlab/issues/42676
+      file = nil if file&.line_for_position(original_position).nil? && importing?
+    end
 
+    file ||= original_position.diff_file(repository)
     file&.unfold_diff_lines(position)
 
     file

changelogs/unreleased/37682-fix-diff-file-creation.yml

+---
+title: Add fallbacks and proper errors for diff file creation
+merge_request: 21034
+author:
+type: fixed

spec/models/diff_note_spec.rb

@@ -91,18 +91,124 @@ describe DiffNote do
   end
 
   describe '#create_diff_file callback' do
-    let(:noteable) { create(:merge_request) }
-    let(:project) { noteable.project }
-
     context 'merge request' do
-      let!(:diff_note) { create(:diff_note_on_merge_request, project: project, noteable: noteable) }
+      let(:position) do
+        Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
+                                   new_path: "files/ruby/popen.rb",
+                                   old_line: nil,
+                                   new_line: 9,
+                                   diff_refs: merge_request.diff_refs)
+      end
 
-      it 'creates a diff note file' do
-        expect(diff_note.reload.note_diff_file).to be_present
+      subject { build(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
+
+      let(:diff_file_from_repository) do
+        position.diff_file(project.repository)
+      end
+
+      let(:diff_file) do
+        diffs = merge_request.diffs
+        raw_diff = diffs.diffable.raw_diffs(diffs.diff_options.merge(paths: ['files/ruby/popen.rb'])).first
+        Gitlab::Diff::File.new(raw_diff,
+                               repository: diffs.project.repository,
+                               diff_refs: diffs.diff_refs,
+                               fallback_diff_refs: diffs.fallback_diff_refs)
+      end
+
+      let(:diff_line) { diff_file.diff_lines.first }
+
+      let(:line_code) { '2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14' }
+
+      before do
+        allow(subject.position).to receive(:line_code).and_return('2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14')
+      end
+
+      context 'when diffs are already created' do
+        before do
+          allow(subject).to receive(:created_at_diff?).and_return(true)
+        end
+
+        context 'when diff_file is found in persisted diffs' do
+          before do
+            allow(merge_request).to receive_message_chain(:diffs, :diff_files, :first).and_return(diff_file)
+          end
+
+          context 'when importing' do
+            before do
+              subject.importing = true
+              subject.line_code = line_code
+            end
+
+            context 'when diff_line is found in persisted diff_file' do
+              before do
+                allow(diff_file).to receive(:line_for_position).with(position).and_return(diff_line)
+              end
+
+              it 'creates a diff note file' do
+                subject.save
+                expect(subject.note_diff_file).to be_present
+              end
+            end
+
+            context 'when diff_line is not found in persisted diff_file' do
+              before do
+                allow(diff_file).to receive(:line_for_position).and_return(nil)
+              end
+
+              it_behaves_like 'a valid diff note with after commit callback'
+            end
+          end
+
+          context 'when not importing' do
+            context 'when diff_line is not found' do
+              before do
+                allow(diff_file).to receive(:line_for_position).with(position).and_return(nil)
+              end
+
+              it 'raises an error' do
+                expect { subject.save }.to raise_error(::DiffNote::NoteDiffFileCreationError,
+                                                       "Failed to find diff line for: #{diff_file.file_path}, "\
+                                                       "old_line: #{position.old_line}"\
+                                                       ", new_line: #{position.new_line}")
+              end
+            end
+
+            context 'when diff_line is found' do
+              before do
+                allow(diff_file).to receive(:line_for_position).with(position).and_return(diff_line)
+              end
+
+              it 'creates a diff note file' do
+                subject.save
+                expect(subject.reload.note_diff_file).to be_present
+              end
+            end
+          end
+        end
+
+        context 'when diff file is not found in persisted diffs' do
+          before do
+            allow_next_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff) do |merge_request_diff|
+              allow(merge_request_diff).to receive(:diff_files).and_return([])
+            end
+          end
+
+          it_behaves_like 'a valid diff note with after commit callback'
+        end
+      end
+
+      context 'when diffs are not already created' do
+        before do
+          allow(subject).to receive(:created_at_diff?).and_return(false)
+        end
+
+        it_behaves_like 'a valid diff note with after commit callback'
       end
 
       it 'does not create diff note file if it is a reply' do
-        expect { create(:diff_note_on_merge_request, noteable: noteable, in_reply_to: diff_note) }
+        diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request)
+
+        expect { create(:diff_note_on_merge_request, noteable: merge_request, in_reply_to: diff_note) }
           .not_to change(NoteDiffFile, :count)
       end
     end

spec/support/shared_examples/models/diff_note_after_commit_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'a valid diff note with after commit callback' do
+  context 'when diff file is fetched from repository' do
+    before do
+      allow_any_instance_of(::Gitlab::Diff::Position).to receive(:diff_file).with(project.repository).and_return(diff_file_from_repository)
+    end
+
+    context 'when diff_line is not found' do
+      it 'raises an error' do
+        allow(diff_file_from_repository).to receive(:line_for_position).with(position).and_return(nil)
+
+        expect { subject.save }.to raise_error(::DiffNote::NoteDiffFileCreationError,
+                                               "Failed to find diff line for: #{diff_file_from_repository.file_path}, "\
+                                               "old_line: #{position.old_line}"\
+                                               ", new_line: #{position.new_line}")
+      end
+    end
+
+    context 'when diff_line is found' do
+      before do
+        allow(diff_file_from_repository).to receive(:line_for_position).with(position).and_return(diff_line)
+      end
+
+      it 'fallback to fetch file from repository' do
+        expect_any_instance_of(::Gitlab::Diff::Position).to receive(:diff_file).with(project.repository)
+
+        subject.save
+      end
+
+      it 'creates a diff note file' do
+        subject.save
+
+        expect(subject.reload.note_diff_file).to be_present
+      end
+    end
+  end
+
+  context 'when diff file is not found in repository' do
+    it 'raises an error' do
+      allow_any_instance_of(::Gitlab::Diff::Position).to receive(:diff_file).with(project.repository).and_return(nil)
+
+      expect { subject.save }.to raise_error(::DiffNote::NoteDiffFileCreationError, 'Failed to find diff file')
+    end
+  end
+end