Merge branch 'dm-blob-viewer-concerns' into 'master'

Move some blob viewer stuff around without changing behavior

See merge request !11358

17 files changed, 125 insertions, 113 deletions

diff --git a/app/controllers/concerns/renders_blob.rb b/app/controllers/concerns/renders_blob.rb
index 4a6630dfd90..1d37e4cb3bd 100644
--- a/app/controllers/concerns/renders_blob.rb
+++ b/app/controllers/concerns/renders_blob.rb
@@ -14,7 +14,7 @@ module RendersBlob
     return render_404 unless viewer
 
     render json: {
-      html: view_to_html_string("projects/blob/_viewer", viewer: viewer, load_asynchronously: false)
+      html: view_to_html_string("projects/blob/_viewer", viewer: viewer, load_async: false)
     }
   end
 
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index eb37f2e0267..7eb3512378c 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -226,7 +226,7 @@ module BlobHelper
 
   def open_raw_blob_button(blob)
     return if blob.empty?
-    
+
     if blob.raw_binary? || blob.stored_externally?
       icon = icon('download')
       title = 'Download'
@@ -242,9 +242,9 @@ module BlobHelper
     case viewer.render_error
     when :too_large
       max_size =
-        if viewer.absolutely_too_large?
-          viewer.absolute_max_size
-        elsif viewer.too_large?
+        if viewer.can_override_max_size?
+          viewer.overridable_max_size
+        else
           viewer.max_size
         end
       "it is larger than #{number_to_human_size(max_size)}"
diff --git a/app/models/blob_viewer/auxiliary.rb b/app/models/blob_viewer/auxiliary.rb
index db124397b27..cd6e596ed60 100644
--- a/app/models/blob_viewer/auxiliary.rb
+++ b/app/models/blob_viewer/auxiliary.rb
@@ -5,8 +5,8 @@ module BlobViewer
     included do
       self.loading_partial_name = 'loading_auxiliary'
       self.type = :auxiliary
+      self.overridable_max_size = 100.kilobytes
       self.max_size = 100.kilobytes
-      self.absolute_max_size = 100.kilobytes
     end
   end
 end
diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb
index 4f38c31714b..c7b8fbfc56a 100644
--- a/app/models/blob_viewer/base.rb
+++ b/app/models/blob_viewer/base.rb
@@ -2,11 +2,11 @@ module BlobViewer
   class Base
     PARTIAL_PATH_PREFIX = 'projects/blob/viewers'.freeze
 
-    class_attribute :partial_name, :loading_partial_name, :type, :extensions, :file_type, :client_side, :binary, :switcher_icon, :switcher_title, :max_size, :absolute_max_size
+    class_attribute :partial_name, :loading_partial_name, :type, :extensions, :file_types, :load_async, :binary, :switcher_icon, :switcher_title, :overridable_max_size, :max_size
 
     self.loading_partial_name = 'loading'
 
-    delegate :partial_path, :loading_partial_path, :rich?, :simple?, :client_side?, :server_side?, :text?, :binary?, to: :class
+    delegate :partial_path, :loading_partial_path, :rich?, :simple?, :text?, :binary?, to: :class
 
     attr_reader :blob
     attr_accessor :override_max_size
@@ -35,12 +35,8 @@ module BlobViewer
       type == :auxiliary
     end
 
-    def self.client_side?
-      client_side
-    end
-
-    def self.server_side?
-      !client_side?
+    def self.load_async?
+      load_async
     end
 
     def self.binary?
@@ -54,21 +50,33 @@ module BlobViewer
     def self.can_render?(blob, verify_binary: true)
       return false if verify_binary && binary? != blob.binary?
       return true if extensions&.include?(blob.extension)
-      return true if file_type && Gitlab::FileDetector.type_of(blob.path) == file_type
+      return true if file_types&.include?(Gitlab::FileDetector.type_of(blob.path))
 
       false
     end
 
