summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/concerns/uploads_actions.rb61
-rw-r--r--app/controllers/groups/uploads_controller.rb30
-rw-r--r--app/controllers/projects/lfs_storage_controller.rb2
-rw-r--r--app/controllers/projects/uploads_controller.rb21
-rw-r--r--app/controllers/uploads_controller.rb75
-rw-r--r--app/models/appearance.rb1
-rw-r--r--app/models/concerns/avatarable.rb24
-rw-r--r--app/models/group.rb13
-rw-r--r--app/models/note.rb1
-rw-r--r--app/models/project.rb15
-rw-r--r--app/models/upload.rb49
-rw-r--r--app/models/user.rb16
-rw-r--r--app/services/projects/hashed_storage/migrate_attachments_service.rb4
-rw-r--r--app/uploaders/attachment_uploader.rb8
-rw-r--r--app/uploaders/avatar_uploader.rb19
-rw-r--r--app/uploaders/file_mover.rb3
-rw-r--r--app/uploaders/file_uploader.rb118
-rw-r--r--app/uploaders/gitlab_uploader.rb77
-rw-r--r--app/uploaders/job_artifact_uploader.rb26
-rw-r--r--app/uploaders/legacy_artifact_uploader.rb26
-rw-r--r--app/uploaders/lfs_object_uploader.rb21
-rw-r--r--app/uploaders/namespace_file_uploader.rb18
-rw-r--r--app/uploaders/personal_file_uploader.rb30
-rw-r--r--app/uploaders/records_uploads.rb80
-rw-r--r--app/uploaders/uploader_helper.rb9
-rw-r--r--app/uploaders/workhorse.rb7
-rw-r--r--app/workers/upload_checksum_worker.rb2
-rw-r--r--config/gitlab.yml.example8
-rw-r--r--config/initializers/1_settings.rb13
-rw-r--r--db/migrate/20180119135717_add_uploader_index_to_uploads.rb20
-rw-r--r--doc/development/file_storage.md104
-rw-r--r--lib/api/runner.rb6
-rw-r--r--lib/backup/artifacts.rb2
-rw-r--r--lib/gitlab/background_migration/populate_untracked_uploads.rb2
-rw-r--r--lib/gitlab/background_migration/prepare_untracked_uploads.rb9
-rw-r--r--lib/gitlab/gfm/uploads_rewriter.rb2
-rw-r--r--lib/gitlab/import_export/uploads_saver.rb8
-rw-r--r--lib/gitlab/uploads_transfer.rb2
-rw-r--r--lib/gitlab/workhorse.rb9
-rw-r--r--spec/controllers/groups/uploads_controller_spec.rb4
-rw-r--r--spec/controllers/projects/artifacts_controller_spec.rb3
-rw-r--r--spec/controllers/projects/raw_controller_spec.rb2
-rw-r--r--spec/controllers/uploads_controller_spec.rb13
-rw-r--r--spec/factories/notes.rb4
-rw-r--r--spec/factories/uploads.rb26
-rw-r--r--spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb53
-rw-r--r--spec/lib/gitlab/gfm/uploads_rewriter_spec.rb2
-rw-r--r--spec/lib/gitlab/import_export/uploads_restorer_spec.rb9
-rw-r--r--spec/lib/gitlab/import_export/uploads_saver_spec.rb4
-rw-r--r--spec/models/namespace_spec.rb2
-rw-r--r--spec/models/upload_spec.rb73
-rw-r--r--spec/requests/api/runner_spec.rb4
-rw-r--r--spec/requests/lfs_http_spec.rb4
-rw-r--r--spec/services/issues/move_service_spec.rb4
-rw-r--r--spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb4
-rw-r--r--spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb62
-rw-r--r--spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb48
-rw-r--r--spec/support/test_env.rb2
-rw-r--r--spec/support/track_untracked_uploads_helpers.rb2
-rw-r--r--spec/uploaders/attachment_uploader_spec.rb18
-rw-r--r--spec/uploaders/avatar_uploader_spec.rb18
-rw-r--r--spec/uploaders/file_mover_spec.rb18
-rw-r--r--spec/uploaders/file_uploader_spec.rb121
-rw-r--r--spec/uploaders/job_artifact_uploader_spec.rb32
-rw-r--r--spec/uploaders/legacy_artifact_uploader_spec.rb49
-rw-r--r--spec/uploaders/lfs_object_uploader_spec.rb38
-rw-r--r--spec/uploaders/namespace_file_uploader_spec.rb21
-rw-r--r--spec/uploaders/personal_file_uploader_spec.rb28
-rw-r--r--spec/uploaders/records_uploads_spec.rb73
-rw-r--r--spec/workers/upload_checksum_worker_spec.rb29
70 files changed, 910 insertions, 801 deletions
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index a6fb1f40001..61554029d09 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -1,6 +1,8 @@
module UploadsActions
include Gitlab::Utils::StrongMemoize
+ UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo).freeze
+
def create
link_to_file = UploadService.new(model, params[:file], uploader_class).execute
@@ -17,34 +19,71 @@ module UploadsActions
end
end
+ # This should either
+ # - send the file directly
+ # - or redirect to its URL
+ #
def show
return render_404 unless uploader.exists?
- disposition = uploader.image_or_video? ? 'inline' : 'attachment'
-
- expires_in 0.seconds, must_revalidate: true, private: true
+ if uploader.file_storage?
+ disposition = uploader.image_or_video? ? 'inline' : 'attachment'
+ expires_in 0.seconds, must_revalidate: true, private: true
- send_file uploader.file.path, disposition: disposition
+ send_file uploader.file.path, disposition: disposition
+ else
+ redirect_to uploader.url
+ end
end
private
+ def uploader_class
+ raise NotImplementedError
+ end
+
+ def upload_mount
+ mounted_as = params[:mounted_as]
+ mounted_as if UPLOAD_MOUNTS.include?(mounted_as)
+ end
+
+ def uploader_mounted?
+ upload_model_class < CarrierWave::Mount::Extension && !upload_mount.nil?
+ end
+
def uploader
strong_memoize(:uploader) do
- return if show_model.nil?
+ if uploader_mounted?
+ model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend
+ else
+ build_uploader_from_upload || build_uploader_from_params
+ end
+ end
+ end
- file_uploader = FileUploader.new(show_model, params[:secret])
- file_uploader.retrieve_from_store!(params[:filename])
+ def build_uploader_from_upload
+ return nil unless params[:secret] && params[:filename]
- file_uploader
- end
+ upload_path = uploader_class.upload_path(params[:secret], params[:filename])
+ upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_path)
+ upload&.build_uploader
+ end
+
+ def build_uploader_from_params
+ uploader = uploader_class.new(model, params[:secret])
+ uploader.retrieve_from_store!(params[:filename])
+ uploader
end
def image_or_video?
uploader && uploader.exists? && uploader.image_or_video?
end
- def uploader_class
- FileUploader
+ def find_model
+ nil
+ end
+
+ def model
+ strong_memoize(:model) { find_model }
end
end
diff --git a/app/controllers/groups/uploads_controller.rb b/app/controllers/groups/uploads_controller.rb
index e6bd9806401..f1578f75e88 100644
--- a/app/controllers/groups/uploads_controller.rb
+++ b/app/controllers/groups/uploads_controller.rb
@@ -7,29 +7,23 @@ class Groups::UploadsController < Groups::ApplicationController
private
- def show_model
- strong_memoize(:show_model) do
- group_id = params[:group_id]
-
- Group.find_by_full_path(group_id)
- end
+ def upload_model_class
+ Group
end
- def authorize_upload_file!
- render_404 unless can?(current_user, :upload_file, group)
+ def uploader_class
+ NamespaceFileUploader
end
- def uploader
- strong_memoize(:uploader) do
- file_uploader = uploader_class.new(show_model, params[:secret])
- file_uploader.retrieve_from_store!(params[:filename])
- file_uploader
- end
- end
+ def find_model
+ return @group if @group
- def uploader_class
- NamespaceFileUploader
+ group_id = params[:group_id]
+
+ Group.find_by_full_path(group_id)
end
- alias_method :model, :group
+ def authorize_upload_file!
+ render_404 unless can?(current_user, :upload_file, group)
+ end
end
diff --git a/app/controllers/projects/lfs_storage_controller.rb b/app/controllers/projects/lfs_storage_controller.rb
index 293869345bd..941638db427 100644
--- a/app/controllers/projects/lfs_storage_controller.rb
+++ b/app/controllers/projects/lfs_storage_controller.rb
@@ -60,7 +60,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
def store_file(oid, size, tmp_file)
# Define tmp_file_path early because we use it in "ensure"
- tmp_file_path = File.join("#{Gitlab.config.lfs.storage_path}/tmp/upload", tmp_file)
+ tmp_file_path = File.join(LfsObjectUploader.workhorse_upload_path, tmp_file)
object = LfsObject.find_or_create_by(oid: oid, size: size)
file_exists = object.file.exists? || move_tmp_file_to_storage(object, tmp_file_path)
diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb
index 4685bbe80b4..f5cf089ad98 100644
--- a/app/controllers/projects/uploads_controller.rb
+++ b/app/controllers/projects/uploads_controller.rb
@@ -1,6 +1,7 @@
class Projects::UploadsController < Projects::ApplicationController
include UploadsActions
+ # These will kick you out if you don't have access.
skip_before_action :project, :repository,
if: -> { action_name == 'show' && image_or_video? }
@@ -8,14 +9,20 @@ class Projects::UploadsController < Projects::ApplicationController
private
- def show_model
- strong_memoize(:show_model) do
- namespace = params[:namespace_id]
- id = params[:project_id]
+ def upload_model_class
+ Project
+ end
- Project.find_by_full_path("#{namespace}/#{id}")
- end
+ def uploader_class
+ FileUploader
end
- alias_method :model, :project
+ def find_model
+ return @project if @project
+
+ namespace = params[:namespace_id]
+ id = params[:project_id]
+
+ Project.find_by_full_path("#{namespace}/#{id}")
+ end
end
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index 16a74f82d3f..3d227b0a955 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -1,19 +1,34 @@
class UploadsController < ApplicationController
include UploadsActions
+ UnknownUploadModelError = Class.new(StandardError)
+
+ MODEL_CLASSES = {
+ "user" => User,
+ "project" => Project,
+ "note" => Note,
+ "group" => Group,
+ "appearance" => Appearance,
+ "personal_snippet" => PersonalSnippet,
+ nil => PersonalSnippet
+ }.freeze
+
+ rescue_from UnknownUploadModelError, with: :render_404
+
skip_before_action :authenticate_user!
+ before_action :upload_mount_satisfied?
before_action :find_model
before_action :authorize_access!, only: [:show]
before_action :authorize_create_access!, only: [:create]
- private
+ def uploader_class
+ PersonalFileUploader
+ end
def find_model
return nil unless params[:id]
- return render_404 unless upload_model && upload_mount
-
- @model = upload_model.find(params[:id])
+ upload_model_class.find(params[:id])
end
def authorize_access!
@@ -53,55 +68,17 @@ class UploadsController < ApplicationController
end
end
- def upload_model
- upload_models = {
- "user" => User,
- "project" => Project,
- "note" => Note,
- "group" => Group,
- "appearance" => Appearance,
- "personal_snippet" => PersonalSnippet
- }
-
- upload_models[params[:model]]
- end
-
- def upload_mount
- return true unless params[:mounted_as]
-
- upload_mounts = %w(avatar attachment file logo header_logo)
-
- if upload_mounts.include?(params[:mounted_as])
- params[:mounted_as]
- end
+ def upload_model_class
+ MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError)
end
- def uploader
- return @uploader if defined?(@uploader)
-
- case model
- when nil
- @uploader = PersonalFileUploader.new(nil, params[:secret])
-
- @uploader.retrieve_from_store!(params[:filename])
- when PersonalSnippet
- @uploader = PersonalFileUploader.new(model, params[:secret])
-
- @uploader.retrieve_from_store!(params[:filename])
- else
- @uploader = @model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend
-
- redirect_to @uploader.url unless @uploader.file_storage?
- end
-
- @uploader
+ def upload_model_class_has_mounts?
+ upload_model_class < CarrierWave::Mount::Extension
end
- def uploader_class
- PersonalFileUploader
- end
+ def upload_mount_satisfied?
+ return true unless upload_model_class_has_mounts?
- def model
- @model ||= find_model
+ upload_model_class.uploader_options.has_key?(upload_mount)
end
end
diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index 76cfe28742a..dcd14c08f3c 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -11,6 +11,7 @@ class Appearance < ActiveRecord::Base
mount_uploader :logo, AttachmentUploader
mount_uploader :header_logo, AttachmentUploader
+
has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
CACHE_KEY = 'current_appearance'.freeze
diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb
index 10659030910..d35e37935fb 100644
--- a/app/models/concerns/avatarable.rb
+++ b/app/models/concerns/avatarable.rb
@@ -1,6 +1,30 @@
module Avatarable
extend ActiveSupport::Concern
+ included do
+ prepend ShadowMethods
+
+ validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
+ validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
+
+ mount_uploader :avatar, AvatarUploader
+ end
+
+ module ShadowMethods
+ def avatar_url(**args)
+ # We use avatar_path instead of overriding avatar_url because of carrierwave.
+ # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
+
+ avatar_path(only_path: args.fetch(:only_path, true)) || super
+ end
+ end
+
+ def avatar_type
+ unless self.avatar.image?
+ self.errors.add :avatar, "only images allowed"
+ end
+ end
+
def avatar_path(only_path: true)
return unless self[:avatar].present?
diff --git a/app/models/group.rb b/app/models/group.rb
index fddace03387..415507a6aea 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -29,17 +29,14 @@ class Group < Namespace
has_many :variables, class_name: 'Ci::GroupVariable'
has_many :custom_attributes, class_name: 'GroupCustomAttribute'
- validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
+ has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+
validate :visibility_level_allowed_by_projects
validate :visibility_level_allowed_by_sub_groups
validate :visibility_level_allowed_by_parent
- validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
-
validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
- mount_uploader :avatar, AvatarUploader
- has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
after_create :post_create_hook
after_destroy :post_destroy_hook
@@ -116,12 +113,6 @@ class Group < Namespace
visibility_level_allowed_by_sub_groups?(level)
end
- def avatar_url(**args)
- # We use avatar_path instead of overriding avatar_url because of carrierwave.
- # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
- avatar_path(args)
- end
-
def lfs_enabled?
return false unless Gitlab.config.lfs.enabled
return Gitlab.config.lfs.enabled if self[:lfs_enabled].nil?
diff --git a/app/models/note.rb b/app/models/note.rb
index 184fbd5f5ae..a84db8982e5 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -88,6 +88,7 @@ class Note < ActiveRecord::Base
end
end
+ # @deprecated attachments are handler by the MarkdownUploader
mount_uploader :attachment, AttachmentUploader
# Scopes
diff --git a/app/models/project.rb b/app/models/project.rb
index 4def590a7a9..90f5df6265d 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -256,9 +256,6 @@ class Project < ActiveRecord::Base
validates :star_count, numericality: { greater_than_or_equal_to: 0 }
validate :check_limit, on: :create
validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? }
- validate :avatar_type,
- if: ->(project) { project.avatar.present? && project.avatar_changed? }
- validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
validate :visibility_level_allowed_by_group
validate :visibility_level_allowed_as_fork
validate :check_wiki_path_conflict
@@ -266,7 +263,6 @@ class Project < ActiveRecord::Base
presence: true,
inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
- mount_uploader :avatar, AvatarUploader
has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
# Scopes
@@ -289,7 +285,6 @@ class Project < ActiveRecord::Base
scope :non_archived, -> { where(archived: false) }
scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }
scope :with_push, -> { joins(:events).where('events.action = ?', Event::PUSHED) }
-
scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
scope :with_statistics, -> { includes(:statistics) }
scope :with_shared_runners, -> { where(shared_runners_enabled: true) }
@@ -923,20 +918,12 @@ class Project < ActiveRecord::Base
issues_tracker.to_param == 'jira'
end
- def avatar_type
- unless self.avatar.image?
- self.errors.add :avatar, 'only images allowed'
- end
- end
-
def avatar_in_git
repository.avatar
end
def avatar_url(**args)
- # We use avatar_path instead of overriding avatar_url because of carrierwave.
- # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
- avatar_path(args) || (Gitlab::Routing.url_helpers.project_avatar_url(self) if avatar_in_git)
+ Gitlab::Routing.url_helpers.project_avatar_url(self) if avatar_in_git
end
# For compatibility with old code
diff --git a/app/models/upload.rb b/app/models/upload.rb
index f194d7bdb80..a9f144d2fa2 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -9,22 +9,11 @@ class Upload < ActiveRecord::Base
validates :model, presence: true
validates :uploader, presence: true
- before_save :calculate_checksum, if: :foreground_checksum?
- after_commit :schedule_checksum, unless: :foreground_checksum?
+ before_save :calculate_checksum!, if: :foreground_checksummable?
+ after_commit :schedule_checksum, if: :checksummable?
- def self.remove_path(path)
- where(path: path).destroy_all
- end
-
- def self.record(uploader)
- remove_path(uploader.relative_path)
-
- create(
- size: uploader.file.size,
- path: uploader.relative_path,
- model: uploader.model,
- uploader: uploader.class.to_s
- )
+ def self.hexdigest(path)
+ Digest::SHA256.file(path).hexdigest
end
def absolute_path
@@ -33,10 +22,18 @@ class Upload < ActiveRecord::Base
uploader_class.absolute_path(self)
end
- def calculate_checksum
- return unless exist?
+ def calculate_checksum!
+ self.checksum = nil
+ return unless checksummable?
- self.checksum = Digest::SHA256.file(absolute_path).hexdigest
+ self.checksum = self.class.hexdigest(absolute_path)
+ end
+
+ def build_uploader
+ uploader_class.new(model).tap do |uploader|
+ uploader.upload = self
+ uploader.retrieve_from_store!(identifier)
+ end
end
def exist?
@@ -45,8 +42,16 @@ class Upload < ActiveRecord::Base
private
- def foreground_checksum?
- size <= CHECKSUM_THRESHOLD
+ def checksummable?
+ checksum.nil? && local? && exist?
+ end
+
+ def local?
+ return true
+ end
+
+ def foreground_checksummable?
+ checksummable? && size <= CHECKSUM_THRESHOLD
end
def schedule_checksum
@@ -57,6 +62,10 @@ class Upload < ActiveRecord::Base
!path.start_with?('/')
end
+ def identifier
+ File.basename(path)
+ end
+
def uploader_class
Object.const_get(uploader)
end
diff --git a/app/models/user.rb b/app/models/user.rb
index fb5d56a68b0..89e787c3274 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -137,6 +137,7 @@ class User < ActiveRecord::Base
has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent
has_many :custom_attributes, class_name: 'UserCustomAttribute'
+ has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
#
# Validations
@@ -159,12 +160,10 @@ class User < ActiveRecord::Base
validate :namespace_uniq, if: :username_changed?
validate :namespace_move_dir_allowed, if: :username_changed?
- validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
validate :unique_email, if: :email_changed?
validate :owns_notification_email, if: :notification_email_changed?
validate :owns_public_email, if: :public_email_changed?
validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
- validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
before_validation :sanitize_attrs
before_validation :set_notification_email, if: :email_changed?
@@ -225,9 +224,6 @@ class User < ActiveRecord::Base
end
end
- mount_uploader :avatar, AvatarUploader
- has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
# Scopes
scope :admins, -> { where(admin: true) }
scope :blocked, -> { with_states(:blocked, :ldap_blocked) }
@@ -527,12 +523,6 @@ class User < ActiveRecord::Base
end
end
- def avatar_type
- unless avatar.image?
- errors.add :avatar, "only images allowed"
- end
- end
-
def unique_email
if !emails.exists?(email: email) && Email.exists?(email: email)
errors.add(:email, 'has already been taken')
@@ -860,9 +850,7 @@ class User < ActiveRecord::Base
end
def avatar_url(size: nil, scale: 2, **args)
- # We use avatar_path instead of overriding avatar_url because of carrierwave.
- # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
- avatar_path(args) || GravatarService.new.execute(email, size, scale, username: username)
+ GravatarService.new.execute(email, size, scale, username: username)
end
def primary_email_verified?
diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb
index f8aaec8a9c0..bc897d891d5 100644
--- a/app/services/projects/hashed_storage/migrate_attachments_service.rb
+++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb
@@ -14,9 +14,9 @@ module Projects
@old_path = project.full_path
@new_path = project.disk_path
- origin = FileUploader.dynamic_path_segment(project)
+ origin = FileUploader.absolute_base_dir(project)
project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments]
- target = FileUploader.dynamic_path_segment(project)
+ target = FileUploader.absolute_base_dir(project)
result = move_folder!(origin, target)
project.save!
diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb
index 109eb2fea0b..4930fb2fca7 100644
--- a/app/uploaders/attachment_uploader.rb
+++ b/app/uploaders/attachment_uploader.rb
@@ -1,10 +1,12 @@
class AttachmentUploader < GitlabUploader
- include RecordsUploads
include UploaderHelper
+ include RecordsUploads::Concern
storage :file
- def store_dir
- "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
+ private
+
+ def dynamic_segment
+ File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s)
end
end
diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb
index cbb79376d5f..5c8e1cea62e 100644
--- a/app/uploaders/avatar_uploader.rb
+++ b/app/uploaders/avatar_uploader.rb
@@ -1,25 +1,24 @@
class AvatarUploader < GitlabUploader
- include RecordsUploads
include UploaderHelper
+ include RecordsUploads::Concern
storage :file
- def store_dir
- "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
- end
-
def exists?
model.avatar.file && model.avatar.file.present?
end
- # We set move_to_store and move_to_cache to 'false' to prevent stealing
- # the avatar file from a project when forking it.
- # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158
- def move_to_store
+ def move_to_cache
false
end
- def move_to_cache
+ def move_to_store
false
end
+
+ private
+
+ def dynamic_segment
+ File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s)
+ end
end
diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb
index 00c2888d224..e7af1483d23 100644
--- a/app/uploaders/file_mover.rb
+++ b/app/uploaders/file_mover.rb
@@ -21,7 +21,8 @@ class FileMover
end
def update_markdown
- updated_text = model.read_attribute(update_field).gsub(temp_file_uploader.to_markdown, uploader.to_markdown)
+ updated_text = model.read_attribute(update_field)
+ .gsub(temp_file_uploader.markdown_link, uploader.markdown_link)
model.update_attribute(update_field, updated_text)
true
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 0b591e3bbbb..85ae9863b13 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -1,23 +1,38 @@
+# This class breaks the actual CarrierWave concept.
+# Every uploader should use a base_dir that is model agnostic so we can build
+# back URLs from base_dir-relative paths saved in the `Upload` model.
+#
+# As the `.base_dir` is model dependent and **not** saved in the upload model (see #upload_path)
+# there is no way to build back the correct file path without the model, which defies
+# CarrierWave way of storing files.
+#
class FileUploader < GitlabUploader
- include RecordsUploads
include UploaderHelper
+ include RecordsUploads::Concern
MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
+ DYNAMIC_PATH_PATTERN = %r{(?<secret>\h{32})/(?<identifier>.*)}
storage :file
- def self.absolute_path(upload_record)
+ def self.root
+ File.join(options.storage_path, 'uploads')
+ end
+
+ def self.absolute_path(upload)
File.join(
- self.dynamic_path_segment(upload_record.model),
- upload_record.path
+ absolute_base_dir(upload.model),
+ upload.path # already contain the dynamic_segment, see #upload_path
)
end
- # Not using `GitlabUploader.base_dir` because all project namespaces are in
- # the `public/uploads` dir.
- #
- def self.base_dir
- root_dir
+ def self.base_dir(model)
+ model_path_segment(model)
+ end
+
+ # used in migrations and import/exports
+ def self.absolute_base_dir(model)
+ File.join(root, base_dir(model))
end
# Returns the part of `store_dir` that can change based on the model's current
@@ -29,63 +44,96 @@ class FileUploader < GitlabUploader
# model - Object that responds to `full_path` and `disk_path`
#
# Returns a String without a trailing slash
- def self.dynamic_path_segment(model)
+ def self.model_path_segment(model)
if model.hashed_storage?(:attachments)
- dynamic_path_builder(model.disk_path)
+ model.disk_path
else
- dynamic_path_builder(model.full_path)
+ model.full_path
end
end
- # Auxiliary method to build dynamic path segment when not using a project model
- #
- # Prefer to use the `.dynamic_path_segment` as it includes Hashed Storage specific logic
- def self.dynamic_path_builder(path)
- File.join(CarrierWave.root, base_dir, path)
+ def self.upload_path(secret, identifier)
+ File.join(secret, identifier)
+ end
+
+ def self.generate_secret
+ SecureRandom.hex
end
attr_accessor :model
- attr_reader :secret
def initialize(model, secret = nil)
@model = model
- @secret = secret || generate_secret
+ @secret = secret
end
- def store_dir
- File.join(dynamic_path_segment, @secret)
+ def base_dir
+ self.class.base_dir(@model)
end
- def relative_path
- self.file.path.sub("#{dynamic_path_segment}/", '')
+ # we don't need to know the actual path, an uploader instance should be
+ # able to yield the file content on demand, so we should build the digest
+ def absolute_path
+ self.class.absolute_path(@upload)
end
- def to_markdown
- to_h[:markdown]
+ def upload_path
+ self.class.upload_path(dynamic_segment, identifier)
end
- def to_h
- filename = image_or_video? ? self.file.basename : self.file.filename
- escaped_filename = filename.gsub("]", "\\]")
+ def model_path_segment
+ self.class.model_path_segment(@model)
+ end
+
+ def store_dir
+ File.join(base_dir, dynamic_segment)
+ end
- markdown = "[#{escaped_filename}](#{secure_url})"
+ def markdown_link
+ markdown = "[#{markdown_name}](#{secure_url})"
markdown.prepend("!") if image_or_video? || dangerous?
+ markdown
+ end
+ def to_h
{
- alt: filename,
+ alt: markdown_name,
url: secure_url,
- markdown: markdown
+ markdown: markdown_link
}
end
+ def filename
+ self.file.filename
+ end
+
+ # the upload does not hold the secret, but holds the path
+ # which contains the secret: extract it
+ def upload=(value)
+ if matches = DYNAMIC_PATH_PATTERN.match(value.path)
+ @secret = matches[:secret]
+ @identifier = matches[:identifier]
+ end
+
+ super
+ end
+
+ def secret
+ @secret ||= self.class.generate_secret
+ end
+
private
- def dynamic_path_segment
- self.class.dynamic_path_segment(model)
+ def markdown_name
+ (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]")
end
- def generate_secret
- SecureRandom.hex
+ def identifier
+ @identifier ||= filename
+ end
+
+ def dynamic_segment
+ secret
end
def secure_url
diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb
index 7f72b3ce471..be8ab05b036 100644
--- a/app/uploaders/gitlab_uploader.rb
+++ b/app/uploaders/gitlab_uploader.rb
@@ -1,28 +1,32 @@
class GitlabUploader < CarrierWave::Uploader::Base
- def self.absolute_path(upload_record)
- File.join(CarrierWave.root, upload_record.path)
- end
+ class_attribute :options
- def self.root_dir
- 'uploads'
- end
+ class << self
+ # DSL setter
+ def storage_options(options)
+ self.options = options
+ end
- # When object storage is used, keep the `root_dir` as `base_dir`.
- # The files aren't really in folders there, they just have a name.
- # The files that contain user input in their name, also contain a hash, so
- # the names are still unique
- #
- # This method is overridden in the `FileUploader`
- def self.base_dir
- return root_dir unless file_storage?
+ def root
+ options.storage_path
+ end
- File.join(root_dir, '-', 'system')
- end
+ # represent the directory namespacing at the class level
+ def base_dir
+ options.fetch('base_dir', '')
+ end
- def self.file_storage?
- self.storage == CarrierWave::Storage::File
+ def file_storage?
+ storage == CarrierWave::Storage::File
+ end
+
+ def absolute_path(upload_record)
+ File.join(root, upload_record.path)
+ end
end
+ storage_options Gitlab.config.uploads
+
delegate :base_dir, :file_storage?, to: :class
def file_cache_storage?
@@ -31,34 +35,28 @@ class GitlabUploader < CarrierWave::Uploader::Base
# Reduce disk IO
def move_to_cache
- true
+ super || true
end
# Reduce disk IO
def move_to_store
- true
- end
-
- # Designed to be overridden by child uploaders that have a dynamic path
- # segment -- that is, a path that changes based on mutable attributes of its
- # associated model
- #
- # For example, `FileUploader` builds the storage path based on the associated
- # project model's `path_with_namespace` value, which can change when the
- # project or its containing namespace is moved or renamed.
- def relative_path
- self.file.path.sub("#{root}/", '')
+ super || true
end
def exists?
file.present?
end
- # Override this if you don't want to save files by default to the Rails.root directory
+ def store_dir
+ File.join(base_dir, dynamic_segment)
+ end
+
+ def cache_dir
+ File.join(root, base_dir, 'tmp/cache')
+ end
+
def work_dir
- # Default path set by CarrierWave:
- # https://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L182
- CarrierWave.tmp_path
+ File.join(root, base_dir, 'tmp/work')
end
def filename
@@ -67,6 +65,13 @@ class GitlabUploader < CarrierWave::Uploader::Base
private
+ # Designed to be overridden by child uploaders that have a dynamic path
+ # segment -- that is, a path that changes based on mutable attributes of its
+ # associated model
+ def dynamic_segment
+ raise(NotImplementedError)
+ end
+
# To prevent files from moving across filesystems, override the default
# implementation:
# http://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L181-L183
@@ -74,6 +79,6 @@ class GitlabUploader < CarrierWave::Uploader::Base
# To be safe, keep this directory outside of the the cache directory
# because calling CarrierWave.clean_cache_files! will remove any files in
# the cache directory.
- File.join(work_dir, @cache_id, version_name.to_s, for_file)
+ File.join(work_dir, cache_id, version_name.to_s, for_file)
end
end
diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb
index 15dfb5a5763..0abb462ab7d 100644
--- a/app/uploaders/job_artifact_uploader.rb
+++ b/app/uploaders/job_artifact_uploader.rb
@@ -1,13 +1,7 @@
class JobArtifactUploader < GitlabUploader
- storage :file
+ extend Workhorse::UploadPath
- def self.local_store_path
- Gitlab.config.artifacts.path
- end
-
- def self.artifacts_upload_path
- File.join(self.local_store_path, 'tmp/uploads/')
- end
+ storage_options Gitlab.config.artifacts
def size
return super if model.size.nil?
@@ -16,24 +10,12 @@ class JobArtifactUploader < GitlabUploader
end
def store_dir
- default_local_path
- end
-
- def cache_dir
- File.join(self.class.local_store_path, 'tmp/cache')
- end
-
- def work_dir
- File.join(self.class.local_store_path, 'tmp/work')
+ dynamic_segment
end
private
- def default_local_path
- File.join(self.class.local_store_path, default_path)
- end
-
- def default_path
+ def dynamic_segment
creation_date = model.created_at.utc.strftime('%Y_%m_%d')
File.join(disk_hash[0..1], disk_hash[2..3], disk_hash,
diff --git a/app/uploaders/legacy_artifact_uploader.rb b/app/uploaders/legacy_artifact_uploader.rb
index 4f7f8a63108..28c458d3ff1 100644
--- a/app/uploaders/legacy_artifact_uploader.rb
+++ b/app/uploaders/legacy_artifact_uploader.rb
@@ -1,33 +1,15 @@
class LegacyArtifactUploader < GitlabUploader
- storage :file
+ extend Workhorse::UploadPath
- def self.local_store_path
- Gitlab.config.artifacts.path
- end
-
- def self.artifacts_upload_path
- File.join(self.local_store_path, 'tmp/uploads/')
- end
+ storage_options Gitlab.config.artifacts
def store_dir
- default_local_path
- end
-
- def cache_dir
- File.join(self.class.local_store_path, 'tmp/cache')
- end
-
- def work_dir
- File.join(self.class.local_store_path, 'tmp/work')
+ dynamic_segment
end
private
- def default_local_path
- File.join(self.class.local_store_path, default_path)
- end
-
- def default_path
+ def dynamic_segment
File.join(model.created_at.utc.strftime('%Y_%m'), model.project_id.to_s, model.id.to_s)
end
end
diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb
index d11ebf0f9ca..e04c97ce179 100644
--- a/app/uploaders/lfs_object_uploader.rb
+++ b/app/uploaders/lfs_object_uploader.rb
@@ -1,19 +1,24 @@
class LfsObjectUploader < GitlabUploader
- storage :file
+ extend Workhorse::UploadPath
- def store_dir
- "#{Gitlab.config.lfs.storage_path}/#{model.oid[0, 2]}/#{model.oid[2, 2]}"
+ # LfsObject are in `tmp/upload` instead of `tmp/uploads`
+ def self.workhorse_upload_path
+ File.join(root, 'tmp/upload')
end
- def cache_dir
- "#{Gitlab.config.lfs.storage_path}/tmp/cache"
- end
+ storage_options Gitlab.config.lfs
def filename
model.oid[4..-1]
end
- def work_dir
- File.join(Gitlab.config.lfs.storage_path, 'tmp', 'work')
+ def store_dir
+ dynamic_segment
+ end
+
+ private
+
+ def dynamic_segment
+ File.join(model.oid[0, 2], model.oid[2, 2])
end
end
diff --git a/app/uploaders/namespace_file_uploader.rb b/app/uploaders/namespace_file_uploader.rb
index 672126e9ec2..993e85fbc13 100644
--- a/app/uploaders/namespace_file_uploader.rb
+++ b/app/uploaders/namespace_file_uploader.rb
@@ -1,15 +1,19 @@
class NamespaceFileUploader < FileUploader
- def self.base_dir
- File.join(root_dir, '-', 'system', 'namespace')
+ # Re-Override
+ def self.root
+ options.storage_path
end
- def self.dynamic_path_segment(model)
- dynamic_path_builder(model.id.to_s)
+ def self.base_dir(model)
+ File.join(options.base_dir, 'namespace', model_path_segment(model))
end
- private
+ def self.model_path_segment(model)
+ File.join(model.id.to_s)
+ end
- def secure_url
- File.join('/uploads', @secret, file.filename)
+ # Re-Override
+ def store_dir
+ File.join(base_dir, dynamic_segment)
end
end
diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb
index 3298ad104ec..e7d9ecd3222 100644
--- a/app/uploaders/personal_file_uploader.rb
+++ b/app/uploaders/personal_file_uploader.rb
@@ -1,23 +1,27 @@
class PersonalFileUploader < FileUploader
- def self.dynamic_path_segment(model)
- File.join(CarrierWave.root, model_path(model))
+ # Re-Override
+ def self.root
+ options.storage_path
end
- def self.base_dir
- File.join(root_dir, '-', 'system')
+ def self.base_dir(model)
+ File.join(options.base_dir, model_path_segment(model))
end
- private
+ def self.model_path_segment(model)
+ return 'temp/' unless model
- def secure_url
- File.join(self.class.model_path(model), secret, file.filename)
+ File.join(model.class.to_s.underscore, model.id.to_s)
+ end
+
+ # Revert-Override
+ def store_dir
+ File.join(base_dir, dynamic_segment)
end
- def self.model_path(model)
- if model
- File.join("/#{base_dir}", model.class.to_s.underscore, model.id.to_s)
- else
- File.join("/#{base_dir}", 'temp')
- end
+ private
+
+ def secure_url
+ File.join('/', base_dir, secret, file.filename)
end
end
diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb
index feb4f04d7b7..dfb8dccec57 100644
--- a/app/uploaders/records_uploads.rb
+++ b/app/uploaders/records_uploads.rb
@@ -1,35 +1,61 @@
module RecordsUploads
- extend ActiveSupport::Concern
+ module Concern
+ extend ActiveSupport::Concern
- included do
- after :store, :record_upload
- before :remove, :destroy_upload
- end
+ attr_accessor :upload
- # After storing an attachment, create a corresponding Upload record
- #
- # NOTE: We're ignoring the argument passed to this callback because we want
- # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the
- # `Tempfile` object the callback gets.
- #
- # Called `after :store`
- def record_upload(_tempfile = nil)
- return unless model
- return unless file_storage?
- return unless file.exists?
-
- Upload.record(self)
- end
+ included do
+ after :store, :record_upload
+ before :remove, :destroy_upload
+ end
+
+ # After storing an attachment, create a corresponding Upload record
+ #
+ # NOTE: We're ignoring the argument passed to this callback because we want
+ # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the
+ # `Tempfile` object the callback gets.
+ #
+ # Called `after :store`
+ def record_upload(_tempfile = nil)
+ return unless model
+ return unless file && file.exists?
+
+ Upload.transaction do
+ uploads.where(path: upload_path).delete_all
+ upload.destroy! if upload
+
+ self.upload = build_upload_from_uploader(self)
+ upload.save!
+ end
+ end
+
+ def upload_path
+ File.join(store_dir, filename.to_s)
+ end
+
+ private
+
+ def uploads
+ Upload.order(id: :desc).where(uploader: self.class.to_s)
+ end
- private
+ def build_upload_from_uploader(uploader)
+ Upload.new(
+ size: uploader.file.size,
+ path: uploader.upload_path,
+ model: uploader.model,
+ uploader: uploader.class.to_s
+ )
+ end
- # Before removing an attachment, destroy any Upload records at the same path
- #
- # Called `before :remove`
- def destroy_upload(*args)
- return unless file_storage?
- return unless file
+ # Before removing an attachment, destroy any Upload records at the same path
+ #
+ # Called `before :remove`
+ def destroy_upload(*args)
+ return unless file && file.exists?
- Upload.remove_path(relative_path)
+ self.upload = nil
+ uploads.where(path: upload_path).delete_all
+ end
end
end
diff --git a/app/uploaders/uploader_helper.rb b/app/uploaders/uploader_helper.rb
index 7635c20ab3a..fd446d31092 100644
--- a/app/uploaders/uploader_helper.rb
+++ b/app/uploaders/uploader_helper.rb
@@ -32,14 +32,7 @@ module UploaderHelper
def extension_match?(extensions)
return false unless file
- extension =
- if file.respond_to?(:extension)
- file.extension
- else
- # Not all CarrierWave storages respond to :extension
- File.extname(file.path).delete('.')
- end
-
+ extension = file.try(:extension) || File.extname(file.path).delete('.')
extensions.include?(extension.downcase)
end
end
diff --git a/app/uploaders/workhorse.rb b/app/uploaders/workhorse.rb
new file mode 100644
index 00000000000..782032cf516
--- /dev/null
+++ b/app/uploaders/workhorse.rb
@@ -0,0 +1,7 @@
+module Workhorse
+ module UploadPath
+ def workhorse_upload_path
+ File.join(root, base_dir, 'tmp/uploads')
+ end
+ end
+end
diff --git a/app/workers/upload_checksum_worker.rb b/app/workers/upload_checksum_worker.rb
index 9222760c031..65d40336f18 100644
--- a/app/workers/upload_checksum_worker.rb
+++ b/app/workers/upload_checksum_worker.rb
@@ -3,7 +3,7 @@ class UploadChecksumWorker
def perform(upload_id)
upload = Upload.find(upload_id)
- upload.calculate_checksum
+ upload.calculate_checksum!
upload.save!
rescue ActiveRecord::RecordNotFound
Rails.logger.error("UploadChecksumWorker: couldn't find upload #{upload_id}, skipping")
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index 25f4085deb2..33230b9355d 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -152,6 +152,12 @@ production: &base
# The location where LFS objects are stored (default: shared/lfs-objects).
# storage_path: shared/lfs-objects
+ ## Uploads (attachments, avatars, etc...)
+ uploads:
+ # The location where uploads objects are stored (default: public/).
+ # storage_path: public/
+ # base_dir: uploads/-/system
+
## GitLab Pages
pages:
enabled: false
@@ -644,6 +650,8 @@ test:
enabled: false
artifacts:
path: tmp/tests/artifacts
+ uploads:
+ storage_path: tmp/tests/public
gitlab:
host: localhost
port: 80
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 5b4e6b5db88..5ad46d47cb6 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -300,8 +300,10 @@ Settings.incoming_email['enabled'] = false if Settings.incoming_email['enabled']
#
Settings['artifacts'] ||= Settingslogic.new({})
Settings.artifacts['enabled'] = true if Settings.artifacts['enabled'].nil?
-Settings.artifacts['path'] = Settings.absolute(Settings.artifacts['path'] || File.join(Settings.shared['path'], "artifacts"))
-Settings.artifacts['max_size'] ||= 100 # in megabytes
+Settings.artifacts['storage_path'] = Settings.absolute(Settings.artifacts.values_at('path', 'storage_path').compact.first || File.join(Settings.shared['path'], "artifacts"))
+# Settings.artifact['path'] is deprecated, use `storage_path` instead
+Settings.artifacts['path'] = Settings.artifacts['storage_path']
+Settings.artifacts['max_size'] ||= 100 # in megabytes
#
# Registry
@@ -339,6 +341,13 @@ Settings.lfs['enabled'] = true if Settings.lfs['enabled'].nil?
Settings.lfs['storage_path'] = Settings.absolute(Settings.lfs['storage_path'] || File.join(Settings.shared['path'], "lfs-objects"))
#
+# Uploads
+#
+Settings['uploads'] ||= Settingslogic.new({})
+Settings.uploads['storage_path'] = Settings.absolute(Settings.uploads['storage_path'] || 'public')
+Settings.uploads['base_dir'] = Settings.uploads['base_dir'] || 'uploads/-/system'
+
+#
# Mattermost
#
Settings['mattermost'] ||= Settingslogic.new({})
diff --git a/db/migrate/20180119135717_add_uploader_index_to_uploads.rb b/db/migrate/20180119135717_add_uploader_index_to_uploads.rb
new file mode 100644
index 00000000000..a678c3d049f
--- /dev/null
+++ b/db/migrate/20180119135717_add_uploader_index_to_uploads.rb
@@ -0,0 +1,20 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddUploaderIndexToUploads < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ remove_concurrent_index :uploads, :path
+ add_concurrent_index :uploads, [:uploader, :path], using: :btree
+ end
+
+ def down
+ remove_concurrent_index :uploads, [:uploader, :path]
+ add_concurrent_index :uploads, :path, using: :btree
+ end
+end
diff --git a/doc/development/file_storage.md b/doc/development/file_storage.md
index cf00e24e11a..76354b92820 100644
--- a/doc/development/file_storage.md
+++ b/doc/development/file_storage.md
@@ -14,8 +14,8 @@ There are many places where file uploading is used, according to contexts:
- User snippet attachments
* Project
- Project avatars
- - Issues/MR Markdown attachments
- - Issues/MR Legacy Markdown attachments
+ - Issues/MR/Notes Markdown attachments
+ - Issues/MR/Notes Legacy Markdown attachments
- CI Build Artifacts
- LFS Objects
@@ -25,7 +25,7 @@ There are many places where file uploading is used, according to contexts:
GitLab started saving everything on local disk. While directory location changed from previous versions,
they are still not 100% standardized. You can see them below:
-| Description | In DB? | Relative path | Uploader class | model_type |
+| Description | In DB? | Relative path (from CarrierWave.root) | Uploader class | model_type |
| ------------------------------------- | ------ | ----------------------------------------------------------- | ---------------------- | ---------- |
| Instance logo | yes | uploads/-/system/appearance/logo/:id/:filename | `AttachmentUploader` | Appearance |
| Header logo | yes | uploads/-/system/appearance/header_logo/:id/:filename | `AttachmentUploader` | Appearance |
@@ -33,17 +33,107 @@ they are still not 100% standardized. You can see them below:
| User avatars | yes | uploads/-/system/user/avatar/:id/:filename | `AvatarUploader` | User |
| User snippet attachments | yes | uploads/-/system/personal_snippet/:id/:random_hex/:filename | `PersonalFileUploader` | Snippet |
| Project avatars | yes | uploads/-/system/project/avatar/:id/:filename | `AvatarUploader` | Project |
-| Issues/MR Markdown attachments | yes | uploads/:project_path_with_namespace/:random_hex/:filename | `FileUploader` | Project |
-| Issues/MR Legacy Markdown attachments | no | uploads/-/system/note/attachment/:id/:filename | `AttachmentUploader` | Note |
+| Issues/MR/Notes Markdown attachments | yes | uploads/:project_path_with_namespace/:random_hex/:filename | `FileUploader` | Project |
+| Issues/MR/Notes Legacy Markdown attachments | no | uploads/-/system/note/attachment/:id/:filename | `AttachmentUploader` | Note |
| CI Artifacts (CE) | yes | shared/artifacts/:year_:month/:project_id/:id | `ArtifactUploader` | Ci::Build |
| LFS Objects (CE) | yes | shared/lfs-objects/:hex/:hex/:object_hash | `LfsObjectUploader` | LfsObject |
CI Artifacts and LFS Objects behave differently in CE and EE. In CE they inherit the `GitlabUploader`
-while in EE they inherit the `ObjectStoreUploader` and store files in and S3 API compatible object store.
+while in EE they inherit the `ObjectStorage` and store files in and S3 API compatible object store.
-In the case of Issues/MR Markdown attachments, there is a different approach using the [Hashed Storage] layout,
+In the case of Issues/MR/Notes Markdown attachments, there is a different approach using the [Hashed Storage] layout,
instead of basing the path into a mutable variable `:project_path_with_namespace`, it's possible to use the
hash of the project ID instead, if project migrates to the new approach (introduced in 10.2).
+### Path segments
+
+Files are stored at multiple locations and use different path schemes.
+All the `GitlabUploader` derived classes should comply with this path segment schema:
+
+```
+| GitlabUploader
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `<gitlab_root>/public/` | `uploads/-/system/` | `user/avatar/:id/` | `:filename` |
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `CarrierWave.root` | `GitlabUploader.base_dir` | `GitlabUploader#dynamic_segment` | `CarrierWave::Uploader#filename` |
+| | `CarrierWave::Uploader#store_dir` | |
+
+| FileUploader
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `<gitlab_root>/shared/` | `artifacts/` | `:year_:month/:id` | `:filename` |
+| `<gitlab_root>/shared/` | `snippets/` | `:secret/` | `:filename` |
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `CarrierWave.root` | `GitlabUploader.base_dir` | `GitlabUploader#dynamic_segment` | `CarrierWave::Uploader#filename` |
+| | `CarrierWave::Uploader#store_dir` | |
+| | | `FileUploader#upload_path |
+
+| ObjectStore::Concern (store = remote)
+| ----------------------- + ------------------------- + ----------------------------------- + -------------------------------- |
+| `<bucket_name>` | <ignored> | `user/avatar/:id/` | `:filename` |
+| ----------------------- + ------------------------- + ----------------------------------- + -------------------------------- |
+| `#fog_dir` | `GitlabUploader.base_dir` | `GitlabUploader#dynamic_segment` | `CarrierWave::Uploader#filename` |
+| | | `ObjectStorage::Concern#store_dir` | |
+| | | `ObjectStorage::Concern#upload_path |
+```
+
+The `RecordsUploads::Concern` concern will create an `Upload` entry for every file stored by a `GitlabUploader` persisting the dynamic parts of the path using
+`GitlabUploader#dynamic_path`. You may then use the `Upload#build_uploader` method to manipulate the file.
+
+## Object Storage
+
+By including the `ObjectStorage::Concern` in the `GitlabUploader` derived class, you may enable the object storage for this uploader. To enable the object storage
+in your uploader, you need to either 1) include `RecordsUpload::Concern` and prepend `ObjectStorage::Extension::RecordsUploads` or 2) mount the uploader and create a new field named `<mount>_store`.
+
+The `CarrierWave::Uploader#store_dir` is overriden to
+
+ - `GitlabUploader.base_dir` + `GitlabUploader.dynamic_segment` when the store is LOCAL
+ - `GitlabUploader.dynamic_segment` when the store is REMOTE (the bucket name is used to namespace)
+
+### Using `ObjectStorage::Extension::RecordsUploads`
+
+> Note: this concern will automatically include `RecordsUploads::Concern` if not already included.
+
+The `ObjectStorage::Concern` uploader will search for the matching `Upload` to select the correct object store. The `Upload` is mapped using `#store_dirs + identifier` for each store (LOCAL/REMOTE).
+
+```ruby
+class SongUploader < GitlabUploader
+ include RecordsUploads::Concern
+ include ObjectStorage::Concern
+ prepend ObjectStorage::Extension::RecordsUploads
+
+ ...
+end
+
+class Thing < ActiveRecord::Base
+ mount :theme, SongUploader # we have a great theme song!
+
+ ...
+end
+```
+
+### Using a mounted uploader
+
+The `ObjectStorage::Concern` will query the `model.<mount>_store` attribute to select the correct object store.
+This column must be present in the model schema.
+
+```ruby
+class SongUploader < GitlabUploader
+ include ObjectStorage::Concern
+
+ ...
+end
+
+class Thing < ActiveRecord::Base
+ attr_reader :theme_store # this is an ActiveRecord attribute
+ mount :theme, SongUploader # we have a great theme song!
+
+ def theme_store
+ super || ObjectStorage::Store::LOCAL
+ end
+
+ ...
+end
+```
+
[CarrierWave]: https://github.com/carrierwaveuploader/carrierwave
[Hashed Storage]: ../administration/repository_storage_types.md
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index 80feb629d54..1f80646a2ea 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -215,9 +215,9 @@ module API
job = authenticate_job!
forbidden!('Job is not running!') unless job.running?
- artifacts_upload_path = JobArtifactUploader.artifacts_upload_path
- artifacts = uploaded_file(:file, artifacts_upload_path)
- metadata = uploaded_file(:metadata, artifacts_upload_path)
+ workhorse_upload_path = JobArtifactUploader.workhorse_upload_path
+ artifacts = uploaded_file(:file, workhorse_upload_path)
+ metadata = uploaded_file(:metadata, workhorse_upload_path)
bad_request!('Missing artifacts file!') unless artifacts
file_to_large! unless artifacts.size < max_artifacts_size
diff --git a/lib/backup/artifacts.rb b/lib/backup/artifacts.rb
index 7a582a20056..4383124d150 100644
--- a/lib/backup/artifacts.rb
+++ b/lib/backup/artifacts.rb
@@ -3,7 +3,7 @@ require 'backup/files'
module Backup
class Artifacts < Files
def initialize
- super('artifacts', LegacyArtifactUploader.local_store_path)
+ super('artifacts', JobArtifactUploader.root)
end
def create_files_dir
diff --git a/lib/gitlab/background_migration/populate_untracked_uploads.rb b/lib/gitlab/background_migration/populate_untracked_uploads.rb
index d60e41d9f9d..8a8e770940e 100644
--- a/lib/gitlab/background_migration/populate_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/populate_untracked_uploads.rb
@@ -143,7 +143,7 @@ module Gitlab
end
def absolute_path
- File.join(CarrierWave.root, path)
+ File.join(Gitlab.config.uploads.storage_path, path)
end
end
diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
index 4e0121ca34d..a7a1bbe1752 100644
--- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
@@ -11,9 +11,12 @@ module Gitlab
FIND_BATCH_SIZE = 500
RELATIVE_UPLOAD_DIR = "uploads".freeze
- ABSOLUTE_UPLOAD_DIR = "#{CarrierWave.root}/#{RELATIVE_UPLOAD_DIR}".freeze
+ ABSOLUTE_UPLOAD_DIR = File.join(
+ Gitlab.config.uploads.storage_path,
+ RELATIVE_UPLOAD_DIR
+ )
FOLLOW_UP_MIGRATION = 'PopulateUntrackedUploads'.freeze
- START_WITH_CARRIERWAVE_ROOT_REGEX = %r{\A#{CarrierWave.root}/}
+ START_WITH_ROOT_REGEX = %r{\A#{Gitlab.config.uploads.storage_path}/}
EXCLUDED_HASHED_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/@hashed/*".freeze
EXCLUDED_TMP_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/tmp/*".freeze
@@ -81,7 +84,7 @@ module Gitlab
paths = []
stdout.each_line("\0") do |line|
- paths << line.chomp("\0").sub(START_WITH_CARRIERWAVE_ROOT_REGEX, '')
+ paths << line.chomp("\0").sub(START_WITH_ROOT_REGEX, '')
if paths.size >= batch_size
yield(paths)
diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb
index 8fab5489616..3fdc3c27f73 100644
--- a/lib/gitlab/gfm/uploads_rewriter.rb
+++ b/lib/gitlab/gfm/uploads_rewriter.rb
@@ -27,7 +27,7 @@ module Gitlab
with_link_in_tmp_dir(file.file) do |open_tmp_file|
new_uploader.store!(open_tmp_file)
end
- new_uploader.to_markdown
+ new_uploader.markdown_link
end
end
diff --git a/lib/gitlab/import_export/uploads_saver.rb b/lib/gitlab/import_export/uploads_saver.rb
index 627a487d577..2f08dda55fd 100644
--- a/lib/gitlab/import_export/uploads_saver.rb
+++ b/lib/gitlab/import_export/uploads_saver.rb
@@ -17,15 +17,13 @@ module Gitlab
false
end
- private
+ def uploads_path
+ FileUploader.absolute_base_dir(@project)
+ end
def uploads_export_path
File.join(@shared.export_path, 'uploads')
end
-
- def uploads_path
- FileUploader.dynamic_path_segment(@project)
- end
end
end
end
diff --git a/lib/gitlab/uploads_transfer.rb b/lib/gitlab/uploads_transfer.rb
index b5f41240529..7d7400bdabf 100644
--- a/lib/gitlab/uploads_transfer.rb
+++ b/lib/gitlab/uploads_transfer.rb
@@ -1,7 +1,7 @@
module Gitlab
class UploadsTransfer < ProjectTransfer
def root_dir
- File.join(CarrierWave.root, FileUploader.base_dir)
+ FileUploader.root
end
end
end
diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb
index 633da44b22d..b3f8b0d174d 100644
--- a/lib/gitlab/workhorse.rb
+++ b/lib/gitlab/workhorse.rb
@@ -55,14 +55,14 @@ module Gitlab
def lfs_upload_ok(oid, size)
{
- StoreLFSPath: "#{Gitlab.config.lfs.storage_path}/tmp/upload",
+ StoreLFSPath: LfsObjectUploader.workhorse_upload_path,
LfsOid: oid,
LfsSize: size
}
end
def artifact_upload_ok
- { TempPath: JobArtifactUploader.artifacts_upload_path }
+ { TempPath: JobArtifactUploader.workhorse_upload_path }
end
def send_git_blob(repository, blob)
@@ -147,8 +147,11 @@ module Gitlab
end
def send_artifacts_entry(build, entry)
+ file = build.artifacts_file
+ archive = file.file_storage? ? file.path : file.url
+
params = {
- 'Archive' => build.artifacts_file.path,
+ 'Archive' => archive,
'Entry' => Base64.encode64(entry.to_s)
}
diff --git a/spec/controllers/groups/uploads_controller_spec.rb b/spec/controllers/groups/uploads_controller_spec.rb
index 67a11e56e94..6a1869d1a48 100644
--- a/spec/controllers/groups/uploads_controller_spec.rb
+++ b/spec/controllers/groups/uploads_controller_spec.rb
@@ -6,5 +6,7 @@ describe Groups::UploadsController do
{ group_id: model }
end
- it_behaves_like 'handle uploads'
+ it_behaves_like 'handle uploads' do
+ let(:uploader_class) { NamespaceFileUploader }
+ end
end
diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb
index 12cb7b2647f..25a2e13fe1a 100644
--- a/spec/controllers/projects/artifacts_controller_spec.rb
+++ b/spec/controllers/projects/artifacts_controller_spec.rb
@@ -145,8 +145,7 @@ describe Projects::ArtifactsController do
context 'when using local file storage' do
it_behaves_like 'a valid file' do
let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
- let(:store) { ObjectStoreUploader::LOCAL_STORE }
- let(:archive_path) { JobArtifactUploader.local_store_path }
+ let(:archive_path) { JobArtifactUploader.root }
end
end
end
diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb
index 3a0c3faa7b4..b7df42168e0 100644
--- a/spec/controllers/projects/raw_controller_spec.rb
+++ b/spec/controllers/projects/raw_controller_spec.rb
@@ -53,7 +53,7 @@ describe Projects::RawController do
end
it 'serves the file' do
- expect(controller).to receive(:send_file).with("#{Gitlab.config.shared.path}/lfs-objects/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment')
+ expect(controller).to receive(:send_file).with("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment')
get(:show,
namespace_id: public_project.namespace.to_param,
project_id: public_project,
diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb
index b1f601a19e5..376b229ffc9 100644
--- a/spec/controllers/uploads_controller_spec.rb
+++ b/spec/controllers/uploads_controller_spec.rb
@@ -180,6 +180,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'user', mounted_as: 'avatar', id: user.id, filename: 'image.png'
+
response
end
end
@@ -196,6 +197,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'user', mounted_as: 'avatar', id: user.id, filename: 'image.png'
+
response
end
end
@@ -220,6 +222,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'image.png'
+
response
end
end
@@ -239,6 +242,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'image.png'
+
response
end
end
@@ -291,6 +295,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'image.png'
+
response
end
end
@@ -322,6 +327,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'image.png'
+
response
end
end
@@ -341,6 +347,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'image.png'
+
response
end
end
@@ -384,6 +391,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'image.png'
+
response
end
end
@@ -420,6 +428,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png'
+
response
end
end
@@ -439,6 +448,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png'
+
response
end
end
@@ -491,6 +501,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png'
+
response
end
end
@@ -522,6 +533,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'appearance', mounted_as: 'header_logo', id: appearance.id, filename: 'dk.png'
+
response
end
end
@@ -541,6 +553,7 @@ describe UploadsController do
it_behaves_like 'content not cached without revalidation' do
subject do
get :show, model: 'appearance', mounted_as: 'logo', id: appearance.id, filename: 'dk.png'
+
response
end
end
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 2defb4935ad..3f4e408b3a6 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -122,11 +122,11 @@ FactoryBot.define do
end
trait :with_attachment do
- attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") }
+ attachment { fixture_file_upload(Rails.root.join( "spec/fixtures/dk.png"), "image/png") }
end
trait :with_svg_attachment do
- attachment { fixture_file_upload(Rails.root + "spec/fixtures/unsanitized.svg", "image/svg+xml") }
+ attachment { fixture_file_upload(Rails.root.join("spec/fixtures/unsanitized.svg"), "image/svg+xml") }
end
transient do
diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb
index c39500faea1..c8cfe251d27 100644
--- a/spec/factories/uploads.rb
+++ b/spec/factories/uploads.rb
@@ -1,24 +1,42 @@
FactoryBot.define do
factory :upload do
model { build(:project) }
- path { "uploads/-/system/project/avatar/avatar.jpg" }
size 100.kilobytes
uploader "AvatarUploader"
- trait :personal_snippet do
+ # we should build a mount agnostic upload by default
+ transient do
+ mounted_as :avatar
+ secret SecureRandom.hex
+ end
+
+ # this needs to comply with RecordsUpload::Concern#upload_path
+ path { File.join("uploads/-/system", model.class.to_s.underscore, mounted_as.to_s, 'avatar.jpg') }
+
+ trait :personal_snippet_upload do
model { build(:personal_snippet) }
+ path { File.join(secret, 'myfile.jpg') }
uploader "PersonalFileUploader"
end
trait :issuable_upload do
- path { "#{SecureRandom.hex}/myfile.jpg" }
+ path { File.join(secret, 'myfile.jpg') }
uploader "FileUploader"
end
trait :namespace_upload do
- path { "#{SecureRandom.hex}/myfile.jpg" }
model { build(:group) }
+ path { File.join(secret, 'myfile.jpg') }
uploader "NamespaceFileUploader"
end
+
+ trait :attachment_upload do
+ transient do
+ mounted_as :attachment
+ end
+
+ model { build(:note) }
+ uploader "AttachmentUploader"
+ end
end
end
diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
index 8bb9ebe0419..1a4b9f427d6 100644
--- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
@@ -23,6 +23,27 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
end
end
+ # E.g. The installation is in use at the time of migration, and someone has
+ # just uploaded a file
+ shared_examples 'does not add files in /uploads/tmp' do
+ let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
+
+ before do
+ FileUtils.mkdir(File.dirname(tmp_file))
+ FileUtils.touch(tmp_file)
+ end
+
+ after do
+ FileUtils.rm(tmp_file)
+ end
+
+ it 'does not add files from /uploads/tmp' do
+ described_class.new.perform
+
+ expect(untracked_files_for_uploads.count).to eq(5)
+ end
+ end
+
it 'ensures the untracked_files_for_uploads table exists' do
expect do
described_class.new.perform
@@ -109,20 +130,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
end
end
- # E.g. The installation is in use at the time of migration, and someone has
- # just uploaded a file
context 'when there are files in /uploads/tmp' do
- let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
-
- before do
- FileUtils.touch(tmp_file)
- end
-
- after do
- FileUtils.rm(tmp_file)
- end
-
- it 'does not add files from /uploads/tmp' do
+ it_behaves_like 'does not add files in /uploads/tmp'
described_class.new.perform
expect(untracked_files_for_uploads.count).to eq(5)
@@ -197,24 +206,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
end
end
- # E.g. The installation is in use at the time of migration, and someone has
- # just uploaded a file
context 'when there are files in /uploads/tmp' do
- let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
-
- before do
- FileUtils.touch(tmp_file)
- end
-
- after do
- FileUtils.rm(tmp_file)
- end
-
- it 'does not add files from /uploads/tmp' do
- described_class.new.perform
-
- expect(untracked_files_for_uploads.count).to eq(5)
- end
+ it_behaves_like 'does not add files in /uploads/tmp'
end
end
end
diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
index 39e3b875c49..326ed2f2ecf 100644
--- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
+++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
@@ -17,7 +17,7 @@ describe Gitlab::Gfm::UploadsRewriter do
end
let(:text) do
- "Text and #{image_uploader.to_markdown} and #{zip_uploader.to_markdown}"
+ "Text and #{image_uploader.markdown_link} and #{zip_uploader.markdown_link}"
end
describe '#rewrite' do
diff --git a/spec/lib/gitlab/import_export/uploads_restorer_spec.rb b/spec/lib/gitlab/import_export/uploads_restorer_spec.rb
index 63992ea8ab8..a685521cbf0 100644
--- a/spec/lib/gitlab/import_export/uploads_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/uploads_restorer_spec.rb
@@ -4,7 +4,6 @@ describe Gitlab::ImportExport::UploadsRestorer do
describe 'bundle a project Git repo' do
let(:export_path) { "#{Dir.tmpdir}/uploads_saver_spec" }
let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: project.full_path) }
- let(:uploads_path) { FileUploader.dynamic_path_segment(project) }
before do
allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
@@ -26,9 +25,9 @@ describe Gitlab::ImportExport::UploadsRestorer do
end
it 'copies the uploads to the project path' do
- restorer.restore
+ subject.restore
- uploads = Dir.glob(File.join(uploads_path, '**/*')).map { |file| File.basename(file) }
+ uploads = Dir.glob(File.join(subject.uploads_path, '**/*')).map { |file| File.basename(file) }
expect(uploads).to include('dummy.txt')
end
@@ -44,9 +43,9 @@ describe Gitlab::ImportExport::UploadsRestorer do
end
it 'copies the uploads to the project path' do
- restorer.restore
+ subject.restore
- uploads = Dir.glob(File.join(uploads_path, '**/*')).map { |file| File.basename(file) }
+ uploads = Dir.glob(File.join(subject.uploads_path, '**/*')).map { |file| File.basename(file) }
expect(uploads).to include('dummy.txt')
end
diff --git a/spec/lib/gitlab/import_export/uploads_saver_spec.rb b/spec/lib/gitlab/import_export/uploads_saver_spec.rb
index e8948de1f3a..959779523f4 100644
--- a/spec/lib/gitlab/import_export/uploads_saver_spec.rb
+++ b/spec/lib/gitlab/import_export/uploads_saver_spec.rb
@@ -30,7 +30,7 @@ describe Gitlab::ImportExport::UploadsSaver do
it 'copies the uploads to the export path' do
saver.save
- uploads = Dir.glob(File.join(shared.export_path, 'uploads', '**/*')).map { |file| File.basename(file) }
+ uploads = Dir.glob(File.join(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) }
expect(uploads).to include('banana_sample.gif')
end
@@ -52,7 +52,7 @@ describe Gitlab::ImportExport::UploadsSaver do
it 'copies the uploads to the export path' do
saver.save
- uploads = Dir.glob(File.join(shared.export_path, 'uploads', '**/*')).map { |file| File.basename(file) }
+ uploads = Dir.glob(File.join(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) }
expect(uploads).to include('banana_sample.gif')
end
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index c3673a0e2a3..6b7dbad128c 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -204,7 +204,7 @@ describe Namespace do
let(:parent) { create(:group, name: 'parent', path: 'parent') }
let(:child) { create(:group, name: 'child', path: 'child', parent: parent) }
let!(:project) { create(:project_empty_repo, path: 'the-project', namespace: child, skip_disk_validation: true) }
- let(:uploads_dir) { File.join(CarrierWave.root, FileUploader.base_dir) }
+ let(:uploads_dir) { FileUploader.root }
let(:pages_dir) { File.join(TestEnv.pages_path) }
before do
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index 345382ea8c7..42f3d609770 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -45,51 +45,6 @@ describe Upload do
end
end
- describe '.remove_path' do
- it 'removes all records at the given path' do
- described_class.create!(
- size: File.size(__FILE__),
- path: __FILE__,
- model: build_stubbed(:user),
- uploader: 'AvatarUploader'
- )
-
- expect { described_class.remove_path(__FILE__) }
- .to change { described_class.count }.from(1).to(0)
- end
- end
-
- describe '.record' do
- let(:fake_uploader) do
- double(
- file: double(size: 12_345),
- relative_path: 'foo/bar.jpg',
- model: build_stubbed(:user),
- class: 'AvatarUploader'
- )
- end
-
- it 'removes existing paths before creation' do
- expect(described_class).to receive(:remove_path)
- .with(fake_uploader.relative_path)
-
- described_class.record(fake_uploader)
- end
-
- it 'creates a new record and assigns size, path, model, and uploader' do
- upload = described_class.record(fake_uploader)
-
- aggregate_failures do
- expect(upload).to be_persisted
- expect(upload.size).to eq fake_uploader.file.size
- expect(upload.path).to eq fake_uploader.relative_path
- expect(upload.model_id).to eq fake_uploader.model.id
- expect(upload.model_type).to eq fake_uploader.model.class.to_s
- expect(upload.uploader).to eq fake_uploader.class
- end
- end
- end
-
describe '#absolute_path' do
it 'returns the path directly when already absolute' do
path = '/path/to/namespace/project/secret/file.jpg'
@@ -111,27 +66,27 @@ describe Upload do
end
end
- describe '#calculate_checksum' do
- it 'calculates the SHA256 sum' do
- upload = described_class.new(
- path: __FILE__,
- size: described_class::CHECKSUM_THRESHOLD - 1.megabyte
- )
+ describe '#calculate_checksum!' do
+ let(:upload) do
+ described_class.new(path: __FILE__,
+ size: described_class::CHECKSUM_THRESHOLD - 1.megabyte)
+ end
+
+ it 'sets `checksum` to SHA256 sum of the file' do
expected = Digest::SHA256.file(__FILE__).hexdigest
- expect { upload.calculate_checksum }
+ expect { upload.calculate_checksum! }
.to change { upload.checksum }.from(nil).to(expected)
end
- it 'returns nil for a non-existant file' do
- upload = described_class.new(
- path: __FILE__,
- size: described_class::CHECKSUM_THRESHOLD - 1.megabyte
- )
-
+ it 'sets `checksum` to nil for a non-existant file' do
expect(upload).to receive(:exist?).and_return(false)
- expect(upload.calculate_checksum).to be_nil
+ checksum = Digest::SHA256.file(__FILE__).hexdigest
+ upload.checksum = checksum
+
+ expect { upload.calculate_checksum! }
+ .to change { upload.checksum }.from(checksum).to(nil)
end
end
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index cb66d23b77c..c5c0b0c2867 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -945,7 +945,7 @@ describe API::Runner do
context 'when artifacts are being stored inside of tmp path' do
before do
# by configuring this path we allow to pass temp file from any path
- allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
+ allow(JobArtifactUploader).to receive(:workhorse_upload_path).and_return('/')
end
context 'when job has been erased' do
@@ -1122,7 +1122,7 @@ describe API::Runner do
# by configuring this path we allow to pass file from @tmpdir only
# but all temporary files are stored in system tmp directory
@tmpdir = Dir.mktmpdir
- allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir)
+ allow(JobArtifactUploader).to receive(:workhorse_upload_path).and_return(@tmpdir)
end
after do
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index bee918a20aa..930ef49b7f3 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -958,7 +958,7 @@ describe 'Git LFS API and storage' do
end
it 'responds with status 200, location of lfs store and object details' do
- expect(json_response['StoreLFSPath']).to eq("#{Gitlab.config.shared.path}/lfs-objects/tmp/upload")
+ expect(json_response['StoreLFSPath']).to eq(LfsObjectUploader.workhorse_upload_path)
expect(json_response['LfsOid']).to eq(sample_oid)
expect(json_response['LfsSize']).to eq(sample_size)
end
@@ -1075,7 +1075,7 @@ describe 'Git LFS API and storage' do
end
it 'with location of lfs store and object details' do
- expect(json_response['StoreLFSPath']).to eq("#{Gitlab.config.shared.path}/lfs-objects/tmp/upload")
+ expect(json_response['StoreLFSPath']).to eq(LfsObjectUploader.workhorse_upload_path)
expect(json_response['LfsOid']).to eq(sample_oid)
expect(json_response['LfsSize']).to eq(sample_size)
end
diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb
index 388c9d63c7b..322c91065e7 100644
--- a/spec/services/issues/move_service_spec.rb
+++ b/spec/services/issues/move_service_spec.rb
@@ -6,7 +6,7 @@ describe Issues::MoveService do
let(:title) { 'Some issue' }
let(:description) { 'Some issue description' }
let(:old_project) { create(:project) }
- let(:new_project) { create(:project) }
+ let(:new_project) { create(:project, group: create(:group)) }
let(:milestone1) { create(:milestone, project_id: old_project.id, title: 'v9.0') }
let(:old_issue) do
@@ -250,7 +250,7 @@ describe Issues::MoveService do
context 'issue description with uploads' do
let(:uploader) { build(:file_uploader, project: old_project) }
- let(:description) { "Text and #{uploader.to_markdown}" }
+ let(:description) { "Text and #{uploader.markdown_link}" }
include_context 'issue move executed'
diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
index 50e59954f73..15699574b3a 100644
--- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
@@ -6,7 +6,7 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
let(:legacy_storage) { Storage::LegacyProject.new(project) }
let(:hashed_storage) { Storage::HashedProject.new(project) }
- let!(:upload) { Upload.find_by(path: file_uploader.relative_path) }
+ let!(:upload) { Upload.find_by(path: file_uploader.upload_path) }
let(:file_uploader) { build(:file_uploader, project: project) }
let(:old_path) { File.join(base_path(legacy_storage), upload.path) }
let(:new_path) { File.join(base_path(hashed_storage), upload.path) }
@@ -58,6 +58,6 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
end
def base_path(storage)
- FileUploader.dynamic_path_builder(storage.disk_path)
+ File.join(FileUploader.root, storage.disk_path)
end
end
diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
index 935c08221e0..7ce80c82439 100644
--- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
+++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
@@ -2,6 +2,8 @@ shared_examples 'handle uploads' do
let(:user) { create(:user) }
let(:jpg) { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
let(:txt) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
+ let(:secret) { FileUploader.generate_secret }
+ let(:uploader_class) { FileUploader }
describe "POST #create" do
context 'when a user is not authorized to upload a file' do
@@ -65,7 +67,12 @@ shared_examples 'handle uploads' do
describe "GET #show" do
let(:show_upload) do
- get :show, params.merge(secret: "123456", filename: "image.jpg")
+ get :show, params.merge(secret: secret, filename: "rails_sample.jpg")
+ end
+
+ before do
+ expect(FileUploader).to receive(:generate_secret).and_return(secret)
+ UploadService.new(model, jpg, uploader_class).execute
end
context "when the model is public" do
@@ -75,11 +82,6 @@ shared_examples 'handle uploads' do
context "when not signed in" do
context "when the file exists" do
- before do
- allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
- allow(jpg).to receive(:exists?).and_return(true)
- end
-
it "responds with status 200" do
show_upload
@@ -88,6 +90,10 @@ shared_examples 'handle uploads' do
end
context "when the file doesn't exist" do
+ before do
+ allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+ end
+
it "responds with status 404" do
show_upload
@@ -102,11 +108,6 @@ shared_examples 'handle uploads' do
end
context "when the file exists" do
- before do
- allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
- allow(jpg).to receive(:exists?).and_return(true)
- end
-
it "responds with status 200" do
show_upload
@@ -115,6 +116,10 @@ shared_examples 'handle uploads' do
end
context "when the file doesn't exist" do
+ before do
+ allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+ end
+
it "responds with status 404" do
show_upload
@@ -131,11 +136,6 @@ shared_examples 'handle uploads' do
context "when not signed in" do
context "when the file exists" do
- before do
- allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
- allow(jpg).to receive(:exists?).and_return(true)
- end
-
context "when the file is an image" do
before do
allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
@@ -149,6 +149,10 @@ shared_examples 'handle uploads' do
end
context "when the file is not an image" do
+ before do
+ allow_any_instance_of(FileUploader).to receive(:image?).and_return(false)
+ end
+
it "redirects to the sign in page" do
show_upload
@@ -158,6 +162,10 @@ shared_examples 'handle uploads' do
end
context "when the file doesn't exist" do
+ before do
+ allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+ end
+
it "redirects to the sign in page" do
show_upload
@@ -177,11 +185,6 @@ shared_examples 'handle uploads' do
end
context "when the file exists" do
- before do
- allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
- allow(jpg).to receive(:exists?).and_return(true)
- end
-
it "responds with status 200" do
show_upload
@@ -190,6 +193,10 @@ shared_examples 'handle uploads' do
end
context "when the file doesn't exist" do
+ before do
+ allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+ end
+
it "responds with status 404" do
show_upload
@@ -200,11 +207,6 @@ shared_examples 'handle uploads' do
context "when the user doesn't have access to the model" do
context "when the file exists" do
- before do
- allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
- allow(jpg).to receive(:exists?).and_return(true)
- end
-
context "when the file is an image" do
before do
allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
@@ -218,6 +220,10 @@ shared_examples 'handle uploads' do
end
context "when the file is not an image" do
+ before do
+ allow_any_instance_of(FileUploader).to receive(:image?).and_return(false)
+ end
+
it "responds with status 404" do
show_upload
@@ -227,6 +233,10 @@ shared_examples 'handle uploads' do
end
context "when the file doesn't exist" do
+ before do
+ allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+ end
+
it "responds with status 404" do
show_upload
diff --git a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb b/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb
new file mode 100644
index 00000000000..934d53e7bba
--- /dev/null
+++ b/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb
@@ -0,0 +1,48 @@
+shared_examples "matches the method pattern" do |method|
+ let(:target) { subject }
+ let(:args) { nil }
+ let(:pattern) { patterns[method] }
+
+ it do
+ return skip "No pattern provided, skipping." unless pattern
+
+ expect(target.method(method).call(*args)).to match(pattern)
+ end
+end
+
+shared_examples "builds correct paths" do |**patterns|
+ let(:patterns) { patterns }
+
+ before do
+ allow(subject).to receive(:filename).and_return('<filename>')
+ end
+
+ describe "#store_dir" do
+ it_behaves_like "matches the method pattern", :store_dir
+ end
+
+ describe "#cache_dir" do
+ it_behaves_like "matches the method pattern", :cache_dir
+ end
+
+ describe "#work_dir" do
+ it_behaves_like "matches the method pattern", :work_dir
+ end
+
+ describe "#upload_path" do
+ it_behaves_like "matches the method pattern", :upload_path
+ end
+
+ describe ".absolute_path" do
+ it_behaves_like "matches the method pattern", :absolute_path do
+ let(:target) { subject.class }
+ let(:args) { [upload] }
+ end
+ end
+
+ describe ".base_dir" do
+ it_behaves_like "matches the method pattern", :base_dir do
+ let(:target) { subject.class }
+ end
+ end
+end
diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb
index 9e5f08fbc51..c275522159c 100644
--- a/spec/support/test_env.rb
+++ b/spec/support/test_env.rb
@@ -237,7 +237,7 @@ module TestEnv
end
def artifacts_path
- Gitlab.config.artifacts.path
+ Gitlab.config.artifacts.storage_path
end
# When no cached assets exist, manually hit the root path to create them
diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb
index d05eda08201..5752078d2a0 100644
--- a/spec/support/track_untracked_uploads_helpers.rb
+++ b/spec/support/track_untracked_uploads_helpers.rb
@@ -1,6 +1,6 @@
module TrackUntrackedUploadsHelpers
def uploaded_file
- fixture_path = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
+ fixture_path = Rails.root.join('spec/fixtures/rails_sample.jpg')
fixture_file_upload(fixture_path)
end
diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb
index 04ee6e9bfad..db378dab97d 100644
--- a/spec/uploaders/attachment_uploader_spec.rb
+++ b/spec/uploaders/attachment_uploader_spec.rb
@@ -1,18 +1,16 @@
require 'spec_helper'
describe AttachmentUploader do
- let(:uploader) { described_class.new(build_stubbed(:user)) }
+ let(:note) { create(:note, :with_attachment) }
+ let(:uploader) { note.attachment }
+ let(:upload) { create(:upload, :attachment_upload, model: uploader.model) }
- describe "#store_dir" do
- it "stores in the system dir" do
- expect(uploader.store_dir).to start_with("uploads/-/system/user")
- end
+ subject { uploader }
- it "uses the old path when using object storage" do
- expect(described_class).to receive(:file_storage?).and_return(false)
- expect(uploader.store_dir).to start_with("uploads/user")
- end
- end
+ it_behaves_like 'builds correct paths',
+ store_dir: %r[uploads/-/system/note/attachment/],
+ upload_path: %r[uploads/-/system/note/attachment/],
+ absolute_path: %r[#{CarrierWave.root}/uploads/-/system/note/attachment/]
describe '#move_to_cache' do
it 'is true' do
diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb
index 1dc574699d8..bf9028c9260 100644
--- a/spec/uploaders/avatar_uploader_spec.rb
+++ b/spec/uploaders/avatar_uploader_spec.rb
@@ -1,18 +1,16 @@
require 'spec_helper'
describe AvatarUploader do
- let(:uploader) { described_class.new(build_stubbed(:user)) }
+ let(:model) { create(:user, :with_avatar) }
+ let(:uploader) { described_class.new(model, :avatar) }
+ let(:upload) { create(:upload, model: model) }
- describe "#store_dir" do
- it "stores in the system dir" do
- expect(uploader.store_dir).to start_with("uploads/-/system/user")
- end
+ subject { uploader }
- it "uses the old path when using object storage" do
- expect(described_class).to receive(:file_storage?).and_return(false)
- expect(uploader.store_dir).to start_with("uploads/user")
- end
- end
+ it_behaves_like 'builds correct paths',
+ store_dir: %r[uploads/-/system/user/avatar/],
+ upload_path: %r[uploads/-/system/user/avatar/],
+ absolute_path: %r[#{CarrierWave.root}/uploads/-/system/user/avatar/]
describe '#move_to_cache' do
it 'is false' do
diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb
index 0cf462e9553..bc024cd307c 100644
--- a/spec/uploaders/file_mover_spec.rb
+++ b/spec/uploaders/file_mover_spec.rb
@@ -3,13 +3,13 @@ require 'spec_helper'
describe FileMover do
let(:filename) { 'banana_sample.gif' }
let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) }
+ let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) }
+
let(:temp_description) do
- 'test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) same ![banana_sample]'\
- '(/uploads/-/system/temp/secret55/banana_sample.gif)'
+ "test ![banana_sample](/#{temp_file_path}) "\
+ "same ![banana_sample](/#{temp_file_path}) "
end
- let(:temp_file_path) { File.join('secret55', filename).to_s }
- let(:file_path) { File.join('uploads', '-', 'system', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s }
-
+ let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) }
let(:snippet) { create(:personal_snippet, description: temp_description) }
subject { described_class.new(file_path, snippet).execute }
@@ -28,8 +28,8 @@ describe FileMover do
expect(snippet.reload.description)
.to eq(
- "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\
- " same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"
+ "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
+ "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "
)
end
@@ -50,8 +50,8 @@ describe FileMover do
expect(snippet.reload.description)
.to eq(
- "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"\
- " same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"
+ "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\
+ "same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "
)
end
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index 845516e5004..a72f853df75 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -1,118 +1,57 @@
require 'spec_helper'
describe FileUploader do
- let(:uploader) { described_class.new(build_stubbed(:project)) }
+ let(:group) { create(:group, name: 'awesome') }
+ let(:project) { create(:project, namespace: group, name: 'project') }
+ let(:uploader) { described_class.new(project) }
+ let(:upload) { double(model: project, path: 'secret/foo.jpg') }
- context 'legacy storage' do
- let(:project) { build_stubbed(:project) }
-
- describe '.absolute_path' do
- it 'returns the correct absolute path by building it dynamically' do
- upload = double(model: project, path: 'secret/foo.jpg')
-
- dynamic_segment = project.full_path
+ subject { uploader }
- expect(described_class.absolute_path(upload))
- .to end_with("#{dynamic_segment}/secret/foo.jpg")
- end
- end
-
- describe "#store_dir" do
- it "stores in the namespace path" do
- uploader = described_class.new(project)
-
- expect(uploader.store_dir).to include(project.full_path)
- expect(uploader.store_dir).not_to include("system")
- end
- end
+ shared_examples 'builds correct legacy storage paths' do
+ include_examples 'builds correct paths',
+ store_dir: %r{awesome/project/\h+},
+ absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg}
end
- context 'hashed storage' do
+ shared_examples 'uses hashed storage' do
context 'when rolled out attachments' do
- let(:project) { build_stubbed(:project, :hashed) }
-
- describe '.absolute_path' do
- it 'returns the correct absolute path by building it dynamically' do
- upload = double(model: project, path: 'secret/foo.jpg')
-
- dynamic_segment = project.disk_path
-
- expect(described_class.absolute_path(upload))
- .to end_with("#{dynamic_segment}/secret/foo.jpg")
- end
+ before do
+ allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed')
end
- describe "#store_dir" do
- it "stores in the namespace path" do
- uploader = described_class.new(project)
+ let(:project) { build_stubbed(:project, :hashed, namespace: group, name: 'project') }
- expect(uploader.store_dir).to include(project.disk_path)
- expect(uploader.store_dir).not_to include("system")
- end
- end
+ it_behaves_like 'builds correct paths',
+ store_dir: %r{ca/fe/fe/ed/\h+},
+ absolute_path: %r{#{described_class.root}/ca/fe/fe/ed/secret/foo.jpg}
end
context 'when only repositories are rolled out' do
- let(:project) { build_stubbed(:project, storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) }
-
- describe '.absolute_path' do
- it 'returns the correct absolute path by building it dynamically' do
- upload = double(model: project, path: 'secret/foo.jpg')
-
- dynamic_segment = project.full_path
-
- expect(described_class.absolute_path(upload))
- .to end_with("#{dynamic_segment}/secret/foo.jpg")
- end
- end
-
- describe "#store_dir" do
- it "stores in the namespace path" do
- uploader = described_class.new(project)
+ let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) }
- expect(uploader.store_dir).to include(project.full_path)
- expect(uploader.store_dir).not_to include("system")
- end
- end
+ it_behaves_like 'builds correct legacy storage paths'
end
end
- describe 'initialize' do
- it 'generates a secret if none is provided' do
- expect(SecureRandom).to receive(:hex).and_return('secret')
-
- uploader = described_class.new(double)
-
- expect(uploader.secret).to eq 'secret'
- end
-
- it 'accepts a secret parameter' do
- expect(SecureRandom).not_to receive(:hex)
-
- uploader = described_class.new(double, 'secret')
-
- expect(uploader.secret).to eq 'secret'
- end
+ context 'legacy storage' do
+ it_behaves_like 'builds correct legacy storage paths'
+ include_examples 'uses hashed storage'
end
- describe '#move_to_cache' do
- it 'is true' do
- expect(uploader.move_to_cache).to eq(true)
- end
- end
+ describe 'initialize' do
+ let(:uploader) { described_class.new(double, 'secret') }
- describe '#move_to_store' do
- it 'is true' do
- expect(uploader.move_to_store).to eq(true)
+ it 'accepts a secret parameter' do
+ expect(described_class).not_to receive(:generate_secret)
+ expect(uploader.secret).to eq('secret')
end
end
- describe '#relative_path' do
- it 'removes the leading dynamic path segment' do
- fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
- uploader.store!(fixture_file_upload(fixture))
-
- expect(uploader.relative_path).to match(%r{\A\h{32}/rails_sample.jpg\z})
+ describe '#secret' do
+ it 'generates a secret if none is provided' do
+ expect(described_class).to receive(:generate_secret).and_return('secret')
+ expect(uploader.secret).to eq('secret')
end
end
end
diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb
index a067c3e75f4..d606404e95d 100644
--- a/spec/uploaders/job_artifact_uploader_spec.rb
+++ b/spec/uploaders/job_artifact_uploader_spec.rb
@@ -3,33 +3,13 @@ require 'spec_helper'
describe JobArtifactUploader do
let(:job_artifact) { create(:ci_job_artifact) }
let(:uploader) { described_class.new(job_artifact, :file) }
- let(:local_path) { Gitlab.config.artifacts.path }
- describe '#store_dir' do
- subject { uploader.store_dir }
+ subject { uploader }
- let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.job_id}/#{job_artifact.id}" }
-
- context 'when using local storage' do
- it { is_expected.to start_with(local_path) }
- it { is_expected.to match(%r{\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z}) }
- it { is_expected.to end_with(path) }
- end
- end
-
- describe '#cache_dir' do
- subject { uploader.cache_dir }
-
- it { is_expected.to start_with(local_path) }
- it { is_expected.to end_with('/tmp/cache') }
- end
-
- describe '#work_dir' do
- subject { uploader.work_dir }
-
- it { is_expected.to start_with(local_path) }
- it { is_expected.to end_with('/tmp/work') }
- end
+ it_behaves_like "builds correct paths",
+ store_dir: %r[\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z],
+ cache_dir: %r[artifacts/tmp/cache],
+ work_dir: %r[artifacts/tmp/work]
context 'file is stored in valid local_path' do
let(:file) do
@@ -43,7 +23,7 @@ describe JobArtifactUploader do
subject { uploader.file.path }
- it { is_expected.to start_with(local_path) }
+ it { is_expected.to start_with("#{uploader.root}/#{uploader.class.base_dir}") }
it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") }
it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") }
it { is_expected.to end_with("ci_build_artifacts.zip") }
diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb
index efeffb78772..54c6a8b869b 100644
--- a/spec/uploaders/legacy_artifact_uploader_spec.rb
+++ b/spec/uploaders/legacy_artifact_uploader_spec.rb
@@ -3,49 +3,22 @@ require 'rails_helper'
describe LegacyArtifactUploader do
let(:job) { create(:ci_build) }
let(:uploader) { described_class.new(job, :legacy_artifacts_file) }
- let(:local_path) { Gitlab.config.artifacts.path }
+ let(:local_path) { described_class.root }
- describe '.local_store_path' do
- subject { described_class.local_store_path }
+ subject { uploader }
- it "delegate to artifacts path" do
- expect(Gitlab.config.artifacts).to receive(:path)
-
- subject
- end
- end
-
- describe '.artifacts_upload_path' do
- subject { described_class.artifacts_upload_path }
+ # TODO: move to Workhorse::UploadPath
+ describe '.workhorse_upload_path' do
+ subject { described_class.workhorse_upload_path }
it { is_expected.to start_with(local_path) }
- it { is_expected.to end_with('tmp/uploads/') }
- end
-
- describe '#store_dir' do
- subject { uploader.store_dir }
-
- let(:path) { "#{job.created_at.utc.strftime('%Y_%m')}/#{job.project_id}/#{job.id}" }
-
- context 'when using local storage' do
- it { is_expected.to start_with(local_path) }
- it { is_expected.to end_with(path) }
- end
+ it { is_expected.to end_with('tmp/uploads') }
end
- describe '#cache_dir' do
- subject { uploader.cache_dir }
-
- it { is_expected.to start_with(local_path) }
- it { is_expected.to end_with('/tmp/cache') }
- end
-
- describe '#work_dir' do
- subject { uploader.work_dir }
-
- it { is_expected.to start_with(local_path) }
- it { is_expected.to end_with('/tmp/work') }
- end
+ it_behaves_like "builds correct paths",
+ store_dir: %r[\d{4}_\d{1,2}/\d+/\d+\z],
+ cache_dir: %r[artifacts/tmp/cache],
+ work_dir: %r[artifacts/tmp/work]
describe '#filename' do
# we need to use uploader, as this makes to use mounter
@@ -69,7 +42,7 @@ describe LegacyArtifactUploader do
subject { uploader.file.path }
- it { is_expected.to start_with(local_path) }
+ it { is_expected.to start_with("#{uploader.root}") }
it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") }
it { is_expected.to include("/#{job.project_id}/") }
it { is_expected.to end_with("ci_build_artifacts.zip") }
diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb
index 7088bc23334..6ebc885daa8 100644
--- a/spec/uploaders/lfs_object_uploader_spec.rb
+++ b/spec/uploaders/lfs_object_uploader_spec.rb
@@ -2,39 +2,13 @@ require 'spec_helper'
describe LfsObjectUploader do
let(:lfs_object) { create(:lfs_object, :with_file) }
- let(:uploader) { described_class.new(lfs_object) }
+ let(:uploader) { described_class.new(lfs_object, :file) }
let(:path) { Gitlab.config.lfs.storage_path }
- describe '#move_to_cache' do
- it 'is true' do
- expect(uploader.move_to_cache).to eq(true)
- end
- end
+ subject { uploader }
- describe '#move_to_store' do
- it 'is true' do
- expect(uploader.move_to_store).to eq(true)
- end
- end
-
- describe '#store_dir' do
- subject { uploader.store_dir }
-
- it { is_expected.to start_with(path) }
- it { is_expected.to end_with("#{lfs_object.oid[0, 2]}/#{lfs_object.oid[2, 2]}") }
- end
-
- describe '#cache_dir' do
- subject { uploader.cache_dir }
-
- it { is_expected.to start_with(path) }
- it { is_expected.to end_with('/tmp/cache') }
- end
-
- describe '#work_dir' do
- subject { uploader.work_dir }
-
- it { is_expected.to start_with(path) }
- it { is_expected.to end_with('/tmp/work') }
- end
+ it_behaves_like "builds correct paths",
+ store_dir: %r[\h{2}/\h{2}],
+ cache_dir: %r[/lfs-objects/tmp/cache],
+ work_dir: %r[/lfs-objects/tmp/work]
end
diff --git a/spec/uploaders/namespace_file_uploader_spec.rb b/spec/uploaders/namespace_file_uploader_spec.rb
index c6c4500c179..24a2fc0f72e 100644
--- a/spec/uploaders/namespace_file_uploader_spec.rb
+++ b/spec/uploaders/namespace_file_uploader_spec.rb
@@ -1,21 +1,16 @@
require 'spec_helper'
+IDENTIFIER = %r{\h+/\S+}
+
describe NamespaceFileUploader do
let(:group) { build_stubbed(:group) }
let(:uploader) { described_class.new(group) }
+ let(:upload) { create(:upload, :namespace_upload, model: group) }
- describe "#store_dir" do
- it "stores in the namespace id directory" do
- expect(uploader.store_dir).to include(group.id.to_s)
- end
- end
-
- describe ".absolute_path" do
- it "stores in thecorrect directory" do
- upload_record = create(:upload, :namespace_upload, model: group)
+ subject { uploader }
- expect(described_class.absolute_path(upload_record))
- .to include("-/system/namespace/#{group.id}")
- end
- end
+ it_behaves_like 'builds correct paths',
+ store_dir: %r[uploads/-/system/namespace/\d+],
+ upload_path: IDENTIFIER,
+ absolute_path: %r[#{CarrierWave.root}/uploads/-/system/namespace/\d+/#{IDENTIFIER}]
end
diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb
index cbafa9f478d..ed1fba6edda 100644
--- a/spec/uploaders/personal_file_uploader_spec.rb
+++ b/spec/uploaders/personal_file_uploader_spec.rb
@@ -1,25 +1,27 @@
require 'spec_helper'
+IDENTIFIER = %r{\h+/\S+}
+
describe PersonalFileUploader do
- let(:uploader) { described_class.new(build_stubbed(:project)) }
- let(:snippet) { create(:personal_snippet) }
+ let(:model) { create(:personal_snippet) }
+ let(:uploader) { described_class.new(model) }
+ let(:upload) { create(:upload, :personal_snippet_upload) }
- describe '.absolute_path' do
- it 'returns the correct absolute path by building it dynamically' do
- upload = double(model: snippet, path: 'secret/foo.jpg')
+ subject { uploader }
- dynamic_segment = "personal_snippet/#{snippet.id}"
-
- expect(described_class.absolute_path(upload)).to end_with("/-/system/#{dynamic_segment}/secret/foo.jpg")
- end
- end
+ it_behaves_like 'builds correct paths',
+ store_dir: %r[uploads/-/system/personal_snippet/\d+],
+ upload_path: IDENTIFIER,
+ absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet/\d+/#{IDENTIFIER}]
describe '#to_h' do
- it 'returns the hass' do
- uploader = described_class.new(snippet, 'secret')
+ before do
+ subject.instance_variable_set(:@secret, 'secret')
+ end
+ it 'is correct' do
allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
- expected_url = "/uploads/-/system/personal_snippet/#{snippet.id}/secret/file_name"
+ expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name"
expect(uploader.to_h).to eq(
alt: 'file_name',
diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb
index 7ef7fb7d758..9a3e5d83e01 100644
--- a/spec/uploaders/records_uploads_spec.rb
+++ b/spec/uploaders/records_uploads_spec.rb
@@ -3,16 +3,16 @@ require 'rails_helper'
describe RecordsUploads do
let!(:uploader) do
class RecordsUploadsExampleUploader < GitlabUploader
- include RecordsUploads
+ include RecordsUploads::Concern
storage :file
- def model
- FactoryBot.build_stubbed(:user)
+ def dynamic_segment
+ 'co/fe/ee'
end
end
- RecordsUploadsExampleUploader.new
+ RecordsUploadsExampleUploader.new(build_stubbed(:user))
end
def upload_fixture(filename)
@@ -20,48 +20,55 @@ describe RecordsUploads do
end
describe 'callbacks' do
- it 'calls `record_upload` after `store`' do
+ let(:upload) { create(:upload) }
+
+ before do
+ uploader.upload = upload
+ end
+
+ it '#record_upload after `store`' do
expect(uploader).to receive(:record_upload).once
uploader.store!(upload_fixture('doc_sample.txt'))
end
- it 'calls `destroy_upload` after `remove`' do
- expect(uploader).to receive(:destroy_upload).once
-
+ it '#destroy_upload after `remove`' do
uploader.store!(upload_fixture('doc_sample.txt'))
+ expect(uploader).to receive(:destroy_upload).once
uploader.remove!
end
end
describe '#record_upload callback' do
- it 'returns early when not using file storage' do
- allow(uploader).to receive(:file_storage?).and_return(false)
- expect(Upload).not_to receive(:record)
-
- uploader.store!(upload_fixture('rails_sample.jpg'))
+ it 'creates an Upload record after store' do
+ expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to change { Upload.count }.by(1)
end
- it "returns early when the file doesn't exist" do
- allow(uploader).to receive(:file).and_return(double(exists?: false))
- expect(Upload).not_to receive(:record)
-
+ it 'creates a new record and assigns size, path, model, and uploader' do
uploader.store!(upload_fixture('rails_sample.jpg'))
+
+ upload = uploader.upload
+ aggregate_failures do
+ expect(upload).to be_persisted
+ expect(upload.size).to eq uploader.file.size
+ expect(upload.path).to eq uploader.upload_path
+ expect(upload.model_id).to eq uploader.model.id
+ expect(upload.model_type).to eq uploader.model.class.to_s
+ expect(upload.uploader).to eq uploader.class.to_s
+ end
end
- it 'creates an Upload record after store' do
- expect(Upload).to receive(:record)
- .with(uploader)
+ it "does not create an Upload record when the file doesn't exist" do
+ allow(uploader).to receive(:file).and_return(double(exists?: false))
- uploader.store!(upload_fixture('rails_sample.jpg'))
+ expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
end
it 'does not create an Upload record if model is missing' do
- expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
- expect(Upload).not_to receive(:record).with(uploader)
+ allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
- uploader.store!(upload_fixture('rails_sample.jpg'))
+ expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
end
it 'it destroys Upload records at the same path before recording' do
@@ -72,29 +79,15 @@ describe RecordsUploads do
uploader: uploader.class.to_s
)
+ uploader.upload = existing
uploader.store!(upload_fixture('rails_sample.jpg'))
expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
- expect(Upload.count).to eq 1
+ expect(Upload.count).to eq(1)
end
end
describe '#destroy_upload callback' do
- it 'returns early when not using file storage' do
- uploader.store!(upload_fixture('rails_sample.jpg'))
-
- allow(uploader).to receive(:file_storage?).and_return(false)
- expect(Upload).not_to receive(:remove_path)
-
- uploader.remove!
- end
-
- it 'returns early when file is nil' do
- expect(Upload).not_to receive(:remove_path)
-
- uploader.remove!
- end
-
it 'it destroys Upload records at the same path after removal' do
uploader.store!(upload_fixture('rails_sample.jpg'))
diff --git a/spec/workers/upload_checksum_worker_spec.rb b/spec/workers/upload_checksum_worker_spec.rb
index 911360da66c..9e50ce15871 100644
--- a/spec/workers/upload_checksum_worker_spec.rb
+++ b/spec/workers/upload_checksum_worker_spec.rb
@@ -2,18 +2,31 @@ require 'rails_helper'
describe UploadChecksumWorker do
describe '#perform' do
- it 'rescues ActiveRecord::RecordNotFound' do
- expect { described_class.new.perform(999_999) }.not_to raise_error
+ subject { described_class.new }
+
+ context 'without a valid record' do
+ it 'rescues ActiveRecord::RecordNotFound' do
+ expect { subject.perform(999_999) }.not_to raise_error
+ end
end
- it 'calls calculate_checksum_without_delay and save!' do
- upload = spy
- expect(Upload).to receive(:find).with(999_999).and_return(upload)
+ context 'with a valid record' do
+ let(:upload) { create(:user, :with_avatar).avatar.upload }
+
+ before do
+ expect(Upload).to receive(:find).and_return(upload)
+ allow(upload).to receive(:foreground_checksumable?).and_return(false)
+ end
- described_class.new.perform(999_999)
+ it 'calls calculate_checksum!' do
+ expect(upload).to receive(:calculate_checksum!)
+ subject.perform(upload.id)
+ end
- expect(upload).to have_received(:calculate_checksum)
- expect(upload).to have_received(:save!)
+ it 'calls save!' do
+ expect(upload).to receive(:save!)
+ subject.perform(upload.id)
+ end
end
end
end