Merge branch '42547-upload-store-mount-point' into 'master'

Store uploader context in uploads

Closes #42547

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

app/controllers/concerns/uploads_actions.rb

@@ -70,7 +70,7 @@ module UploadsActions
   end
 
   def build_uploader_from_params
-    uploader = uploader_class.new(model, params[:secret])
+    uploader = uploader_class.new(model, secret: params[:secret])
     uploader.retrieve_from_store!(params[:filename])
     uploader
   end

app/models/upload.rb

@@ -30,7 +30,7 @@ class Upload < ActiveRecord::Base
   end
 
   def build_uploader
-    uploader_class.new(model).tap do |uploader|
+    uploader_class.new(model, mount_point, **uploader_context).tap do |uploader|
       uploader.upload = self
       uploader.retrieve_from_store!(identifier)
     end
@@ -40,6 +40,13 @@ class Upload < ActiveRecord::Base
     File.exist?(absolute_path)
   end
 
+  def uploader_context
+    {
+      identifier: identifier,
+      secret: secret
+    }.compact
+  end
+
   private
 
   def checksummable?
@@ -62,11 +69,15 @@ class Upload < ActiveRecord::Base
     !path.start_with?('/')
   end
 
+  def uploader_class
+    Object.const_get(uploader)
+  end
+
   def identifier
     File.basename(path)
   end
 
-  def uploader_class
+  def mount_point
-    Object.const_get(uploader)
+    super&.to_sym
   end
 end

app/uploaders/file_mover.rb

@@ -49,11 +49,11 @@ class FileMover
   end
 
   def uploader
-    @uploader ||= PersonalFileUploader.new(model, secret)
+    @uploader ||= PersonalFileUploader.new(model, secret: secret)
   end
 
   def temp_file_uploader
-    @temp_file_uploader ||= PersonalFileUploader.new(nil, secret)
+    @temp_file_uploader ||= PersonalFileUploader.new(nil, secret: secret)
   end
 
   def revert

app/uploaders/file_uploader.rb

@@ -62,9 +62,11 @@ class FileUploader < GitlabUploader
 
   attr_accessor :model
 
-  def initialize(model, secret = nil)
+  def initialize(model, mounted_as = nil, **uploader_context)
+    super(model, nil, **uploader_context)
+
     @model = model
-    @secret = secret
+    apply_context!(uploader_context)
   end
 
   def base_dir
@@ -107,15 +109,17 @@ class FileUploader < GitlabUploader
     self.file.filename
   end
 
-  # the upload does not hold the secret, but holds the path
-  # which contains the secret: extract it
   def upload=(value)
+    super
+
+    return unless value
+    return if apply_context!(value.uploader_context)
+
+    # fallback to the regex based extraction
     if matches = DYNAMIC_PATH_PATTERN.match(value.path)
       @secret = matches[:secret]
       @identifier = matches[:identifier]
     end
-
-    super
   end
 
   def secret
@@ -124,6 +128,18 @@ class FileUploader < GitlabUploader
 
   private
 
+  def apply_context!(uploader_context)
+    @secret, @identifier = uploader_context.values_at(:secret, :identifier)
+
+    !!(@secret && @identifier)
+  end
+
+  def build_upload
+    super.tap do |upload|
+      upload.secret = secret
+    end
+  end
+
   def markdown_name
     (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]")
   end

app/uploaders/gitlab_uploader.rb

@@ -29,6 +29,10 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   delegate :base_dir, :file_storage?, to: :class
 
+  def initialize(model, mounted_as = nil, **uploader_context)
+    super(model, mounted_as)
+  end
+
   def file_cache_storage?
     cache_storage.is_a?(CarrierWave::Storage::File)
   end

app/uploaders/records_uploads.rb

@@ -24,7 +24,7 @@ module RecordsUploads
         uploads.where(path: upload_path).delete_all
         upload.destroy! if upload
 
-        self.upload = build_upload_from_uploader(self)
+        self.upload = build_upload
         upload.save!
       end
     end
@@ -39,12 +39,13 @@ module RecordsUploads
       Upload.order(id: :desc).where(uploader: self.class.to_s)
     end
 
-    def build_upload_from_uploader(uploader)
+    def build_upload
       Upload.new(
-        size: uploader.file.size,
+        uploader: self.class.to_s,
-        path: uploader.upload_path,
+        size: file.size,
-        model: uploader.model,
+        path: upload_path,
-        uploader: uploader.class.to_s
+        model: model,
+        mount_point: mounted_as
       )
     end
 

changelogs/unreleased/42547-upload-store-mount-point.yml

@@ -0,0 +1,5 @@
+---
+title: Added uploader metadata to the uploads.
+merge_request: 16779
+author:
+type: added

db/migrate/20180129193323_add_uploads_builder_context.rb

@@ -0,0 +1,14 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddUploadsBuilderContext < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def change
+    add_column :uploads, :mount_point, :string
+    add_column :uploads, :secret, :string
+  end
+end

