diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index b3d77335c2a6d9cb9526e003ee8ac1e79a16536c..ddffbb17ace67160718551b24abdb2f27f42cf50 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/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 diff --git a/changelogs/unreleased/osw-fix-grouping-by-file-path.yml b/changelogs/unreleased/osw-fix-grouping-by-file-path.yml new file mode 100644 index 0000000000000000000000000000000000000000..dff3116e7c609b18904297820ceeee5ab79ea269 --- /dev/null +++ b/changelogs/unreleased/osw-fix-grouping-by-file-path.yml @@ -0,0 +1,5 @@ +--- +title: Avoid 500's when serializing legacy diff notes +merge_request: 23544 +author: +type: fixed diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb index 10df037a0dd3ce7f4206f95e9b7f650bcc2e742d..c5bbf522f7c6aa65f0a823a0ba22bfbb39b1ab1d 100644 --- a/lib/gitlab/diff/file_collection/base.rb +++ b/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 diff --git a/lib/gitlab/diff/file_collection/compare.rb b/lib/gitlab/diff/file_collection/compare.rb index 586c5cf87afd52e434a17e7440cec002cd13602a..663bad95db7bd5276871691d14cfd30ddf8356bb 100644 --- a/lib/gitlab/diff/file_collection/compare.rb +++ b/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 diff --git a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb index 9dc06436c72e235f2b948baf7afa584287163881..8fc5d302af665d65bdf94c79b6d82997eaa8ba50 100644 --- a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/diff/file_collection/commit_spec.rb b/spec/lib/gitlab/diff/file_collection/commit_spec.rb index 6d1b66deb6a060d5bba34d6eec143f0dbfbea912..34ed22b8941b89ca32eff299d95aeb2b39a19a52 100644 --- a/spec/lib/gitlab/diff/file_collection/commit_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb index fbcf515281eaa0295c2e045c8fd58a0e45d09023..256166dbad3496a5f85372aada7dafcb9914fbdd 100644 --- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb +++ b/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 diff --git a/spec/support/shared_examples/diff_file_collections.rb b/spec/support/shared_examples/diff_file_collections.rb index 55ce160add0b93178e57e966b6b94d378a58d12a..367ddf06c28bd5b8c297868fcf6ee95b8b3dd003 100644 --- a/spec/support/shared_examples/diff_file_collections.rb +++ b/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