From 720cc14a754f1e528006c28fec4110f47297fd60 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Tue, 2 May 2017 17:45:50 -0500
Subject: [PATCH] Refactor Blob support of external storage in preparation of
 job artifact blobs

---
 app/controllers/projects/raw_controller.rb    |   2 +-
 app/helpers/blob_helper.rb                    |  24 +++-
 app/models/blob.rb                            |  46 ++++---
 app/models/blob_viewer/base.rb                |  11 +-
 app/models/concerns/blob_like.rb              |  48 +++++++
 app/models/snippet_blob.rb                    |  30 +----
 lib/gitlab/git/blob.rb                        |  12 +-
 .../features/projects/blobs/blob_show_spec.rb |  43 +++++-
 spec/helpers/blob_helper_spec.rb              |  50 ++++---
 spec/models/blob_spec.rb                      | 124 ++++++++++++++++--
 spec/models/blob_viewer/base_spec.rb          |   6 +-
 spec/support/helpers/fake_blob_helpers.rb     |  18 +--
 12 files changed, 309 insertions(+), 105 deletions(-)
 create mode 100644 app/models/concerns/blob_like.rb

diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb
index a0b08ad130f..a02cc477e08 100644
--- a/app/controllers/projects/raw_controller.rb
+++ b/app/controllers/projects/raw_controller.rb
@@ -15,7 +15,7 @@ class Projects::RawController < Projects::ApplicationController
 
       return if cached_blob?
 
-      if @blob.valid_lfs_pointer?
+      if @blob.stored_externally?
         send_lfs_object
       else
         send_git_blob @repository, @blob
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index 377b080b3c6..37b6f4ad5cc 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -52,7 +52,7 @@ module BlobHelper
 
     if !on_top_of_branch?(project, ref)
       button_tag label, class: "#{common_classes} disabled has-tooltip", title: "You can only #{action} files when you are on a branch", data: { container: 'body' }
-    elsif blob.valid_lfs_pointer?
+    elsif blob.stored_externally?
       button_tag label, class: "#{common_classes} disabled has-tooltip", title: "It is not possible to #{action} files that are stored in LFS using the web interface", data: { container: 'body' }
     elsif can_modify_blob?(blob, project, ref)
       button_tag label, class: "#{common_classes}", 'data-target' => "#modal-#{modal_type}-blob", 'data-toggle' => 'modal'
@@ -95,7 +95,7 @@ module BlobHelper
   end
 
   def can_modify_blob?(blob, project = @project, ref = @ref)
-    !blob.valid_lfs_pointer? && can_edit_tree?(project, ref)
+    !blob.stored_externally? && can_edit_tree?(project, ref)
   end
 
   def leave_edit_message
@@ -223,7 +223,9 @@ module BlobHelper
   end
 
   def open_raw_blob_button(blob)
-    if blob.raw_binary?
+    return if blob.empty?
+    
+    if blob.raw_binary? || blob.stored_externally?
       icon = icon('download')
       title = 'Download'
     else
@@ -244,19 +246,27 @@ module BlobHelper
           viewer.max_size
         end
       "it is larger than #{number_to_human_size(max_size)}"
-    when :server_side_but_stored_in_lfs
-      "it is stored in LFS"
+    when :server_side_but_stored_externally
+      case viewer.blob.external_storage
+      when :lfs
+        'it is stored in LFS'
+      else
+        'it is stored externally'
+      end
     end
   end
 
   def blob_render_error_options(viewer)
+    error = viewer.render_error
     options = []
 
-    if viewer.render_error == :too_large && viewer.can_override_max_size?
+    if error == :too_large && viewer.can_override_max_size?
       options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil)))
     end
 
-    if viewer.rich? && viewer.blob.rendered_as_text?
+    # If the error is `:server_side_but_stored_externally`, the simple viewer will show the same error,
+    # so don't bother switching.
+    if viewer.rich? && viewer.blob.rendered_as_text? && error != :server_side_but_stored_externally
       options << link_to('view the source', '#', class: 'js-blob-viewer-switch-btn', data: { viewer: 'simple' })
     end
 
diff --git a/app/models/blob.rb b/app/models/blob.rb
index 1cdb8811cff..a4fae22a0c4 100644
--- a/app/models/blob.rb
+++ b/app/models/blob.rb
@@ -28,7 +28,7 @@ class Blob < SimpleDelegator
     BlobViewer::Sketch,
 
     BlobViewer::Video,
-    
+
     BlobViewer::PDF,
 
     BlobViewer::BinarySTL,
