diff options
-rw-r--r-- | app/models/upload.rb | 8 | ||||
-rw-r--r-- | app/uploaders/file_uploader.rb | 6 | ||||
-rw-r--r-- | changelogs/unreleased/14256-upload-destroy-removes-file.yml | 5 | ||||
-rw-r--r-- | spec/models/upload_spec.rb | 12 | ||||
-rw-r--r-- | spec/uploaders/file_uploader_spec.rb | 23 |
5 files changed, 54 insertions, 0 deletions
diff --git a/app/models/upload.rb b/app/models/upload.rb index 2024228537a..99ad37dc892 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -12,6 +12,10 @@ class Upload < ActiveRecord::Base before_save :calculate_checksum!, if: :foreground_checksummable? after_commit :schedule_checksum, if: :checksummable? + # as the FileUploader is not mounted, the default CarrierWave ActiveRecord + # hooks are not executed and the file will not be deleted + after_destroy :delete_file!, if: -> { uploader_class <= FileUploader } + def self.hexdigest(path) Digest::SHA256.file(path).hexdigest end @@ -49,6 +53,10 @@ class Upload < ActiveRecord::Base private + def delete_file! + build_uploader.remove! + end + def checksummable? checksum.nil? && local? && exist? end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 2310e67cb2e..bde1161dfa8 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -15,6 +15,8 @@ class FileUploader < GitlabUploader storage :file + after :remove, :prune_store_dir + def self.root File.join(options.storage_path, 'uploads') end @@ -140,6 +142,10 @@ class FileUploader < GitlabUploader end end + def prune_store_dir + storage.delete_dir!(store_dir) # only remove when empty + end + def markdown_name (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]") end diff --git a/changelogs/unreleased/14256-upload-destroy-removes-file.yml b/changelogs/unreleased/14256-upload-destroy-removes-file.yml new file mode 100644 index 00000000000..d97188e23f1 --- /dev/null +++ b/changelogs/unreleased/14256-upload-destroy-removes-file.yml @@ -0,0 +1,5 @@ +--- +title: Deleting an upload will correctly clean up the filesystem. +merge_request: 16799 +author: +type: fixed diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 0dcaa026332..36b8e5d304f 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -43,6 +43,18 @@ describe Upload do .to(a_string_matching(/\A\h{64}\z/)) end end + + describe 'after_destroy' do + context 'uploader is FileUploader-based' do + subject { create(:upload, :issuable_upload) } + + it 'calls delete_file!' do + is_expected.to receive(:delete_file!) + + subject.destroy + end + end + end end describe '#absolute_path' do diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index a559681a079..6a92e7fae51 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -48,6 +48,29 @@ describe FileUploader do end end + describe 'callbacks' do + describe '#prune_store_dir after :remove' do + before do + uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) + end + + def store_dir + File.expand_path(uploader.store_dir, uploader.root) + end + + it 'is called' do + expect(uploader).to receive(:prune_store_dir).once + + uploader.remove! + end + + it 'prune the store directory' do + expect { uploader.remove! } + .to change { File.exist?(store_dir) }.from(true).to(false) + end + end + end + describe '#secret' do it 'generates a secret if none is provided' do expect(described_class).to receive(:generate_secret).and_return('secret') |