fixing a first round of review

app/controllers/concerns/uploads_actions.rb

@@ -2,7 +2,6 @@ module UploadsActions
   include Gitlab::Utils::StrongMemoize
 
   def create
-    # TODO why not pass a GitlabUploader instance
     link_to_file = UploadService.new(model, params[:file], uploader_class).execute
 
     respond_to do |format|
@@ -18,7 +17,10 @@ module UploadsActions
     end
   end
 
-  # This should either find the @file and redirect to its URL
+  # This should either
+  #   - find the file and redirect to its URL
+  #   - send the file
+  #
   def show
     return render_404 unless uploader.exists?
 
@@ -47,21 +49,21 @@ module UploadsActions
     upload_model_class < CarrierWave::Mount::Extension && !upload_mount.nil?
   end
 
-  # TODO: this method is too complex
-  #
   def uploader
-    @uploader ||= if uploader_mounted?
-                    model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend
-                  else
-                    build_uploader_from_upload || build_uploader_from_params
-                  end
+    strong_memoize(:uploader) do
+      if uploader_mounted?
+        model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend
+      else
+        build_uploader_from_upload || build_uploader_from_params
+      end
+    end
   end
 
   def build_uploader_from_upload
     return nil unless params[:secret] && params[:filename]
 
     upload_path = uploader_class.upload_path(params[:secret], params[:filename])
-    upload = Upload.where(uploader: uploader_class.to_s, path: upload_path)&.last
+    upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_path)
     upload&.build_uploader
   end
 

app/controllers/uploads_controller.rb

-# Used out-of-context uploads
-#   see #upload_model_classs
-#
 class UploadsController < ApplicationController
   include UploadsActions
 
   UnknownUploadModelError = Class.new(StandardError)
 
   MODEL_CLASSES = {
-    "user"    => User,
-    "project" => Project,
-    "note"    => Note,
-    "group"   => Group,
-    "appearance" => Appearance,
+    "user"             => User,
+    "project"          => Project,
+    "note"             => Note,
+    "group"            => Group,
+    "appearance"       => Appearance,
     "personal_snippet" => PersonalSnippet,
-    nil => PersonalSnippet
+    nil                => PersonalSnippet
   }.freeze
 
   rescue_from UnknownUploadModelError, with: :render_404
@@ -72,9 +69,7 @@ class UploadsController < ApplicationController
   end
 
   def upload_model_class
-    raise UnknownUploadModelError unless cls = MODEL_CLASSES[params[:model]]
-
-    cls
+    MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError)
   end
 
   def upload_model_class_has_mounts?

app/models/appearance.rb

@@ -11,7 +11,6 @@ class Appearance < ActiveRecord::Base
 
   mount_uploader :logo,         AttachmentUploader
   mount_uploader :header_logo,  AttachmentUploader
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   CACHE_KEY = 'current_appearance'.freeze
 
@@ -30,21 +29,4 @@ class Appearance < ActiveRecord::Base
       errors.add(:single_appearance_row, 'Only 1 appearances row can exist')
     end
   end
-
-  def logo_upload(uploader)
-    find_upload(uploader, logo_identifier)
-  end
-
-  def header_logo_upload(uploader)
-    find_upload(uploader, header_logo_identifier)
-  end
-
-  private
-
-  def find_upload(uploader, identifier)
-    return unless identifier
-
-    paths = uploader.store_dirs.map { |store, path| File.join(path, identifier) }
-    uploads.where(uploader: uploader.class.to_s, path: paths)&.last
-  end
 end

app/models/concerns/avatarable.rb

@@ -7,10 +7,6 @@ module Avatarable
       validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
       mount_uploader :avatar, AvatarUploader
-
-      # the AvatarUploader < RecordsUploads::Concern
-      # TODO: rename to avatar_uploads and use a scope
-      has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
     end
   end
 
@@ -20,16 +16,6 @@ module Avatarable
     end
   end
 
-  # As the associated `Upload` may have multiple difference paths, we need to find
-  # the last one that fits. There should only be one upload per file anyways.
-  #
-  def avatar_upload(uploader)
-    return unless avatar_identifier
-
-    paths = uploader.store_dirs.map {|store, path| File.join(path, avatar_identifier) }
-    uploads.where(uploader: uploader.class.to_s, path: paths)&.last
-  end
-
   def avatar_path(only_path: true)
     return unless self[:avatar].present?
 

