From 528b5eeb761f627bb1972fa8a25a09dc1cea4556 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 3 Jan 2018 14:18:13 +0000
Subject: [PATCH] Fix error when viewing diffs without blobs

Old merge requests can have diffs without corresponding blobs. (This also may be
possible for commit diffs in corrupt repositories.)

We can't use the `&.` operator on the blobs, because the blob objects are never
nil, but `BatchLoader` instances that delegate to `Blob`. We can't use
`Object#try`, because `Blob` doesn't inherit from `Object`.

`BatchLoader` provides a `__sync` method that returns the delegated object, but
using `itself` also works because it's forwarded, and will work for
non-`BatchLoader` instances too. So the simplest solution is to just use that
with the `&.` operator.
---
 ...defined-method-binary-for-nil-nilclass.yml |  5 +++
 lib/gitlab/diff/file.rb                       | 31 ++++++++++++++-----
 spec/lib/gitlab/diff/file_spec.rb             | 25 +++++++++++++++
 3 files changed, 53 insertions(+), 8 deletions(-)
 create mode 100644 changelogs/unreleased/41468-error-500-trying-to-view-a-merge-request-json-undefined-method-binary-for-nil-nilclass.yml

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 00000000000..f69116382f0
--- /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 cd490aaa291..34b070dd375 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 ff9acfd08b9..9204ea37963 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
-- 
GitLab