FIX: Incorrect path being passed to `S3Store#remove_file`.

lib/file_store/base_store.rb

@@ -17,14 +17,14 @@ module FileStore
     end
 
     def remove_upload(upload)
-      remove_file(upload.url)
+      remove_file(upload.url, get_path_for_upload(upload))
     end
 
     def remove_optimized_image(optimized_image)
-      remove_file(optimized_image.url)
+      remove_file(optimized_image.url, get_path_for_optimized_image(optimized_image))
     end
 
-    def remove_file(url)
+    def remove_file(url, path)
       not_implemented
     end
 

lib/file_store/local_store.rb

@@ -9,7 +9,7 @@ module FileStore
       "#{Discourse.base_uri}#{path}"
     end
 
-    def remove_file(url)
+    def remove_file(url, _)
       return unless is_relative?(url)
       source = "#{public_dir}#{url}"
       return unless File.exists?(source)

lib/file_store/s3_store.rb

@@ -40,11 +40,10 @@ module FileStore
       "#{absolute_base_url}/#{path}"
     end
 
-    def remove_file(url)
+    def remove_file(url, path)
       return unless has_been_uploaded?(url)
-      filename = File.basename(url)
       # copy the removed file to tombstone
-      @s3_helper.remove(filename, true)
+      @s3_helper.remove(path, true)
     end
 
     def has_been_uploaded?(url)

lib/s3_helper.rb

@@ -11,19 +11,23 @@ class S3Helper
     check_missing_site_settings
   end
 
-  def upload(file, unique_filename, options={})
-    obj = s3_bucket.object(unique_filename)
+  def upload(file, path, options={})
+    obj = s3_bucket.object(path)
     obj.upload_file(file, options)
   end
 
-  def remove(unique_filename, copy_to_tombstone=false)
+  def remove(path, copy_to_tombstone=false)
     bucket = s3_bucket
+
     # copy the file in tombstone
     if copy_to_tombstone && @tombstone_prefix.present?
-      bucket.object(@tombstone_prefix + unique_filename).copy_from(copy_source: "#{@s3_bucket}/#{unique_filename}")
+      bucket
+        .object(File.join(@tombstone_prefix, path))
+        .copy_from(copy_source: File.join(@s3_bucket, path))
     end
+
     # delete the file
-    bucket.object(unique_filename).delete
+    bucket.object(path).delete
   rescue Aws::S3::Errors::NoSuchKey
   end
 

spec/components/file_store/local_store_spec.rb

@@ -32,8 +32,6 @@ describe FileStore::LocalStore do
 
     it "does not delete non uploaded" do
       FileUtils.expects(:mkdir_p).never
-      upload = Upload.new
-      upload.stubs(:url).returns("/path/to/file")
       store.remove_upload(upload)
     end
 
@@ -41,22 +39,19 @@ describe FileStore::LocalStore do
       FileUtils.expects(:mkdir_p)
       FileUtils.expects(:move)
       File.expects(:exists?).returns(true)
-      upload = Upload.new
-      upload.stubs(:url).returns("/uploads/default/42/253dc8edf9d4ada1.png")
       store.remove_upload(upload)
     end
 
   end
 
   describe ".remove_optimized_image" do
+    let(:optimized_image) { Fabricate(:optimized_image, url: "/uploads/default/_optimized/42/253dc8edf9d4ada1.png") }
 
     it "moves the file to the tombstone" do
       FileUtils.expects(:mkdir_p)
       FileUtils.expects(:move)
       File.expects(:exists?).returns(true)
-      oi = OptimizedImage.new
-      oi.stubs(:url).returns("/uploads/default/_optimized/42/253dc8edf9d4ada1.png")
-      store.remove_optimized_image(upload)
+      store.remove_optimized_image(optimized_image)
     end
 
   end

spec/components/file_store/s3_store_spec.rb

@@ -14,16 +14,16 @@ describe FileStore::S3Store do
   let(:optimized_image_file) { file_from_fixtures("logo.png") }
 
   before(:each) do
-    SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket")
-    SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
-    SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key")
+    SiteSetting.s3_upload_bucket = "s3-upload-bucket"
+    SiteSetting.s3_access_key_id = "s3-access-key-id"
+    SiteSetting.s3_secret_access_key = "s3-secret-access-key"
   end
 
   describe ".store_upload" do
 
     it "returns an absolute schemaless url" do
       s3_helper.expects(:upload)
