From aa5de3c40acebe3f3577a977cf57ed7630a4b740 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 12 Aug 2016 17:18:19 +0800 Subject: [PATCH] FEATURE: Support subfolders in S3 bucket name. This commit also fixes a bug where s3 uploads are not moved to a tombstone folder when removed. --- config/site_settings.yml | 2 +- lib/file_store/s3_store.rb | 32 +++- lib/s3_helper.rb | 10 +- spec/components/file_store/s3_store_spec.rb | 162 +++++++++++++++++--- 4 files changed, 170 insertions(+), 36 deletions(-) diff --git a/config/site_settings.yml b/config/site_settings.yml index 6c2a7d5de..a0f37772f 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -690,7 +690,7 @@ files: enum: 'S3RegionSiteSetting' s3_upload_bucket: default: '' - regex: "^[a-z0-9-]+$" # can't use '.' when using HTTPS + regex: '^[a-z0-9\-\/]+$' # can't use '.' when using HTTPS s3_cdn_url: default: '' regex: '^https?:\/\/.+[^\/]$' diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 83dd28559..8b834ec32 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -7,18 +7,36 @@ require_dependency "file_helper" module FileStore class S3Store < BaseStore + attr_reader :s3_bucket, :s3_bucket_folder_path TOMBSTONE_PREFIX ||= "tombstone/" def initialize(s3_helper=nil) - @s3_helper = s3_helper || S3Helper.new(s3_bucket, TOMBSTONE_PREFIX) + @s3_bucket, @s3_bucket_folder_path = begin + raise Discourse::SiteSettingMissing.new("s3_upload_bucket") if SiteSetting.s3_upload_bucket.blank? + SiteSetting.s3_upload_bucket.downcase.split("/".freeze, 2) + end + + tombstone_prefix = + if @s3_bucket_folder_path + File.join(@s3_bucket_folder_path, TOMBSTONE_PREFIX) + else + TOMBSTONE_PREFIX + end + + @s3_helper = s3_helper || S3Helper.new(s3_bucket, tombstone_prefix) end def store_upload(file, upload, content_type = nil) - path = get_path_for_upload(upload) + path = get_path_for_s3_upload(get_path_for_upload(upload)) store_file(file, path, filename: upload.original_filename, content_type: content_type, cache_locally: true) end + def store_optimized_image(file, optimized_image) + path = get_path_for_s3_upload(get_path_for_optimized_image(optimized_image)) + store_file(file, path) + end + # options # - filename # - content_type @@ -43,7 +61,7 @@ module FileStore def remove_file(url, path) return unless has_been_uploaded?(url) # copy the removed file to tombstone - @s3_helper.remove(path, true) + @s3_helper.remove(get_path_for_s3_upload(path), path) end def has_been_uploaded?(url) @@ -97,11 +115,9 @@ module FileStore UserAvatar.external_avatar_url(user_id, avatar.upload_id, avatar.width) end - def s3_bucket - @s3_bucket ||= begin - raise Discourse::SiteSettingMissing.new("s3_upload_bucket") if SiteSetting.s3_upload_bucket.blank? - SiteSetting.s3_upload_bucket.downcase - end + def get_path_for_s3_upload(path) + path = File.join(@s3_bucket_folder_path, path) if @s3_bucket_folder_path + path end end diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index 095894906..bc35c0df0 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -16,18 +16,18 @@ class S3Helper obj.upload_file(file, options) end - def remove(path, copy_to_tombstone=false) + def remove(s3_filename, tombstone_filename=false) bucket = s3_bucket # copy the file in tombstone - if copy_to_tombstone && @tombstone_prefix.present? + if tombstone_filename && @tombstone_prefix.present? bucket - .object(File.join(@tombstone_prefix, path)) - .copy_from(copy_source: File.join(@s3_bucket, path)) + .object(File.join(@tombstone_prefix, tombstone_filename)) + .copy_from(copy_source: File.join(@s3_bucket, s3_filename)) end # delete the file - bucket.object(path).delete + bucket.object(s3_filename).delete rescue Aws::S3::Errors::NoSuchKey end diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb index 360253aec..57df6e4c6 100644 --- a/spec/components/file_store/s3_store_spec.rb +++ b/spec/components/file_store/s3_store_spec.rb @@ -19,25 +19,7 @@ describe FileStore::S3Store do 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/) - end - - end - - describe ".store_optimized_image" 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/) - end - - end - - context 'removal from s3' do + shared_context 's3 helpers' do let(:store) { FileStore::S3Store.new } let(:client) { Aws::S3::Client.new(stub_responses: true) } let(:resource) { Aws::S3::Resource.new(client: client) } @@ -45,10 +27,84 @@ describe FileStore::S3Store do let(:s3_helper) { store.instance_variable_get(:@s3_helper) } before do - SiteSetting.s3_region = 'us-west-1' + SiteSetting.s3_region ='us-west-1' + end + end + + context 'uploading to s3' do + include_context "s3 helpers" + + describe "#store_upload" do + it "returns an absolute schemaless url" do + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + + s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object) + s3_object.expects(:upload_file) + + expect(store.store_upload(uploaded_file, upload)).to eq( + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png" + ) + end + + describe "when s3_upload_bucket includes folders path" do + before do + SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads" + end + + it "returns an absolute schemaless url" do + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + + s3_bucket.expects(:object).with("discourse-uploads/original/1X/#{upload.sha1}.png").returns(s3_object) + s3_object.expects(:upload_file) + + expect(store.store_upload(uploaded_file, upload)).to eq( + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/discourse-uploads/original/1X/#{upload.sha1}.png" + ) + end + end end - describe ".remove_upload" do + describe "#store_optimized_image" do + it "returns an absolute schemaless url" do + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + path = "optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png" + + s3_bucket.expects(:object).with(path).returns(s3_object) + s3_object.expects(:upload_file) + + expect(store.store_optimized_image(optimized_image_file, optimized_image)).to eq( + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/#{path}" + ) + end + + describe "when s3_upload_bucket includes folders path" do + before do + SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads" + end + + it "returns an absolute schemaless url" do + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + path = "discourse-uploads/optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png" + + s3_bucket.expects(:object).with(path).returns(s3_object) + s3_object.expects(:upload_file) + + expect(store.store_optimized_image(optimized_image_file, optimized_image)).to eq( + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/#{path}" + ) + end + end + end + end + + context 'removal from s3' do + include_context "s3 helpers" + + 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") @@ -61,9 +117,28 @@ describe FileStore::S3Store do store.remove_upload(upload) end + + describe "when s3_upload_bucket includes folders path" do + before do + SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads" + end + + 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/discourse-uploads/original/1X/#{upload.sha1}.png") + s3_object = stub + + s3_bucket.expects(:object).with("discourse-uploads/tombstone/original/1X/#{upload.sha1}.png").returns(s3_object) + s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/discourse-uploads/original/1X/#{upload.sha1}.png") + s3_bucket.expects(:object).with("discourse-uploads/original/1X/#{upload.sha1}.png").returns(s3_object) + s3_object.expects(:delete) + + store.remove_upload(upload) + end + end end - describe ".remove_optimized_image" do + 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", @@ -82,6 +157,24 @@ describe FileStore::S3Store do store.remove_optimized_image(optimized_image) end + + describe "when s3_upload_bucket includes folders path" do + before do + SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads" + 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("discourse-uploads/tombstone/optimized/1X/#{upload.sha1}_1_100x200.png").returns(s3_object) + s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/discourse-uploads/optimized/1X/#{upload.sha1}_1_100x200.png") + s3_bucket.expects(:object).with("discourse-uploads/optimized/1X/#{upload.sha1}_1_100x200.png").returns(s3_object) + s3_object.expects(:delete) + + store.remove_optimized_image(optimized_image) + end + end end end @@ -152,4 +245,29 @@ describe FileStore::S3Store do end end + describe "#s3_bucket" do + it "should return the right bucket name" do + expect(store.s3_bucket).to eq('s3-upload-bucket') + end + + it "should return the right bucket name when folders is included" do + SiteSetting.s3_upload_bucket = 's3-upload-bucket/this-site-upload-folder' + expect(store.s3_bucket).to eq('s3-upload-bucket') + end + end + + describe "s3_bucket_folder_path" do + context "when no folder path is specified" do + it "should return the right folder path" do + expect(store.s3_bucket_folder_path).to eq(nil) + end + end + + context "when site setting contains a folder path" do + it "should return the right folder path" do + SiteSetting.s3_upload_bucket = 's3-upload-bucket/this-site-upload-folder' + expect(store.s3_bucket_folder_path).to eq("this-site-upload-folder") + end + end + end end