Merge branch '3867-port-to-ce' into 'master'

Port of gitlab-ee!3867 to CE

See merge request gitlab-org/gitlab-ce!16775

app/controllers/concerns/uploads_actions.rb

 module UploadsActions
   include Gitlab::Utils::StrongMemoize
 
+  UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo).freeze
+
   def create
     link_to_file = UploadService.new(model, params[:file], uploader_class).execute
 
@@ -17,34 +19,71 @@ module UploadsActions
     end
   end
 
+  # This should either
+  #   - send the file directly
+  #   - or redirect to its URL
+  #
   def show
     return render_404 unless uploader.exists?
 
-    disposition = uploader.image_or_video? ? 'inline' : 'attachment'
-
-    expires_in 0.seconds, must_revalidate: true, private: true
+    if uploader.file_storage?
+      disposition = uploader.image_or_video? ? 'inline' : 'attachment'
+      expires_in 0.seconds, must_revalidate: true, private: true
 
-    send_file uploader.file.path, disposition: disposition
+      send_file uploader.file.path, disposition: disposition
+    else
+      redirect_to uploader.url
+    end
   end
 
   private
 
+  def uploader_class
+    raise NotImplementedError
+  end
+
+  def upload_mount
+    mounted_as = params[:mounted_as]
+    mounted_as if UPLOAD_MOUNTS.include?(mounted_as)
+  end
+
+  def uploader_mounted?
+    upload_model_class < CarrierWave::Mount::Extension && !upload_mount.nil?
+  end
+
   def uploader
     strong_memoize(:uploader) do
-      return if show_model.nil?
+      if uploader_mounted?
+        model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend
+      else
+        build_uploader_from_upload || build_uploader_from_params
+      end
+    end
+  end
 
-      file_uploader = FileUploader.new(show_model, params[:secret])
-      file_uploader.retrieve_from_store!(params[:filename])
+  def build_uploader_from_upload
+    return nil unless params[:secret] && params[:filename]
 
-      file_uploader
-    end
+    upload_path = uploader_class.upload_path(params[:secret], params[:filename])
+    upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_path)
+    upload&.build_uploader
+  end
+
+  def build_uploader_from_params
+    uploader = uploader_class.new(model, params[:secret])
+    uploader.retrieve_from_store!(params[:filename])
+    uploader
   end
 
   def image_or_video?
     uploader && uploader.exists? && uploader.image_or_video?
   end
 
-  def uploader_class
-    FileUploader
+  def find_model
+    nil
+  end
+
+  def model
+    strong_memoize(:model) { find_model }
   end
 end

app/controllers/groups/uploads_controller.rb

@@ -7,29 +7,23 @@ class Groups::UploadsController < Groups::ApplicationController
 
   private
 
-  def show_model
-    strong_memoize(:show_model) do
-      group_id = params[:group_id]
-
-      Group.find_by_full_path(group_id)
-    end
+  def upload_model_class
+    Group
   end
 
-  def authorize_upload_file!
-    render_404 unless can?(current_user, :upload_file, group)
+  def uploader_class
+    NamespaceFileUploader
   end
 
-  def uploader
-    strong_memoize(:uploader) do
-      file_uploader = uploader_class.new(show_model, params[:secret])
-      file_uploader.retrieve_from_store!(params[:filename])
-      file_uploader
-    end
-  end
+  def find_model
+    return @group if @group
 
-  def uploader_class
-    NamespaceFileUploader
+    group_id = params[:group_id]
+
+    Group.find_by_full_path(group_id)
   end
 
-  alias_method :model, :group
+  def authorize_upload_file!
+    render_404 unless can?(current_user, :upload_file, group)
+  end
 end

app/controllers/projects/lfs_storage_controller.rb

@@ -60,7 +60,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
 
   def store_file(oid, size, tmp_file)
     # Define tmp_file_path early because we use it in "ensure"
-    tmp_file_path = File.join("#{Gitlab.config.lfs.storage_path}/tmp/upload", tmp_file)
+    tmp_file_path = File.join(LfsObjectUploader.workhorse_upload_path, tmp_file)
 
     object = LfsObject.find_or_create_by(oid: oid, size: size)
     file_exists = object.file.exists? || move_tmp_file_to_storage(object, tmp_file_path)

app/controllers/projects/uploads_controller.rb

 class Projects::UploadsController < Projects::ApplicationController
   include UploadsActions
 
+  # These will kick you out if you don't have access.
   skip_before_action :project, :repository,
     if: -> { action_name == 'show' && image_or_video? }
 
@@ -8,14 +9,20 @@ class Projects::UploadsController < Projects::ApplicationController
 
   private
 
-  def show_model
-    strong_memoize(:show_model) do
-      namespace = params[:namespace_id]
-      id = params[:project_id]
+  def upload_model_class
+    Project
+  end
 
-      Project.find_by_full_path("#{namespace}/#{id}")
-    end
+  def uploader_class
+    FileUploader
   end
 
-  alias_method :model, :project
+  def find_model
+    return @project if @project
+
+    namespace = params[:namespace_id]
+    id = params[:project_id]
+
+    Project.find_by_full_path("#{namespace}/#{id}")
+  end
 end

app/controllers/uploads_controller.rb

 class UploadsController < ApplicationController
   include UploadsActions
 
+  UnknownUploadModelError = Class.new(StandardError)
+
+  MODEL_CLASSES = {
+    "user"             => User,
+    "project"          => Project,
+    "note"             => Note,
+    "group"            => Group,
+    "appearance"       => Appearance,
+    "personal_snippet" => PersonalSnippet,
+    nil                => PersonalSnippet
+  }.freeze
+
+  rescue_from UnknownUploadModelError, with: :render_404
+
   skip_before_action :authenticate_user!
+  before_action :upload_mount_satisfied?
   before_action :find_model
   before_action :authorize_access!, only: [:show]
   before_action :authorize_create_access!, only: [:create]
 
-  private
+  def uploader_class
+    PersonalFileUploader
+  end
 
   def find_model
     return nil unless params[:id]
 
-    return render_404 unless upload_model && upload_mount
-
-    @model = upload_model.find(params[:id])
+    upload_model_class.find(params[:id])
   end
 
   def authorize_access!
@@ -53,55 +68,17 @@ class UploadsController < ApplicationController
     end
   end
 
-  def upload_model
-    upload_models = {
-      "user"    => User,
-      "project" => Project,
-      "note"    => Note,
-      "group"   => Group,
-      "appearance" => Appearance,
-      "personal_snippet" => PersonalSnippet
-    }
-
-    upload_models[params[:model]]
-  end
-
-  def upload_mount
-    return true unless params[:mounted_as]
-
-    upload_mounts = %w(avatar attachment file logo header_logo)
-
-    if upload_mounts.include?(params[:mounted_as])
-      params[:mounted_as]
-    end
+  def upload_model_class
+    MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError)
   end
 
-  def uploader
-    return @uploader if defined?(@uploader)
-
-    case model
-    when nil
-      @uploader = PersonalFileUploader.new(nil, params[:secret])
-
-      @uploader.retrieve_from_store!(params[:filename])
-    when PersonalSnippet
-      @uploader = PersonalFileUploader.new(model, params[:secret])
-
-      @uploader.retrieve_from_store!(params[:filename])
-    else
-      @uploader = @model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend
-
-      redirect_to @uploader.url unless @uploader.file_storage?
-    end
-
-    @uploader
+  def upload_model_class_has_mounts?
+    upload_model_class < CarrierWave::Mount::Extension
   end
 
-  def uploader_class
-    PersonalFileUploader
-  end
+  def upload_mount_satisfied?
+    return true unless upload_model_class_has_mounts?
 
-  def model
-    @model ||= find_model
+    upload_model_class.uploader_options.has_key?(upload_mount)
   end
 end

app/models/appearance.rb

@@ -11,6 +11,7 @@ class Appearance < ActiveRecord::Base
 
   mount_uploader :logo,         AttachmentUploader
   mount_uploader :header_logo,  AttachmentUploader
+
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   CACHE_KEY = 'current_appearance'.freeze

app/models/concerns/avatarable.rb

 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?
 

app/models/group.rb

@@ -29,18 +29,14 @@ class Group < Namespace
   has_many :variables, class_name: 'Ci::GroupVariable'
   has_many :custom_attributes, class_name: 'GroupCustomAttribute'
 
-  validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
+  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+
   validate :visibility_level_allowed_by_projects
   validate :visibility_level_allowed_by_sub_groups
   validate :visibility_level_allowed_by_parent
 
-  validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
-
   validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
 
-  mount_uploader :avatar, AvatarUploader
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   after_create :post_create_hook
   after_destroy :post_destroy_hook
   after_save :update_two_factor_requirement
@@ -116,12 +112,6 @@ class Group < Namespace
       visibility_level_allowed_by_sub_groups?(level)
   end
 
-  def avatar_url(**args)
-    # We use avatar_path instead of overriding avatar_url because of carrierwave.
-    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
-    avatar_path(args)
-  end
-
   def lfs_enabled?
     return false unless Gitlab.config.lfs.enabled
     return Gitlab.config.lfs.enabled if self[:lfs_enabled].nil?

app/models/note.rb

@@ -88,6 +88,7 @@ class Note < ActiveRecord::Base
     end
   end
 
+  # @deprecated attachments are handler by the MarkdownUploader
   mount_uploader :attachment, AttachmentUploader
 
   # Scopes

app/models/project.rb

@@ -256,9 +256,6 @@ class Project < ActiveRecord::Base
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? }
-  validate :avatar_type,
-    if: ->(project) { project.avatar.present? && project.avatar_changed? }
-  validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
   validate :visibility_level_allowed_by_group
   validate :visibility_level_allowed_as_fork
   validate :check_wiki_path_conflict
@@ -266,7 +263,6 @@ class Project < ActiveRecord::Base
     presence: true,
     inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
 
-  mount_uploader :avatar, AvatarUploader
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   # Scopes
@@ -289,7 +285,6 @@ class Project < ActiveRecord::Base
   scope :non_archived, -> { where(archived: false) }
   scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }
   scope :with_push, -> { joins(:events).where('events.action = ?', Event::PUSHED) }
-
   scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
   scope :with_statistics, -> { includes(:statistics) }
   scope :with_shared_runners, -> { where(shared_runners_enabled: true) }
@@ -923,20 +918,12 @@ class Project < ActiveRecord::Base
     issues_tracker.to_param == 'jira'
   end
 
-  def avatar_type
-    unless self.avatar.image?
-      self.errors.add :avatar, 'only images allowed'
-    end
-  end
-
   def avatar_in_git
     repository.avatar
   end
 
   def avatar_url(**args)
-    # We use avatar_path instead of overriding avatar_url because of carrierwave.
-    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
-    avatar_path(args) || (Gitlab::Routing.url_helpers.project_avatar_url(self) if avatar_in_git)
+    Gitlab::Routing.url_helpers.project_avatar_url(self) if avatar_in_git
   end
 
   # For compatibility with old code

app/models/upload.rb

@@ -9,22 +9,11 @@ class Upload < ActiveRecord::Base
   validates :model, presence: true
   validates :uploader, presence: true
 
