Commit 61bc233e authored by Nick Thomas's avatar Nick Thomas

Merge branch 'osw-fix-grouping-by-file-path' into 'master'

Avoid 500's when serializing legacy diff notes

Closes #54793

See merge request gitlab-org/gitlab-ce!23544
parents da2da0fa 89a67601
...@@ -22,12 +22,9 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -22,12 +22,9 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
def render_diffs def render_diffs
@environment = @merge_request.environments_for(current_user).last @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| note_positions = renderable_notes.map(&:position).compact
notes = notes_grouped_by_path.fetch(diff_file.file_path, []) @diffs.unfold_diff_files(note_positions)
notes.each { |note| diff_file.unfold_diff_lines(note.position) }
end
@diffs.write_cache @diffs.write_cache
......
---
title: Avoid 500's when serializing legacy diff notes
merge_request: 23544
author:
type: fixed
...@@ -34,6 +34,16 @@ module Gitlab ...@@ -34,6 +34,16 @@ module Gitlab
@diff_files ||= diffs.decorate! { |diff| decorate_diff!(diff) } @diff_files ||= diffs.decorate! { |diff| decorate_diff!(diff) }
end 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) def diff_file_with_old_path(old_path)
diff_files.find { |diff_file| diff_file.old_path == old_path } diff_files.find { |diff_file| diff_file.old_path == old_path }
end end
......
...@@ -10,6 +10,10 @@ module Gitlab ...@@ -10,6 +10,10 @@ module Gitlab
diff_options: diff_options, diff_options: diff_options,
diff_refs: diff_refs) diff_refs: diff_refs)
end end
def unfold_diff_lines(positions)
# no-op
end
end end
end end
end end
......
...@@ -36,6 +36,18 @@ describe Projects::MergeRequests::DiffsController do ...@@ -36,6 +36,18 @@ describe Projects::MergeRequests::DiffsController do
end end
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 context 'with forked projects with submodules' do
render_views render_views
......
...@@ -12,4 +12,8 @@ describe Gitlab::Diff::FileCollection::Commit do ...@@ -12,4 +12,8 @@ describe Gitlab::Diff::FileCollection::Commit do
let(:diffable) { project.commit } let(:diffable) { project.commit }
let(:stub_path) { 'bar/branch-test.txt' } let(:stub_path) { 'bar/branch-test.txt' }
end end
it_behaves_like 'unfoldable diff' do
let(:diffable) { project.commit }
end
end end
...@@ -2,22 +2,29 @@ require 'spec_helper' ...@@ -2,22 +2,29 @@ require 'spec_helper'
describe Gitlab::Diff::FileCollection::MergeRequestDiff do describe Gitlab::Diff::FileCollection::MergeRequestDiff do
let(:merge_request) { create(:merge_request) } 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 describe '#diff_files' do
allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(false) 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 diff_files
end 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 expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
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) diff_files
end
end
diff_files it_behaves_like 'unfoldable diff' do
let(:diffable) { merge_request.merge_request_diff }
end end
it 'it uses a different cache key if diff line keys change' do it 'it uses a different cache key if diff line keys change' do
......
...@@ -45,3 +45,19 @@ shared_examples 'diff statistics' do |test_include_stats_flag: true| ...@@ -45,3 +45,19 @@ shared_examples 'diff statistics' do |test_include_stats_flag: true|
end end
end 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment