improvements after code review

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
 
@@ -18,19 +20,20 @@ module UploadsActions
   end
 
   # This should either
-  #   - find the file and redirect to its URL
-  #   - send the file
+  #   - send the file directly
+  #   - or redirect to its URL
   #
   def show
     return render_404 unless uploader.exists?
 
-    # send to the remote URL
-    redirect_to uploader.url unless uploader.file_storage?
+    if uploader.file_storage?
+      disposition = uploader.image_or_video? ? 'inline' : 'attachment'
+      expires_in 0.seconds, must_revalidate: true, private: true
 
-    # or send the file
-    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
@@ -41,8 +44,7 @@ module UploadsActions
 
   def upload_mount
     mounted_as = params[:mounted_as]
-    upload_mounts = %w(avatar attachment file logo header_logo)
-    mounted_as if upload_mounts.include? mounted_as
+    mounted_as if UPLOAD_MOUNTS.include?(mounted_as)
   end
 
   def uploader_mounted?

app/models/concerns/avatarable.rb

@@ -2,11 +2,20 @@ module Avatarable
   extend ActiveSupport::Concern
 
   included do
-    class_eval do
-      validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
-      validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
+    prepend ShadowMethods
 
-      mount_uploader :avatar, AvatarUploader
+    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
 
@@ -41,4 +50,5 @@ module Avatarable
 
     url_base + avatar.url
   end
+
 end

app/models/group.rb

@@ -128,10 +128,6 @@ class Group < Namespace
       visibility_level_allowed_by_sub_groups?(level)
   end
 
-  def avatar_url(**args)
-    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

@@ -207,13 +207,6 @@ class Note < ActiveRecord::Base
     current_application_settings.max_attachment_size.megabytes.to_i
   end
 
-  def attachment_upload(uploader)
-    return unless attachment_identifier
-
-    paths = uploader.store_dirs.map { |store, path| File.join(path, attachment_identifier) }
-    Upload.where(model: self, uploader: uploader.class.to_s, path: paths)&.last
-  end
-
   def hook_attrs
     attributes
   end

app/models/project.rb

@@ -930,9 +930,7 @@ class Project < ActiveRecord::Base
   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/user.rb

@@ -864,9 +864,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/uploaders/attachment_uploader.rb

@@ -4,8 +4,6 @@ class AttachmentUploader < GitlabUploader
   prepend ObjectStorage::Extension::RecordsUploads
   include UploaderHelper
 
-  storage_options Gitlab.config.uploads
-
   private
 
   def dynamic_segment

app/uploaders/avatar_uploader.rb

@@ -4,8 +4,6 @@ class AvatarUploader < GitlabUploader
   include ObjectStorage::Concern
   prepend ObjectStorage::Extension::RecordsUploads
 
-  storage_options Gitlab.config.uploads
-
   def exists?
     model.avatar.file && model.avatar.file.present?
   end

app/uploaders/file_uploader.rb

@@ -17,10 +17,8 @@ class FileUploader < GitlabUploader
 
   attr_accessor :model
 
-  storage_options Gitlab.config.uploads
-
   def self.root
-    File.join(storage_options&.storage_path, 'uploads')
+    File.join(options.storage_path, 'uploads')
   end
 
   def self.absolute_path(upload)

app/uploaders/gitlab_uploader.rb

 class GitlabUploader < CarrierWave::Uploader::Base
+  class_attribute :options
+
   class << self
     # DSL setter
-    def storage_options(options = nil)
-      @storage_options = options if options
-      @storage_options
+    def storage_options(options)
+      self.options = options
     end
 
     def root
-      storage_options&.storage_path
+      options.storage_path
     end
 
     # represent the directory namespacing at the class level
     def base_dir
-      storage_options.fetch('base_dir', '')
+      options.fetch('base_dir', '')
     end
 
     def file_storage?
@@ -24,6 +25,8 @@ class GitlabUploader < CarrierWave::Uploader::Base
     end
   end
 
+  storage_options Gitlab.config.uploads
+
   delegate :base_dir, :file_storage?, to: :class
 
   def file_cache_storage?

app/uploaders/namespace_file_uploader.rb

 class NamespaceFileUploader < FileUploader
-  storage_options Gitlab.config.uploads
-
   # Re-Override
   def self.root
-    storage_options&.storage_path
+    options.storage_path
   end
 
   def self.base_dir(model)
-    File.join(storage_options&.base_dir, 'namespace', model_path_segment(model))
+    File.join(options.base_dir, 'namespace', model_path_segment(model))
   end
 
   def self.model_path_segment(model)

app/uploaders/personal_file_uploader.rb

 class PersonalFileUploader < FileUploader
-  storage_options Gitlab.config.uploads
-
   # Re-Override
   def self.root
-    storage_options&.storage_path
+    options.storage_path
   end
 
   def self.base_dir(model)
-    File.join(storage_options&.base_dir, model_path_segment(model))
+    File.join(options.base_dir, model_path_segment(model))
   end
 
   def self.model_path_segment(model)

ee/app/uploaders/object_storage.rb

@@ -30,7 +30,7 @@ module ObjectStorage
         paths = store_dirs.map { |store, path| File.join(path, identifier) }
 
         unless current_upload_satisfies?(paths, model)
-          # we already have the right upload, don't fetch
+          # the upload we already have isn't right, find the correct one
           self.upload = uploads.find_by(model: model, path: paths)
         end
 
@@ -56,7 +56,7 @@ module ObjectStorage
 
         paths.include?(upload.path) &&
           upload.model_id == model.id &&
-          upload.model_type == model.class.to_s
+          upload.model_type == model.base_class.sti_name
       end
     end
   end
@@ -73,23 +73,23 @@ module ObjectStorage
 
     class_methods do
       def object_store_options
-        storage_options&.object_store
+        options.object_store
       end
 
       def object_store_enabled?
-        object_store_options&.enabled
+        object_store_options.enabled
       end
 
       def background_upload_enabled?
-        object_store_options&.background_upload
+        object_store_options.background_upload
       end
 
       def object_store_credentials
-        object_store_options&.connection&.to_hash&.deep_symbolize_keys
+        object_store_options.connection.to_hash.deep_symbolize_keys
       end
 
       def remote_store_path
-        object_store_options&.remote_directory
+        object_store_options.remote_directory
       end
 
       def licensed?
@@ -252,7 +252,7 @@ module ObjectStorage
     # this is a hack around CarrierWave. The #migrate method needs to be
     # able to force the current file to the migrated file upon success.
     def file=(file)
-      @file = file
+      @file = file # rubocop:disable Gitlab/ModuleWithInstanceVariables
     end
 
     def serialization_column

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
@@ -213,12 +215,14 @@ describe UploadsController do
         context "when not signed in" do
           it "responds with status 200" do
             get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
+
             expect(response).to have_gitlab_http_status(200)
           end
 
           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
@@ -238,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
@@ -290,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
@@ -321,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
@@ -340,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
@@ -383,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
@@ -419,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
@@ -438,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
@@ -490,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
@@ -521,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
@@ -540,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/support/shared_examples/uploaders/object_storage_shared_examples.rb

@@ -4,7 +4,7 @@ shared_context 'with storage' do |store, **stub_params|
   end
 end
 
-shared_examples "migrates" do | to_store: , from_store: nil |
+shared_examples "migrates" do |to_store: , from_store: nil|
   let(:to) { to_store }
   let(:from) { from_store || subject.object_store }