-    def too_large?
-      blob.raw_size > max_size
+    def load_async?
+      self.class.load_async? && render_error.nil?
     end
 
-    def absolutely_too_large?
-      blob.raw_size > absolute_max_size
+    def exceeds_overridable_max_size?
+      overridable_max_size && blob.raw_size > overridable_max_size
+    end
+
+    def exceeds_max_size?
+      max_size && blob.raw_size > max_size
     end
 
     def can_override_max_size?
-      too_large? && !absolutely_too_large?
+      exceeds_overridable_max_size? && !exceeds_max_size?
+    end
+
+    def too_large?
+      if override_max_size
+        exceeds_max_size?
+      else
+        exceeds_overridable_max_size?
+      end
     end
 
     # This method is used on the server side to check whether we can attempt to
@@ -83,29 +91,13 @@ module BlobViewer
     # binary from `blob_raw_url` and does its own format validation and error
     # rendering, especially for potentially large binary formats.
     def render_error
-      return @render_error if defined?(@render_error)
-
-      @render_error =
-        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_externally
-        elsif override_max_size ? absolutely_too_large? : too_large?
-          :too_large
-        end
+      if too_large?
+        :too_large
+      end
     end
 
     def prepare!
       # To be overridden by subclasses
     end
-
-    private
-
-    def server_side_but_stored_externally?
-      server_side? && blob.stored_externally?
-    end
   end
 end
diff --git a/app/models/blob_viewer/client_side.rb b/app/models/blob_viewer/client_side.rb
index 42ec68f864b..cc68236f92b 100644
--- a/app/models/blob_viewer/client_side.rb
+++ b/app/models/blob_viewer/client_side.rb
@@ -3,9 +3,9 @@ module BlobViewer
     extend ActiveSupport::Concern
 
     included do
-      self.client_side = true
-      self.max_size = 10.megabytes
-      self.absolute_max_size = 50.megabytes
+      self.load_async = false
+      self.overridable_max_size = 10.megabytes
+      self.max_size = 50.megabytes
     end
   end
 end
diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb
index adc06587f69..074e7204814 100644
--- a/app/models/blob_viewer/download.rb
+++ b/app/models/blob_viewer/download.rb
@@ -1,17 +1,9 @@
 module BlobViewer
   class Download < Base
     include Simple
-    # We treat the Download viewer as if it renders the content client-side,
-    # so that it doesn't attempt to load the entire blob contents and is
-    # rendered synchronously instead of loaded asynchronously.
-    include ClientSide
+    include Static
 
     self.partial_name = 'download'
     self.binary = true
-
-    # We can always render the Download viewer, even if the blob is in LFS or too large.
-    def render_error
-      nil
-    end
   end
 end
diff --git a/app/models/blob_viewer/gitlab_ci_yml.rb b/app/models/blob_viewer/gitlab_ci_yml.rb
index 81afab2f49b..7267c3965d3 100644
--- a/app/models/blob_viewer/gitlab_ci_yml.rb
+++ b/app/models/blob_viewer/gitlab_ci_yml.rb
@@ -5,7 +5,7 @@ module BlobViewer
 
     self.partial_name = 'gitlab_ci_yml'
     self.loading_partial_name = 'gitlab_ci_yml_loading'
-    self.file_type = :gitlab_ci
+    self.file_types = %i(gitlab_ci)
     self.binary = false
 
     def validation_message
diff --git a/app/models/blob_viewer/license.rb b/app/models/blob_viewer/license.rb
index 3ad49570c88..6751ea15a5d 100644
--- a/app/models/blob_viewer/license.rb
+++ b/app/models/blob_viewer/license.rb
@@ -1,13 +1,10 @@
 module BlobViewer
   class License < Base
-    # We treat the License viewer as if it renders the content client-side,
-    # so that it doesn't attempt to load the entire blob contents and is
-    # rendered synchronously instead of loaded asynchronously.
-    include ClientSide
     include Auxiliary