app/models/project.rb

@@ -268,7 +268,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

app/models/upload.rb

@@ -9,23 +9,13 @@ class Upload < ActiveRecord::Base
   validates :model, presence: true
   validates :uploader, presence: true
 
-  before_save  :calculate_checksum!, if: :foreground_checksumable?
-  after_commit :schedule_checksum,   if: :checksumable?
+  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)
-    create(
-      size: uploader.file.size,
-      path: uploader.upload_path,
-      model: uploader.model,
-      uploader: uploader.class.to_s,
-      store: uploader.try(:object_store) || ObjectStorage::Store::LOCAL
-    )
-  end
-
   def self.hexdigest(path)
     Digest::SHA256.file(path).hexdigest
   end
@@ -39,14 +29,15 @@ class Upload < ActiveRecord::Base
 
   def calculate_checksum!
     self.checksum = nil
-    return unless checksumable?
+    return unless checksummable?
 
     self.checksum = self.class.hexdigest(absolute_path)
   end
 
-  def build_uploader(from = nil)
-    (from || uploader_class.new(model)).tap do |uploader|
+  def build_uploader
+    uploader_class.new(model).tap do |uploader|
       uploader.upload = self
+      uploader.retrieve_from_store!(identifier)
     end
   end
 
@@ -56,7 +47,7 @@ class Upload < ActiveRecord::Base
 
   private
 
-  def checksumable?
+  def checksummable?
     checksum.nil? && local? && exist?
   end
 
@@ -66,8 +57,8 @@ class Upload < ActiveRecord::Base
     store == ObjectStorage::Store::LOCAL
   end
 
-  def foreground_checksumable?
-    checksumable? && size <= CHECKSUM_THRESHOLD
+  def foreground_checksummable?
+    checksummable? && size <= CHECKSUM_THRESHOLD
   end
 
   def schedule_checksum
@@ -78,6 +69,10 @@ class Upload < ActiveRecord::Base
     !path.start_with?('/')
   end
 
+  def identifier
+    File.basename(path)
+  end
+
   def uploader_class
     Object.const_get(uploader)
   end

app/uploaders/file_uploader.rb

@@ -125,8 +125,6 @@ class FileUploader < GitlabUploader
     if matches = DYNAMIC_PATH_PATTERN.match(value.path)
       @secret = matches[:secret]
       @identifier = matches[:identifier]
-
-      retrieve_from_store!(@identifier)
     end
 
     super

app/uploaders/records_uploads.rb

@@ -5,7 +5,7 @@ module RecordsUploads
     attr_accessor :upload
 
     included do
-      before :store,  :destroy_upload
+      before :store,  :destroy_previous_upload
       after  :store,  :record_upload
       before :remove, :destroy_upload
     end
@@ -21,7 +21,12 @@ module RecordsUploads
       return unless model
       return unless file && file.exists?
 
-      Upload.record(self)
+      Upload.create(
+        size: file.size,
+        path: upload_path,
+        model: model,
+        uploader: self.class.to_s
+      )
     end
 
     def upload_path
@@ -30,6 +35,12 @@ module RecordsUploads
 
     private
 
+    def destroy_previous_upload(*args)
+      return unless upload
+
+      upload.destroy!
+    end
+
     # Before removing an attachment, destroy any Upload records at the same path
     #
     # Called `before :remove`

config/gitlab.yml.example

@@ -145,15 +145,15 @@ production: &base
     enabled: true
     # The location where build artifacts are stored (default: shared/artifacts).
     # path: shared/artifacts
-    object_store:
-      enabled: false
-      remote_directory: artifacts # The bucket name
-      # background_upload: false # Temporary option to limit automatic upload (Default: true)
-      connection:
-        provider: AWS # Only AWS supported at the moment
-        aws_access_key_id: AWS_ACCESS_KEY_ID
-        aws_secret_access_key: AWS_SECRET_ACCESS_KEY
-        region: eu-central-1
+    # object_store:
+    #   enabled: false
+    #   remote_directory: artifacts # The bucket name
+    #   background_upload: false # Temporary option to limit automatic upload (Default: true)
+    #   connection:
+    #     provider: AWS # Only AWS supported at the moment
+    #     aws_access_key_id: AWS_ACCESS_KEY_ID
+    #     aws_secret_access_key: AWS_SECRET_ACCESS_KEY
+    #     region: eu-central-1
 
   ## Git LFS
   lfs:

