Avoid 500's when serializing legacy diff notes

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -22,12 +22,9 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
   def render_diffs
     @environment = @merge_request.environments_for(current_user).last
-    notes_grouped_by_path = renderable_notes.group_by { |note| note.position.file_path }
 
-    @diffs.diff_files.each do |diff_file|
-      notes = notes_grouped_by_path.fetch(diff_file.file_path, [])
-      notes.each { |note| diff_file.unfold_diff_lines(note.position) }
-    end
+    note_positions = renderable_notes.map(&:position).compact
+    @diffs.unfold_diff_files(note_positions)
 
     @diffs.write_cache
 

changelogs/unreleased/osw-fix-grouping-by-file-path.yml

+---
+title: Avoid 500's when serializing legacy diff notes
+merge_request: 23544
+author:
+type: fixed

lib/gitlab/diff/file_collection/base.rb

@@ -34,6 +34,16 @@ module Gitlab
           @diff_files ||= diffs.decorate! { |diff| decorate_diff!(diff) }
         end
 
+        # This mutates `diff_files` lines.
+        def unfold_diff_files(positions)
+          positions_grouped_by_path = positions.group_by { |position| position.file_path }
+
+          diff_files.each do |diff_file|
+            positions = positions_grouped_by_path.fetch(diff_file.file_path, [])
+            positions.each { |position| diff_file.unfold_diff_lines(position) }
+          end
+        end
+
         def diff_file_with_old_path(old_path)
           diff_files.find { |diff_file| diff_file.old_path == old_path }
         end

lib/gitlab/diff/file_collection/compare.rb

@@ -10,6 +10,10 @@ module Gitlab
             diff_options: diff_options,
             diff_refs:    diff_refs)
         end
+
+        def unfold_diff_lines(positions)
+          # no-op
+        end
       end
     end
   end

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -36,6 +36,18 @@ describe Projects::MergeRequests::DiffsController do
         end
       end
 
+      context 'when note has no position' do
+        before do
+          create(:legacy_diff_note_on_merge_request, project: project, noteable: merge_request, position: nil)
+        end
+
+        it 'serializes merge request diff collection' do
+          expect_any_instance_of(DiffsSerializer).to receive(:represent).with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff), an_instance_of(Hash))
+
+          go
+        end
+      end
+
       context 'with forked projects with submodules' do
         render_views
 

spec/lib/gitlab/diff/file_collection/commit_spec.rb

@@ -12,4 +12,8 @@ describe Gitlab::Diff::FileCollection::Commit do
     let(:diffable) { project.commit }
     let(:stub_path) { 'bar/branch-test.txt' }
   end
+
+  it_behaves_like 'unfoldable diff' do
+    let(:diffable) { project.commit }
+  end
 end

spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb

@@ -2,22 +2,29 @@ require 'spec_helper'
 
 describe Gitlab::Diff::FileCollection::MergeRequestDiff do
   let(:merge_request) { create(:merge_request) }
-  let(:diff_files) { described_class.new(merge_request.merge_request_diff, diff_options: nil).diff_files }
+  let(:subject) { described_class.new(merge_request.merge_request_diff, diff_options: nil) }
+  let(:diff_files) { subject.diff_files }
 
-  it 'does not highlight binary files' do
-    allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(false)
+  describe '#diff_files' do
+    it 'does not highlight binary files' do
+      allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(false)
 
-    expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
+      expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
 
-    diff_files
-  end
+      diff_files
+    end
+
+    it 'does not highlight files marked as undiffable in .gitattributes' do
+      allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(false)
 
-  it 'does not highlight files marked as undiffable in .gitattributes' do
-    allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(false)
+      expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
 
-    expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
+      diff_files
+    end
+  end
 
-    diff_files
+  it_behaves_like 'unfoldable diff' do
+    let(:diffable) { merge_request.merge_request_diff }
   end
 
   it 'it uses a different cache key if diff line keys change' do

spec/support/shared_examples/diff_file_collections.rb

@@ -45,3 +45,19 @@ shared_examples 'diff statistics' do |test_include_stats_flag: true|
     end
   end
 end
+
+shared_examples 'unfoldable diff' do
+  let(:subject) { described_class.new(diffable, diff_options: nil) }
+
+  it 'calls Gitlab::Diff::File#unfold_diff_lines with correct position' do
+    position = instance_double(Gitlab::Diff::Position, file_path: 'README')
+    readme_file = instance_double(Gitlab::Diff::File, file_path: 'README')
+    other_file = instance_double(Gitlab::Diff::File, file_path: 'foo.rb')
+    nil_path_file = instance_double(Gitlab::Diff::File, file_path: nil)
+
+    allow(subject).to receive(:diff_files) { [readme_file, other_file, nil_path_file] }
+    expect(readme_file).to receive(:unfold_diff_lines).with(position)
+
+    subject.unfold_diff_files([position])
+  end
+end