Commit c1e2da92 authored by Sean McGivern's avatar Sean McGivern

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

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

See merge request !11048
parents 907f006c 720cc14a
...@@ -15,7 +15,7 @@ class Projects::RawController < Projects::ApplicationController ...@@ -15,7 +15,7 @@ class Projects::RawController < Projects::ApplicationController
return if cached_blob? return if cached_blob?
if @blob.valid_lfs_pointer? if @blob.stored_externally?
send_lfs_object send_lfs_object
else else
send_git_blob @repository, @blob send_git_blob @repository, @blob
......
...@@ -52,7 +52,7 @@ module BlobHelper ...@@ -52,7 +52,7 @@ module BlobHelper
if !on_top_of_branch?(project, ref) 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' } 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' } 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) elsif can_modify_blob?(blob, project, ref)
button_tag label, class: "#{common_classes}", 'data-target' => "#modal-#{modal_type}-blob", 'data-toggle' => 'modal' button_tag label, class: "#{common_classes}", 'data-target' => "#modal-#{modal_type}-blob", 'data-toggle' => 'modal'
...@@ -95,7 +95,7 @@ module BlobHelper ...@@ -95,7 +95,7 @@ module BlobHelper
end end
def can_modify_blob?(blob, project = @project, ref = @ref) 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 end
def leave_edit_message def leave_edit_message
...@@ -223,7 +223,9 @@ module BlobHelper ...@@ -223,7 +223,9 @@ module BlobHelper
end end
def open_raw_blob_button(blob) def open_raw_blob_button(blob)
if blob.raw_binary? return if blob.empty?
if blob.raw_binary? || blob.stored_externally?
icon = icon('download') icon = icon('download')
title = 'Download' title = 'Download'
else else
...@@ -244,19 +246,27 @@ module BlobHelper ...@@ -244,19 +246,27 @@ module BlobHelper
viewer.max_size viewer.max_size
end end
"it is larger than #{number_to_human_size(max_size)}" "it is larger than #{number_to_human_size(max_size)}"
when :server_side_but_stored_in_lfs when :server_side_but_stored_externally
"it is stored in LFS" case viewer.blob.external_storage
when :lfs
'it is stored in LFS'
else
'it is stored externally'
end
end end
end end
def blob_render_error_options(viewer) def blob_render_error_options(viewer)
error = viewer.render_error
options = [] 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))) options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil)))
end 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' }) options << link_to('view the source', '#', class: 'js-blob-viewer-switch-btn', data: { viewer: 'simple' })
end end
......
...@@ -28,7 +28,7 @@ class Blob < SimpleDelegator ...@@ -28,7 +28,7 @@ class Blob < SimpleDelegator
BlobViewer::Sketch, BlobViewer::Sketch,
BlobViewer::Video, BlobViewer::Video,
BlobViewer::PDF, BlobViewer::PDF,
BlobViewer::BinarySTL, BlobViewer::BinarySTL,
...@@ -75,19 +75,37 @@ class Blob < SimpleDelegator ...@@ -75,19 +75,37 @@ class Blob < SimpleDelegator
end end
def no_highlighting? def no_highlighting?
size && size > MAXIMUM_TEXT_HIGHLIGHT_SIZE raw_size && raw_size > MAXIMUM_TEXT_HIGHLIGHT_SIZE
end
def empty?
raw_size == 0
end end
def too_large? def too_large?
size && truncated? size && truncated?
end 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 # 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 # LFS pointer, this is the size of the file stored in LFS. Otherwise, this is
# the size of the blob itself. # the size of the blob itself.
def raw_size def raw_size
if valid_lfs_pointer? if stored_externally?
lfs_size external_size
else else
size size
end end
...@@ -98,9 +116,13 @@ class Blob < SimpleDelegator ...@@ -98,9 +116,13 @@ class Blob < SimpleDelegator
# text-based rich blob viewer matched on the file's extension. Otherwise, this # text-based rich blob viewer matched on the file's extension. Otherwise, this
# depends on the type of the blob itself. # depends on the type of the blob itself.
def raw_binary? def raw_binary?
if valid_lfs_pointer? if stored_externally?
if rich_viewer if rich_viewer
rich_viewer.binary? rich_viewer.binary?
elsif Linguist::Language.find_by_filename(name).any?
false
elsif _mime_type
_mime_type.binary?
else else
true true
end end
...@@ -118,15 +140,7 @@ class Blob < SimpleDelegator ...@@ -118,15 +140,7 @@ class Blob < SimpleDelegator
end end
def readable_text? def readable_text?
text? && !valid_lfs_pointer? && !too_large? text? && !stored_externally? && !too_large?
end
def valid_lfs_pointer?
lfs_pointer? && project&.lfs_enabled?
end
def invalid_lfs_pointer?
lfs_pointer? && !project&.lfs_enabled?
end end
def simple_viewer def simple_viewer
...@@ -165,10 +179,10 @@ class Blob < SimpleDelegator ...@@ -165,10 +179,10 @@ class Blob < SimpleDelegator
end end
def rich_viewer_class def rich_viewer_class
return if invalid_lfs_pointer? || empty? return if empty? || external_storage_error?
classes = classes =
if valid_lfs_pointer? if stored_externally?
BINARY_VIEWERS + TEXT_VIEWERS BINARY_VIEWERS + TEXT_VIEWERS
elsif binary? elsif binary?
BINARY_VIEWERS BINARY_VIEWERS
......
...@@ -70,12 +70,13 @@ module BlobViewer ...@@ -70,12 +70,13 @@ module BlobViewer
return @render_error if defined?(@render_error) return @render_error if defined?(@render_error)
@render_error = @render_error =
if server_side_but_stored_in_lfs? if server_side_but_stored_externally?
# Files stored in LFS can only be rendered using a client-side viewer, # 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 # 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 # server side. Client-side viewers use JS and can fetch the file from
# `blob_raw_url` using AJAX. # `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? elsif override_max_size ? absolutely_too_large? : too_large?
:too_large :too_large
end end
...@@ -89,8 +90,8 @@ module BlobViewer ...@@ -89,8 +90,8 @@ module BlobViewer
private private
def server_side_but_stored_in_lfs? def server_side_but_stored_externally?
server_side? && blob.valid_lfs_pointer? server_side? && blob.stored_externally?
end end
end end
end end
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
class SnippetBlob class SnippetBlob
include Linguist::BlobHelper include BlobLike
attr_reader :snippet attr_reader :snippet
...@@ -28,32 +28,4 @@ class SnippetBlob ...@@ -28,32 +28,4 @@ class SnippetBlob
Banzai.render_field(snippet, :content) Banzai.render_field(snippet, :content)
end 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 end
...@@ -128,6 +128,10 @@ module Gitlab ...@@ -128,6 +128,10 @@ module Gitlab
encode! @name encode! @name
end end
def truncated?
size && (size > loaded_size)
end
# Valid LFS object pointer is a text file consisting of # Valid LFS object pointer is a text file consisting of
# version # version
# oid # oid
...@@ -155,10 +159,14 @@ module Gitlab ...@@ -155,10 +159,14 @@ module Gitlab
nil nil
end end
def truncated? def external_storage
size && (size > loaded_size) return unless lfs_pointer?
:lfs
end end
alias_method :external_size, :lfs_size
private private
def has_lfs_version_key? def has_lfs_version_key?
......
...@@ -159,7 +159,7 @@ feature 'File blob', :js, feature: true do ...@@ -159,7 +159,7 @@ feature 'File blob', :js, feature: true do
expect(page).to have_selector('.blob-viewer[data-type="rich"]') expect(page).to have_selector('.blob-viewer[data-type="rich"]')
# shows an error message # 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 # shows a viewer switcher
expect(page).to have_selector('.js-blob-viewer-switcher') expect(page).to have_selector('.js-blob-viewer-switcher')
...@@ -167,8 +167,8 @@ feature 'File blob', :js, feature: true do ...@@ -167,8 +167,8 @@ feature 'File blob', :js, feature: true do
# does not show a copy button # does not show a copy button
expect(page).not_to have_selector('.js-copy-blob-source-btn') expect(page).not_to have_selector('.js-copy-blob-source-btn')
# shows a raw button # shows a download button
expect(page).to have_link('Open raw') expect(page).to have_link('Download')
end end
end end
...@@ -332,4 +332,41 @@ feature 'File blob', :js, feature: true do ...@@ -332,4 +332,41 @@ feature 'File blob', :js, feature: true do
end end
end 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 end
...@@ -145,7 +145,7 @@ describe BlobHelper do ...@@ -145,7 +145,7 @@ describe BlobHelper do
end end
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) } let(:blob) { fake_blob(lfs: true) }
it 'returns an error message' do it 'returns an error message' do
...@@ -183,40 +183,56 @@ describe BlobHelper do ...@@ -183,40 +183,56 @@ describe BlobHelper do
expect(helper.blob_render_error_options(viewer)).not_to include(/load it anyway/) expect(helper.blob_render_error_options(viewer)).not_to include(/load it anyway/)
end end
end end
end
context 'when the viewer is rich' do context 'when the viewer is rich' do
context 'the blob is rendered as text' do context 'the blob is rendered as text' do
let(:blob) { fake_blob(path: 'file.md', lfs: true) } 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 it 'does not include a "view the source" link' do
expect(helper.blob_render_error_options(viewer)).to include(/view the source/) expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
end
end end
end end
context 'the blob is not rendered as text' do context 'when the viewer is not rich' do
let(:blob) { fake_blob(path: 'file.pdf', binary: true, lfs: true) } 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 it 'does not include a "view the source" link' do
expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/) expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
end end
end end
end
context 'when the viewer is not rich' do it 'includes a "download it" link' do
before do expect(helper.blob_render_error_options(viewer)).to include(/download it/)
viewer_class.type = :simple
end end
end
context 'for error :server_side_but_stored_externally' do
let(:blob) { fake_blob(path: 'file.md', lfs: true) } 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 it 'does not include a "view the source" link' do
expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/) expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
end end
end
it 'includes a "download it" link' do it 'includes a "download it" link' do
expect(helper.blob_render_error_options(viewer)).to include(/download it/) expect(helper.blob_render_error_options(viewer)).to include(/download it/)
end
end end
end end
end end
......
...@@ -35,8 +35,68 @@ describe Blob do ...@@ -35,8 +35,68 @@ describe Blob do
end end
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 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 extension has a rich viewer' do
context 'if the viewer is binary' do context 'if the viewer is binary' do
it 'returns true' do it 'returns true' do
...@@ -56,15 +116,63 @@ describe Blob do ...@@ -56,15 +116,63 @@ describe Blob do
end end
context "if the extension doesn't have a rich viewer" do context "if the extension doesn't have a rich viewer" do
it 'returns true' do context 'if the extension has a text mime type' do
blob = fake_blob(path: 'file.exe', lfs: true) 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 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 context 'if the blob is binary' do
it 'returns true' do it 'returns true' do
blob = fake_blob(path: 'file.pdf', binary: true) blob = fake_blob(path: 'file.pdf', binary: true)
...@@ -94,7 +202,7 @@ describe Blob do ...@@ -94,7 +202,7 @@ describe Blob do
describe '#simple_viewer' do describe '#simple_viewer' do
context 'when the blob is empty' do context 'when the blob is empty' do
it 'returns an empty viewer' 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) expect(blob.simple_viewer).to be_a(BlobViewer::Empty)
end end
...@@ -118,7 +226,7 @@ describe Blob do ...@@ -118,7 +226,7 @@ describe Blob do
end end
describe '#rich_viewer' do 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 before do
project.lfs_enabled = false project.lfs_enabled = false
end end
...@@ -138,7 +246,7 @@ describe Blob do ...@@ -138,7 +246,7 @@ describe Blob do
end end
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 it 'returns a matching viewer' do
blob = fake_blob(path: 'file.pdf', lfs: true) blob = fake_blob(path: 'file.pdf', lfs: true)
......
...@@ -139,7 +139,7 @@ describe BlobViewer::Base, model: true do ...@@ -139,7 +139,7 @@ describe BlobViewer::Base, model: true do
end end
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(:project) { build(:empty_project, lfs_enabled: true) }
let(:blob) { fake_blob(path: 'file.pdf', lfs: true) } let(:blob) { fake_blob(path: 'file.pdf', lfs: true) }
...@@ -148,8 +148,8 @@ describe BlobViewer::Base, model: true do ...@@ -148,8 +148,8 @@ describe BlobViewer::Base, model: true do
allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
end end
it 'return :server_side_but_stored_in_lfs' do it 'return :server_side_but_stored_externally' do
expect(viewer.render_error).to eq(:server_side_but_stored_in_lfs) expect(viewer.render_error).to eq(:server_side_but_stored_externally)
end end
end end
end end
......
module FakeBlobHelpers module FakeBlobHelpers
class FakeBlob class FakeBlob
include Linguist::BlobHelper include BlobLike
attr_reader :path, :size, :data, :lfs_oid, :lfs_size attr_reader :path, :size, :data, :lfs_oid, :lfs_size
...@@ -19,10 +19,6 @@ module FakeBlobHelpers ...@@ -19,10 +19,6 @@ module FakeBlobHelpers
alias_method :name, :path alias_method :name, :path
def mode
nil
end
def id def id
0 0
end end
...@@ -31,17 +27,11 @@ module FakeBlobHelpers ...@@ -31,17 +27,11 @@ module FakeBlobHelpers
@binary @binary
end end
def load_all_data!(repository) def external_storage
# No-op :lfs if @lfs_pointer
end end
def lfs_pointer? alias_method :external_size, :lfs_size
@lfs_pointer
end
def truncated?
false
end
end end
def fake_blob(**kwargs) def fake_blob(**kwargs)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment