Merge branch...

Merge branch '41468-error-500-trying-to-view-a-merge-request-json-undefined-method-binary-for-nil-nilclass' into 'master'

Resolve "Error 500 trying to view a merge request JSON: undefined method `binary?' for nil:NilClass"

Closes #41468

See merge request gitlab-org/gitlab-ce!16193

changelogs/unreleased/41468-error-500-trying-to-view-a-merge-request-json-undefined-method-binary-for-nil-nilclass.yml

+---
+title: Fix viewing merge request diffs where the underlying blobs are unavailable
+merge_request:
+author:
+type: fixed

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

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