From e0102a533ad598c27f90447f813e11ba5a91d216 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Wed, 25 Aug 2021 09:22:36 +1000
Subject: [PATCH] FIX: Restructure temp/ folders for direct S3 uploads (#14137)

Previously we had temp/ in the middle of the S3 key path like so

* /uploads/default/temp/randomstring/test.png (normal site)
* /sitename/uploads/default/temp/randomstring/test.png (s3 folder path site)
* /standard10/uploads/sitename/temp/randomstring/test.png (multisite site)

However this necessitates making a lifecycle rule to clean up incomplete
S3 multipart uploads for every site, something which we cannot do. It makes
much more sense to have a structure with /temp at the start of the key,
which is what this commit does:

* /temp/uploads/default/randomstring/test.png (normal site)
* /temp/sitename/uploads/default/randomstring/test.png (s3 folder path site)
* /temp/standard10/uploads/sitename/randomstring/test.png (multisite site)
---
 lib/file_store/base_store.rb    | 5 +++--
 lib/file_store/s3_store.rb      | 3 +--
 spec/multisite/s3_store_spec.rb | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb
index 49759e7735b..865929e3b63 100644
--- a/lib/file_store/base_store.rb
+++ b/lib/file_store/base_store.rb
@@ -41,10 +41,11 @@ module FileStore
       File.join(path, "test_#{ENV['TEST_ENV_NUMBER'].presence || '0'}")
     end
 
-    def temporary_upload_path(file_name)
+    def temporary_upload_path(file_name, folder_prefix: "")
       File.join(
-        upload_path,
         TEMPORARY_UPLOAD_PREFIX,
+        folder_prefix,
+        upload_path,
         SecureRandom.hex,
         file_name
       )
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index 66642ec91d8..53c54e69c86 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -236,8 +236,7 @@ module FileStore
     end
 
     def temporary_upload_path(file_name)
-      path = super(file_name)
-      s3_bucket_folder_path.nil? ? path : File.join(s3_bucket_folder_path, path)
+      s3_bucket_folder_path.nil? ? super(file_name) : super(file_name, folder_prefix: s3_bucket_folder_path)
     end
 
     def object_from_path(path)
diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb
index 9d2d38eacaf..e86535f441a 100644
--- a/spec/multisite/s3_store_spec.rb
+++ b/spec/multisite/s3_store_spec.rb
@@ -313,7 +313,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         url = store.signed_url_for_temporary_upload("test.png")
         key = store.path_from_url(url)
         expect(url).to match(/Amz-Expires/)
-        expect(key).to match(/uploads\/default\/test_[0-9]\/temp\/[a-zA-z0-9]{0,32}\/test.png/)
+        expect(key).to match(/temp\/uploads\/default\/test_[0-9]\/[a-zA-z0-9]{0,32}\/test.png/)
       end
 
       it "presigned url contans the metadata when provided" do
@@ -329,7 +329,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         url = store.signed_url_for_temporary_upload("test.png")
         key = store.path_from_url(url)
         expect(url).to match(/Amz-Expires/)
-        expect(key).to match(/site\/uploads\/default\/test_[0-9]\/temp\/[a-zA-z0-9]{0,32}\/test.png/)
+        expect(key).to match(/temp\/site\/uploads\/default\/test_[0-9]\/[a-zA-z0-9]{0,32}\/test.png/)
       end
     end
 
@@ -341,7 +341,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
           url = store.signed_url_for_temporary_upload("test.png")
           key = store.path_from_url(url)
           expect(url).to match(/Amz-Expires/)
-          expect(key).to match(/standard99\/uploads\/second\/test_[0-9]\/temp\/[a-zA-z0-9]{0,32}\/test.png/)
+          expect(key).to match(/temp\/standard99\/uploads\/second\/test_[0-9]\/[a-zA-z0-9]{0,32}\/test.png/)
         end
       end
     end