From 7ba7fa5048f26373baf3524af0612e9f353488ec Mon Sep 17 00:00:00 2001 From: AlexWayfer Date: Mon, 30 Oct 2017 12:30:31 +0000 Subject: Fix 500 error for old (somewhat) MRs --- .../unreleased/fix-500-on-old-merge-requests.yml | 5 +++ lib/gitlab/diff/position.rb | 8 +++-- spec/lib/gitlab/diff/position_spec.rb | 37 ++++++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix-500-on-old-merge-requests.yml diff --git a/changelogs/unreleased/fix-500-on-old-merge-requests.yml b/changelogs/unreleased/fix-500-on-old-merge-requests.yml new file mode 100644 index 00000000000..765d7466819 --- /dev/null +++ b/changelogs/unreleased/fix-500-on-old-merge-requests.yml @@ -0,0 +1,5 @@ +--- +title: Fix 500 errors caused by empty diffs in some discussions +merge_request: 14945 +author: Alexander Popov +type: fixed diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index bd0a9502a5e..ccfb908bcca 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -94,7 +94,9 @@ module Gitlab end def diff_file(repository) - @diff_file ||= begin + return @diff_file if defined?(@diff_file) + + @diff_file = begin if RequestStore.active? key = { project_id: repository.project.id, @@ -122,8 +124,8 @@ module Gitlab def find_diff_file(repository) return unless diff_refs.complete? - - diff_refs.compare_in(repository.project).diffs(paths: paths, expanded: true).diff_files.first + return unless comparison = diff_refs.compare_in(repository.project) + comparison.diffs(paths: paths, expanded: true).diff_files.first end def get_formatter_class(type) diff --git a/spec/lib/gitlab/diff/position_spec.rb b/spec/lib/gitlab/diff/position_spec.rb index 245f24e96d4..677eb373d22 100644 --- a/spec/lib/gitlab/diff/position_spec.rb +++ b/spec/lib/gitlab/diff/position_spec.rb @@ -364,6 +364,43 @@ describe Gitlab::Diff::Position do end end + describe "position for a missing ref" do + let(:diff_refs) do + Gitlab::Diff::DiffRefs.new( + base_sha: "not_existing_sha", + head_sha: "existing_sha" + ) + end + + subject do + described_class.new( + old_path: "files/ruby/feature.rb", + new_path: "files/ruby/feature.rb", + old_line: 3, + new_line: nil, + diff_refs: diff_refs + ) + end + + describe "#diff_file" do + it "does not raise exception" do + expect { subject.diff_file(project.repository) }.not_to raise_error + end + end + + describe "#diff_line" do + it "does not raise exception" do + expect { subject.diff_line(project.repository) }.not_to raise_error + end + end + + describe "#line_code" do + it "does not raise exception" do + expect { subject.line_code(project.repository) }.not_to raise_error + end + end + end + describe "position for a file in the initial commit" do let(:commit) { project.commit("1a0b36b3cdad1d2ee32457c102a8c0b7056fa863") } -- cgit v1.2.1