-  before_save  :calculate_checksum, if:     :foreground_checksum?
-  after_commit :schedule_checksum,  unless: :foreground_checksum?
+  before_save  :calculate_checksum!, if: :foreground_checksummable?
+  after_commit :schedule_checksum,   if: :checksummable?
 
-  def self.remove_path(path)
-    where(path: path).destroy_all
-  end
-
-  def self.record(uploader)
-    remove_path(uploader.relative_path)
-
-    create(
-      size: uploader.file.size,
-      path: uploader.relative_path,
-      model: uploader.model,
-      uploader: uploader.class.to_s
-    )
+  def self.hexdigest(path)
+    Digest::SHA256.file(path).hexdigest
   end
 
   def absolute_path
@@ -33,10 +22,18 @@ class Upload < ActiveRecord::Base
     uploader_class.absolute_path(self)
   end
 
-  def calculate_checksum
-    return unless exist?
+  def calculate_checksum!
+    self.checksum = nil
+    return unless checksummable?
 
-    self.checksum = Digest::SHA256.file(absolute_path).hexdigest
+    self.checksum = self.class.hexdigest(absolute_path)
+  end
+
+  def build_uploader
+    uploader_class.new(model).tap do |uploader|
+      uploader.upload = self
+      uploader.retrieve_from_store!(identifier)
+    end
   end
 
   def exist?
@@ -45,8 +42,16 @@ class Upload < ActiveRecord::Base
 
   private
 
-  def foreground_checksum?
-    size <= CHECKSUM_THRESHOLD
+  def checksummable?
+    checksum.nil? && local? && exist?
+  end
+
+  def local?
+    true
+  end
+
+  def foreground_checksummable?
+    checksummable? && size <= CHECKSUM_THRESHOLD
   end
 
   def schedule_checksum
@@ -57,6 +62,10 @@ class Upload < ActiveRecord::Base
     !path.start_with?('/')
   end
 
+  def identifier
+    File.basename(path)
+  end
+
   def uploader_class
     Object.const_get(uploader)
   end

app/models/user.rb

@@ -137,6 +137,7 @@ class User < ActiveRecord::Base
   has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :custom_attributes, class_name: 'UserCustomAttribute'
+  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   #
   # Validations
@@ -159,12 +160,10 @@ class User < ActiveRecord::Base
   validate :namespace_uniq, if: :username_changed?
   validate :namespace_move_dir_allowed, if: :username_changed?
 
-  validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :unique_email, if: :email_changed?
   validate :owns_notification_email, if: :notification_email_changed?
   validate :owns_public_email, if: :public_email_changed?
   validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
-  validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: :email_changed?
@@ -225,9 +224,6 @@ class User < ActiveRecord::Base
     end
   end
 
-  mount_uploader :avatar, AvatarUploader
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   # Scopes
   scope :admins, -> { where(admin: true) }
   scope :blocked, -> { with_states(:blocked, :ldap_blocked) }
@@ -527,12 +523,6 @@ class User < ActiveRecord::Base
     end
   end
 
-  def avatar_type
-    unless avatar.image?
-      errors.add :avatar, "only images allowed"
-    end
-  end
-
   def unique_email
     if !emails.exists?(email: email) && Email.exists?(email: email)
       errors.add(:email, 'has already been taken')
@@ -860,9 +850,7 @@ class User < ActiveRecord::Base
   end
 
   def avatar_url(size: nil, scale: 2, **args)
-    # We use avatar_path instead of overriding avatar_url because of carrierwave.
-    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
-    avatar_path(args) || GravatarService.new.execute(email, size, scale, username: username)
+    GravatarService.new.execute(email, size, scale, username: username)
   end
 
   def primary_email_verified?

app/services/projects/hashed_storage/migrate_attachments_service.rb

@@ -14,9 +14,9 @@ module Projects
         @old_path = project.full_path
         @new_path = project.disk_path
 
-        origin = FileUploader.dynamic_path_segment(project)
+        origin = FileUploader.absolute_base_dir(project)
         project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments]
-        target = FileUploader.dynamic_path_segment(project)
+        target = FileUploader.absolute_base_dir(project)
 
         result = move_folder!(origin, target)
         project.save!

app/uploaders/attachment_uploader.rb

 class AttachmentUploader < GitlabUploader
-  include RecordsUploads
   include UploaderHelper
+  include RecordsUploads::Concern
 
   storage :file
 
-  def store_dir
-    "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
+  private
+
+  def dynamic_segment
+    File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s)
   end
 end

app/uploaders/avatar_uploader.rb

 class AvatarUploader < GitlabUploader
-  include RecordsUploads
   include UploaderHelper
+  include RecordsUploads::Concern
 
   storage :file
 
-  def store_dir
-    "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
-  end
-
   def exists?
     model.avatar.file && model.avatar.file.present?
   end
 
-  # We set move_to_store and move_to_cache to 'false' to prevent stealing
-  # the avatar file from a project when forking it.
-  # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158
-  def move_to_store
+  def move_to_cache
     false
   end
 
-  def move_to_cache
+  def move_to_store
     false
   end
+
+  private
+
+  def dynamic_segment
+    File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s)
+  end
 end

app/uploaders/file_mover.rb

@@ -21,7 +21,8 @@ class FileMover
   end
 
   def update_markdown
-    updated_text = model.read_attribute(update_field).gsub(temp_file_uploader.to_markdown, uploader.to_markdown)
+    updated_text = model.read_attribute(update_field)
+                        .gsub(temp_file_uploader.markdown_link, uploader.markdown_link)
     model.update_attribute(update_field, updated_text)
 
     true

app/uploaders/file_uploader.rb

+# This class breaks the actual CarrierWave concept.
+# Every uploader should use a base_dir that is model agnostic so we can build
+# back URLs from base_dir-relative paths saved in the `Upload` model.
+#
+# As the `.base_dir` is model dependent and **not** saved in the upload model (see #upload_path)
+# there is no way to build back the correct file path without the model, which defies
+# CarrierWave way of storing files.
+#
 class FileUploader < GitlabUploader
-  include RecordsUploads
   include UploaderHelper
+  include RecordsUploads::Concern
 
   MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
+  DYNAMIC_PATH_PATTERN = %r{(?<secret>\h{32})/(?<identifier>.*)}
 
   storage :file
 
-  def self.absolute_path(upload_record)
+  def self.root
+    File.join(options.storage_path, 'uploads')
+  end
+
+  def self.absolute_path(upload)
     File.join(
-      self.dynamic_path_segment(upload_record.model),
-      upload_record.path
+      absolute_base_dir(upload.model),
+      upload.path # already contain the dynamic_segment, see #upload_path
     )
   end
 
-  # Not using `GitlabUploader.base_dir` because all project namespaces are in
-  # the `public/uploads` dir.
-  #
-  def self.base_dir
-    root_dir
+  def self.base_dir(model)
+    model_path_segment(model)
+  end
+
+  # used in migrations and import/exports
+  def self.absolute_base_dir(model)
+    File.join(root, base_dir(model))
   end
 
   # Returns the part of `store_dir` that can change based on the model's current
@@ -29,63 +44,96 @@ class FileUploader < GitlabUploader
   # model - Object that responds to `full_path` and `disk_path`
   #
   # Returns a String without a trailing slash
-  def self.dynamic_path_segment(model)
+  def self.model_path_segment(model)
     if model.hashed_storage?(:attachments)
-      dynamic_path_builder(model.disk_path)
+      model.disk_path
     else
-      dynamic_path_builder(model.full_path)
+      model.full_path
     end
   end
 
-  # Auxiliary method to build dynamic path segment when not using a project model
-  #
-  # Prefer to use the `.dynamic_path_segment` as it includes Hashed Storage specific logic
-  def self.dynamic_path_builder(path)
-    File.join(CarrierWave.root, base_dir, path)
+  def self.upload_path(secret, identifier)
+    File.join(secret, identifier)
+  end
+
+  def self.generate_secret
+    SecureRandom.hex
   end
 
   attr_accessor :model
-  attr_reader :secret
 
   def initialize(model, secret = nil)
     @model = model
-    @secret = secret || generate_secret
+    @secret = secret
   end
 
-  def store_dir
-    File.join(dynamic_path_segment, @secret)
+  def base_dir
+    self.class.base_dir(@model)
   end
 
-  def relative_path
-    self.file.path.sub("#{dynamic_path_segment}/", '')
+  # we don't need to know the actual path, an uploader instance should be
+  # able to yield the file content on demand, so we should build the digest
+  def absolute_path
+    self.class.absolute_path(@upload)
   end
 
-  def to_markdown
-    to_h[:markdown]
+  def upload_path
+    self.class.upload_path(dynamic_segment, identifier)
   end
 
-  def to_h
-    filename = image_or_video? ? self.file.basename : self.file.filename
-    escaped_filename = filename.gsub("]", "\\]")
+  def model_path_segment
+    self.class.model_path_segment(@model)
+  end
+
+  def store_dir
+    File.join(base_dir, dynamic_segment)
+  end
 
-    markdown = "[#{escaped_filename}](#{secure_url})"
+  def markdown_link
+    markdown = "[#{markdown_name}](#{secure_url})"
     markdown.prepend("!") if image_or_video? || dangerous?
+    markdown
+  end
 
+  def to_h
     {
-      alt:      filename,
+      alt:      markdown_name,
       url:      secure_url,
-      markdown: markdown
+      markdown: markdown_link
     }
   end
 
+  def filename
+    self.file.filename
+  end
+
+  # the upload does not hold the secret, but holds the path
+  # which contains the secret: extract it
+  def upload=(value)
+    if matches = DYNAMIC_PATH_PATTERN.match(value.path)
+      @secret = matches[:secret]
+      @identifier = matches[:identifier]
+    end
+
+    super
+  end
+
+  def secret
+    @secret ||= self.class.generate_secret
+  end
+
   private
 
-  def dynamic_path_segment
-    self.class.dynamic_path_segment(model)
+  def markdown_name
+    (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]")
   end
 
-  def generate_secret
-    SecureRandom.hex
+  def identifier
+    @identifier ||= filename
+  end
+
+  def dynamic_segment
+    secret
   end
 
   def secure_url

app/uploaders/gitlab_uploader.rb

 class GitlabUploader < CarrierWave::Uploader::Base
-  def self.absolute_path(upload_record)
-    File.join(CarrierWave.root, upload_record.path)
-  end
+  class_attribute :options
 
-  def self.root_dir
-    'uploads'
-  end
+  class << self
+    # DSL setter
+    def storage_options(options)
+      self.options = options
+    end
 
-  # When object storage is used, keep the `root_dir` as `base_dir`.
-  # The files aren't really in folders there, they just have a name.
-  # The files that contain user input in their name, also contain a hash, so
-  # the names are still unique
-  #
-  # This method is overridden in the `FileUploader`
-  def self.base_dir
-    return root_dir unless file_storage?
+    def root
+      options.storage_path
+    end
 
-    File.join(root_dir, '-', 'system')
-  end
+    # represent the directory namespacing at the class level
+    def base_dir
+      options.fetch('base_dir', '')
+    end
 
