Commit 74ddc805 authored by Micaël Bergeron's avatar Micaël Bergeron

add the uploader context to the upload model

parent 54a575f1
......@@ -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
......
......@@ -30,7 +30,8 @@ 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 +41,13 @@ class Upload < ActiveRecord::Base
File.exist?(absolute_path)
end
def uploader_context
{
identifier: identifier,
secret: secret
}.compact
end
private
def checksummable?
......@@ -62,11 +70,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
Object.const_get(uploader)
def mount_point
super&.to_sym
end
end
......@@ -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
......
......@@ -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,12 +109,12 @@ 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)
if matches = DYNAMIC_PATH_PATTERN.match(value.path)
@secret = matches[:secret]
@identifier = matches[:identifier]
unless apply_context!(value.uploader_context)
if matches = DYNAMIC_PATH_PATTERN.match(value.path)
@secret = matches[:secret]
@identifier = matches[:identifier]
end
end
super
......@@ -124,6 +126,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
......
......@@ -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)
  • Hi @mbergeron, just curious, why uploader_context is passed to GitlabUploader but it is not used. Is that intentional? Thanks!

  • @lulalala the only reason I can see it to make the constructor signature consistent across the whole inheritance chain of GitlabUploader.

Please register or sign in to reply
end
def file_cache_storage?
cache_storage.is_a?(CarrierWave::Storage::File)
end
......
......@@ -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,
path: uploader.upload_path,
model: uploader.model,
uploader: uploader.class.to_s
uploader: self.class.to_s,
size: file.size,
path: upload_path,
model: model,
mount_point: mounted_as
)
end
......
# 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
......@@ -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
......
......@@ -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
secret SecureRandom.hex
filename 'myfile.jpg'
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) }
......
......@@ -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
......@@ -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)
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment