diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 61554029d09..7ad79a1e56c 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/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 diff --git a/app/models/upload.rb b/app/models/upload.rb index fb55fd8007b..2024228537a 100644 --- a/app/models/upload.rb +++ b/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 - Object.const_get(uploader) + def mount_point + super&.to_sym end end diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index e7af1483d23..8f56f09c9f7 100644 --- a/app/uploaders/file_mover.rb +++ b/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 diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 85ae9863b13..2310e67cb2e 100644 --- a/app/uploaders/file_uploader.rb +++ b/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 diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index b12829efe73..a9e5c028b03 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/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 diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index dfb8dccec57..458928bc067 100644 --- a/app/uploaders/records_uploads.rb +++ b/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, - 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 diff --git a/changelogs/unreleased/42547-upload-store-mount-point.yml b/changelogs/unreleased/42547-upload-store-mount-point.yml new file mode 100644 index 00000000000..35ae022984e --- /dev/null +++ b/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 diff --git a/db/migrate/20180129193323_add_uploads_builder_context.rb b/db/migrate/20180129193323_add_uploads_builder_context.rb new file mode 100644 index 00000000000..b3909a770ca --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 14024164359..50a2ceaaeee 100644 --- a/db/schema.rb +++ b/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 diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index 3fdc3c27f73..1b74f735679 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/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 diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb index c8cfe251d27..ff3a2a76acc 100644 --- a/spec/factories/uploads.rb +++ b/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 - 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) } diff --git a/spec/lib/file_size_validator_spec.rb b/spec/lib/file_size_validator_spec.rb index c44bc1840df..ebd907ecb7f 100644 --- a/spec/lib/file_size_validator_spec.rb +++ b/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 } } } diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 42f3d609770..0dcaa026332 100644 --- a/spec/models/upload_spec.rb +++ b/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 diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index a72f853df75..a559681a079 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/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 diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb index a144b39f74f..60e35dcf235 100644 --- a/spec/uploaders/gitlab_uploader_spec.rb +++ b/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