Merge branch 'dm-blob-external-storage' into 'master'

Refactor Blob support of external storage in preparation of job artifact blobs

See merge request !11048

5 files changed, 195 insertions, 44 deletions

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/)
+            end
+          end
+
+          context 'the blob is not rendered as text' do
+            let(:blob) { fake_blob(path: 'file.pdf', binary: true, size: 2.megabytes) }
 
-          it 'includes a "view the source" link' do
-            expect(helper.blob_render_error_options(viewer)).to include(/view the source/)
+            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
-      end
 
-      context 'when the viewer is not rich' do
-        before do
-          viewer_class.type = :simple
+        it 'includes a "download it" link' do
+          expect(helper.blob_render_error_options(viewer)).to include(/download it/)
         end
+      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)