config/initializers/1_settings.rb

@@ -341,8 +341,9 @@ Settings.incoming_email['enabled'] = false if Settings.incoming_email['enabled']
 #
 Settings['artifacts'] ||= Settingslogic.new({})
 Settings.artifacts['enabled']      = true if Settings.artifacts['enabled'].nil?
-# DEPRECATED use `storage_path`
 Settings.artifacts['storage_path'] = Settings.absolute(Settings.artifacts.values_at('path', 'storage_path').compact.first || File.join(Settings.shared['path'], "artifacts"))
+# Settings.artifact['path'] is deprecated, use `storage_path` instead
+Settings.artifacts['path'] = Settings.artifacts['storage_path']
 Settings.artifacts['max_size']   ||= 100 # in megabytes
 
 Settings.artifacts['object_store'] ||= Settingslogic.new({})

db/migrate/20171214144320_add_store_column_to_uploads.rb

@@ -9,8 +9,4 @@ class AddStoreColumnToUploads < ActiveRecord::Migration
   def change
     add_column :uploads, :store, :integer
   end
-
-  def down
-    add_column :uploads, :store
-  end
 end

doc/development/file_storage.md

@@ -82,23 +82,24 @@ The `RecordsUploads::Concern` concern will create an `Upload` entry for every fi
 ## 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 `RecordsUploads::Concern` or 2) mount the uploader and create a new field named `<mount>_store`.
+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`
 
-### Using `RecordsUploads::Concern`
+> Note: this concern will automatically include `RecordsUploads::Concern` if not already included.
 
-The `ObjectStorage::Concern` uploader will search for the correct `Upload` model in the `RecordsUploads::Concern#uploads` relationship to select the correct object store.
-`Upload` is mapped using the `CarrierWave::Uploader#upload_path` for each store (LOCAL/REMOTE).
+The `ObjectStorage::Concern` uploader will search for the correct `Upload` in the model's `uploads` relationship to select the correct object store. A basic implementation of the `ObjectStorage:: The `Upload` is mapped using the `RecordsUploads::Concern#upload_path` for each store (LOCAL/REMOTE).
 
 ```ruby
 class SongUploader < GitlabUploader
-  include ObjectStorage::Concern
   include RecordsUploads::Concern
+  include ObjectStorage::Concern
+  prepend ObjectStorage::Extension::RecordsUploads
 
   ...
 end
@@ -112,7 +113,7 @@ end
 
 ### Using a mounted uploader
 
-The `ObjectStorage::Concern` will query the `model.<mount>_store' attribute to select the correct object store.
+The `ObjectStorage::Concern` will query the `model.<mount>_store` attribute to select the correct object store.
 
 ```ruby
 class SongUploader < GitlabUploader

ee/app/uploaders/object_storage.rb

@@ -26,14 +26,28 @@ module ObjectStorage
         base.include(::RecordsUploads::Concern)
       end
 
-      def initialize(model = nil, mounted_as = nil)
-        super
+      def retrieve_from_store!(identifier)
+        paths = store_dirs.map { |store, path| File.join(path, identifier) }
+
+        unless paths.include?(upload&.path)
+          # we already have the right upload, don't fetch
+          self.upload = Upload.find_by(uploader: self.class.to_s, model: model, path: paths)
+        end
 
-        self.upload = model&.try(:"#{mounted_as}_upload", self) if mounted_as
+        super
       end
 
       def record_upload(_tempfile = nil)
-        self.upload = super
+        return unless model
+        return unless file && file.exists?
+
+        self.upload = Upload.create(
+          size: file.size,
+          path: upload_path,
+          model: model,
+          uploader: self.class.to_s,
+          store: object_store
+        )
       end
 
       def destroy_upload(_tempfile = nil)
@@ -152,7 +166,7 @@ module ObjectStorage
       with_callbacks(:store, file_to_delete) do # for #store_versions!
         storage.store!(file).tap do |new_file|
           begin
-            retrieve_from_store!(identifier)
+            @file = new_file
             persist_object_store!
             file_to_delete.delete if new_file.exists?
           rescue => e