+    include Static
 
     self.partial_name = 'license'
-    self.file_type = :license
+    self.file_types = %i(license)
     self.binary = false
 
     def license
diff --git a/app/models/blob_viewer/route_map.rb b/app/models/blob_viewer/route_map.rb
index 1ca730c1ea0..153b4eeb2c9 100644
--- a/app/models/blob_viewer/route_map.rb
+++ b/app/models/blob_viewer/route_map.rb
@@ -5,7 +5,7 @@ module BlobViewer
 
     self.partial_name = 'route_map'
     self.loading_partial_name = 'route_map_loading'
-    self.file_type = :route_map
+    self.file_types = %i(route_map)
     self.binary = false
 
     def validation_message
diff --git a/app/models/blob_viewer/server_side.rb b/app/models/blob_viewer/server_side.rb
index e8c5c17b824..87884dcd6bf 100644
--- a/app/models/blob_viewer/server_side.rb
+++ b/app/models/blob_viewer/server_side.rb
@@ -3,9 +3,9 @@ module BlobViewer
     extend ActiveSupport::Concern
 
     included do
-      self.client_side = false
-      self.max_size = 2.megabytes
-      self.absolute_max_size = 5.megabytes
+      self.load_async = true
+      self.overridable_max_size = 2.megabytes
+      self.max_size = 5.megabytes
     end
 
     def prepare!
@@ -13,5 +13,18 @@ module BlobViewer
         blob.load_all_data!(blob.project.repository)
       end
     end
+
+    def render_error
+      if blob.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.
+        return :server_side_but_stored_externally
+      end
+
+      super
+    end
   end
 end
diff --git a/app/models/blob_viewer/static.rb b/app/models/blob_viewer/static.rb
new file mode 100644
index 00000000000..c9e257e5388
--- /dev/null
+++ b/app/models/blob_viewer/static.rb
@@ -0,0 +1,14 @@
+module BlobViewer
+  module Static
+    extend ActiveSupport::Concern
+
+    included do
+      self.load_async = false
+    end
+
+    # We can always render a static viewer, even if the blob is too large.
+    def render_error
+      nil
+    end
+  end
+end
diff --git a/app/models/blob_viewer/text.rb b/app/models/blob_viewer/text.rb
index e27b2c2b493..eddca50b4d4 100644
--- a/app/models/blob_viewer/text.rb
+++ b/app/models/blob_viewer/text.rb
@@ -5,7 +5,7 @@ module BlobViewer
 
     self.partial_name = 'text'
     self.binary = false
-    self.max_size = 1.megabyte
-    self.absolute_max_size = 10.megabytes
+    self.overridable_max_size = 1.megabyte
+    self.max_size = 10.megabytes
   end
 end
diff --git a/app/views/projects/blob/_viewer.html.haml b/app/views/projects/blob/_viewer.html.haml
index 3d9c3a59980..4252f27d007 100644
--- a/app/views/projects/blob/_viewer.html.haml
+++ b/app/views/projects/blob/_viewer.html.haml
@@ -1,10 +1,10 @@
 - hidden = local_assigns.fetch(:hidden, false)
 - render_error = viewer.render_error
-- load_asynchronously = local_assigns.fetch(:load_asynchronously, viewer.server_side?) && render_error.nil?
+- load_async = local_assigns.fetch(:load_async, viewer.load_async?)
 
-- viewer_url = local_assigns.fetch(:viewer_url) { url_for(params.merge(viewer: viewer.type, format: :json)) } if load_asynchronously
+- viewer_url = local_assigns.fetch(:viewer_url) { url_for(params.merge(viewer: viewer.type, format: :json)) } if load_async
 .blob-viewer{ data: { type: viewer.type, url: viewer_url }, class: ('hidden' if hidden) }
-  - if load_asynchronously
+  - if load_async
     = render viewer.loading_partial_path, viewer: viewer
   - elsif render_error
     = render 'projects/blob/render_error', viewer: viewer
diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb
index 1b4393e6167..41b5df12522 100644
--- a/spec/helpers/blob_helper_spec.rb
+++ b/spec/helpers/blob_helper_spec.rb
@@ -116,10 +116,11 @@ describe BlobHelper do
 
     let(:viewer_class) do
       Class.new(BlobViewer::Base) do
-        self.max_size = 1.megabyte
-        self.absolute_max_size = 5.megabytes
+        include BlobViewer::ServerSide
+
+        self.overridable_max_size = 1.megabyte
+        self.max_size = 5.megabytes
         self.type = :rich
-        self.client_side = false
       end
     end
 
diff --git a/spec/models/blob_viewer/base_spec.rb b/spec/models/blob_viewer/base_spec.rb
index a6641970e1b..92fbf64a6b7 100644
--- a/spec/models/blob_viewer/base_spec.rb
+++ b/spec/models/blob_viewer/base_spec.rb
@@ -7,11 +7,12 @@ describe BlobViewer::Base, model: true do
 
   let(:viewer_class) do
     Class.new(described_class) do
+      include BlobViewer::ServerSide
+
       self.extensions = %w(pdf)
       self.binary = true
-      self.max_size = 1.megabyte
-      self.absolute_max_size = 5.megabytes
-      self.client_side = false
+      self.overridable_max_size = 1.megabyte
+      self.max_size = 5.megabytes
     end
   end
 
@@ -38,10 +39,10 @@ describe BlobViewer::Base, model: true do
 
     context 'when the file type is supported' do
       before do
-        viewer_class.file_type = :license
+        viewer_class.file_types = %i(license)
         viewer_class.binary = false
       end
-      
+
       context 'when the binaryness matches' do
         let(:blob) { fake_blob(path: 'LICENSE', binary: false) }
 
@@ -68,45 +69,45 @@ describe BlobViewer::Base, model: true do
     end
   end
 
-  describe '#too_large?' do
-    context 'when the blob size is larger than the max size' do
+  describe '#exceeds_overridable_max_size?' do
+    context 'when the blob size is larger than the overridable max size' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
       it 'returns true' do
-        expect(viewer.too_large?).to be_truthy
+        expect(viewer.exceeds_overridable_max_size?).to be_truthy
       end
     end
 
-    context 'when the blob size is smaller than the max size' do
+    context 'when the blob size is smaller than the overridable max size' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) }
 
       it 'returns false' do
-        expect(viewer.too_large?).to be_falsey
+        expect(viewer.exceeds_overridable_max_size?).to be_falsey
       end
     end
   end
 
-  describe '#absolutely_too_large?' do
-    context 'when the blob size is larger than the absolute max size' do
+  describe '#exceeds_max_size?' do
+    context 'when the blob size is larger than the max size' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) }
 
       it 'returns true' do
-        expect(viewer.absolutely_too_large?).to be_truthy
+        expect(viewer.exceeds_max_size?).to be_truthy
       end
     end
 
-    context 'when the blob size is smaller than the absolute max size' do
+    context 'when the blob size is smaller than the max size' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
       it 'returns false' do
-        expect(viewer.absolutely_too_large?).to be_falsey
+        expect(viewer.exceeds_max_size?).to be_falsey
       end
     end
   end
 
   describe '#can_override_max_size?' do
-    context 'when the blob size is larger than the max size' do
-      context 'when the blob size is larger than the absolute max size' do
+    context 'when the blob size is larger than the overridable max size' do
+      context 'when the blob size is larger than the max size' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) }
 
         it 'returns false' do
@@ -114,7 +115,7 @@ describe BlobViewer::Base, model: true do
         end
       end
 
-      context 'when the blob size is smaller than the absolute max size' do
+      context 'when the blob size is smaller than the max size' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
         it 'returns true' do
@@ -123,7 +124,7 @@ describe BlobViewer::Base, model: true do
       end
     end
 
