From ec341a2bbd81eac9639faf5c5e8e8b670f535096 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Thu, 18 Apr 2019 05:10:21 -0700
Subject: [PATCH] Clean up CarrierWave's import/export files

Unlike uploads that have been uploaded with Tempfile, the project
import/export archives are stored in a temporary cache directory and
remain there if:

1. Object storage is enabled
2. `move_to_store` is set to `true`.

CarrierWave will leave these files there until disk space runs out or a
clean step is run manually.

If `move_to_store` is set to `false`, CarrierWave will remove the files
after storing them. However, unlike a local file, with object storage,
the file is still copied, so setting `move_to_store` to `true`
doesn't buy us anything.

To ensure files are cleaned up, we can just inherit from the
GitlabUploader implementation of `move_to_store`, which returns `true`
if it's a local file, `false` otherwise.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/60656
---
 app/uploaders/import_export_uploader.rb       |  4 ---
 .../unreleased/sh-cleanup-import-export.yml   |  5 +++
 spec/uploaders/import_export_uploader_spec.rb | 32 +++++++++++++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 changelogs/unreleased/sh-cleanup-import-export.yml

diff --git a/app/uploaders/import_export_uploader.rb b/app/uploaders/import_export_uploader.rb
index 716922bc017..104d5d3b3dd 100644
--- a/app/uploaders/import_export_uploader.rb
+++ b/app/uploaders/import_export_uploader.rb
@@ -7,10 +7,6 @@ class ImportExportUploader < AttachmentUploader
     EXTENSION_WHITELIST
   end
 
-  def move_to_store
-    true
-  end
-
   def move_to_cache
     false
   end
diff --git a/changelogs/unreleased/sh-cleanup-import-export.yml b/changelogs/unreleased/sh-cleanup-import-export.yml
new file mode 100644
index 00000000000..3d5d6f3c907
--- /dev/null
+++ b/changelogs/unreleased/sh-cleanup-import-export.yml
@@ -0,0 +1,5 @@
+---
+title: Clean up CarrierWave's import/export files
+merge_request: 27487
+author:
+type: fixed
diff --git a/spec/uploaders/import_export_uploader_spec.rb b/spec/uploaders/import_export_uploader_spec.rb
index 825c1cabc14..2dea48e3a88 100644
--- a/spec/uploaders/import_export_uploader_spec.rb
+++ b/spec/uploaders/import_export_uploader_spec.rb
@@ -3,9 +3,18 @@ require 'spec_helper'
 describe ImportExportUploader do
   let(:model) { build_stubbed(:import_export_upload) }
   let(:upload) { create(:upload, model: model) }
+  let(:import_export_upload) { ImportExportUpload.new }
 
   subject { described_class.new(model, :import_file) }
 
+  context 'local store' do
+    describe '#move_to_store' do
+      it 'returns true' do
+        expect(subject.move_to_store).to be true
+      end
+    end
+  end
+
   context "object_store is REMOTE" do
     before do
       stub_uploads_object_storage
@@ -16,5 +25,28 @@ describe ImportExportUploader do
     it_behaves_like 'builds correct paths',
                     store_dir: %r[import_export_upload/import_file/],
                     upload_path: %r[import_export_upload/import_file/]
+
+    describe '#move_to_store' do
+      it 'returns false' do
+        expect(subject.move_to_store).to be false
+      end
+    end
+
+    describe 'with an export file directly uploaded' do
+      let(:tempfile) { Tempfile.new(['test', '.gz']) }
+
+      before do
+        stub_uploads_object_storage(described_class, direct_upload: true)
+        import_export_upload.export_file = tempfile
+      end
+
+      it 'cleans up cached file' do
+        cache_dir = File.join(import_export_upload.export_file.cache_path(nil), '*')
+
+        import_export_upload.save!
+
+        expect(Dir[cache_dir]).to be_empty
+      end
+    end
   end
 end