Commit 9ab4c5b7 authored by Sean McGivern's avatar Sean McGivern 🎄

Merge branch '14256-upload-destroy-removes-file' into 'master'

Uploads should delete files when destroyed

Closes #14256

See merge request gitlab-org/gitlab-ce!16799
parents 86342966 939391af
Pipeline #17161517 passed with stages
in 34 minutes and 41 seconds
...@@ -12,6 +12,10 @@ class Upload < ActiveRecord::Base ...@@ -12,6 +12,10 @@ class Upload < ActiveRecord::Base
before_save :calculate_checksum!, if: :foreground_checksummable? before_save :calculate_checksum!, if: :foreground_checksummable?
after_commit :schedule_checksum, if: :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) def self.hexdigest(path)
Digest::SHA256.file(path).hexdigest Digest::SHA256.file(path).hexdigest
end end
...@@ -49,6 +53,10 @@ class Upload < ActiveRecord::Base ...@@ -49,6 +53,10 @@ class Upload < ActiveRecord::Base
private private
def delete_file!
build_uploader.remove!
end
def checksummable? def checksummable?
checksum.nil? && local? && exist? checksum.nil? && local? && exist?
end end
......
...@@ -15,6 +15,8 @@ class FileUploader < GitlabUploader ...@@ -15,6 +15,8 @@ class FileUploader < GitlabUploader
storage :file storage :file
after :remove, :prune_store_dir
def self.root def self.root
File.join(options.storage_path, 'uploads') File.join(options.storage_path, 'uploads')
end end
...@@ -140,6 +142,10 @@ class FileUploader < GitlabUploader ...@@ -140,6 +142,10 @@ class FileUploader < GitlabUploader
end end
end end
def prune_store_dir
storage.delete_dir!(store_dir) # only remove when empty
end
def markdown_name def markdown_name
(image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]") (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]")
end end
......
---
title: Deleting an upload will correctly clean up the filesystem.
merge_request: 16799
author:
type: fixed
...@@ -43,6 +43,18 @@ describe Upload do ...@@ -43,6 +43,18 @@ describe Upload do
.to(a_string_matching(/\A\h{64}\z/)) .to(a_string_matching(/\A\h{64}\z/))
end end
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 end
describe '#absolute_path' do describe '#absolute_path' do
......
...@@ -48,6 +48,29 @@ describe FileUploader do ...@@ -48,6 +48,29 @@ describe FileUploader do
end end
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 describe '#secret' do
it 'generates a secret if none is provided' do it 'generates a secret if none is provided' do
expect(described_class).to receive(:generate_secret).and_return('secret') expect(described_class).to receive(:generate_secret).and_return('secret')
......
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