-    context 'when the blob size is smaller than the max size' do
+    context 'when the blob size is smaller than the overridable max size' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) }
 
       it 'returns false' do
@@ -138,7 +139,7 @@ describe BlobViewer::Base, model: true do
         viewer.override_max_size = true
       end
 
-      context 'when the blob size is larger than the absolute max size' do
+      context 'when the blob size is larger than the max size' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) }
 
         it 'returns :too_large' do
@@ -146,7 +147,7 @@ describe BlobViewer::Base, model: true do
         end
       end
 
-      context 'when the blob size is smaller than the absolute max size' do
+      context 'when the blob size is smaller than the max size' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
         it 'returns nil' do
@@ -156,7 +157,7 @@ describe BlobViewer::Base, model: true do
     end
 
     context 'when the max size is not overridden' do
-      context 'when the blob size is larger than the max size' do
+      context 'when the blob size is larger than the overridable max size' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
         it 'returns :too_large' do
@@ -164,7 +165,7 @@ describe BlobViewer::Base, model: true do
         end
       end
 
-      context 'when the blob size is smaller than the max size' do
+      context 'when the blob size is smaller than the overridable max size' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) }
 
         it 'returns nil' do
@@ -172,19 +173,5 @@ describe BlobViewer::Base, model: true do
         end
       end
     end
-
-    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) }
-
-      before do
-        allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
-      end
-
-      it 'return :server_side_but_stored_externally' do
-        expect(viewer.render_error).to eq(:server_side_but_stored_externally)
-      end
-    end
   end
 end
diff --git a/spec/models/blob_viewer/server_side_spec.rb b/spec/models/blob_viewer/server_side_spec.rb
index ddca9b79390..f047953d540 100644
--- a/spec/models/blob_viewer/server_side_spec.rb
+++ b/spec/models/blob_viewer/server_side_spec.rb
@@ -22,4 +22,20 @@ describe BlobViewer::ServerSide, model: true do
       subject.prepare!
     end
   end
+
+  describe '#render_error' do
+    context 'when the blob is stored externally' do
+      let(:project) { build(:empty_project, lfs_enabled: true) }
+
+      let(:blob) { fake_blob(path: 'file.pdf', lfs: true) }
+
+      before do
+        allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+      end
+
+      it 'return :server_side_but_stored_externally' do
+        expect(subject.render_error).to eq(:server_side_but_stored_externally)
+      end
+    end
+  end
 end
diff --git a/spec/views/projects/blob/_viewer.html.haml_spec.rb b/spec/views/projects/blob/_viewer.html.haml_spec.rb
index 08018767624..c6b0ed8da3c 100644
--- a/spec/views/projects/blob/_viewer.html.haml_spec.rb
+++ b/spec/views/projects/blob/_viewer.html.haml_spec.rb
@@ -10,9 +10,9 @@ describe 'projects/blob/_viewer.html.haml', :view do
       include BlobViewer::Rich
 
       self.partial_name = 'text'
-      self.max_size = 1.megabyte
-      self.absolute_max_size = 5.megabytes
-      self.client_side = false
+      self.overridable_max_size = 1.megabyte
+      self.max_size = 5.megabytes
+      self.load_async = true
     end
   end
 
@@ -35,9 +35,9 @@ describe 'projects/blob/_viewer.html.haml', :view do
     render partial: 'projects/blob/viewer', locals: { viewer: viewer }
   end
 
-  context 'when the viewer is server side' do
+  context 'when the viewer is loaded asynchronously' do
     before do
-      viewer_class.client_side = false
+      viewer_class.load_async = true
     end
 
     context 'when there is no render error' do
@@ -65,9 +65,9 @@ describe 'projects/blob/_viewer.html.haml', :view do
     end
   end
 
-  context 'when the viewer is client side' do
+  context 'when the viewer is loaded synchronously' do
     before do
-      viewer_class.client_side = true
+      viewer_class.load_async = false
     end
 
     context 'when there is no render error' do