-  def self.file_storage?
-    self.storage == CarrierWave::Storage::File
+    def file_storage?
+      storage == CarrierWave::Storage::File
+    end
+
+    def absolute_path(upload_record)
+      File.join(root, upload_record.path)
+    end
   end
 
+  storage_options Gitlab.config.uploads
+
   delegate :base_dir, :file_storage?, to: :class
 
   def file_cache_storage?
@@ -31,34 +35,28 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   # Reduce disk IO
   def move_to_cache
-    true
+    file_storage?
   end
 
   # Reduce disk IO
   def move_to_store
-    true
-  end
-
-  # Designed to be overridden by child uploaders that have a dynamic path
-  # segment -- that is, a path that changes based on mutable attributes of its
-  # associated model
-  #
-  # For example, `FileUploader` builds the storage path based on the associated
-  # project model's `path_with_namespace` value, which can change when the
-  # project or its containing namespace is moved or renamed.
-  def relative_path
-    self.file.path.sub("#{root}/", '')
+    file_storage?
   end
 
   def exists?
     file.present?
   end
 
-  # Override this if you don't want to save files by default to the Rails.root directory
+  def store_dir
+    File.join(base_dir, dynamic_segment)
+  end
+
+  def cache_dir
+    File.join(root, base_dir, 'tmp/cache')
+  end
+
   def work_dir
-    # Default path set by CarrierWave:
-    # https://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L182
-    CarrierWave.tmp_path
+    File.join(root, base_dir, 'tmp/work')
   end
 
   def filename
@@ -67,6 +65,13 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   private
 
+  # Designed to be overridden by child uploaders that have a dynamic path
+  # segment -- that is, a path that changes based on mutable attributes of its
+  # associated model
+  def dynamic_segment
+    raise(NotImplementedError)
+  end
+
   # To prevent files from moving across filesystems, override the default
   # implementation:
   # http://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L181-L183
@@ -74,6 +79,6 @@ class GitlabUploader < CarrierWave::Uploader::Base
     # To be safe, keep this directory outside of the the cache directory
     # because calling CarrierWave.clean_cache_files! will remove any files in
     # the cache directory.
-    File.join(work_dir, @cache_id, version_name.to_s, for_file)
+    File.join(work_dir, cache_id, version_name.to_s, for_file)
   end
 end

app/uploaders/job_artifact_uploader.rb

 class JobArtifactUploader < GitlabUploader
-  storage :file
+  extend Workhorse::UploadPath
 
-  def self.local_store_path
-    Gitlab.config.artifacts.path
-  end
-
-  def self.artifacts_upload_path
-    File.join(self.local_store_path, 'tmp/uploads/')
-  end
+  storage_options Gitlab.config.artifacts
 
   def size
     return super if model.size.nil?
@@ -16,24 +10,12 @@ class JobArtifactUploader < GitlabUploader
   end
 
   def store_dir
-    default_local_path
-  end
-
-  def cache_dir
-    File.join(self.class.local_store_path, 'tmp/cache')
-  end
-
-  def work_dir
-    File.join(self.class.local_store_path, 'tmp/work')
+    dynamic_segment
   end
 
   private
 
