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

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')