@@ -75,19 +75,37 @@ class Blob < SimpleDelegator
   end
 
   def no_highlighting?
-    size && size > MAXIMUM_TEXT_HIGHLIGHT_SIZE
+    raw_size && raw_size > MAXIMUM_TEXT_HIGHLIGHT_SIZE
+  end
+
+  def empty?
+    raw_size == 0
   end
 
   def too_large?
     size && truncated?
   end
 
+  def external_storage_error?
+    if external_storage == :lfs
+      !project&.lfs_enabled?
+    else
+      false
+    end
+  end
+
+  def stored_externally?
+    return @stored_externally if defined?(@stored_externally)
+
+    @stored_externally = external_storage && !external_storage_error?
+  end
+
   # Returns the size of the file that this blob represents. If this blob is an
   # LFS pointer, this is the size of the file stored in LFS. Otherwise, this is
   # the size of the blob itself.
   def raw_size
-    if valid_lfs_pointer?
-      lfs_size
+    if stored_externally?
+      external_size
     else
       size
     end
@@ -98,9 +116,13 @@ class Blob < SimpleDelegator
   # text-based rich blob viewer matched on the file's extension. Otherwise, this
   # depends on the type of the blob itself.
   def raw_binary?
-    if valid_lfs_pointer?
+    if stored_externally?
       if rich_viewer
         rich_viewer.binary?
+      elsif Linguist::Language.find_by_filename(name).any?
+        false
+      elsif _mime_type
+        _mime_type.binary?
       else
         true
       end
@@ -118,15 +140,7 @@ class Blob < SimpleDelegator
   end
 
   def readable_text?
-    text? && !valid_lfs_pointer? && !too_large?
-  end
-
-  def valid_lfs_pointer?
-    lfs_pointer? && project&.lfs_enabled?
-  end
-
-  def invalid_lfs_pointer?
-    lfs_pointer? && !project&.lfs_enabled?
+    text? && !stored_externally? && !too_large?
   end
 
   def simple_viewer
@@ -165,10 +179,10 @@ class Blob < SimpleDelegator
   end
 
   def rich_viewer_class
-    return if invalid_lfs_pointer? || empty?
+    return if empty? || external_storage_error?
 
     classes =
-      if valid_lfs_pointer?
+      if stored_externally?
         BINARY_VIEWERS + TEXT_VIEWERS
       elsif binary?
         BINARY_VIEWERS
diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb
index f944b00c9d3..a8b91d8d6bc 100644
--- a/app/models/blob_viewer/base.rb
+++ b/app/models/blob_viewer/base.rb
@@ -70,12 +70,13 @@ module BlobViewer
       return @render_error if defined?(@render_error)
 
       @render_error =
-        if server_side_but_stored_in_lfs?
-          # Files stored in LFS can only be rendered using a client-side viewer,
+        if server_side_but_stored_externally?
+          # Files that are not stored in the repository, like LFS files and
+          # build artifacts, can only be rendered using a client-side viewer,
           # since we do not want to read large amounts of data into memory on the
           # server side. Client-side viewers use JS and can fetch the file from
           # `blob_raw_url` using AJAX.
-          :server_side_but_stored_in_lfs
+          :server_side_but_stored_externally
         elsif override_max_size ? absolutely_too_large? : too_large?
           :too_large
         end
@@ -89,8 +90,8 @@ module BlobViewer
 
     private
 
-    def server_side_but_stored_in_lfs?
-      server_side? && blob.valid_lfs_pointer?
+    def server_side_but_stored_externally?
+      server_side? && blob.stored_externally?
     end
   end
 end
diff --git a/app/models/concerns/blob_like.rb b/app/models/concerns/blob_like.rb
new file mode 100644
index 00000000000..adb81561000
--- /dev/null
+++ b/app/models/concerns/blob_like.rb
@@ -0,0 +1,48 @@
+module BlobLike
+  extend ActiveSupport::Concern
+  include Linguist::BlobHelper
+
+  def id
+    raise NotImplementedError
+  end
+
+  def name
+    raise NotImplementedError
+  end
+
+  def path
+    raise NotImplementedError
+  end
+
+  def size
+    0
+  end
+
+  def data
+    nil
+  end
+
+  def mode
+    nil
+  end
+
+  def binary?
+    false
+  end
+
+  def load_all_data!(repository)
+    # No-op
+  end
+
+  def truncated?
+    false
+  end
+
+  def external_storage
+    nil
+  end
+
+  def external_size
+    nil
+  end
+end
diff --git a/app/models/snippet_blob.rb b/app/models/snippet_blob.rb
index d6cab74eb1a..fa5fa151607 100644
--- a/app/models/snippet_blob.rb
+++ b/app/models/snippet_blob.rb
@@ -1,5 +1,5 @@
 class SnippetBlob