-  def default_local_path
-    File.join(self.class.local_store_path, default_path)
-  end
-
-  def default_path
+  def dynamic_segment
     creation_date = model.created_at.utc.strftime('%Y_%m_%d')
 
     File.join(disk_hash[0..1], disk_hash[2..3], disk_hash,

app/uploaders/legacy_artifact_uploader.rb

 class LegacyArtifactUploader < GitlabUploader
-  storage :file
+  extend Workhorse::UploadPath
 
-  def self.local_store_path
-    Gitlab.config.artifacts.path
-  end
-
-  def self.artifacts_upload_path
-    File.join(self.local_store_path, 'tmp/uploads/')
-  end
+  storage_options Gitlab.config.artifacts
 
   def store_dir
-    default_local_path
-  end
-
-  def cache_dir
-    File.join(self.class.local_store_path, 'tmp/cache')
-  end
-
-  def work_dir
-    File.join(self.class.local_store_path, 'tmp/work')
+    dynamic_segment
   end
 
   private
 
-  def default_local_path
-    File.join(self.class.local_store_path, default_path)
-  end
-
-  def default_path
+  def dynamic_segment
     File.join(model.created_at.utc.strftime('%Y_%m'), model.project_id.to_s, model.id.to_s)
   end
 end

app/uploaders/lfs_object_uploader.rb

 class LfsObjectUploader < GitlabUploader
-  storage :file
+  extend Workhorse::UploadPath
 
-  def store_dir
-    "#{Gitlab.config.lfs.storage_path}/#{model.oid[0, 2]}/#{model.oid[2, 2]}"
+  # LfsObject are in `tmp/upload` instead of `tmp/uploads`
+  def self.workhorse_upload_path
+    File.join(root, 'tmp/upload')
   end
 
-  def cache_dir
-    "#{Gitlab.config.lfs.storage_path}/tmp/cache"
-  end
+  storage_options Gitlab.config.lfs
 
   def filename
     model.oid[4..-1]
   end
 
-  def work_dir
-    File.join(Gitlab.config.lfs.storage_path, 'tmp', 'work')
+  def store_dir
+    dynamic_segment
+  end
+
+  private
+
+  def dynamic_segment
+    File.join(model.oid[0, 2], model.oid[2, 2])
   end
 end

app/uploaders/namespace_file_uploader.rb

 class NamespaceFileUploader < FileUploader
-  def self.base_dir
-    File.join(root_dir, '-', 'system', 'namespace')
+  # Re-Override
+  def self.root
+    options.storage_path
   end
 
-  def self.dynamic_path_segment(model)
-    dynamic_path_builder(model.id.to_s)
+  def self.base_dir(model)
+    File.join(options.base_dir, 'namespace', model_path_segment(model))
   end
 
-  private
+  def self.model_path_segment(model)
+    File.join(model.id.to_s)
+  end
 
-  def secure_url
-    File.join('/uploads', @secret, file.filename)
+  # Re-Override
+  def store_dir
+    File.join(base_dir, dynamic_segment)
   end
 end

app/uploaders/personal_file_uploader.rb

 class PersonalFileUploader < FileUploader
-  def self.dynamic_path_segment(model)
-    File.join(CarrierWave.root, model_path(model))
+  # Re-Override
+  def self.root
+    options.storage_path
   end
 
-  def self.base_dir
-    File.join(root_dir, '-', 'system')
+  def self.base_dir(model)
+    File.join(options.base_dir, model_path_segment(model))
   end
 
-  private
+  def self.model_path_segment(model)
+    return 'temp/' unless model
 
-  def secure_url
-    File.join(self.class.model_path(model), secret, file.filename)
+    File.join(model.class.to_s.underscore, model.id.to_s)
+  end
+
+  # Revert-Override
+  def store_dir
+    File.join(base_dir, dynamic_segment)
   end
 
-  def self.model_path(model)
-    if model
-      File.join("/#{base_dir}", model.class.to_s.underscore, model.id.to_s)
-    else
-      File.join("/#{base_dir}", 'temp')
-    end
+  private
+
+  def secure_url
+    File.join('/', base_dir, secret, file.filename)
   end
 end

app/uploaders/records_uploads.rb

 module RecordsUploads
-  extend ActiveSupport::Concern
+  module Concern
+    extend ActiveSupport::Concern
 
-  included do
-    after :store,   :record_upload
-    before :remove, :destroy_upload
-  end
+    attr_accessor :upload
 
-  # After storing an attachment, create a corresponding Upload record
-  #
-  # NOTE: We're ignoring the argument passed to this callback because we want
-  # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the
-  # `Tempfile` object the callback gets.
-  #
-  # Called `after :store`
-  def record_upload(_tempfile = nil)
-    return unless model
-    return unless file_storage?
-    return unless file.exists?
-
-    Upload.record(self)
-  end
+    included do
+      after  :store,  :record_upload
+      before :remove, :destroy_upload
+    end
+
+    # After storing an attachment, create a corresponding Upload record
+    #
+    # NOTE: We're ignoring the argument passed to this callback because we want
+    # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the
+    # `Tempfile` object the callback gets.
+    #
+    # Called `after :store`
+    def record_upload(_tempfile = nil)
+      return unless model
+      return unless file && file.exists?
+
+      Upload.transaction do
+        uploads.where(path: upload_path).delete_all
+        upload.destroy! if upload
+
+        self.upload = build_upload_from_uploader(self)
+        upload.save!
+      end
+    end
+
+    def upload_path
+      File.join(store_dir, filename.to_s)
+    end
+
+    private
+
+    def uploads
+      Upload.order(id: :desc).where(uploader: self.class.to_s)
+    end
 
-  private
+    def build_upload_from_uploader(uploader)
+      Upload.new(
+        size: uploader.file.size,
+        path: uploader.upload_path,
+        model: uploader.model,
+        uploader: uploader.class.to_s
+      )
+    end
 
-  # Before removing an attachment, destroy any Upload records at the same path
-  #
-  # Called `before :remove`
-  def destroy_upload(*args)
-    return unless file_storage?
-    return unless file
+    # Before removing an attachment, destroy any Upload records at the same path
+    #
+    # Called `before :remove`
+    def destroy_upload(*args)
+      return unless file && file.exists?
 
-    Upload.remove_path(relative_path)
+      self.upload = nil
+      uploads.where(path: upload_path).delete_all
+    end
   end
 end

app/uploaders/uploader_helper.rb

@@ -32,14 +32,7 @@ module UploaderHelper
   def extension_match?(extensions)
     return false unless file
 
-    extension =
-      if file.respond_to?(:extension)
-        file.extension
-      else
-        # Not all CarrierWave storages respond to :extension
-        File.extname(file.path).delete('.')
-      end
-
+    extension = file.try(:extension) || File.extname(file.path).delete('.')
     extensions.include?(extension.downcase)
   end
 end

app/uploaders/workhorse.rb

+module Workhorse
+  module UploadPath
+    def workhorse_upload_path
+      File.join(root, base_dir, 'tmp/uploads')
+    end
+  end
+end

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")

config/gitlab.yml.example

@@ -152,6 +152,12 @@ production: &base
     # The location where LFS objects are stored (default: shared/lfs-objects).
     # storage_path: shared/lfs-objects
 
+  ## Uploads (attachments, avatars, etc...)
+  uploads:
+    # The location where uploads objects are stored (default: public/).
+    # storage_path: public/
+    # base_dir: uploads/-/system
+
   ## GitLab Pages
   pages:
     enabled: false
@@ -644,6 +650,8 @@ test:
     enabled: false
   artifacts:
     path: tmp/tests/artifacts
+  uploads:
+    storage_path: tmp/tests/public
   gitlab:
     host: localhost
     port: 80

config/initializers/1_settings.rb

@@ -300,8 +300,10 @@ Settings.incoming_email['enabled'] = false if Settings.incoming_email['enabled']
 #
 Settings['artifacts'] ||= Settingslogic.new({})
 Settings.artifacts['enabled']      = true if Settings.artifacts['enabled'].nil?
-Settings.artifacts['path']         = Settings.absolute(Settings.artifacts['path'] || File.join(Settings.shared['path'], "artifacts"))
-Settings.artifacts['max_size']   ||= 100 # in megabytes
+Settings.artifacts['storage_path'] = Settings.absolute(Settings.artifacts.values_at('path', 'storage_path').compact.first || File.join(Settings.shared['path'], "artifacts"))
+# Settings.artifact['path'] is deprecated, use `storage_path` instead
+Settings.artifacts['path']         = Settings.artifacts['storage_path']
+Settings.artifacts['max_size'] ||= 100 # in megabytes
 
 #
 # Registry
@@ -338,6 +340,13 @@ Settings['lfs'] ||= Settingslogic.new({})
 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
 #

db/migrate/20180119135717_add_uploader_index_to_uploads.rb

+# 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

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", ["path"], name: "index_uploads_on_path", using: :btree
+  add_index "uploads", ["uploader", "path"], name: "index_uploads_on_uploader_and_path", using: :btree
 
   create_table "user_agent_details", force: :cascade do |t|
     t.string "user_agent", null: false

doc/development/file_storage.md

@@ -14,8 +14,8 @@ There are many places where file uploading is used, according to contexts:
   - User snippet attachments
 * Project
   - Project avatars
-  - Issues/MR Markdown attachments
-  - Issues/MR Legacy Markdown attachments
+  - Issues/MR/Notes Markdown attachments
+  - Issues/MR/Notes Legacy Markdown attachments
   - CI Build Artifacts
   - LFS Objects
 
@@ -25,7 +25,7 @@ There are many places where file uploading is used, according to contexts:
 GitLab started saving everything on local disk. While directory location changed from previous versions,
 they are still not 100% standardized. You can see them below:
 
-| Description                           | In DB? | Relative path                                               | Uploader class         | model_type |
+| Description                           | In DB? | Relative path (from CarrierWave.root)                       | Uploader class         | model_type |
 | ------------------------------------- | ------ | ----------------------------------------------------------- | ---------------------- | ---------- |
 | Instance logo                         | yes    | uploads/-/system/appearance/logo/:id/:filename              | `AttachmentUploader`   | Appearance |
 | Header logo                           | yes    | uploads/-/system/appearance/header_logo/:id/:filename       | `AttachmentUploader`   | Appearance |
@@ -33,17 +33,107 @@ they are still not 100% standardized. You can see them below:
 | User avatars                          | yes    | uploads/-/system/user/avatar/:id/:filename                  | `AvatarUploader`       | User       |
 | User snippet attachments              | yes    | uploads/-/system/personal_snippet/:id/:random_hex/:filename | `PersonalFileUploader` | Snippet    |
 | Project avatars                       | yes    | uploads/-/system/project/avatar/:id/:filename               | `AvatarUploader`       | Project    |
-| Issues/MR Markdown attachments        | yes    | uploads/:project_path_with_namespace/:random_hex/:filename  | `FileUploader`         | Project    |
-| Issues/MR Legacy Markdown attachments | no     | uploads/-/system/note/attachment/:id/:filename              | `AttachmentUploader`   | Note       |
+| Issues/MR/Notes Markdown attachments        | yes    | uploads/:project_path_with_namespace/:random_hex/:filename  | `FileUploader`         | Project    |
+| Issues/MR/Notes Legacy Markdown attachments | no     | uploads/-/system/note/attachment/:id/:filename              | `AttachmentUploader`   | Note       |
 | CI Artifacts (CE)                     | yes    | shared/artifacts/:year_:month/:project_id/:id               | `ArtifactUploader`     | Ci::Build  |
 | LFS Objects  (CE)                     | yes    | shared/lfs-objects/:hex/:hex/:object_hash                   | `LfsObjectUploader`    | LfsObject  |
 
 CI Artifacts and LFS Objects behave differently in CE and EE. In CE they inherit the `GitlabUploader`
-while in EE they inherit the `ObjectStoreUploader` and store files in and S3 API compatible object store.
+while in EE they inherit the `ObjectStorage` and store files in and S3 API compatible object store.
 
-In the case of Issues/MR Markdown attachments, there is a different approach using the [Hashed Storage] layout,
+In the case of Issues/MR/Notes Markdown attachments, there is a different approach using the [Hashed Storage] layout,
 instead of basing the path into a mutable variable `:project_path_with_namespace`, it's possible to use the
 hash of the project ID instead, if project migrates to the new approach (introduced in 10.2).
 
+### Path segments
+
+Files are stored at multiple locations and use different path schemes. 
+All the `GitlabUploader` derived classes should comply with this path segment schema:
+
+```
+|   GitlabUploader
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `<gitlab_root>/public/` | `uploads/-/system/`       | `user/avatar/:id/`                | `:filename`                      |
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `CarrierWave.root`      | `GitlabUploader.base_dir` | `GitlabUploader#dynamic_segment`  | `CarrierWave::Uploader#filename` |
+|                         | `CarrierWave::Uploader#store_dir`                             |                                  | 
+
+|   FileUploader
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `<gitlab_root>/shared/` | `artifacts/`              | `:year_:month/:id`                | `:filename`                      |
+| `<gitlab_root>/shared/` | `snippets/`               | `:secret/`                        | `:filename`                      |
+| ----------------------- + ------------------------- + --------------------------------- + -------------------------------- |
+| `CarrierWave.root`      | `GitlabUploader.base_dir` | `GitlabUploader#dynamic_segment`  | `CarrierWave::Uploader#filename` |
+|                         | `CarrierWave::Uploader#store_dir`                             |                                  | 
+|                         |                           | `FileUploader#upload_path                                            |
+
+|   ObjectStore::Concern (store = remote)
+| ----------------------- + ------------------------- + ----------------------------------- + -------------------------------- |
+| `<bucket_name>`         | <ignored>                 | `user/avatar/:id/`                  | `:filename`                      |
+| ----------------------- + ------------------------- + ----------------------------------- + -------------------------------- |
+| `#fog_dir`              | `GitlabUploader.base_dir` | `GitlabUploader#dynamic_segment`    | `CarrierWave::Uploader#filename` |
+|                         |                           | `ObjectStorage::Concern#store_dir`  |                                  | 
+|                         |                           | `ObjectStorage::Concern#upload_path                                    |
+```
+
+The `RecordsUploads::Concern` concern will create an `Upload` entry for every file stored by a `GitlabUploader` persisting the dynamic parts of the path using
+`GitlabUploader#dynamic_path`. You may then use the `Upload#build_uploader` method to manipulate the file.
+
+## Object Storage
+
+By including the `ObjectStorage::Concern` in the `GitlabUploader` derived class, you may enable the object storage for this uploader. To enable the object storage
+in your uploader, you need to either 1) include `RecordsUpload::Concern` and prepend `ObjectStorage::Extension::RecordsUploads` or 2) mount the uploader and create a new field named `<mount>_store`.
+
+The `CarrierWave::Uploader#store_dir` is overriden to
+
+ - `GitlabUploader.base_dir` + `GitlabUploader.dynamic_segment` when the store is LOCAL
+ - `GitlabUploader.dynamic_segment` when the store is REMOTE (the bucket name is used to namespace)
+
+### Using `ObjectStorage::Extension::RecordsUploads`
+
+> Note: this concern will automatically include `RecordsUploads::Concern` if not already included.
+
+The `ObjectStorage::Concern` uploader will search for the matching `Upload` to select the correct object store. The `Upload` is mapped using `#store_dirs + identifier` for each store (LOCAL/REMOTE).
+
+```ruby
+class SongUploader < GitlabUploader
+  include RecordsUploads::Concern
+  include ObjectStorage::Concern
+  prepend ObjectStorage::Extension::RecordsUploads
+
+  ...
+end
+
+class Thing < ActiveRecord::Base
+  mount :theme, SongUploader # we have a great theme song!
+
+  ...
+end
+```
+
+### Using a mounted uploader
+
+The `ObjectStorage::Concern` will query the `model.<mount>_store` attribute to select the correct object store.
+This column must be present in the model schema.
+
+```ruby
+class SongUploader < GitlabUploader
+  include ObjectStorage::Concern
+
+  ...
+end
+
+class Thing < ActiveRecord::Base
+  attr_reader :theme_store # this is an ActiveRecord attribute
+  mount :theme, SongUploader # we have a great theme song!
+
+  def theme_store
+    super || ObjectStorage::Store::LOCAL
+  end
+
+  ...
+end
+```
+
 [CarrierWave]: https://github.com/carrierwaveuploader/carrierwave
 [Hashed Storage]: ../administration/repository_storage_types.md

lib/api/runner.rb

@@ -215,9 +215,9 @@ module API
         job = authenticate_job!
         forbidden!('Job is not running!') unless job.running?
 
-        artifacts_upload_path = JobArtifactUploader.artifacts_upload_path
-        artifacts = uploaded_file(:file, artifacts_upload_path)
-        metadata = uploaded_file(:metadata, artifacts_upload_path)
+        workhorse_upload_path = JobArtifactUploader.workhorse_upload_path
+        artifacts = uploaded_file(:file, workhorse_upload_path)
+        metadata = uploaded_file(:metadata, workhorse_upload_path)
 
         bad_request!('Missing artifacts file!') unless artifacts
         file_to_large! unless artifacts.size < max_artifacts_size

lib/backup/artifacts.rb

@@ -3,7 +3,7 @@ require 'backup/files'
 module Backup
   class Artifacts < Files
     def initialize
-      super('artifacts', LegacyArtifactUploader.local_store_path)
+      super('artifacts', JobArtifactUploader.root)
     end
 
     def create_files_dir

lib/gitlab/background_migration/populate_untracked_uploads.rb

@@ -143,7 +143,7 @@ module Gitlab
         end
 
         def absolute_path
-          File.join(CarrierWave.root, path)
+          File.join(Gitlab.config.uploads.storage_path, path)
         end
       end
 

lib/gitlab/background_migration/prepare_untracked_uploads.rb

@@ -11,9 +11,12 @@ module Gitlab
 
       FIND_BATCH_SIZE = 500
       RELATIVE_UPLOAD_DIR = "uploads".freeze
-      ABSOLUTE_UPLOAD_DIR = "#{CarrierWave.root}/#{RELATIVE_UPLOAD_DIR}".freeze
+      ABSOLUTE_UPLOAD_DIR = File.join(
+        Gitlab.config.uploads.storage_path,
+        RELATIVE_UPLOAD_DIR
+      )
       FOLLOW_UP_MIGRATION = 'PopulateUntrackedUploads'.freeze
