diff --git a/changelogs/unreleased/41468-error-500-trying-to-view-a-merge-request-json-undefined-method-binary-for-nil-nilclass.yml b/changelogs/unreleased/41468-error-500-trying-to-view-a-merge-request-json-undefined-method-binary-for-nil-nilclass.yml new file mode 100644 index 0000000000000000000000000000000000000000..f69116382f009a4dc61521099ddb98be6ccf63a7 --- /dev/null +++ b/changelogs/unreleased/41468-error-500-trying-to-view-a-merge-request-json-undefined-method-binary-for-nil-nilclass.yml @@ -0,0 +1,5 @@ +--- +title: Fix viewing merge request diffs where the underlying blobs are unavailable +merge_request: +author: +type: fixed diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index cd490aaa291b8db085dd708e6161a9182eff77f4..34b070dd375de0b7dec0d759b0c88527560d5587 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -116,8 +116,10 @@ module Gitlab new_content_sha || old_content_sha end + # Use #itself to check the value wrapped by a BatchLoader instance, rather + # than if the BatchLoader instance itself is falsey. def blob - new_blob || old_blob + new_blob&.itself || old_blob&.itself end attr_writer :highlighted_diff_lines @@ -173,7 +175,7 @@ module Gitlab end def binary? - has_binary_notice? || old_blob&.binary? || new_blob&.binary? + has_binary_notice? || try_blobs(:binary?) end def text? @@ -181,15 +183,15 @@ module Gitlab end def external_storage_error? - old_blob&.external_storage_error? || new_blob&.external_storage_error? + try_blobs(:external_storage_error?) end def stored_externally? - old_blob&.stored_externally? || new_blob&.stored_externally? + try_blobs(:stored_externally?) end def external_storage - old_blob&.external_storage || new_blob&.external_storage + try_blobs(:external_storage) end def content_changed? @@ -204,15 +206,15 @@ module Gitlab end def size - [old_blob&.size, new_blob&.size].compact.sum + valid_blobs.map(&:size).sum end def raw_size - [old_blob&.raw_size, new_blob&.raw_size].compact.sum + valid_blobs.map(&:raw_size).sum end def raw_binary? - old_blob&.raw_binary? || new_blob&.raw_binary? + try_blobs(:raw_binary?) end def raw_text? @@ -235,6 +237,19 @@ module Gitlab private + # The blob instances are instances of BatchLoader, which means calling + # &. directly on them won't work. Object#try also won't work, because Blob + # doesn't inherit from Object, but from BasicObject (via SimpleDelegator). + def try_blobs(meth) + old_blob&.itself&.public_send(meth) || new_blob&.itself&.public_send(meth) + end + + # We can't use #compact for the same reason we can't use &., but calling + # #nil? explicitly does work because it is proxied to the blob itself. + def valid_blobs + [old_blob, new_blob].reject(&:nil?) + end + def text_position_properties(line) { old_line: line.old_line, new_line: line.new_line } end diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index ff9acfd08b9d5d881df9fb0187c42b5fd9508da2..9204ea3796309644e380be27101edf4ed52fadc2 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -431,4 +431,29 @@ describe Gitlab::Diff::File do end end end + + context 'when neither blob exists' do + let(:blank_diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: Gitlab::Git::BLANK_SHA, head_sha: Gitlab::Git::BLANK_SHA) } + let(:diff_file) { described_class.new(diff, diff_refs: blank_diff_refs, repository: project.repository) } + + describe '#blob' do + it 'returns a concrete nil so it can be used in boolean expressions' do + actual = diff_file.blob && true + + expect(actual).to be_nil + end + end + + describe '#binary?' do + it 'returns false' do + expect(diff_file).not_to be_binary + end + end + + describe '#size' do + it 'returns zero' do + expect(diff_file.size).to be_zero + end + end + end end