-  include Linguist::BlobHelper
+  include BlobLike
 
   attr_reader :snippet
 
@@ -28,32 +28,4 @@ class SnippetBlob
 
     Banzai.render_field(snippet, :content)
   end
-
-  def mode
-    nil
-  end
-
-  def binary?
-    false
-  end
-
-  def load_all_data!(repository)
-    # No-op
-  end
-
-  def lfs_pointer?
-    false
-  end
-
-  def lfs_oid
-    nil
-  end
-
-  def lfs_size
-    nil
-  end
-
-  def truncated?
-    false
-  end
 end
diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb
index e8bb9e1f805..12458f9f410 100644
--- a/lib/gitlab/git/blob.rb
+++ b/lib/gitlab/git/blob.rb
@@ -128,6 +128,10 @@ module Gitlab
         encode! @name
       end
 
+      def truncated?
+        size && (size > loaded_size)
+      end
+
       # Valid LFS object pointer is a text file consisting of
       # version
       # oid
@@ -155,10 +159,14 @@ module Gitlab
         nil
       end
 
-      def truncated?
-        size && (size > loaded_size)
+      def external_storage
+        return unless lfs_pointer?
+
+        :lfs
       end
 
+      alias_method :external_size, :lfs_size
+
       private
 
       def has_lfs_version_key?
diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb
index 8dba2ccbafa..5955623f565 100644
--- a/spec/features/projects/blobs/blob_show_spec.rb
+++ b/spec/features/projects/blobs/blob_show_spec.rb
@@ -159,7 +159,7 @@ feature 'File blob', :js, feature: true do
           expect(page).to have_selector('.blob-viewer[data-type="rich"]')
 
           # shows an error message
-          expect(page).to have_content('The rendered file could not be displayed because it is stored in LFS. You can view the source or download it instead.')
+          expect(page).to have_content('The rendered file could not be displayed because it is stored in LFS. You can download it instead.')
 
           # shows a viewer switcher
           expect(page).to have_selector('.js-blob-viewer-switcher')
@@ -167,8 +167,8 @@ feature 'File blob', :js, feature: true do
           # does not show a copy button
           expect(page).not_to have_selector('.js-copy-blob-source-btn')
 
-          # shows a raw button
-          expect(page).to have_link('Open raw')
+          # shows a download button
+          expect(page).to have_link('Download')
         end
       end
 
@@ -332,4 +332,41 @@ feature 'File blob', :js, feature: true do
       end
     end
   end
+
+  context 'empty file' do
+    before do
+      project.add_master(project.creator)
+
+      Files::CreateService.new(
+        project,
+        project.creator,
+        start_branch: 'master',
+        branch_name: 'master',
+        commit_message: "Add empty file",
+        file_path: 'files/empty.md',
+        file_content: ''
+      ).execute
+
+      visit_blob('files/empty.md')
+
+      wait_for_ajax
+    end
+
+    it 'displays an error' do
+      aggregate_failures do
+        # shows an error message
+        expect(page).to have_content('Empty file')
+
+        # does not show a viewer switcher
+        expect(page).not_to have_selector('.js-blob-viewer-switcher')
+
+        # does not show a copy button
+        expect(page).not_to have_selector('.js-copy-blob-source-btn')
+
+        # does not show a download or raw button
+        expect(page).not_to have_link('Download')
+        expect(page).not_to have_link('Open raw')
+      end
+    end
+  end
 end
diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb
index 075f1887d91..1b4393e6167 100644
--- a/spec/helpers/blob_helper_spec.rb
+++ b/spec/helpers/blob_helper_spec.rb
@@ -145,7 +145,7 @@ describe BlobHelper do
         end
       end
 
-      context 'for error :server_side_but_stored_in_lfs' do
+      context 'for error :server_side_but_stored_externally' do
         let(:blob) { fake_blob(lfs: true) }
 
         it 'returns an error message' do
@@ -183,40 +183,56 @@ describe BlobHelper do
             expect(helper.blob_render_error_options(viewer)).not_to include(/load it anyway/)
           end
         end
-      end
 
-      context 'when the viewer is rich' do
-        context 'the blob is rendered as text' do
-          let(:blob) { fake_blob(path: 'file.md', lfs: true) }
+        context 'when the viewer is rich' do
+          context 'the blob is rendered as text' do
+            let(:blob) { fake_blob(path: 'file.md', size: 2.megabytes) }
 