-      START_WITH_CARRIERWAVE_ROOT_REGEX = %r{\A#{CarrierWave.root}/}
+      START_WITH_ROOT_REGEX = %r{\A#{Gitlab.config.uploads.storage_path}/}
       EXCLUDED_HASHED_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/@hashed/*".freeze
       EXCLUDED_TMP_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/tmp/*".freeze
 
@@ -81,7 +84,7 @@ module Gitlab
         paths = []
 
         stdout.each_line("\0") do |line|
-          paths << line.chomp("\0").sub(START_WITH_CARRIERWAVE_ROOT_REGEX, '')
+          paths << line.chomp("\0").sub(START_WITH_ROOT_REGEX, '')
 
           if paths.size >= batch_size
             yield(paths)

lib/gitlab/gfm/uploads_rewriter.rb

@@ -27,7 +27,7 @@ module Gitlab
           with_link_in_tmp_dir(file.file) do |open_tmp_file|
             new_uploader.store!(open_tmp_file)
           end
-          new_uploader.to_markdown
+          new_uploader.markdown_link
         end
       end
 

lib/gitlab/import_export/uploads_saver.rb

@@ -17,15 +17,13 @@ module Gitlab
         false
       end
 
-      private
+      def uploads_path
+        FileUploader.absolute_base_dir(@project)
+      end
 
       def uploads_export_path
         File.join(@shared.export_path, 'uploads')
       end
-
-      def uploads_path
-        FileUploader.dynamic_path_segment(@project)
-      end
     end
   end
 end

lib/gitlab/uploads_transfer.rb

 module Gitlab
   class UploadsTransfer < ProjectTransfer
     def root_dir
-      File.join(CarrierWave.root, FileUploader.base_dir)
+      FileUploader.root
     end
   end
 end

lib/gitlab/workhorse.rb

@@ -55,14 +55,14 @@ module Gitlab
 
       def lfs_upload_ok(oid, size)
         {
-          StoreLFSPath: "#{Gitlab.config.lfs.storage_path}/tmp/upload",
+          StoreLFSPath: LfsObjectUploader.workhorse_upload_path,
           LfsOid: oid,
           LfsSize: size
         }
       end
 
       def artifact_upload_ok
-        { TempPath: JobArtifactUploader.artifacts_upload_path }
+        { TempPath: JobArtifactUploader.workhorse_upload_path }
       end
 
       def send_git_blob(repository, blob)
@@ -147,8 +147,11 @@ module Gitlab
       end
 
       def send_artifacts_entry(build, entry)
+        file = build.artifacts_file
+        archive = file.file_storage? ? file.path : file.url
+
         params = {
-          'Archive' => build.artifacts_file.path,
+          'Archive' => archive,
           'Entry' => Base64.encode64(entry.to_s)
         }
 

spec/controllers/groups/uploads_controller_spec.rb

@@ -6,5 +6,7 @@ describe Groups::UploadsController do
     { group_id: model }
   end
 
-  it_behaves_like 'handle uploads'
+  it_behaves_like 'handle uploads' do
+    let(:uploader_class) { NamespaceFileUploader }
+  end
 end

spec/controllers/projects/artifacts_controller_spec.rb

@@ -145,8 +145,7 @@ describe Projects::ArtifactsController do
       context 'when using local file storage' do
         it_behaves_like 'a valid file' do
           let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
-          let(:store) { ObjectStoreUploader::LOCAL_STORE }
-          let(:archive_path) { JobArtifactUploader.local_store_path }
+          let(:archive_path) { JobArtifactUploader.root }
         end
       end
     end

spec/controllers/projects/raw_controller_spec.rb

@@ -53,7 +53,7 @@ describe Projects::RawController do
           end
 
           it 'serves the file' do
-            expect(controller).to receive(:send_file).with("#{Gitlab.config.shared.path}/lfs-objects/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment')
+            expect(controller).to receive(:send_file).with("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment')
             get(:show,
                 namespace_id: public_project.namespace.to_param,
                 project_id: public_project,

spec/controllers/uploads_controller_spec.rb

@@ -180,6 +180,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'user', mounted_as: 'avatar', id: user.id, filename: 'image.png'
+
               response
             end
           end
@@ -196,6 +197,7 @@ describe UploadsController do
         it_behaves_like 'content not cached without revalidation' do
           subject do
             get :show, model: 'user', mounted_as: 'avatar', id: user.id, filename: 'image.png'
+
             response
           end
         end
@@ -220,6 +222,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'image.png'
+
               response
             end
           end
@@ -239,6 +242,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'image.png'
+
               response
             end
           end
@@ -291,6 +295,7 @@ describe UploadsController do
               it_behaves_like 'content not cached without revalidation' do
                 subject do
                   get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'image.png'
+
                   response
                 end
               end
@@ -322,6 +327,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'image.png'
+
               response
             end
           end
@@ -341,6 +347,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'image.png'
+
               response
             end
           end
@@ -384,6 +391,7 @@ describe UploadsController do
               it_behaves_like 'content not cached without revalidation' do
                 subject do
                   get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'image.png'
+
                   response
                 end
               end
@@ -420,6 +428,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png'
+
               response
             end
           end
@@ -439,6 +448,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png'
+
               response
             end
           end
@@ -491,6 +501,7 @@ describe UploadsController do
               it_behaves_like 'content not cached without revalidation' do
                 subject do
                   get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png'
+
                   response
                 end
               end
@@ -522,6 +533,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'appearance', mounted_as: 'header_logo', id: appearance.id, filename: 'dk.png'
+
               response
             end
           end
@@ -541,6 +553,7 @@ describe UploadsController do
           it_behaves_like 'content not cached without revalidation' do
             subject do
               get :show, model: 'appearance', mounted_as: 'logo', id: appearance.id, filename: 'dk.png'
+
               response
             end
           end

spec/factories/groups.rb

@@ -18,7 +18,7 @@ FactoryBot.define do
     end
 
     trait :with_avatar do
-      avatar { File.open(Rails.root.join('spec/fixtures/dk.png')) }
+      avatar { fixture_file_upload('spec/fixtures/dk.png') }
     end
 
     trait :access_requestable do

spec/factories/notes.rb

@@ -122,11 +122,11 @@ FactoryBot.define do
     end
 
     trait :with_attachment do
-      attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") }
+      attachment { fixture_file_upload(Rails.root.join( "spec/fixtures/dk.png"), "image/png") }
     end
 
     trait :with_svg_attachment do
-      attachment { fixture_file_upload(Rails.root + "spec/fixtures/unsanitized.svg", "image/svg+xml") }
+      attachment { fixture_file_upload(Rails.root.join("spec/fixtures/unsanitized.svg"), "image/svg+xml") }
     end
 
     transient do

spec/factories/projects.rb

@@ -90,7 +90,7 @@ FactoryBot.define do
     end
 
     trait :with_avatar do
-      avatar { File.open(Rails.root.join('spec/fixtures/dk.png')) }
+      avatar { fixture_file_upload('spec/fixtures/dk.png') }
     end
 
     trait :broken_storage do

spec/factories/uploads.rb

 FactoryBot.define do
   factory :upload do
     model { build(:project) }
-    path { "uploads/-/system/project/avatar/avatar.jpg" }
     size 100.kilobytes
     uploader "AvatarUploader"
 
-    trait :personal_snippet do
+    # we should build a mount agnostic upload by default
+    transient do
+      mounted_as :avatar
+      secret SecureRandom.hex
+    end
+
+    # this needs to comply with RecordsUpload::Concern#upload_path
+    path { File.join("uploads/-/system", model.class.to_s.underscore, mounted_as.to_s, 'avatar.jpg') }
+
+    trait :personal_snippet_upload do
       model { build(:personal_snippet) }
+      path { File.join(secret, 'myfile.jpg') }
       uploader "PersonalFileUploader"
     end
 
     trait :issuable_upload do
-      path { "#{SecureRandom.hex}/myfile.jpg" }
+      path { File.join(secret, 'myfile.jpg') }
       uploader "FileUploader"
     end
 
     trait :namespace_upload do
-      path { "#{SecureRandom.hex}/myfile.jpg" }
       model { build(:group) }
+      path { File.join(secret, 'myfile.jpg') }
       uploader "NamespaceFileUploader"
     end
+
+    trait :attachment_upload do
+      transient do
+        mounted_as :attachment
+      end
+
+      model { build(:note) }
+      uploader "AttachmentUploader"
+    end
   end
 end

spec/factories/users.rb

@@ -38,7 +38,7 @@ FactoryBot.define do
     end
 
     trait :with_avatar do
-      avatar { File.open(Rails.root.join('spec/fixtures/dk.png')) }
+      avatar { fixture_file_upload('spec/fixtures/dk.png') }
     end
 
     trait :two_factor_via_otp do

spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb

@@ -23,6 +23,27 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
     end
   end
 
+  # E.g. The installation is in use at the time of migration, and someone has
+  # just uploaded a file
+  shared_examples 'does not add files in /uploads/tmp' do
+    let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
+
+    before do
+      FileUtils.mkdir(File.dirname(tmp_file))
+      FileUtils.touch(tmp_file)
+    end
+
+    after do
+      FileUtils.rm(tmp_file)
+    end
+
+    it 'does not add files from /uploads/tmp' do
+      described_class.new.perform
+
+      expect(untracked_files_for_uploads.count).to eq(5)
+    end
+  end
+
   it 'ensures the untracked_files_for_uploads table exists' do
     expect do
       described_class.new.perform
@@ -109,24 +130,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
         end
       end
 
-      # E.g. The installation is in use at the time of migration, and someone has
-      # just uploaded a file
       context 'when there are files in /uploads/tmp' do
-        let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
-
-        before do
-          FileUtils.touch(tmp_file)
-        end
-
-        after do
-          FileUtils.rm(tmp_file)
-        end
-
-        it 'does not add files from /uploads/tmp' do
-          described_class.new.perform
-
-          expect(untracked_files_for_uploads.count).to eq(5)
-        end
+        it_behaves_like 'does not add files in /uploads/tmp'
       end
     end
   end
@@ -197,24 +202,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
         end
       end
 
-      # E.g. The installation is in use at the time of migration, and someone has
-      # just uploaded a file
       context 'when there are files in /uploads/tmp' do
-        let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
-
-        before do
-          FileUtils.touch(tmp_file)
-        end
-
-        after do
-          FileUtils.rm(tmp_file)
-        end
-
-        it 'does not add files from /uploads/tmp' do
-          described_class.new.perform
-
-          expect(untracked_files_for_uploads.count).to eq(5)
-        end
+        it_behaves_like 'does not add files in /uploads/tmp'
       end
     end
   end

spec/lib/gitlab/gfm/uploads_rewriter_spec.rb

@@ -17,7 +17,7 @@ describe Gitlab::Gfm::UploadsRewriter do
     end
 
     let(:text) do
-      "Text and #{image_uploader.to_markdown} and #{zip_uploader.to_markdown}"
+      "Text and #{image_uploader.markdown_link} and #{zip_uploader.markdown_link}"
     end
 
     describe '#rewrite' do

spec/lib/gitlab/import_export/uploads_restorer_spec.rb

@@ -4,7 +4,6 @@ describe Gitlab::ImportExport::UploadsRestorer do
   describe 'bundle a project Git repo' do
     let(:export_path) { "#{Dir.tmpdir}/uploads_saver_spec" }
     let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: project.full_path) }
-    let(:uploads_path) { FileUploader.dynamic_path_segment(project) }
 
     before do
       allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
@@ -26,9 +25,9 @@ describe Gitlab::ImportExport::UploadsRestorer do
       end
 
       it 'copies the uploads to the project path' do
-        restorer.restore
+        subject.restore
 
-        uploads = Dir.glob(File.join(uploads_path, '**/*')).map { |file| File.basename(file) }
+        uploads = Dir.glob(File.join(subject.uploads_path, '**/*')).map { |file| File.basename(file) }
 
         expect(uploads).to include('dummy.txt')
       end
@@ -44,9 +43,9 @@ describe Gitlab::ImportExport::UploadsRestorer do
       end
 
       it 'copies the uploads to the project path' do
-        restorer.restore
+        subject.restore
 
-        uploads = Dir.glob(File.join(uploads_path, '**/*')).map { |file| File.basename(file) }
+        uploads = Dir.glob(File.join(subject.uploads_path, '**/*')).map { |file| File.basename(file) }
 
         expect(uploads).to include('dummy.txt')
       end

spec/lib/gitlab/import_export/uploads_saver_spec.rb

@@ -30,7 +30,7 @@ describe Gitlab::ImportExport::UploadsSaver do
       it 'copies the uploads to the export path' do
         saver.save
 
-        uploads = Dir.glob(File.join(shared.export_path, 'uploads', '**/*')).map { |file| File.basename(file) }
+        uploads = Dir.glob(File.join(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) }
 
         expect(uploads).to include('banana_sample.gif')
       end
@@ -52,7 +52,7 @@ describe Gitlab::ImportExport::UploadsSaver do
       it 'copies the uploads to the export path' do
         saver.save
 
-        uploads = Dir.glob(File.join(shared.export_path, 'uploads', '**/*')).map { |file| File.basename(file) }
+        uploads = Dir.glob(File.join(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) }
 
         expect(uploads).to include('banana_sample.gif')
       end

spec/models/namespace_spec.rb

@@ -204,7 +204,7 @@ describe Namespace do
       let(:parent) { create(:group, name: 'parent', path: 'parent') }
       let(:child) { create(:group, name: 'child', path: 'child', parent: parent) }
       let!(:project) { create(:project_empty_repo, path: 'the-project', namespace: child, skip_disk_validation: true) }
-      let(:uploads_dir) { File.join(CarrierWave.root, FileUploader.base_dir) }
+      let(:uploads_dir) { FileUploader.root }
       let(:pages_dir) { File.join(TestEnv.pages_path) }
 
       before do

spec/models/upload_spec.rb

@@ -45,51 +45,6 @@ describe Upload do
     end
   end
 
-  describe '.remove_path' do
-    it 'removes all records at the given path' do
-      described_class.create!(
-        size: File.size(__FILE__),
-        path: __FILE__,
-        model: build_stubbed(:user),
-        uploader: 'AvatarUploader'
-      )
-
-      expect { described_class.remove_path(__FILE__) }
-        .to change { described_class.count }.from(1).to(0)
-    end
-  end
-
-  describe '.record' do
-    let(:fake_uploader) do
-      double(
-        file: double(size: 12_345),
-        relative_path: 'foo/bar.jpg',
-        model: build_stubbed(:user),
-        class: 'AvatarUploader'
-      )
-    end
-
-    it 'removes existing paths before creation' do
-      expect(described_class).to receive(:remove_path)
-        .with(fake_uploader.relative_path)
-
-      described_class.record(fake_uploader)
-    end
-
-    it 'creates a new record and assigns size, path, model, and uploader' do
-      upload = described_class.record(fake_uploader)
-
-      aggregate_failures do
-        expect(upload).to be_persisted
-        expect(upload.size).to eq fake_uploader.file.size
-        expect(upload.path).to eq fake_uploader.relative_path
-        expect(upload.model_id).to eq fake_uploader.model.id
-        expect(upload.model_type).to eq fake_uploader.model.class.to_s
-        expect(upload.uploader).to eq fake_uploader.class
-      end
-    end
-  end
-
   describe '#absolute_path' do
     it 'returns the path directly when already absolute' do
       path = '/path/to/namespace/project/secret/file.jpg'
@@ -111,27 +66,27 @@ describe Upload do
     end
   end
 
-  describe '#calculate_checksum' do
-    it 'calculates the SHA256 sum' do
-      upload = described_class.new(
-        path: __FILE__,
-        size: described_class::CHECKSUM_THRESHOLD - 1.megabyte
-      )
+  describe '#calculate_checksum!' do
+    let(:upload) do
+      described_class.new(path: __FILE__,
+                          size: described_class::CHECKSUM_THRESHOLD - 1.megabyte)
+    end
+
+    it 'sets `checksum` to SHA256 sum of the file' do
       expected = Digest::SHA256.file(__FILE__).hexdigest
 
-      expect { upload.calculate_checksum }
+      expect { upload.calculate_checksum! }
         .to change { upload.checksum }.from(nil).to(expected)
     end
 
-    it 'returns nil for a non-existant file' do
-      upload = described_class.new(
-        path: __FILE__,
-        size: described_class::CHECKSUM_THRESHOLD - 1.megabyte
-      )
-
+    it 'sets `checksum` to nil for a non-existant file' do
       expect(upload).to receive(:exist?).and_return(false)
 
-      expect(upload.calculate_checksum).to be_nil
+      checksum = Digest::SHA256.file(__FILE__).hexdigest
+      upload.checksum = checksum
+
+      expect { upload.calculate_checksum! }
+        .to change { upload.checksum }.from(checksum).to(nil)
     end
   end
 

spec/requests/api/runner_spec.rb

@@ -945,7 +945,7 @@ describe API::Runner do
         context 'when artifacts are being stored inside of tmp path' do
           before do
             # by configuring this path we allow to pass temp file from any path
-            allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
+            allow(JobArtifactUploader).to receive(:workhorse_upload_path).and_return('/')
           end
 
           context 'when job has been erased' do
@@ -1122,7 +1122,7 @@ describe API::Runner do
             # by configuring this path we allow to pass file from @tmpdir only
             # but all temporary files are stored in system tmp directory
             @tmpdir = Dir.mktmpdir
-            allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir)
+            allow(JobArtifactUploader).to receive(:workhorse_upload_path).and_return(@tmpdir)
           end
 
           after do

spec/requests/lfs_http_spec.rb

@@ -958,7 +958,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200, location of lfs store and object details' do
-              expect(json_response['StoreLFSPath']).to eq("#{Gitlab.config.shared.path}/lfs-objects/tmp/upload")
+              expect(json_response['StoreLFSPath']).to eq(LfsObjectUploader.workhorse_upload_path)
               expect(json_response['LfsOid']).to eq(sample_oid)
               expect(json_response['LfsSize']).to eq(sample_size)
             end
@@ -1075,7 +1075,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'with location of lfs store and object details' do
-              expect(json_response['StoreLFSPath']).to eq("#{Gitlab.config.shared.path}/lfs-objects/tmp/upload")
+              expect(json_response['StoreLFSPath']).to eq(LfsObjectUploader.workhorse_upload_path)
               expect(json_response['LfsOid']).to eq(sample_oid)
               expect(json_response['LfsSize']).to eq(sample_size)
             end

spec/services/issues/move_service_spec.rb

@@ -6,7 +6,7 @@ describe Issues::MoveService do
   let(:title) { 'Some issue' }
   let(:description) { 'Some issue description' }
   let(:old_project) { create(:project) }
-  let(:new_project) { create(:project) }
+  let(:new_project) { create(:project, group: create(:group)) }
   let(:milestone1) { create(:milestone, project_id: old_project.id, title: 'v9.0') }
 
   let(:old_issue) do
@@ -250,7 +250,7 @@ describe Issues::MoveService do
 
         context 'issue description with uploads' do
           let(:uploader) { build(:file_uploader, project: old_project) }
-          let(:description) { "Text and #{uploader.to_markdown}" }
+          let(:description) { "Text and #{uploader.markdown_link}" }
 
           include_context 'issue move executed'
 

spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb

@@ -6,7 +6,7 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
 
-  let!(:upload) { Upload.find_by(path: file_uploader.relative_path) }
+  let!(:upload) { Upload.find_by(path: file_uploader.upload_path) }
   let(:file_uploader) { build(:file_uploader, project: project) }
   let(:old_path) { File.join(base_path(legacy_storage), upload.path) }
   let(:new_path) { File.join(base_path(hashed_storage), upload.path) }
@@ -58,6 +58,6 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
   end
 
   def base_path(storage)
-    FileUploader.dynamic_path_builder(storage.disk_path)
+    File.join(FileUploader.root, storage.disk_path)
   end
 end

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -2,6 +2,8 @@ shared_examples 'handle uploads' do
   let(:user)  { create(:user) }
   let(:jpg)   { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
   let(:txt)   { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
+  let(:secret) { FileUploader.generate_secret }
+  let(:uploader_class) { FileUploader }
 
   describe "POST #create" do
     context 'when a user is not authorized to upload a file' do
@@ -65,7 +67,12 @@ shared_examples 'handle uploads' do
 
   describe "GET #show" do
     let(:show_upload) do
-      get :show, params.merge(secret: "123456", filename: "image.jpg")
+      get :show, params.merge(secret: secret, filename: "rails_sample.jpg")
+    end
+
+    before do
+      expect(FileUploader).to receive(:generate_secret).and_return(secret)
+      UploadService.new(model, jpg, uploader_class).execute
     end
 
     context "when the model is public" do
@@ -75,11 +82,6 @@ shared_examples 'handle uploads' do
 
       context "when not signed in" do
         context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
           it "responds with status 200" do
             show_upload
 
@@ -88,6 +90,10 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file doesn't exist" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+          end
+
           it "responds with status 404" do
             show_upload
 
@@ -102,11 +108,6 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
           it "responds with status 200" do
             show_upload
 
@@ -115,6 +116,10 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file doesn't exist" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+          end
+
           it "responds with status 404" do
             show_upload
 
@@ -131,11 +136,6 @@ shared_examples 'handle uploads' do
 
       context "when not signed in" do
         context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
           context "when the file is an image" do
             before do
               allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
@@ -149,6 +149,10 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file is not an image" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:image?).and_return(false)
+            end
+
             it "redirects to the sign in page" do
               show_upload
 
@@ -158,6 +162,10 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file doesn't exist" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+          end
+
           it "redirects to the sign in page" do
             show_upload
 
@@ -177,11 +185,6 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file exists" do
-            before do
-              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-              allow(jpg).to receive(:exists?).and_return(true)
-            end
-
             it "responds with status 200" do
               show_upload
 
@@ -190,6 +193,10 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file doesn't exist" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+            end
+
             it "responds with status 404" do
               show_upload
 
@@ -200,11 +207,6 @@ shared_examples 'handle uploads' do
 
         context "when the user doesn't have access to the model" do
           context "when the file exists" do
-            before do
-              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-              allow(jpg).to receive(:exists?).and_return(true)
-            end
-
             context "when the file is an image" do
               before do
                 allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
@@ -218,6 +220,10 @@ shared_examples 'handle uploads' do
             end
 
             context "when the file is not an image" do
+              before do
+                allow_any_instance_of(FileUploader).to receive(:image?).and_return(false)
+              end
+
               it "responds with status 404" do
                 show_upload
 
@@ -227,6 +233,10 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file doesn't exist" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+            end
+
             it "responds with status 404" do
               show_upload
 

spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb

+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

spec/support/test_env.rb

@@ -237,7 +237,7 @@ module TestEnv
   end
 
   def artifacts_path
-    Gitlab.config.artifacts.path
+    Gitlab.config.artifacts.storage_path
   end
 
   # When no cached assets exist, manually hit the root path to create them

spec/support/track_untracked_uploads_helpers.rb

 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
 

spec/uploaders/attachment_uploader_spec.rb

 require 'spec_helper'
 
 describe AttachmentUploader do
-  let(:uploader) { described_class.new(build_stubbed(:user)) }
+  let(:note) { create(:note, :with_attachment) }
+  let(:uploader) { note.attachment }
+  let(:upload) { create(:upload, :attachment_upload, model: uploader.model) }
 
-  describe "#store_dir" do
-    it "stores in the system dir" do
-      expect(uploader.store_dir).to start_with("uploads/-/system/user")
-    end
+  subject { uploader }
 
-    it "uses the old path when using object storage" do
-      expect(described_class).to receive(:file_storage?).and_return(false)
-      expect(uploader.store_dir).to start_with("uploads/user")
-    end
-  end
-
-  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
+  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/]
 end

spec/uploaders/avatar_uploader_spec.rb

 require 'spec_helper'
 
 describe AvatarUploader do
-  let(:uploader) { described_class.new(build_stubbed(:user)) }
+  let(:model) { create(:user, :with_avatar) }
+  let(:uploader) { described_class.new(model, :avatar) }
+  let(:upload) { create(:upload, model: model) }
 
-  describe "#store_dir" do
-    it "stores in the system dir" do
-      expect(uploader.store_dir).to start_with("uploads/-/system/user")
-    end
+  subject { uploader }
 
-    it "uses the old path when using object storage" do
-      expect(described_class).to receive(:file_storage?).and_return(false)
-      expect(uploader.store_dir).to start_with("uploads/user")
-    end
-  end
+  it_behaves_like 'builds correct paths',
+                  store_dir: %r[uploads/-/system/user/avatar/],
+                  upload_path: %r[uploads/-/system/user/avatar/],
+                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/user/avatar/]
 
   describe '#move_to_cache' do
     it 'is false' do

spec/uploaders/file_mover_spec.rb

@@ -3,13 +3,13 @@ require 'spec_helper'
 describe FileMover do
   let(:filename) { 'banana_sample.gif' }
   let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) }
