From 3378ee223f1e22292cd66e8d041b3b09f7112aed Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 15 Aug 2016 11:21:24 +0800 Subject: [PATCH] FIX: Incorrect path being passed to `S3Store#remove_file`. --- lib/file_store/base_store.rb | 6 +- lib/file_store/local_store.rb | 2 +- lib/file_store/s3_store.rb | 5 +- lib/s3_helper.rb | 14 ++-- .../components/file_store/local_store_spec.rb | 9 +-- spec/components/file_store/s3_store_spec.rb | 72 +++++++++++++------ 6 files changed, 68 insertions(+), 40 deletions(-) diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb index 77c7214cd..6f7556494 100644 --- a/lib/file_store/base_store.rb +++ b/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 diff --git a/lib/file_store/local_store.rb b/lib/file_store/local_store.rb index 3891656b0..411b93ed0 100644 --- a/lib/file_store/local_store.rb +++ b/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) diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 0a50ce445..83dd28559 100644 --- a/lib/file_store/s3_store.rb +++ b/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) diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index c203cd89b..095894906 100644 --- a/lib/s3_helper.rb +++ b/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 diff --git a/spec/components/file_store/local_store_spec.rb b/spec/components/file_store/local_store_spec.rb index 142c83d85..2ccebf098 100644 --- a/spec/components/file_store/local_store_spec.rb +++ b/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 diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb index 2976a0cbe..360253aec 100644 --- a/spec/components/file_store/s3_store_spec.rb +++ b/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