-          it 'includes a "view the source" link' do
-            expect(helper.blob_render_error_options(viewer)).to include(/view the source/)
+            it 'includes a "view the source" link' do
+              expect(helper.blob_render_error_options(viewer)).to include(/view the source/)
+            end
+          end
+
+          context 'the blob is not rendered as text' do
+            let(:blob) { fake_blob(path: 'file.pdf', binary: true, size: 2.megabytes) }
+
+            it 'does not include a "view the source" link' do
+              expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
+            end
           end
         end
 
-        context 'the blob is not rendered as text' do
-          let(:blob) { fake_blob(path: 'file.pdf', binary: true, lfs: true) }
+        context 'when the viewer is not rich' do
+          before do
+            viewer_class.type = :simple
+          end
+
+          let(:blob) { fake_blob(path: 'file.md', size: 2.megabytes) }
 
           it 'does not include a "view the source" link' do
             expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
           end
         end
+
+        it 'includes a "download it" link' do
+          expect(helper.blob_render_error_options(viewer)).to include(/download it/)
+        end
       end
 
-      context 'when the viewer is not rich' do
-        before do
-          viewer_class.type = :simple
-        end
-
+      context 'for error :server_side_but_stored_externally' do
         let(:blob) { fake_blob(path: 'file.md', lfs: true) }
 
+        it 'does not include a "load it anyway" link' do
+          expect(helper.blob_render_error_options(viewer)).not_to include(/load it anyway/)
+        end
+
         it 'does not include a "view the source" link' do
           expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
         end
-      end
 
-      it 'includes a "download it" link' do
-        expect(helper.blob_render_error_options(viewer)).to include(/download it/)
+        it 'includes a "download it" link' do
+          expect(helper.blob_render_error_options(viewer)).to include(/download it/)
+        end
       end
     end
   end
diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb
index 7e8a1c8add7..f84c6b48173 100644
--- a/spec/models/blob_spec.rb
+++ b/spec/models/blob_spec.rb
@@ -35,8 +35,68 @@ describe Blob do
     end
   end
 
+  describe '#external_storage_error?' do
+    context 'if the blob is stored in LFS' do
+      let(:blob) { fake_blob(path: 'file.pdf', lfs: true) }
+
+      context 'when the project has LFS enabled' do
+        it 'returns false' do
+          expect(blob.external_storage_error?).to be_falsey
+        end
+      end
+
+      context 'when the project does not have LFS enabled' do
+        before do
+          project.lfs_enabled = false
+        end
+
+        it 'returns true' do
+          expect(blob.external_storage_error?).to be_truthy
+        end
+      end
+    end
+
+    context 'if the blob is not stored in LFS' do
+      let(:blob) { fake_blob(path: 'file.md') }
+
+      it 'returns false' do
+        expect(blob.external_storage_error?).to be_falsey
+      end
+    end
+  end
+
+  describe '#stored_externally?' do
+    context 'if the blob is stored in LFS' do
+      let(:blob) { fake_blob(path: 'file.pdf', lfs: true) }
+
+      context 'when the project has LFS enabled' do
+        it 'returns true' do
+          expect(blob.stored_externally?).to be_truthy
+        end
+      end
+
+      context 'when the project does not have LFS enabled' do
+        before do
+          project.lfs_enabled = false
+        end
+
+        it 'returns false' do
+          expect(blob.stored_externally?).to be_falsey
+        end
+      end
+    end
+
+    context 'if the blob is not stored in LFS' do
+      let(:blob) { fake_blob(path: 'file.md') }
+
+      it 'returns false' do
+        expect(blob.stored_externally?).to be_falsey
+      end
+    end
+  end
+
   describe '#raw_binary?' do
-    context 'if the blob is a valid LFS pointer' do
+    context 'if the blob is stored externally' do
       context 'if the extension has a rich viewer' do
         context 'if the viewer is binary' do
           it 'returns true' do
@@ -56,15 +116,63 @@ describe Blob do
       end
 
       context "if the extension doesn't have a rich viewer" do
-        it 'returns true' do
-          blob = fake_blob(path: 'file.exe', lfs: true)
+        context 'if the extension has a text mime type' do
+          context 'if the extension is for a programming language' do
+            it 'returns false' do
+              blob = fake_blob(path: 'file.txt', lfs: true)
 
