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

Port 16799 to EE

See merge request gitlab-org/gitlab-ee!4389

app/models/upload.rb

@@ -14,6 +14,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
@@ -52,6 +56,10 @@ class Upload < ActiveRecord::Base
 
   private
 
+  def delete_file!
+    build_uploader.remove!
+  end
+
   def checksummable?
     checksum.nil? && local? && exist?
   end

app/uploaders/file_uploader.rb

@@ -15,6 +15,8 @@ class FileUploader < GitlabUploader
   MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
   DYNAMIC_PATH_PATTERN = %r{(?<secret>\h{32})/(?<identifier>.*)}
 
+  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

changelogs/unreleased/14256-upload-destroy-removes-file.yml

+---
+title: Deleting an upload will correctly clean up the filesystem.
+merge_request: 16799
+author:
+type: fixed

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

spec/uploaders/file_uploader_spec.rb

@@ -66,6 +66,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 "#migrate!" do
     before do
       uploader.store!(fixture_file_upload(Rails.root.join('spec/fixtures/dk.png')))