Fix diff file creation

  Add specs
  Add NoteDiffFileCreationError
  Fallback to repository if line or file are not found

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}, new_pos: %{new_pos}, old_pos: %{old_pos}")
+  DIFF_FILE_NOT_FOUND_MESSAGE = _("Failed to find diff file")
+
   after_commit :create_diff_file, on: :create
 
   def discussion_class(*)
@@ -33,12 +39,17 @@ 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)
+    raise  NoteDiffFileCreationError,  DIFF_LINE_NOT_FOUND_MESSAGE % {
+        file_path: diff_file.file_path,
+        new_pos: original_position.new_line,
+        old_pos: original_position.old_line
+    } unless diff_line
 
     creation_params = diff_file.diff.to_hash
       .except(:too_large)
-
-    creation_params.merge(diff: diff_file.diff_hunk(diff_line)) if diff_line
+      .merge(diff: diff_file.diff_hunk(diff_line))
 
     create_note_diff_file(creation_params)
   end
@@ -118,9 +129,10 @@ class DiffNote < Note
       # 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
+      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,106 @@ 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
+      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 }
+      before do
+        allow_any_instance_of(::Gitlab::Diff::Position).to receive(:line_code).with(project.repository).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 = '2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14'
+            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
 
-      it 'creates a diff note file' do
-        expect(diff_note.reload.note_diff_file).to be_present
+          context 'when not importing' do
+            context 'when diff_line is not found' do
+              it 'raises an error' do
+                allow(diff_file).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.file_path}, "\
+                                                       "new_pos: #{position.new_line}"\
+                                                       ", old_pos: #{position.old_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_any_instance_of(::Gitlab::Diff::FileCollection::MergeRequestDiff).to receive(:diff_files).and_return([])
+          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}, "\
+                                               "new_pos: #{position.new_line}"\
+                                               ", old_pos: #{position.old_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