-          expect(blob.raw_binary?).to be_truthy
+              expect(blob.raw_binary?).to be_falsey
+            end
+          end
+
+          context 'if the extension is not for a programming language' do
+            it 'returns false' do
+              blob = fake_blob(path: 'file.ics', lfs: true)
+
+              expect(blob.raw_binary?).to be_falsey
+            end
+          end
+        end
+
+        context 'if the extension has a binary mime type' do
+          context 'if the extension is for a programming language' do
+            it 'returns false' do
+              blob = fake_blob(path: 'file.rb', lfs: true)
+
+              expect(blob.raw_binary?).to be_falsey
+            end
+          end
+
+          context 'if the extension is not for a programming language' do
+            it 'returns true' do
+              blob = fake_blob(path: 'file.exe', lfs: true)
+
+              expect(blob.raw_binary?).to be_truthy
+            end
+          end
+        end
+
+        context 'if the extension has an unknown mime type' do
+          context 'if the extension is for a programming language' do
+            it 'returns false' do
+              blob = fake_blob(path: 'file.ini', lfs: true)
+
+              expect(blob.raw_binary?).to be_falsey
+            end
+          end
+
+          context 'if the extension is not for a programming language' do
+            it 'returns true' do
+              blob = fake_blob(path: 'file.wtf', lfs: true)
+
+              expect(blob.raw_binary?).to be_truthy
+            end
+          end
         end
       end
     end
 
-    context 'if the blob is not an LFS pointer' do
+    context 'if the blob is not stored externally' do
       context 'if the blob is binary' do
         it 'returns true' do
           blob = fake_blob(path: 'file.pdf', binary: true)
@@ -94,7 +202,7 @@ describe Blob do
   describe '#simple_viewer' do
     context 'when the blob is empty' do
       it 'returns an empty viewer' do
-        blob = fake_blob(data: '')
+        blob = fake_blob(data: '', size: 0)
 
         expect(blob.simple_viewer).to be_a(BlobViewer::Empty)
       end
@@ -118,7 +226,7 @@ describe Blob do
   end
 
   describe '#rich_viewer' do
-    context 'when the blob is an invalid LFS pointer' do
+    context 'when the blob has an external storage error' do
       before do
         project.lfs_enabled = false
       end
@@ -138,7 +246,7 @@ describe Blob do
       end
     end
 
-    context 'when the blob is a valid LFS pointer' do
+    context 'when the blob is stored externally' do
       it 'returns a matching viewer' do
         blob = fake_blob(path: 'file.pdf', lfs: true)
 
diff --git a/spec/models/blob_viewer/base_spec.rb b/spec/models/blob_viewer/base_spec.rb
index a3e598de56d..740ad9d275e 100644
--- a/spec/models/blob_viewer/base_spec.rb
+++ b/spec/models/blob_viewer/base_spec.rb
@@ -139,7 +139,7 @@ describe BlobViewer::Base, model: true do
       end
     end
 
-    context 'when the viewer is server side but the blob is stored in LFS' do
+    context 'when the viewer is server side but the blob is stored externally' do
       let(:project) { build(:empty_project, lfs_enabled: true) }
 
       let(:blob) { fake_blob(path: 'file.pdf', lfs: true) }
@@ -148,8 +148,8 @@ describe BlobViewer::Base, model: true do
         allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
       end
 
-      it 'return :server_side_but_stored_in_lfs' do
-        expect(viewer.render_error).to eq(:server_side_but_stored_in_lfs)
+      it 'return :server_side_but_stored_externally' do
+        expect(viewer.render_error).to eq(:server_side_but_stored_externally)
       end
     end
   end
diff --git a/spec/support/helpers/fake_blob_helpers.rb b/spec/support/helpers/fake_blob_helpers.rb
index b29af732ad3..bc9686ed9cf 100644
--- a/spec/support/helpers/fake_blob_helpers.rb
+++ b/spec/support/helpers/fake_blob_helpers.rb
@@ -1,6 +1,6 @@
 module FakeBlobHelpers
   class FakeBlob
-    include Linguist::BlobHelper
+    include BlobLike
 
     attr_reader :path, :size, :data, :lfs_oid, :lfs_size
 
@@ -19,10 +19,6 @@ module FakeBlobHelpers
 
     alias_method :name, :path
 
-    def mode
-      nil
-    end
-
     def id
       0
     end
@@ -31,17 +27,11 @@ module FakeBlobHelpers
       @binary
     end
 
-    def load_all_data!(repository)
-      # No-op
+    def external_storage
+      :lfs if @lfs_pointer
     end
 
-    def lfs_pointer?
-      @lfs_pointer
-    end
-
-    def truncated?
-      false
-    end
+    alias_method :external_size, :lfs_size
   end
 
   def fake_blob(**kwargs)