+  let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) }
+
   let(:temp_description) do
-    'test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) same ![banana_sample]'\
-    '(/uploads/-/system/temp/secret55/banana_sample.gif)'
+    "test ![banana_sample](/#{temp_file_path}) "\
+    "same ![banana_sample](/#{temp_file_path}) "
   end
-  let(:temp_file_path) { File.join('secret55', filename).to_s }
-  let(:file_path) { File.join('uploads', '-', 'system', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s }
-
+  let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) }
   let(:snippet) { create(:personal_snippet, description: temp_description) }
 
   subject { described_class.new(file_path, snippet).execute }
@@ -28,8 +28,8 @@ describe FileMover do
 
         expect(snippet.reload.description)
           .to eq(
-            "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\
-            " same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"
+            "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
+            "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "
           )
       end
 
@@ -50,8 +50,8 @@ describe FileMover do
 
         expect(snippet.reload.description)
           .to eq(
-            "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"\
-            " same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"
+            "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\
+            "same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "
           )
       end
 

spec/uploaders/file_uploader_spec.rb

 require 'spec_helper'
 
 describe FileUploader do
-  let(:uploader) { described_class.new(build_stubbed(:project)) }
+  let(:group) { create(:group, name: 'awesome') }
+  let(:project) { create(:project, namespace: group, name: 'project') }
+  let(:uploader) { described_class.new(project) }
+  let(:upload)  { double(model: project, path: 'secret/foo.jpg') }
 
