diff options
74 files changed, 822 insertions, 914 deletions
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 61554029d09..a6fb1f40001 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -1,8 +1,6 @@ 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 @@ -19,71 +17,34 @@ module UploadsActions end end - # This should either - # - send the file directly - # - or redirect to its URL - # def show return render_404 unless uploader.exists? - 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 - else - redirect_to uploader.url - end - end - - private + disposition = uploader.image_or_video? ? 'inline' : 'attachment' - def uploader_class - raise NotImplementedError - end + expires_in 0.seconds, must_revalidate: true, private: true - def upload_mount - mounted_as = params[:mounted_as] - mounted_as if UPLOAD_MOUNTS.include?(mounted_as) + send_file uploader.file.path, disposition: disposition end - def uploader_mounted? - upload_model_class < CarrierWave::Mount::Extension && !upload_mount.nil? - end + private def uploader strong_memoize(:uploader) do - if uploader_mounted? - model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend - else - build_uploader_from_upload || build_uploader_from_params - end - end - end - - def build_uploader_from_upload - return nil unless params[:secret] && params[:filename] + return if show_model.nil? - 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 + file_uploader = FileUploader.new(show_model, params[:secret]) + file_uploader.retrieve_from_store!(params[:filename]) - def build_uploader_from_params - uploader = uploader_class.new(model, params[:secret]) - uploader.retrieve_from_store!(params[:filename]) - uploader + file_uploader + end end def image_or_video? uploader && uploader.exists? && uploader.image_or_video? end - def find_model - nil - end - - def model - strong_memoize(:model) { find_model } + def uploader_class + FileUploader end end diff --git a/app/controllers/groups/uploads_controller.rb b/app/controllers/groups/uploads_controller.rb index f1578f75e88..e6bd9806401 100644 --- a/app/controllers/groups/uploads_controller.rb +++ b/app/controllers/groups/uploads_controller.rb @@ -7,23 +7,29 @@ class Groups::UploadsController < Groups::ApplicationController private - def upload_model_class - Group - end + def show_model + strong_memoize(:show_model) do + group_id = params[:group_id] - def uploader_class - NamespaceFileUploader + Group.find_by_full_path(group_id) + end end - def find_model - return @group if @group - - group_id = params[:group_id] + def authorize_upload_file! + render_404 unless can?(current_user, :upload_file, group) + end - Group.find_by_full_path(group_id) + 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 authorize_upload_file! - render_404 unless can?(current_user, :upload_file, group) + def uploader_class + NamespaceFileUploader end + + alias_method :model, :group end diff --git a/app/controllers/projects/lfs_storage_controller.rb b/app/controllers/projects/lfs_storage_controller.rb index 941638db427..293869345bd 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(LfsObjectUploader.workhorse_upload_path, tmp_file) + tmp_file_path = File.join("#{Gitlab.config.lfs.storage_path}/tmp/upload", 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 f5cf089ad98..4685bbe80b4 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -1,7 +1,6 @@ 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? } @@ -9,20 +8,14 @@ class Projects::UploadsController < Projects::ApplicationController private - def upload_model_class - Project - end + def show_model + strong_memoize(:show_model) do + namespace = params[:namespace_id] + id = params[:project_id] - def uploader_class - FileUploader + Project.find_by_full_path("#{namespace}/#{id}") + end end - def find_model - return @project if @project - - namespace = params[:namespace_id] - id = params[:project_id] - - Project.find_by_full_path("#{namespace}/#{id}") - end + alias_method :model, :project end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 3d227b0a955..16a74f82d3f 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -1,34 +1,19 @@ 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] - def uploader_class - PersonalFileUploader - end + private def find_model return nil unless params[:id] - upload_model_class.find(params[:id]) + return render_404 unless upload_model && upload_mount + + @model = upload_model.find(params[:id]) end def authorize_access! @@ -68,17 +53,55 @@ class UploadsController < ApplicationController end end - def upload_model_class - MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError) + 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 end - def upload_model_class_has_mounts? - upload_model_class < CarrierWave::Mount::Extension + 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 end - def upload_mount_satisfied? - return true unless upload_model_class_has_mounts? + def uploader_class + PersonalFileUploader + end - upload_model_class.uploader_options.has_key?(upload_mount) + def model + @model ||= find_model end end diff --git a/app/models/appearance.rb b/app/models/appearance.rb index dcd14c08f3c..76cfe28742a 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -11,7 +11,6 @@ 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 d35e37935fb..10659030910 100644 --- a/app/models/concerns/avatarable.rb +++ b/app/models/concerns/avatarable.rb @@ -1,30 +1,6 @@ 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 62b1322ebe6..fddace03387 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -29,14 +29,18 @@ class Group < Namespace has_many :variables, class_name: 'Ci::GroupVariable' has_many :custom_attributes, class_name: 'GroupCustomAttribute' - has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - + validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } 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 after_save :update_two_factor_requirement @@ -112,6 +116,12 @@ 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 a84db8982e5..184fbd5f5ae 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -88,7 +88,6 @@ 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 90f5df6265d..4def590a7a9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -256,6 +256,9 @@ 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 @@ -263,6 +266,7 @@ 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 @@ -285,6 +289,7 @@ 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) } @@ -918,12 +923,20 @@ 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) - Gitlab::Routing.url_helpers.project_avatar_url(self) if avatar_in_git + # 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) end # For compatibility with old code diff --git a/app/models/upload.rb b/app/models/upload.rb index fb55fd8007b..f194d7bdb80 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -9,11 +9,22 @@ class Upload < ActiveRecord::Base validates :model, presence: true validates :uploader, presence: true - before_save :calculate_checksum!, if: :foreground_checksummable? - after_commit :schedule_checksum, if: :checksummable? + before_save :calculate_checksum, if: :foreground_checksum? + after_commit :schedule_checksum, unless: :foreground_checksum? - def self.hexdigest(path) - Digest::SHA256.file(path).hexdigest + 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 + ) end def absolute_path @@ -22,18 +33,10 @@ class Upload < ActiveRecord::Base uploader_class.absolute_path(self) end - def calculate_checksum! - self.checksum = nil - return unless checksummable? + def calculate_checksum + return unless exist? - 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 + self.checksum = Digest::SHA256.file(absolute_path).hexdigest end def exist? @@ -42,16 +45,8 @@ class Upload < ActiveRecord::Base private - def checksummable? - checksum.nil? && local? && exist? - end - - def local? - true - end - - def foreground_checksummable? - checksummable? && size <= CHECKSUM_THRESHOLD + def foreground_checksum? + size <= CHECKSUM_THRESHOLD end def schedule_checksum @@ -62,10 +57,6 @@ 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 89e787c3274..fb5d56a68b0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -137,7 +137,6 @@ 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 @@ -160,10 +159,12 @@ 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? @@ -224,6 +225,9 @@ 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) } @@ -523,6 +527,12 @@ 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') @@ -850,7 +860,9 @@ class User < ActiveRecord::Base end def avatar_url(size: nil, scale: 2, **args) - GravatarService.new.execute(email, size, scale, username: username) + # 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) 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 bc897d891d5..f8aaec8a9c0 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.absolute_base_dir(project) + origin = FileUploader.dynamic_path_segment(project) project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] - target = FileUploader.absolute_base_dir(project) + target = FileUploader.dynamic_path_segment(project) result = move_folder!(origin, target) project.save! diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb index 4930fb2fca7..109eb2fea0b 100644 --- a/app/uploaders/attachment_uploader.rb +++ b/app/uploaders/attachment_uploader.rb @@ -1,12 +1,10 @@ class AttachmentUploader < GitlabUploader + include RecordsUploads include UploaderHelper - include RecordsUploads::Concern storage :file - private - - def dynamic_segment - File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s) + def store_dir + "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end end diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb index 5c8e1cea62e..cbb79376d5f 100644 --- a/app/uploaders/avatar_uploader.rb +++ b/app/uploaders/avatar_uploader.rb @@ -1,24 +1,25 @@ class AvatarUploader < GitlabUploader + include RecordsUploads include UploaderHelper - include RecordsUploads::Concern storage :file - def exists? - model.avatar.file && model.avatar.file.present? + def store_dir + "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end - def move_to_cache - false + 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 false end - private - - def dynamic_segment - File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s) + def move_to_cache + false end end diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index e7af1483d23..00c2888d224 100644 --- a/app/uploaders/file_mover.rb +++ b/app/uploaders/file_mover.rb @@ -21,8 +21,7 @@ class FileMover end def update_markdown - updated_text = model.read_attribute(update_field) - .gsub(temp_file_uploader.markdown_link, uploader.markdown_link) + updated_text = model.read_attribute(update_field).gsub(temp_file_uploader.to_markdown, uploader.to_markdown) model.update_attribute(update_field, updated_text) true diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 85ae9863b13..0b591e3bbbb 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -1,38 +1,23 @@ -# 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.root - File.join(options.storage_path, 'uploads') - end - - def self.absolute_path(upload) + def self.absolute_path(upload_record) File.join( - absolute_base_dir(upload.model), - upload.path # already contain the dynamic_segment, see #upload_path + self.dynamic_path_segment(upload_record.model), + upload_record.path ) end - 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)) + # Not using `GitlabUploader.base_dir` because all project namespaces are in + # the `public/uploads` dir. + # + def self.base_dir + root_dir end # Returns the part of `store_dir` that can change based on the model's current @@ -44,96 +29,63 @@ class FileUploader < GitlabUploader # model - Object that responds to `full_path` and `disk_path` # # Returns a String without a trailing slash - def self.model_path_segment(model) + def self.dynamic_path_segment(model) if model.hashed_storage?(:attachments) - model.disk_path + dynamic_path_builder(model.disk_path) else - model.full_path + dynamic_path_builder(model.full_path) end end - def self.upload_path(secret, identifier) - File.join(secret, identifier) - end - - def self.generate_secret - SecureRandom.hex + # 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) end attr_accessor :model + attr_reader :secret def initialize(model, secret = nil) @model = model - @secret = secret - end - - def base_dir - self.class.base_dir(@model) + @secret = secret || generate_secret end - # 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) + def store_dir + File.join(dynamic_path_segment, @secret) end - def upload_path - self.class.upload_path(dynamic_segment, identifier) + def relative_path + self.file.path.sub("#{dynamic_path_segment}/", '') end - def model_path_segment - self.class.model_path_segment(@model) + def to_markdown + to_h[:markdown] end - def store_dir - File.join(base_dir, dynamic_segment) - end + def to_h + filename = image_or_video? ? self.file.basename : self.file.filename + escaped_filename = filename.gsub("]", "\\]") - def markdown_link - markdown = "[#{markdown_name}](#{secure_url})" + markdown = "[#{escaped_filename}](#{secure_url})" markdown.prepend("!") if image_or_video? || dangerous? - markdown - end - def to_h { - alt: markdown_name, + alt: filename, url: secure_url, - markdown: markdown_link + markdown: markdown } 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 markdown_name - (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]") + def dynamic_path_segment + self.class.dynamic_path_segment(model) end - def identifier - @identifier ||= filename - end - - def dynamic_segment - secret + def generate_secret + SecureRandom.hex end def secure_url diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index b12829efe73..7f72b3ce471 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -1,31 +1,27 @@ class GitlabUploader < CarrierWave::Uploader::Base - class_attribute :options - - class << self - # DSL setter - def storage_options(options) - self.options = options - end - - def root - options.storage_path - end + def self.absolute_path(upload_record) + File.join(CarrierWave.root, upload_record.path) + end - # represent the directory namespacing at the class level - def base_dir - options.fetch('base_dir', '') - end + def self.root_dir + 'uploads' + end - def file_storage? - storage == CarrierWave::Storage::File - 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 absolute_path(upload_record) - File.join(root, upload_record.path) - end + File.join(root_dir, '-', 'system') end - storage_options Gitlab.config.uploads + def self.file_storage? + self.storage == CarrierWave::Storage::File + end delegate :base_dir, :file_storage?, to: :class @@ -35,28 +31,34 @@ class GitlabUploader < CarrierWave::Uploader::Base # Reduce disk IO def move_to_cache - file_storage? + true end # Reduce disk IO def move_to_store - file_storage? + true end - def exists? - file.present? - end - - def store_dir - File.join(base_dir, dynamic_segment) + # 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}/", '') end - def cache_dir - File.join(root, base_dir, 'tmp/cache') + def exists? + file.present? end + # Override this if you don't want to save files by default to the Rails.root directory def work_dir - File.join(root, base_dir, 'tmp/work') + # Default path set by CarrierWave: + # https://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L182 + CarrierWave.tmp_path end def filename @@ -65,13 +67,6 @@ 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 @@ -79,6 +74,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 0abb462ab7d..15dfb5a5763 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -1,7 +1,13 @@ class JobArtifactUploader < GitlabUploader - extend Workhorse::UploadPath + storage :file - storage_options Gitlab.config.artifacts + def self.local_store_path + Gitlab.config.artifacts.path + end + + def self.artifacts_upload_path + File.join(self.local_store_path, 'tmp/uploads/') + end def size return super if model.size.nil? @@ -10,12 +16,24 @@ class JobArtifactUploader < GitlabUploader end def store_dir - dynamic_segment + 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') end private - def dynamic_segment + def default_local_path + File.join(self.class.local_store_path, default_path) + end + + def default_path 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 28c458d3ff1..4f7f8a63108 100644 --- a/app/uploaders/legacy_artifact_uploader.rb +++ b/app/uploaders/legacy_artifact_uploader.rb @@ -1,15 +1,33 @@ class LegacyArtifactUploader < GitlabUploader - extend Workhorse::UploadPath + storage :file - storage_options Gitlab.config.artifacts + def self.local_store_path + Gitlab.config.artifacts.path + end + + def self.artifacts_upload_path + File.join(self.local_store_path, 'tmp/uploads/') + end def store_dir - dynamic_segment + 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') end private - def dynamic_segment + def default_local_path + File.join(self.class.local_store_path, default_path) + end + + def default_path 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 e04c97ce179..d11ebf0f9ca 100644 --- a/app/uploaders/lfs_object_uploader.rb +++ b/app/uploaders/lfs_object_uploader.rb @@ -1,24 +1,19 @@ class LfsObjectUploader < GitlabUploader - extend Workhorse::UploadPath + storage :file - # LfsObject are in `tmp/upload` instead of `tmp/uploads` - def self.workhorse_upload_path - File.join(root, 'tmp/upload') + def store_dir + "#{Gitlab.config.lfs.storage_path}/#{model.oid[0, 2]}/#{model.oid[2, 2]}" end - storage_options Gitlab.config.lfs + def cache_dir + "#{Gitlab.config.lfs.storage_path}/tmp/cache" + end def filename model.oid[4..-1] end - def store_dir - dynamic_segment - end - - private - - def dynamic_segment - File.join(model.oid[0, 2], model.oid[2, 2]) + def work_dir + File.join(Gitlab.config.lfs.storage_path, 'tmp', 'work') end end diff --git a/app/uploaders/namespace_file_uploader.rb b/app/uploaders/namespace_file_uploader.rb index 993e85fbc13..672126e9ec2 100644 --- a/app/uploaders/namespace_file_uploader.rb +++ b/app/uploaders/namespace_file_uploader.rb @@ -1,19 +1,15 @@ class NamespaceFileUploader < FileUploader - # Re-Override - def self.root - options.storage_path + def self.base_dir + File.join(root_dir, '-', 'system', 'namespace') end - def self.base_dir(model) - File.join(options.base_dir, 'namespace', model_path_segment(model)) + def self.dynamic_path_segment(model) + dynamic_path_builder(model.id.to_s) end - def self.model_path_segment(model) - File.join(model.id.to_s) - end + private - # Re-Override - def store_dir - File.join(base_dir, dynamic_segment) + def secure_url + File.join('/uploads', @secret, file.filename) end end diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index e7d9ecd3222..3298ad104ec 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -1,27 +1,23 @@ class PersonalFileUploader < FileUploader - # Re-Override - def self.root - options.storage_path + def self.dynamic_path_segment(model) + File.join(CarrierWave.root, model_path(model)) end - def self.base_dir(model) - File.join(options.base_dir, model_path_segment(model)) - end - - def self.model_path_segment(model) - return 'temp/' unless model - - File.join(model.class.to_s.underscore, model.id.to_s) - end - - # Revert-Override - def store_dir - File.join(base_dir, dynamic_segment) + def self.base_dir + File.join(root_dir, '-', 'system') end private def secure_url - File.join('/', base_dir, secret, file.filename) + File.join(self.class.model_path(model), secret, file.filename) + 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 end end diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index dfb8dccec57..feb4f04d7b7 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -1,61 +1,35 @@ module RecordsUploads - module Concern - extend ActiveSupport::Concern + extend ActiveSupport::Concern - attr_accessor :upload - - 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 + included do + after :store, :record_upload + before :remove, :destroy_upload + end - def uploads - Upload.order(id: :desc).where(uploader: self.class.to_s) - 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_storage? + return unless file.exists? + + Upload.record(self) + end - 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 + private - # Before removing an attachment, destroy any Upload records at the same path - # - # Called `before :remove` - def destroy_upload(*args) - return unless file && file.exists? + # 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 - self.upload = nil - uploads.where(path: upload_path).delete_all - end + Upload.remove_path(relative_path) end end diff --git a/app/uploaders/uploader_helper.rb b/app/uploaders/uploader_helper.rb index fd446d31092..7635c20ab3a 100644 --- a/app/uploaders/uploader_helper.rb +++ b/app/uploaders/uploader_helper.rb @@ -32,7 +32,14 @@ module UploaderHelper def extension_match?(extensions) return false unless file - extension = file.try(:extension) || File.extname(file.path).delete('.') + extension = + if file.respond_to?(:extension) + file.extension + else + # Not all CarrierWave storages respond to :extension + File.extname(file.path).delete('.') + end + extensions.include?(extension.downcase) end end diff --git a/app/uploaders/workhorse.rb b/app/uploaders/workhorse.rb deleted file mode 100644 index 782032cf516..00000000000 --- a/app/uploaders/workhorse.rb +++ /dev/null @@ -1,7 +0,0 @@ -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 65d40336f18..9222760c031 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 33230b9355d..25f4085deb2 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -152,12 +152,6 @@ 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 @@ -650,8 +644,6 @@ 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 5ad46d47cb6..5b4e6b5db88 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -300,10 +300,8 @@ 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['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 +Settings.artifacts['path'] = Settings.absolute(Settings.artifacts['path'] || File.join(Settings.shared['path'], "artifacts")) +Settings.artifacts['max_size'] ||= 100 # in megabytes # # Registry @@ -341,13 +339,6 @@ 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 deleted file mode 100644 index a678c3d049f..00000000000 --- a/db/migrate/20180119135717_add_uploader_index_to_uploads.rb +++ /dev/null @@ -1,20 +0,0 @@ -# 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/db/schema.rb b/db/schema.rb index 01a2df13dd3..0d97b6f9ddd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1755,7 +1755,7 @@ ActiveRecord::Schema.define(version: 20180201145907) do add_index "uploads", ["checksum"], name: "index_uploads_on_checksum", using: :btree add_index "uploads", ["model_id", "model_type"], name: "index_uploads_on_model_id_and_model_type", using: :btree - add_index "uploads", ["uploader", "path"], name: "index_uploads_on_uploader_and_path", using: :btree + add_index "uploads", ["path"], name: "index_uploads_on_path", using: :btree create_table "user_agent_details", force: :cascade do |t| t.string "user_agent", null: false diff --git a/doc/development/file_storage.md b/doc/development/file_storage.md index 76354b92820..cf00e24e11a 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/Notes Markdown attachments - - Issues/MR/Notes Legacy Markdown attachments + - Issues/MR Markdown attachments + - Issues/MR 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 (from CarrierWave.root) | Uploader class | model_type | +| Description | In DB? | Relative path | 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,107 +33,17 @@ 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/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 | +| 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 | | 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 `ObjectStorage` and store files in and S3 API compatible object store. +while in EE they inherit the `ObjectStoreUploader` and store files in and S3 API compatible object store. -In the case of Issues/MR/Notes Markdown attachments, there is a different approach using the [Hashed Storage] layout, +In the case of Issues/MR 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 1f80646a2ea..80feb629d54 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? - workhorse_upload_path = JobArtifactUploader.workhorse_upload_path - artifacts = uploaded_file(:file, workhorse_upload_path) - metadata = uploaded_file(:metadata, workhorse_upload_path) + artifacts_upload_path = JobArtifactUploader.artifacts_upload_path + artifacts = uploaded_file(:file, artifacts_upload_path) + metadata = uploaded_file(:metadata, artifacts_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 4383124d150..7a582a20056 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', JobArtifactUploader.root) + super('artifacts', LegacyArtifactUploader.local_store_path) 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 8a8e770940e..d60e41d9f9d 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(Gitlab.config.uploads.storage_path, path) + File.join(CarrierWave.root, path) end end diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb index a7a1bbe1752..4e0121ca34d 100644 --- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb +++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb @@ -11,12 +11,9 @@ module Gitlab FIND_BATCH_SIZE = 500 RELATIVE_UPLOAD_DIR = "uploads".freeze - ABSOLUTE_UPLOAD_DIR = File.join( - Gitlab.config.uploads.storage_path, - RELATIVE_UPLOAD_DIR - ) + ABSOLUTE_UPLOAD_DIR = "#{CarrierWave.root}/#{RELATIVE_UPLOAD_DIR}".freeze FOLLOW_UP_MIGRATION = 'PopulateUntrackedUploads'.freeze - START_WITH_ROOT_REGEX = %r{\A#{Gitlab.config.uploads.storage_path}/} + START_WITH_CARRIERWAVE_ROOT_REGEX = %r{\A#{CarrierWave.root}/} EXCLUDED_HASHED_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/@hashed/*".freeze EXCLUDED_TMP_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/tmp/*".freeze @@ -84,7 +81,7 @@ module Gitlab paths = [] stdout.each_line("\0") do |line| - paths << line.chomp("\0").sub(START_WITH_ROOT_REGEX, '') + paths << line.chomp("\0").sub(START_WITH_CARRIERWAVE_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 3fdc3c27f73..8fab5489616 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.markdown_link + new_uploader.to_markdown end end diff --git a/lib/gitlab/import_export/uploads_saver.rb b/lib/gitlab/import_export/uploads_saver.rb index 2f08dda55fd..627a487d577 100644 --- a/lib/gitlab/import_export/uploads_saver.rb +++ b/lib/gitlab/import_export/uploads_saver.rb @@ -17,13 +17,15 @@ module Gitlab false end - def uploads_path - FileUploader.absolute_base_dir(@project) - end + private 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 7d7400bdabf..b5f41240529 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 - FileUploader.root + File.join(CarrierWave.root, FileUploader.base_dir) end end end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index b3f8b0d174d..633da44b22d 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -55,14 +55,14 @@ module Gitlab def lfs_upload_ok(oid, size) { - StoreLFSPath: LfsObjectUploader.workhorse_upload_path, + StoreLFSPath: "#{Gitlab.config.lfs.storage_path}/tmp/upload", LfsOid: oid, LfsSize: size } end def artifact_upload_ok - { TempPath: JobArtifactUploader.workhorse_upload_path } + { TempPath: JobArtifactUploader.artifacts_upload_path } end def send_git_blob(repository, blob) @@ -147,11 +147,8 @@ module Gitlab end def send_artifacts_entry(build, entry) - file = build.artifacts_file - archive = file.file_storage? ? file.path : file.url - params = { - 'Archive' => archive, + 'Archive' => build.artifacts_file.path, '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 6a1869d1a48..67a11e56e94 100644 --- a/spec/controllers/groups/uploads_controller_spec.rb +++ b/spec/controllers/groups/uploads_controller_spec.rb @@ -6,7 +6,5 @@ describe Groups::UploadsController do { group_id: model } end - it_behaves_like 'handle uploads' do - let(:uploader_class) { NamespaceFileUploader } - end + it_behaves_like 'handle uploads' end diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb index 25a2e13fe1a..12cb7b2647f 100644 --- a/spec/controllers/projects/artifacts_controller_spec.rb +++ b/spec/controllers/projects/artifacts_controller_spec.rb @@ -145,7 +145,8 @@ 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(:archive_path) { JobArtifactUploader.root } + let(:store) { ObjectStoreUploader::LOCAL_STORE } + let(:archive_path) { JobArtifactUploader.local_store_path } end end end diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index b7df42168e0..3a0c3faa7b4 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("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment') + expect(controller).to receive(:send_file).with("#{Gitlab.config.shared.path}/lfs-objects/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 376b229ffc9..b1f601a19e5 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -180,7 +180,6 @@ 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 @@ -197,7 +196,6 @@ 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 @@ -222,7 +220,6 @@ 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 @@ -242,7 +239,6 @@ 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 @@ -295,7 +291,6 @@ 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 @@ -327,7 +322,6 @@ 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 @@ -347,7 +341,6 @@ 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 @@ -391,7 +384,6 @@ 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 @@ -428,7 +420,6 @@ 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 @@ -448,7 +439,6 @@ 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 @@ -501,7 +491,6 @@ 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 @@ -533,7 +522,6 @@ 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 @@ -553,7 +541,6 @@ 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/groups.rb b/spec/factories/groups.rb index 8c531cf5909..1512f5a0e58 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -18,7 +18,7 @@ FactoryBot.define do end trait :with_avatar do - avatar { fixture_file_upload('spec/fixtures/dk.png') } + avatar { File.open(Rails.root.join('spec/fixtures/dk.png')) } end trait :access_requestable do diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 3f4e408b3a6..2defb4935ad 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.join( "spec/fixtures/dk.png"), "image/png") } + attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") } end trait :with_svg_attachment do - attachment { fixture_file_upload(Rails.root.join("spec/fixtures/unsanitized.svg"), "image/svg+xml") } + attachment { fixture_file_upload(Rails.root + "spec/fixtures/unsanitized.svg", "image/svg+xml") } end transient do diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 16d328a5bc2..d0f3911f730 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -90,7 +90,7 @@ FactoryBot.define do end trait :with_avatar do - avatar { fixture_file_upload('spec/fixtures/dk.png') } + avatar { File.open(Rails.root.join('spec/fixtures/dk.png')) } end trait :broken_storage do diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb index c8cfe251d27..c39500faea1 100644 --- a/spec/factories/uploads.rb +++ b/spec/factories/uploads.rb @@ -1,42 +1,24 @@ FactoryBot.define do factory :upload do model { build(:project) } + path { "uploads/-/system/project/avatar/avatar.jpg" } size 100.kilobytes uploader "AvatarUploader" - # 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 + trait :personal_snippet do model { build(:personal_snippet) } - path { File.join(secret, 'myfile.jpg') } uploader "PersonalFileUploader" end trait :issuable_upload do - path { File.join(secret, 'myfile.jpg') } + path { "#{SecureRandom.hex}/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/factories/users.rb b/spec/factories/users.rb index 769fd656e7a..e62e0b263ca 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -38,7 +38,7 @@ FactoryBot.define do end trait :with_avatar do - avatar { fixture_file_upload('spec/fixtures/dk.png') } + avatar { File.open(Rails.root.join('spec/fixtures/dk.png')) } end trait :two_factor_via_otp do 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 370c2490b97..8bb9ebe0419 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -23,27 +23,6 @@ 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 @@ -130,8 +109,24 @@ 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 - it_behaves_like 'does not add files in /uploads/tmp' + 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 end end end @@ -202,8 +197,24 @@ 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 - it_behaves_like 'does not add files in /uploads/tmp' + 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 end end end diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index 326ed2f2ecf..39e3b875c49 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.markdown_link} and #{zip_uploader.markdown_link}" + "Text and #{image_uploader.to_markdown} and #{zip_uploader.to_markdown}" 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 a685521cbf0..63992ea8ab8 100644 --- a/spec/lib/gitlab/import_export/uploads_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/uploads_restorer_spec.rb @@ -4,6 +4,7 @@ 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) @@ -25,9 +26,9 @@ describe Gitlab::ImportExport::UploadsRestorer do end it 'copies the uploads to the project path' do - subject.restore + restorer.restore - uploads = Dir.glob(File.join(subject.uploads_path, '**/*')).map { |file| File.basename(file) } + uploads = Dir.glob(File.join(uploads_path, '**/*')).map { |file| File.basename(file) } expect(uploads).to include('dummy.txt') end @@ -43,9 +44,9 @@ describe Gitlab::ImportExport::UploadsRestorer do end it 'copies the uploads to the project path' do - subject.restore + restorer.restore - uploads = Dir.glob(File.join(subject.uploads_path, '**/*')).map { |file| File.basename(file) } + uploads = Dir.glob(File.join(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 959779523f4..e8948de1f3a 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(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) } + uploads = Dir.glob(File.join(shared.export_path, 'uploads', '**/*')).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(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) } + uploads = Dir.glob(File.join(shared.export_path, 'uploads', '**/*')).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 6b7dbad128c..c3673a0e2a3 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) { FileUploader.root } + let(:uploads_dir) { File.join(CarrierWave.root, FileUploader.base_dir) } 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 42f3d609770..345382ea8c7 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -45,6 +45,51 @@ 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' @@ -66,27 +111,27 @@ describe Upload do end end - 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 + describe '#calculate_checksum' do + it 'calculates the SHA256 sum' do + upload = described_class.new( + path: __FILE__, + size: described_class::CHECKSUM_THRESHOLD - 1.megabyte + ) expected = Digest::SHA256.file(__FILE__).hexdigest - expect { upload.calculate_checksum! } + expect { upload.calculate_checksum } .to change { upload.checksum }.from(nil).to(expected) end - it 'sets `checksum` to nil for a non-existant file' do - expect(upload).to receive(:exist?).and_return(false) + it 'returns nil for a non-existant file' do + upload = described_class.new( + path: __FILE__, + size: described_class::CHECKSUM_THRESHOLD - 1.megabyte + ) - checksum = Digest::SHA256.file(__FILE__).hexdigest - upload.checksum = checksum + expect(upload).to receive(:exist?).and_return(false) - expect { upload.calculate_checksum! } - .to change { upload.checksum }.from(checksum).to(nil) + expect(upload.calculate_checksum).to be_nil end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index c5c0b0c2867..cb66d23b77c 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(:workhorse_upload_path).and_return('/') + allow(JobArtifactUploader).to receive(:artifacts_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(:workhorse_upload_path).and_return(@tmpdir) + allow(JobArtifactUploader).to receive(:artifacts_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 930ef49b7f3..bee918a20aa 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(LfsObjectUploader.workhorse_upload_path) + expect(json_response['StoreLFSPath']).to eq("#{Gitlab.config.shared.path}/lfs-objects/tmp/upload") 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(LfsObjectUploader.workhorse_upload_path) + expect(json_response['StoreLFSPath']).to eq("#{Gitlab.config.shared.path}/lfs-objects/tmp/upload") 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 322c91065e7..388c9d63c7b 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, group: create(:group)) } + let(:new_project) { create(:project) } 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.markdown_link}" } + let(:description) { "Text and #{uploader.to_markdown}" } 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 15699574b3a..50e59954f73 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.upload_path) } + let!(:upload) { Upload.find_by(path: file_uploader.relative_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) - File.join(FileUploader.root, storage.disk_path) + FileUploader.dynamic_path_builder(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 7ce80c82439..935c08221e0 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -2,8 +2,6 @@ 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 @@ -67,12 +65,7 @@ shared_examples 'handle uploads' do describe "GET #show" do let(:show_upload) do - 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 + get :show, params.merge(secret: "123456", filename: "image.jpg") end context "when the model is public" do @@ -82,6 +75,11 @@ 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 @@ -90,10 +88,6 @@ 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 @@ -108,6 +102,11 @@ 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 @@ -116,10 +115,6 @@ 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 @@ -136,6 +131,11 @@ 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,10 +149,6 @@ 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 @@ -162,10 +158,6 @@ 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 @@ -185,6 +177,11 @@ 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 @@ -193,10 +190,6 @@ 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 @@ -207,6 +200,11 @@ 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) @@ -220,10 +218,6 @@ 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 @@ -233,10 +227,6 @@ 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 deleted file mode 100644 index 934d53e7bba..00000000000 --- a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb +++ /dev/null @@ -1,48 +0,0 @@ -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 c275522159c..9e5f08fbc51 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.storage_path + Gitlab.config.artifacts.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 5752078d2a0..d05eda08201 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 091ba824fc6..04ee6e9bfad 100644 --- a/spec/uploaders/attachment_uploader_spec.rb +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -1,14 +1,28 @@ require 'spec_helper' describe AttachmentUploader do - let(:note) { create(:note, :with_attachment) } - let(:uploader) { note.attachment } - let(:upload) { create(:upload, :attachment_upload, model: uploader.model) } + let(:uploader) { described_class.new(build_stubbed(:user)) } - subject { uploader } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/-/system/user") + 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/] + 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 + + describe '#move_to_cache' do + it 'is true' do + expect(uploader.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(uploader.move_to_store).to eq(true) + end + end end diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb index bf9028c9260..1dc574699d8 100644 --- a/spec/uploaders/avatar_uploader_spec.rb +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -1,16 +1,18 @@ require 'spec_helper' describe AvatarUploader do - let(:model) { create(:user, :with_avatar) } - let(:uploader) { described_class.new(model, :avatar) } - let(:upload) { create(:upload, model: model) } + let(:uploader) { described_class.new(build_stubbed(:user)) } - subject { uploader } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/-/system/user") + 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/] + 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 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 bc024cd307c..0cf462e9553 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  "\ - "same  " + 'test  same ![banana_sample]'\ + '(/uploads/-/system/temp/secret55/banana_sample.gif)' end - let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) } + 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(: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  "\ - "same  " + "test "\ + " same " ) end @@ -50,8 +50,8 @@ describe FileMover do expect(snippet.reload.description) .to eq( - "test  "\ - "same  " + "test "\ + " same " ) end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index a72f853df75..845516e5004 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -1,57 +1,118 @@ require 'spec_helper' describe FileUploader do - 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') } + let(:uploader) { described_class.new(build_stubbed(:project)) } - subject { uploader } + 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 - 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} + 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 end - shared_examples 'uses hashed storage' do + context 'hashed storage' do context 'when rolled out attachments' do - before do - allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed') + 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 end - let(:project) { build_stubbed(:project, :hashed, namespace: group, name: 'project') } + describe "#store_dir" do + it "stores in the namespace path" do + uploader = described_class.new(project) - 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} + expect(uploader.store_dir).to include(project.disk_path) + expect(uploader.store_dir).not_to include("system") + end + end end context 'when only repositories are rolled out' do - let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } + let(:project) { build_stubbed(:project, storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } - it_behaves_like 'builds correct legacy storage paths' - end - end + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + upload = double(model: project, path: 'secret/foo.jpg') - context 'legacy storage' do - it_behaves_like 'builds correct legacy storage paths' - include_examples 'uses hashed storage' + 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) + + expect(uploader.store_dir).to include(project.full_path) + expect(uploader.store_dir).not_to include("system") + end + end + end end describe 'initialize' do - let(:uploader) { described_class.new(double, 'secret') } + 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(described_class).not_to receive(:generate_secret) - expect(uploader.secret).to eq('secret') + expect(SecureRandom).not_to receive(:hex) + + uploader = described_class.new(double, 'secret') + + expect(uploader.secret).to eq 'secret' end end - 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') + describe '#move_to_cache' do + it 'is true' do + expect(uploader.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(uploader.move_to_store).to eq(true) + 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}) end end end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index d606404e95d..a067c3e75f4 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -3,13 +3,33 @@ 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 } - subject { uploader } + describe '#store_dir' do + subject { uploader.store_dir } - 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] + 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 context 'file is stored in valid local_path' do let(:file) do @@ -23,7 +43,7 @@ describe JobArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with("#{uploader.root}/#{uploader.class.base_dir}") } + it { is_expected.to start_with(local_path) } 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 54c6a8b869b..efeffb78772 100644 --- a/spec/uploaders/legacy_artifact_uploader_spec.rb +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -3,22 +3,49 @@ require 'rails_helper' describe LegacyArtifactUploader do let(:job) { create(:ci_build) } let(:uploader) { described_class.new(job, :legacy_artifacts_file) } - let(:local_path) { described_class.root } + let(:local_path) { Gitlab.config.artifacts.path } - subject { uploader } + describe '.local_store_path' do + subject { described_class.local_store_path } - # TODO: move to Workhorse::UploadPath - describe '.workhorse_upload_path' do - subject { described_class.workhorse_upload_path } + 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 } it { is_expected.to start_with(local_path) } - it { is_expected.to end_with('tmp/uploads') } + 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 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 '#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 describe '#filename' do # we need to use uploader, as this makes to use mounter @@ -42,7 +69,7 @@ describe LegacyArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with("#{uploader.root}") } + it { is_expected.to start_with(local_path) } 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 6ebc885daa8..7088bc23334 100644 --- a/spec/uploaders/lfs_object_uploader_spec.rb +++ b/spec/uploaders/lfs_object_uploader_spec.rb @@ -2,13 +2,39 @@ require 'spec_helper' describe LfsObjectUploader do let(:lfs_object) { create(:lfs_object, :with_file) } - let(:uploader) { described_class.new(lfs_object, :file) } + let(:uploader) { described_class.new(lfs_object) } let(:path) { Gitlab.config.lfs.storage_path } - subject { uploader } + describe '#move_to_cache' do + it 'is true' do + expect(uploader.move_to_cache).to eq(true) + end + 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] + 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 end diff --git a/spec/uploaders/namespace_file_uploader_spec.rb b/spec/uploaders/namespace_file_uploader_spec.rb index 24a2fc0f72e..c6c4500c179 100644 --- a/spec/uploaders/namespace_file_uploader_spec.rb +++ b/spec/uploaders/namespace_file_uploader_spec.rb @@ -1,16 +1,21 @@ 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) } - subject { uploader } + 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) - 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}] + expect(described_class.absolute_path(upload_record)) + .to include("-/system/namespace/#{group.id}") + end + end end diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index ed1fba6edda..cbafa9f478d 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -1,27 +1,25 @@ require 'spec_helper' -IDENTIFIER = %r{\h+/\S+} - describe PersonalFileUploader do - let(:model) { create(:personal_snippet) } - let(:uploader) { described_class.new(model) } - let(:upload) { create(:upload, :personal_snippet_upload) } + let(:uploader) { described_class.new(build_stubbed(:project)) } + let(:snippet) { create(:personal_snippet) } - subject { uploader } + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + upload = double(model: snippet, path: 'secret/foo.jpg') - 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}] + dynamic_segment = "personal_snippet/#{snippet.id}" - describe '#to_h' do - before do - subject.instance_variable_set(:@secret, 'secret') + expect(described_class.absolute_path(upload)).to end_with("/-/system/#{dynamic_segment}/secret/foo.jpg") end + end + + describe '#to_h' do + it 'returns the hass' do + uploader = described_class.new(snippet, 'secret') - it 'is correct' do allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) - expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name" + expected_url = "/uploads/-/system/personal_snippet/#{snippet.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 9a3e5d83e01..7ef7fb7d758 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::Concern + include RecordsUploads storage :file - def dynamic_segment - 'co/fe/ee' + def model + FactoryBot.build_stubbed(:user) end end - RecordsUploadsExampleUploader.new(build_stubbed(:user)) + RecordsUploadsExampleUploader.new end def upload_fixture(filename) @@ -20,55 +20,48 @@ describe RecordsUploads do end describe 'callbacks' do - let(:upload) { create(:upload) } - - before do - uploader.upload = upload - end - - it '#record_upload after `store`' do + it 'calls `record_upload` after `store`' do expect(uploader).to receive(:record_upload).once uploader.store!(upload_fixture('doc_sample.txt')) end - it '#destroy_upload after `remove`' do + it 'calls `destroy_upload` after `remove`' do + expect(uploader).to receive(:destroy_upload).once + uploader.store!(upload_fixture('doc_sample.txt')) - expect(uploader).to receive(:destroy_upload).once uploader.remove! end end describe '#record_upload callback' do - 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 not using file storage' do + allow(uploader).to receive(:file_storage?).and_return(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 "does not create an Upload record when the file doesn't exist" do + 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) - expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count } + uploader.store!(upload_fixture('rails_sample.jpg')) + end + + it 'creates an Upload record after store' do + expect(Upload).to receive(:record) + .with(uploader) + + uploader.store!(upload_fixture('rails_sample.jpg')) end it 'does not create an Upload record if model is missing' do - allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil) + expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil) + expect(Upload).not_to receive(:record).with(uploader) - expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count } + uploader.store!(upload_fixture('rails_sample.jpg')) end it 'it destroys Upload records at the same path before recording' do @@ -79,15 +72,29 @@ 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 9e50ce15871..911360da66c 100644 --- a/spec/workers/upload_checksum_worker_spec.rb +++ b/spec/workers/upload_checksum_worker_spec.rb @@ -2,31 +2,18 @@ require 'rails_helper' describe UploadChecksumWorker do describe '#perform' do - 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 + it 'rescues ActiveRecord::RecordNotFound' do + expect { described_class.new.perform(999_999) }.not_to raise_error end - 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 + it 'calls calculate_checksum_without_delay and save!' do + upload = spy + expect(Upload).to receive(:find).with(999_999).and_return(upload) - it 'calls calculate_checksum!' do - expect(upload).to receive(:calculate_checksum!) - subject.perform(upload.id) - end + described_class.new.perform(999_999) - it 'calls save!' do - expect(upload).to receive(:save!) - subject.perform(upload.id) - end + expect(upload).to have_received(:calculate_checksum) + expect(upload).to have_received(:save!) end end end |