From dba22bbde25000ed496ae597c4ac6f7954ddf496 Mon Sep 17 00:00:00 2001 From: Andrew Schleifer Date: Fri, 6 Jul 2018 17:15:28 -0500 Subject: [PATCH] rollback changes This reverts: * 1baba84c438e "fix s3 subfolders harder" * ea5e57938edf "fix test for absolute_base_url change" --- app/models/global_setting.rb | 4 ---- app/models/site_setting.rb | 11 ++++++--- lib/file_store/s3_store.rb | 9 +++----- spec/components/file_store/s3_store_spec.rb | 25 ++++++++++++--------- spec/components/pretty_text_spec.rb | 2 +- 5 files changed, 26 insertions(+), 25 deletions(-) diff --git a/app/models/global_setting.rb b/app/models/global_setting.rb index 4bd6cac6547..c81430d2cd5 100644 --- a/app/models/global_setting.rb +++ b/app/models/global_setting.rb @@ -89,10 +89,6 @@ class GlobalSetting @s3_bucket_name ||= s3_bucket.downcase.split("/")[0] end - def self.s3_bucket_folder_path - @s3_bucket_folder_path ||= s3_bucket.downcase.split("/")[1] - end - # for testing def self.reset_s3_cache! @use_s3 = nil diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index 8549cb219e3..858afb57987 100644 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -139,10 +139,15 @@ class SiteSetting < ActiveRecord::Base def self.absolute_base_url bucket = SiteSetting.enable_s3_uploads ? Discourse.store.s3_bucket_name : GlobalSetting.s3_bucket_name - domain = SiteSetting.Upload.s3_region =~ /^cn-/ ? "amazonaws.com.cn" : "amazonaws.com" - path = SiteSetting.enable_s3_uploads ? Discourse.store.s3_bucket_folder_path : GlobalSetting.s3_bucket_folder_path - "//#{bucket}.s3.#{SiteSetting.Upload.s3_region}.#{domain}#{path ? '/' + path : ''}" + # cf. http://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region + if SiteSetting.Upload.s3_region == "us-east-1" + "//#{bucket}.s3.amazonaws.com" + elsif SiteSetting.Upload.s3_region == 'cn-north-1' + "//#{bucket}.s3.cn-north-1.amazonaws.com.cn" + else + "//#{bucket}.s3-#{SiteSetting.Upload.s3_region}.amazonaws.com" + end end end diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 1b354c193f3..483b41e02f0 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -41,7 +41,7 @@ module FileStore # if this fails, it will throw an exception path = @s3_helper.upload(file, path, options) # return the upload url - "//#{URI.parse(absolute_base_url).hostname}/#{path}" + "#{absolute_base_url}/#{path}" end def remove_file(url, path) @@ -65,10 +65,6 @@ module FileStore @s3_helper.s3_bucket_name end - def s3_bucket_folder_path - @s3_helper.s3_bucket_folder_path - end - def absolute_base_url @absolute_base_url ||= SiteSetting.Upload.absolute_base_url end @@ -89,7 +85,8 @@ module FileStore def cdn_url(url) return url if SiteSetting.Upload.s3_cdn_url.blank? schema = url[/^(https?:)?\/\//, 1] - url.sub("#{schema}#{absolute_base_url}", SiteSetting.Upload.s3_cdn_url) + folder = @s3_helper.s3_bucket_folder_path.nil? ? "" : "#{@s3_helper.s3_bucket_folder_path}/" + url.sub("#{schema}#{absolute_base_url}/#{folder}", "#{SiteSetting.Upload.s3_cdn_url}/") end def cache_avatar(avatar, user_id) diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb index 3992f93d986..00df4f36f91 100644 --- a/spec/components/file_store/s3_store_spec.rb +++ b/spec/components/file_store/s3_store_spec.rb @@ -48,7 +48,7 @@ describe FileStore::S3Store do 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" + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png" ) end @@ -66,7 +66,7 @@ describe FileStore::S3Store do 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" + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/discourse-uploads/original/1X/#{upload.sha1}.png" ) end end @@ -83,7 +83,7 @@ describe FileStore::S3Store do 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}" + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/#{path}" ) end @@ -102,7 +102,7 @@ describe FileStore::S3Store do 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}" + "//s3-upload-bucket.s3-us-west-1.amazonaws.com/#{path}" ) end end @@ -116,7 +116,7 @@ describe FileStore::S3Store do it "removes the file from s3 with the right paths" do store.expects(:get_depth_for).with(upload.id).returns(0) 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") + upload.update_attributes!(url: "//s3-upload-bucket.s3-us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png") s3_object = stub s3_bucket.expects(:object).with("tombstone/original/1X/#{upload.sha1}.png").returns(s3_object) @@ -135,7 +135,7 @@ describe FileStore::S3Store do it "removes the file from s3 with the right paths" do store.expects(:get_depth_for).with(upload.id).returns(0) 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") + 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) @@ -151,7 +151,7 @@ describe FileStore::S3Store 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", + url: "//s3-upload-bucket.s3-us-west-1.amazonaws.com/optimized/1X/#{upload.sha1}_1_100x200.png", upload: upload ) end @@ -193,25 +193,28 @@ describe FileStore::S3Store do describe ".has_been_uploaded?" do it "identifies S3 uploads" do - expect(store.has_been_uploaded?("//s3-upload-bucket.s3.us-east-1.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?("//s4_upload_bucket.s3.us-east-1.amazonaws.com/1337.png")).to eq(false) + expect(store.has_been_uploaded?("//s4_upload_bucket.s3.amazonaws.com/1337.png")).to eq(false) end end describe ".absolute_base_url" do it "returns a lowercase schemaless absolute url" do - expect(store.absolute_base_url).to eq("//s3-upload-bucket.s3.us-east-1.amazonaws.com") + expect(store.absolute_base_url).to eq("//s3-upload-bucket.s3.amazonaws.com") end it "uses the proper endpoint" do + SiteSetting.s3_region = "us-east-1" + expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3-upload-bucket.s3.amazonaws.com") + SiteSetting.s3_region = "us-west-2" - expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3-upload-bucket.s3.us-west-2.amazonaws.com") + expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3-upload-bucket.s3-us-west-2.amazonaws.com") SiteSetting.s3_region = "cn-north-1" expect(FileStore::S3Store.new(s3_helper).absolute_base_url).to eq("//s3-upload-bucket.s3.cn-north-1.amazonaws.com.cn") diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb index 977ab2ea765..10fddf0e503 100644 --- a/spec/components/pretty_text_spec.rb +++ b/spec/components/pretty_text_spec.rb @@ -737,7 +737,7 @@ describe PrettyText do def test_s3_with_subfolder_cdn raw = <<~RAW - + RAW html = <<~HTML