-      expect(store.store_upload(uploaded_file, upload)).to match(/\/\/s3_upload_bucket\.s3\.amazonaws\.com\/original\/.+#{upload.sha1}\.png/)
+      expect(store.store_upload(uploaded_file, upload)).to match(/\/\/s3-upload-bucket\.s3\.amazonaws\.com\/original\/.+#{upload.sha1}\.png/)
     end
 
   end
@@ -32,38 +32,68 @@ describe FileStore::S3Store do
 
     it "returns an absolute schemaless url" do
       s3_helper.expects(:upload)
-      expect(store.store_optimized_image(optimized_image_file, optimized_image)).to match(/\/\/s3_upload_bucket\.s3\.amazonaws\.com\/optimized\/.+#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200\.png/)
+      expect(store.store_optimized_image(optimized_image_file, optimized_image)).to match(/\/\/s3-upload-bucket\.s3\.amazonaws\.com\/optimized\/.+#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200\.png/)
     end
 
   end
 
-  describe ".remove_upload" do
+  context 'removal from s3' do
+    let(:store) { FileStore::S3Store.new }
+    let(:client) { Aws::S3::Client.new(stub_responses: true) }
+    let(:resource) { Aws::S3::Resource.new(client: client) }
+    let(:s3_bucket) { resource.bucket("s3-upload-bucket") }
+    let(:s3_helper) { store.instance_variable_get(:@s3_helper) }
 
-    it "calls remove_file with the url" do
-      store.expects(:remove_file).with(upload.url)
-      store.remove_upload(upload)
+    before do
+      SiteSetting.s3_region = 'us-west-1'
     end
 
-  end
+    describe ".remove_upload" do
+      it "removes the file from s3 with the right paths" do
+        s3_helper.expects(:s3_bucket).returns(s3_bucket)
+        upload.update_attributes!(url: "//s3-upload-bucket.s3-us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png")
+        s3_object = stub
 
-  describe ".remove_optimized_image" do
+        s3_bucket.expects(:object).with("tombstone/original/1X/#{upload.sha1}.png").returns(s3_object)
+        s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/original/1X/#{upload.sha1}.png")
+        s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object)
+        s3_object.expects(:delete)
 
-    it "calls remove_file with the url" do
-      store.expects(:remove_file).with(optimized_image.url)
-      store.remove_optimized_image(optimized_image)
+        store.remove_upload(upload)
+      end
     end
 
+    describe ".remove_optimized_image" do
+      let(:optimized_image) do
+        Fabricate(:optimized_image,
+          url: "//s3-upload-bucket.s3-us-west-1.amazonaws.com/optimized/1X/#{upload.sha1}_1_100x200.png",
+          upload: upload
+        )
+      end
+
+      it "removes the file from s3 with the right paths" do
+        s3_helper.expects(:s3_bucket).returns(s3_bucket)
+        s3_object = stub
+
+        s3_bucket.expects(:object).with("tombstone/optimized/1X/#{upload.sha1}_1_100x200.png").returns(s3_object)
+        s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/optimized/1X/#{upload.sha1}_1_100x200.png")
+        s3_bucket.expects(:object).with("optimized/1X/#{upload.sha1}_1_100x200.png").returns(s3_object)
+        s3_object.expects(:delete)
+
+        store.remove_optimized_image(optimized_image)
+      end
+    end
   end
 
   describe ".has_been_uploaded?" do
 
     it "identifies S3 uploads" do
-      expect(store.has_been_uploaded?("//s3_upload_bucket.s3.amazonaws.com/1337.png")).to eq(true)
+      expect(store.has_been_uploaded?("//s3-upload-bucket.s3.amazonaws.com/1337.png")).to eq(true)
     end
 
     it "does not match other s3 urls" do
-      expect(store.has_been_uploaded?("//s3_upload_bucket.s3-us-east-1.amazonaws.com/1337.png")).to eq(false)
-      expect(store.has_been_uploaded?("//s3.amazonaws.com/s3_upload_bucket/1337.png")).to eq(false)
+      expect(store.has_been_uploaded?("//s3-upload-bucket.s3-us-east-1.amazonaws.com/1337.png")).to eq(false)
+      expect(store.has_been_uploaded?("//s3.amazonaws.com/s3-upload-bucket/1337.png")).to eq(false)
       expect(store.has_been_uploaded?("//s4_upload_bucket.s3.amazonaws.com/1337.png")).to eq(false)
     end
 
@@ -72,18 +102,18 @@ describe FileStore::S3Store do
   describe ".absolute_base_url" do
 
     it "returns a lowercase schemaless absolute url" do
-      expect(store.absolute_base_url).to eq("//s3_upload_bucket.s3.amazonaws.com")
+      expect(store.absolute_base_url).to eq("//s3-upload-bucket.s3.amazonaws.com")
     end
 
     it "uses the proper endpoint" do
       SiteSetting.stubs(:s3_region).returns("us-east-1")
-      expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3_upload_bucket.s3.amazonaws.com")
+      expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3-upload-bucket.s3.amazonaws.com")
 
       SiteSetting.stubs(:s3_region).returns("us-east-2")
-      expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3_upload_bucket.s3-us-east-2.amazonaws.com")
+      expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3-upload-bucket.s3-us-east-2.amazonaws.com")
 
       SiteSetting.stubs(:s3_region).returns("cn-north-1")
-      expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3_upload_bucket.s3.cn-north-1.amazonaws.com.cn")
+      expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3-upload-bucket.s3.cn-north-1.amazonaws.com.cn")
 
     end