db/schema.rb

@@ -1751,6 +1751,8 @@ ActiveRecord::Schema.define(version: 20180202111106) do
     t.string "model_type"
     t.string "uploader", null: false
     t.datetime "created_at", null: false
+    t.string "mount_point"
+    t.string "secret"
   end
 
   add_index "uploads", ["checksum"], name: "index_uploads_on_checksum", using: :btree

lib/gitlab/gfm/uploads_rewriter.rb

@@ -46,7 +46,7 @@ module Gitlab
       private
 
       def find_file(project, secret, file)
-        uploader = FileUploader.new(project, secret)
+        uploader = FileUploader.new(project, secret: secret)
         uploader.retrieve_from_store!(file)
         uploader.file
       end

spec/factories/uploads.rb

@@ -3,36 +3,40 @@ FactoryBot.define do
     model { build(:project) }
     size 100.kilobytes
     uploader "AvatarUploader"
+    mount_point :avatar
+    secret nil
 
     # we should build a mount agnostic upload by default
     transient do
-      mounted_as :avatar
+      filename 'myfile.jpg'
-      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') }
+    path { File.join("uploads/-/system", model.class.to_s.underscore, mount_point.to_s, 'avatar.jpg') }
 
     trait :personal_snippet_upload do
-      model { build(:personal_snippet) }
-      path { File.join(secret, 'myfile.jpg') }
       uploader "PersonalFileUploader"
+      path { File.join(secret, filename) }
+      model { build(:personal_snippet) }
+      secret SecureRandom.hex
     end
 
     trait :issuable_upload do
-      path { File.join(secret, 'myfile.jpg') }
       uploader "FileUploader"
+      path { File.join(secret, filename) }
+      secret SecureRandom.hex
     end
 
     trait :namespace_upload do
       model { build(:group) }
-      path { File.join(secret, 'myfile.jpg') }
+      path { File.join(secret, filename) }
       uploader "NamespaceFileUploader"
+      secret SecureRandom.hex
     end
 
     trait :attachment_upload do
       transient do
-        mounted_as :attachment
+        mount_point :attachment
       end
 
       model { build(:note) }

spec/lib/file_size_validator_spec.rb

@@ -2,8 +2,8 @@ require 'spec_helper'
 
 describe FileSizeValidator do
   let(:validator) { described_class.new(options) }
-  let(:attachment) { AttachmentUploader.new }
   let(:note) { create(:note) }
+  let(:attachment) { AttachmentUploader.new(note) }
 
   describe 'options uses an integer' do
     let(:options) { { maximum: 10, attributes: { attachment: attachment } } }

spec/models/upload_spec.rb

@@ -103,4 +103,10 @@ describe Upload do
       expect(upload).not_to exist
     end
   end
+
+  describe "#uploader_context" do
+    subject { create(:upload, :issuable_upload, secret: 'secret', filename: 'file.txt') }
+
+    it { expect(subject.uploader_context).to match(a_hash_including(secret: 'secret', identifier: 'file.txt')) }
+  end
 end

spec/uploaders/file_uploader_spec.rb

@@ -40,7 +40,7 @@ describe FileUploader do
   end
 
   describe 'initialize' do
-    let(:uploader) { described_class.new(double, 'secret') }
+    let(:uploader) { described_class.new(double, secret: 'secret') }
 
     it 'accepts a secret parameter' do
       expect(described_class).not_to receive(:generate_secret)
@@ -54,4 +54,31 @@ describe FileUploader do
       expect(uploader.secret).to eq('secret')
     end
   end
+
+  describe '#upload=' do
+    let(:secret) { SecureRandom.hex }
+    let(:upload) { create(:upload, :issuable_upload, secret: secret, filename: 'file.txt') }
+
+    it 'handles nil' do
+      expect(uploader).not_to receive(:apply_context!)
+
+      uploader.upload = nil
+    end
+
+    it 'extract the uploader context from it' do
+      expect(uploader).to receive(:apply_context!).with(a_hash_including(secret: secret, identifier: 'file.txt'))
+
+      uploader.upload = upload
+    end
+
+    context 'uploader_context is empty' do
+      it 'fallbacks to regex based extraction' do
+        expect(upload).to receive(:uploader_context).and_return({})
+
+        uploader.upload = upload
+        expect(uploader.secret).to eq(secret)
+        expect(uploader.instance_variable_get(:@identifier)).to eq('file.txt')
+      end
+    end
+  end
 end

spec/uploaders/gitlab_uploader_spec.rb

@@ -4,7 +4,7 @@ require 'carrierwave/storage/fog'
 describe GitlabUploader do
   let(:uploader_class) { Class.new(described_class) }
 
-  subject { uploader_class.new }
+  subject { uploader_class.new(double) }
 
   describe '#file_storage?' do
     context 'when file storage is used' do