-  context 'legacy storage' do
-    let(:project) { build_stubbed(:project) }
-
-    describe '.absolute_path' do
-      it 'returns the correct absolute path by building it dynamically' do
-        upload = double(model: project, path: 'secret/foo.jpg')
-
-        dynamic_segment = project.full_path
+  subject { uploader }
 
-        expect(described_class.absolute_path(upload))
-          .to end_with("#{dynamic_segment}/secret/foo.jpg")
-      end
-    end
-
-    describe "#store_dir" do
-      it "stores in the namespace path" do
-        uploader = described_class.new(project)
-
-        expect(uploader.store_dir).to include(project.full_path)
-        expect(uploader.store_dir).not_to include("system")
-      end
-    end
+  shared_examples 'builds correct legacy storage paths' do
+    include_examples 'builds correct paths',
+                     store_dir: %r{awesome/project/\h+},
+                     absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg}
   end
 
-  context 'hashed storage' do
+  shared_examples 'uses hashed storage' do
     context 'when rolled out attachments' do
-      let(:project) { build_stubbed(:project, :hashed) }
-
-      describe '.absolute_path' do
-        it 'returns the correct absolute path by building it dynamically' do
-          upload = double(model: project, path: 'secret/foo.jpg')
-
-          dynamic_segment = project.disk_path
-
-          expect(described_class.absolute_path(upload))
-            .to end_with("#{dynamic_segment}/secret/foo.jpg")
-        end
+      before do
+        allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed')
       end
 
-      describe "#store_dir" do
-        it "stores in the namespace path" do
-          uploader = described_class.new(project)
+      let(:project) { build_stubbed(:project, :hashed, namespace: group, name: 'project') }
 
-          expect(uploader.store_dir).to include(project.disk_path)
-          expect(uploader.store_dir).not_to include("system")
-        end
-      end
+      it_behaves_like 'builds correct paths',
+                      store_dir: %r{ca/fe/fe/ed/\h+},
+                      absolute_path: %r{#{described_class.root}/ca/fe/fe/ed/secret/foo.jpg}
     end
 
     context 'when only repositories are rolled out' do
-      let(:project) { build_stubbed(:project, storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) }
-
-      describe '.absolute_path' do
-        it 'returns the correct absolute path by building it dynamically' do
-          upload = double(model: project, path: 'secret/foo.jpg')
-
-          dynamic_segment = project.full_path
-
-          expect(described_class.absolute_path(upload))
-            .to end_with("#{dynamic_segment}/secret/foo.jpg")
-        end
-      end
-
-      describe "#store_dir" do
-        it "stores in the namespace path" do
-          uploader = described_class.new(project)
+      let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) }
 
-          expect(uploader.store_dir).to include(project.full_path)
-          expect(uploader.store_dir).not_to include("system")
-        end
-      end
+      it_behaves_like 'builds correct legacy storage paths'
     end
   end
 
-  describe 'initialize' do
-    it 'generates a secret if none is provided' do
-      expect(SecureRandom).to receive(:hex).and_return('secret')
-
-      uploader = described_class.new(double)
-
-      expect(uploader.secret).to eq 'secret'
-    end
-
-    it 'accepts a secret parameter' do
-      expect(SecureRandom).not_to receive(:hex)
-
-      uploader = described_class.new(double, 'secret')
-
-      expect(uploader.secret).to eq 'secret'
-    end
+  context 'legacy storage' do
+    it_behaves_like 'builds correct legacy storage paths'
+    include_examples 'uses hashed storage'
   end
 
-  describe '#move_to_cache' do
-    it 'is true' do
-      expect(uploader.move_to_cache).to eq(true)
-    end
-  end
+  describe 'initialize' do
+    let(:uploader) { described_class.new(double, 'secret') }
 
-  describe '#move_to_store' do
-    it 'is true' do
-      expect(uploader.move_to_store).to eq(true)
+    it 'accepts a secret parameter' do
+      expect(described_class).not_to receive(:generate_secret)
+      expect(uploader.secret).to eq('secret')
     end
   end
 
-  describe '#relative_path' do
-    it 'removes the leading dynamic path segment' do
-      fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
-      uploader.store!(fixture_file_upload(fixture))
-
-      expect(uploader.relative_path).to match(%r{\A\h{32}/rails_sample.jpg\z})
+  describe '#secret' do
+    it 'generates a secret if none is provided' do
+      expect(described_class).to receive(:generate_secret).and_return('secret')
+      expect(uploader.secret).to eq('secret')
     end
   end
 end

spec/uploaders/job_artifact_uploader_spec.rb

@@ -3,33 +3,13 @@ require 'spec_helper'
 describe JobArtifactUploader do
   let(:job_artifact) { create(:ci_job_artifact) }
   let(:uploader) { described_class.new(job_artifact, :file) }
-  let(:local_path) { Gitlab.config.artifacts.path }
 
-  describe '#store_dir' do
-    subject { uploader.store_dir }
+  subject { uploader }
 
-    let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.job_id}/#{job_artifact.id}" }
-
-    context 'when using local storage' do
-      it { is_expected.to start_with(local_path) }
-      it { is_expected.to match(%r{\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z}) }
-      it { is_expected.to end_with(path) }
-    end
-  end
-
-  describe '#cache_dir' do
-    subject { uploader.cache_dir }
-
-    it { is_expected.to start_with(local_path) }
-    it { is_expected.to end_with('/tmp/cache') }
-  end
-
-  describe '#work_dir' do
-    subject { uploader.work_dir }
-
-    it { is_expected.to start_with(local_path) }
-    it { is_expected.to end_with('/tmp/work') }
-  end
+  it_behaves_like "builds correct paths",
+                  store_dir: %r[\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z],
+                  cache_dir: %r[artifacts/tmp/cache],
+                  work_dir: %r[artifacts/tmp/work]
 
   context 'file is stored in valid local_path' do
     let(:file) do
@@ -43,7 +23,7 @@ describe JobArtifactUploader do
 
     subject { uploader.file.path }
 
-    it { is_expected.to start_with(local_path) }
+    it { is_expected.to start_with("#{uploader.root}/#{uploader.class.base_dir}") }
     it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") }
     it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") }
     it { is_expected.to end_with("ci_build_artifacts.zip") }

spec/uploaders/legacy_artifact_uploader_spec.rb

@@ -3,49 +3,22 @@ require 'rails_helper'
 describe LegacyArtifactUploader do
   let(:job) { create(:ci_build) }
   let(:uploader) { described_class.new(job, :legacy_artifacts_file) }
-  let(:local_path) { Gitlab.config.artifacts.path }
+  let(:local_path) { described_class.root }
 
-  describe '.local_store_path' do
-    subject { described_class.local_store_path }
+  subject { uploader }
 
-    it "delegate to artifacts path" do
-      expect(Gitlab.config.artifacts).to receive(:path)
-
-      subject
-    end
-  end
-
-  describe '.artifacts_upload_path' do
-    subject { described_class.artifacts_upload_path }
+  # TODO: move to Workhorse::UploadPath
+  describe '.workhorse_upload_path' do
+    subject { described_class.workhorse_upload_path }
 
     it { is_expected.to start_with(local_path) }
-    it { is_expected.to end_with('tmp/uploads/') }
-  end
-
-  describe '#store_dir' do
-    subject { uploader.store_dir }
-
-    let(:path) { "#{job.created_at.utc.strftime('%Y_%m')}/#{job.project_id}/#{job.id}" }
-
-    context 'when using local storage' do
-      it { is_expected.to start_with(local_path) }
-      it { is_expected.to end_with(path) }
-    end
+    it { is_expected.to end_with('tmp/uploads') }
   end
 
-  describe '#cache_dir' do
-    subject { uploader.cache_dir }
-
-    it { is_expected.to start_with(local_path) }
-    it { is_expected.to end_with('/tmp/cache') }
-  end
-
-  describe '#work_dir' do
-    subject { uploader.work_dir }
-
-    it { is_expected.to start_with(local_path) }
-    it { is_expected.to end_with('/tmp/work') }
-  end
+  it_behaves_like "builds correct paths",
+                  store_dir: %r[\d{4}_\d{1,2}/\d+/\d+\z],
+                  cache_dir: %r[artifacts/tmp/cache],
+                  work_dir: %r[artifacts/tmp/work]
 
   describe '#filename' do
     # we need to use uploader, as this makes to use mounter
@@ -69,7 +42,7 @@ describe LegacyArtifactUploader do
 
     subject { uploader.file.path }
 
-    it { is_expected.to start_with(local_path) }
+    it { is_expected.to start_with("#{uploader.root}") }
     it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") }
     it { is_expected.to include("/#{job.project_id}/") }
     it { is_expected.to end_with("ci_build_artifacts.zip") }

spec/uploaders/lfs_object_uploader_spec.rb

@@ -2,39 +2,13 @@ require 'spec_helper'
 
 describe LfsObjectUploader do
   let(:lfs_object) { create(:lfs_object, :with_file) }
-  let(:uploader) { described_class.new(lfs_object) }
+  let(:uploader) { described_class.new(lfs_object, :file) }
   let(:path) { Gitlab.config.lfs.storage_path }
 
-  describe '#move_to_cache' do
-    it 'is true' do
-      expect(uploader.move_to_cache).to eq(true)
-    end
-  end
+  subject { uploader }
 
-  describe '#move_to_store' do
-    it 'is true' do
-      expect(uploader.move_to_store).to eq(true)
-    end
-  end
-
-  describe '#store_dir' do
-    subject { uploader.store_dir }
-
-    it { is_expected.to start_with(path) }
-    it { is_expected.to end_with("#{lfs_object.oid[0, 2]}/#{lfs_object.oid[2, 2]}") }
-  end
-
-  describe '#cache_dir' do
-    subject { uploader.cache_dir }
-
-    it { is_expected.to start_with(path) }
-    it { is_expected.to end_with('/tmp/cache') }
-  end
-
-  describe '#work_dir' do
-    subject { uploader.work_dir }
-
-    it { is_expected.to start_with(path) }
-    it { is_expected.to end_with('/tmp/work') }
-  end
+  it_behaves_like "builds correct paths",
+                  store_dir: %r[\h{2}/\h{2}],
+                  cache_dir: %r[/lfs-objects/tmp/cache],
+                  work_dir: %r[/lfs-objects/tmp/work]
 end

spec/uploaders/namespace_file_uploader_spec.rb

 require 'spec_helper'
 
+IDENTIFIER = %r{\h+/\S+}
+
 describe NamespaceFileUploader do
   let(:group) { build_stubbed(:group) }
   let(:uploader) { described_class.new(group) }
+  let(:upload) { create(:upload, :namespace_upload, model: group) }
 
-  describe "#store_dir" do
-    it "stores in the namespace id directory" do
-      expect(uploader.store_dir).to include(group.id.to_s)
-    end
-  end
-
-  describe ".absolute_path" do
-    it "stores in thecorrect directory" do
-      upload_record = create(:upload, :namespace_upload, model: group)
+  subject { uploader }
 
-      expect(described_class.absolute_path(upload_record))
-        .to include("-/system/namespace/#{group.id}")
-    end
-  end
+  it_behaves_like 'builds correct paths',
+                  store_dir: %r[uploads/-/system/namespace/\d+],
+                  upload_path: IDENTIFIER,
+                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/namespace/\d+/#{IDENTIFIER}]
 end

spec/uploaders/personal_file_uploader_spec.rb

 require 'spec_helper'
 
+IDENTIFIER = %r{\h+/\S+}
+
 describe PersonalFileUploader do
-  let(:uploader) { described_class.new(build_stubbed(:project)) }
-  let(:snippet) { create(:personal_snippet) }
+  let(:model) { create(:personal_snippet) }
+  let(:uploader) { described_class.new(model) }
+  let(:upload) { create(:upload, :personal_snippet_upload) }
 
-  describe '.absolute_path' do
-    it 'returns the correct absolute path by building it dynamically' do
-      upload = double(model: snippet, path: 'secret/foo.jpg')
+  subject { uploader }
 
-      dynamic_segment = "personal_snippet/#{snippet.id}"
-
-      expect(described_class.absolute_path(upload)).to end_with("/-/system/#{dynamic_segment}/secret/foo.jpg")
-    end
-  end
+  it_behaves_like 'builds correct paths',
+                  store_dir: %r[uploads/-/system/personal_snippet/\d+],
+                  upload_path: IDENTIFIER,
+                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet/\d+/#{IDENTIFIER}]
 
   describe '#to_h' do
-    it 'returns the hass' do
-      uploader = described_class.new(snippet, 'secret')
+    before do
+      subject.instance_variable_set(:@secret, 'secret')
+    end
 
+    it 'is correct' do
       allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
-      expected_url = "/uploads/-/system/personal_snippet/#{snippet.id}/secret/file_name"
+      expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name"
 
       expect(uploader.to_h).to eq(
         alt: 'file_name',

spec/uploaders/records_uploads_spec.rb

@@ -3,16 +3,16 @@ require 'rails_helper'
 describe RecordsUploads do
   let!(:uploader) do
     class RecordsUploadsExampleUploader < GitlabUploader
-      include RecordsUploads
+      include RecordsUploads::Concern
 
       storage :file
 
-      def model
-        FactoryBot.build_stubbed(:user)
+      def dynamic_segment
+        'co/fe/ee'
       end
     end
 
-    RecordsUploadsExampleUploader.new
+    RecordsUploadsExampleUploader.new(build_stubbed(:user))
   end
 
   def upload_fixture(filename)
@@ -20,48 +20,55 @@ describe RecordsUploads do
   end
 
   describe 'callbacks' do
-    it 'calls `record_upload` after `store`' do
+    let(:upload) { create(:upload) }
+
+    before do
+      uploader.upload = upload
+    end
+
+    it '#record_upload after `store`' do
       expect(uploader).to receive(:record_upload).once
 
       uploader.store!(upload_fixture('doc_sample.txt'))
     end
 
-    it 'calls `destroy_upload` after `remove`' do
-      expect(uploader).to receive(:destroy_upload).once
-
+    it '#destroy_upload after `remove`' do
       uploader.store!(upload_fixture('doc_sample.txt'))
 
+      expect(uploader).to receive(:destroy_upload).once
       uploader.remove!
     end
   end
 
   describe '#record_upload callback' do
-    it 'returns early when not using file storage' do
-      allow(uploader).to receive(:file_storage?).and_return(false)
-      expect(Upload).not_to receive(:record)
-
-      uploader.store!(upload_fixture('rails_sample.jpg'))
+    it 'creates an Upload record after store' do
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to change { Upload.count }.by(1)
     end
 
-    it "returns early when the file doesn't exist" do
-      allow(uploader).to receive(:file).and_return(double(exists?: false))
-      expect(Upload).not_to receive(:record)
-
+    it 'creates a new record and assigns size, path, model, and uploader' do
       uploader.store!(upload_fixture('rails_sample.jpg'))
+
+      upload = uploader.upload
+      aggregate_failures do
+        expect(upload).to be_persisted
+        expect(upload.size).to eq uploader.file.size
+        expect(upload.path).to eq uploader.upload_path
+        expect(upload.model_id).to eq uploader.model.id
+        expect(upload.model_type).to eq uploader.model.class.to_s
+        expect(upload.uploader).to eq uploader.class.to_s
+      end
     end
 
-    it 'creates an Upload record after store' do
-      expect(Upload).to receive(:record)
-        .with(uploader)
+    it "does not create an Upload record when the file doesn't exist" do
+      allow(uploader).to receive(:file).and_return(double(exists?: false))
 
-      uploader.store!(upload_fixture('rails_sample.jpg'))
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
     end
 
     it 'does not create an Upload record if model is missing' do
-      expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
-      expect(Upload).not_to receive(:record).with(uploader)
+      allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
 
-      uploader.store!(upload_fixture('rails_sample.jpg'))
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
     end
 
     it 'it destroys Upload records at the same path before recording' do
@@ -72,29 +79,15 @@ describe RecordsUploads do
         uploader: uploader.class.to_s
       )
 
+      uploader.upload = existing
       uploader.store!(upload_fixture('rails_sample.jpg'))
 
       expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
-      expect(Upload.count).to eq 1
+      expect(Upload.count).to eq(1)
     end
   end
 
   describe '#destroy_upload callback' do
-    it 'returns early when not using file storage' do
-      uploader.store!(upload_fixture('rails_sample.jpg'))
-
-      allow(uploader).to receive(:file_storage?).and_return(false)
-      expect(Upload).not_to receive(:remove_path)
-
-      uploader.remove!
-    end
-
-    it 'returns early when file is nil' do
-      expect(Upload).not_to receive(:remove_path)
-
-      uploader.remove!
-    end
-
     it 'it destroys Upload records at the same path after removal' do
       uploader.store!(upload_fixture('rails_sample.jpg'))
 

spec/workers/upload_checksum_worker_spec.rb

@@ -2,18 +2,31 @@ require 'rails_helper'
 
 describe UploadChecksumWorker do
   describe '#perform' do
-    it 'rescues ActiveRecord::RecordNotFound' do
-      expect { described_class.new.perform(999_999) }.not_to raise_error
+    subject { described_class.new }
+
+    context 'without a valid record' do
+      it 'rescues ActiveRecord::RecordNotFound' do
+        expect { subject.perform(999_999) }.not_to raise_error
+      end
     end
 
-    it 'calls calculate_checksum_without_delay and save!' do
-      upload = spy
-      expect(Upload).to receive(:find).with(999_999).and_return(upload)
+    context 'with a valid record' do
+      let(:upload) { create(:user, :with_avatar).avatar.upload }
+
+      before do
+        expect(Upload).to receive(:find).and_return(upload)
+        allow(upload).to receive(:foreground_checksumable?).and_return(false)
+      end
 
-      described_class.new.perform(999_999)
+      it 'calls calculate_checksum!' do
+        expect(upload).to receive(:calculate_checksum!)
+        subject.perform(upload.id)
+      end
 
-      expect(upload).to have_received(:calculate_checksum)
-      expect(upload).to have_received(:save!)
+      it 'calls save!' do
+        expect(upload).to receive(:save!)
+        subject.perform(upload.id)
+      end
     end
   end
 end