From f40cad0f4bf546052530489ada68720df94de3f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Tue, 16 Jul 2013 23:51:12 +0200 Subject: [PATCH 1/3] changed s3 urls to using the folder convention --- lib/s3.rb | 2 +- spec/components/s3_spec.rb | 5 +++-- spec/models/upload_spec.rb | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/s3.rb b/lib/s3.rb index 09d485a72d..9ae94c85a7 100644 --- a/lib/s3.rb +++ b/lib/s3.rb @@ -13,7 +13,7 @@ module S3 end def self.base_url - "//#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com" + "//s3.amazonaws.com/#{SiteSetting.s3_upload_bucket}" end def self.remove_file(url) diff --git a/spec/components/s3_spec.rb b/spec/components/s3_spec.rb index d0de2d44a9..1005924cb9 100644 --- a/spec/components/s3_spec.rb +++ b/spec/components/s3_spec.rb @@ -17,14 +17,15 @@ describe S3 do let(:image_info) { FastImage.new(file) } before(:each) do - SiteSetting.stubs(:s3_upload_bucket).returns("s3_upload_bucket") + 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") Fog.mock! end it 'returns the url of the S3 upload if successful' do - S3.store_file(file, "SHA", 1).should == '//s3_upload_bucket.s3.amazonaws.com/1SHA.png' + # NOTE: s3 bucket's name are case sensitive so we can't use it as a subdomain... + S3.store_file(file, "SHA", 1).should == '//s3.amazonaws.com/S3_Upload_Bucket/1SHA.png' end after(:each) do diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 139e484db8..8cce0dc72c 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -163,13 +163,13 @@ describe Upload do it "identifies S3 uploads" do SiteSetting.stubs(:enable_s3_uploads).returns(true) - SiteSetting.stubs(:s3_upload_bucket).returns("bucket") - Upload.has_been_uploaded?("//bucket.s3.amazonaws.com/1337.png").should == true + SiteSetting.stubs(:s3_upload_bucket).returns("Bucket") + Upload.has_been_uploaded?("//s3.amazonaws.com/Bucket/1337.png").should == true end it "identifies external urls" do Upload.has_been_uploaded?("http://domain.com/uploads/default/42/0123456789ABCDEF.jpg").should == false - Upload.has_been_uploaded?("//bucket.s3.amazonaws.com/1337.png").should == false + Upload.has_been_uploaded?("//s3.amazonaws.com/Bucket/1337.png").should == false end end From 7ae2fe304dd9a3553d20935651eb01d714f43190 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 17 Jul 2013 00:06:47 +0200 Subject: [PATCH 2/3] renamed s3 to s3_store --- app/models/upload.rb | 8 ++++---- lib/cooked_post_processor.rb | 2 +- lib/{s3.rb => s3_store.rb} | 18 +++++++++--------- .../{s3_spec.rb => s3_store_spec.rb} | 6 +++--- spec/models/upload_spec.rb | 8 ++++---- 5 files changed, 21 insertions(+), 21 deletions(-) rename lib/{s3.rb => s3_store.rb} (77%) rename spec/components/{s3_spec.rb => s3_store_spec.rb} (85%) diff --git a/app/models/upload.rb b/app/models/upload.rb index 91aea72e42..d7fd2f7ae2 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -2,7 +2,7 @@ require 'digest/sha1' require 'image_sizer' require 'tempfile' require 'pathname' -require_dependency 's3' +require_dependency 's3_store' require_dependency 'local_store' class Upload < ActiveRecord::Base @@ -77,12 +77,12 @@ class Upload < ActiveRecord::Base end def self.store_file(file, sha1, upload_id) - return S3.store_file(file, sha1, upload_id) if SiteSetting.enable_s3_uploads? + return S3Store.store_file(file, sha1, upload_id) if SiteSetting.enable_s3_uploads? return LocalStore.store_file(file, sha1, upload_id) end def self.remove_file(url) - return S3.remove_file(url) if SiteSetting.enable_s3_uploads? + return S3Store.remove_file(url) if SiteSetting.enable_s3_uploads? return LocalStore.remove_file(url) end @@ -99,7 +99,7 @@ class Upload < ActiveRecord::Base end def self.is_on_s3?(url) - SiteSetting.enable_s3_uploads? && url.start_with?(S3.base_url) + SiteSetting.enable_s3_uploads? && url.start_with?(S3Store.base_url) end def self.get_from_url(url) diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 211272f0b2..ae10848ad4 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -223,7 +223,7 @@ class CookedPostProcessor def attachments if SiteSetting.enable_s3_uploads? - @doc.css("a[href^=\"#{S3.base_url}\"]") + @doc.css("a[href^=\"#{S3Store.base_url}\"]") else # local uploads are identified using a relative uri @doc.css("a[href^=\"#{LocalStore.directory}\"]") diff --git a/lib/s3.rb b/lib/s3_store.rb similarity index 77% rename from lib/s3.rb rename to lib/s3_store.rb index 9ae94c85a7..753f804597 100644 --- a/lib/s3.rb +++ b/lib/s3_store.rb @@ -1,15 +1,15 @@ -module S3 +module S3Store def self.store_file(file, sha1, upload_id) - S3.check_missing_site_settings + S3Store.check_missing_site_settings - directory = S3.get_or_create_directory(SiteSetting.s3_upload_bucket) + directory = S3Store.get_or_create_directory(SiteSetting.s3_upload_bucket) extension = File.extname(file.original_filename) remote_filename = "#{upload_id}#{sha1}#{extension}" # if this fails, it will throw an exception - file = S3.upload(file, remote_filename, directory) - "#{S3.base_url}/#{remote_filename}" + file = S3Store.upload(file, remote_filename, directory) + "#{S3Store.base_url}/#{remote_filename}" end def self.base_url @@ -17,11 +17,11 @@ module S3 end def self.remove_file(url) - S3.check_missing_site_settings + S3Store.check_missing_site_settings - directory = S3.get_or_create_directory(SiteSetting.s3_upload_bucket) + directory = S3Store.get_or_create_directory(SiteSetting.s3_upload_bucket) - file = S3.destroy(url, directory) + file = S3Store.destroy(url, directory) end def self.check_missing_site_settings @@ -33,7 +33,7 @@ module S3 def self.get_or_create_directory(name) @fog_loaded = require 'fog' unless @fog_loaded - options = S3.generate_options + options = S3Store.generate_options fog = Fog::Storage.new(options) diff --git a/spec/components/s3_spec.rb b/spec/components/s3_store_spec.rb similarity index 85% rename from spec/components/s3_spec.rb rename to spec/components/s3_store_spec.rb index 1005924cb9..7fd490e65c 100644 --- a/spec/components/s3_spec.rb +++ b/spec/components/s3_store_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' require 'fog' -require 's3' +require 's3_store' -describe S3 do +describe S3Store do describe "store_file" do @@ -25,7 +25,7 @@ describe S3 do it 'returns the url of the S3 upload if successful' do # NOTE: s3 bucket's name are case sensitive so we can't use it as a subdomain... - S3.store_file(file, "SHA", 1).should == '//s3.amazonaws.com/S3_Upload_Bucket/1SHA.png' + S3Store.store_file(file, "SHA", 1).should == '//s3.amazonaws.com/S3_Upload_Bucket/1SHA.png' end after(:each) do diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 8cce0dc72c..34df67d3a0 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -119,12 +119,12 @@ describe Upload do it "store files on s3 when enabled" do SiteSetting.expects(:enable_s3_uploads?).returns(true) LocalStore.expects(:store_file).never - S3.expects(:store_file) + S3Store.expects(:store_file) Upload.store_file(image, image_sha1, 1) end it "store files locally by default" do - S3.expects(:store_file).never + S3Store.expects(:store_file).never LocalStore.expects(:store_file) Upload.store_file(image, image_sha1, 1) end @@ -136,12 +136,12 @@ describe Upload do it "remove files on s3 when enabled" do SiteSetting.expects(:enable_s3_uploads?).returns(true) LocalStore.expects(:remove_file).never - S3.expects(:remove_file) + S3Store.expects(:remove_file) Upload.remove_file(upload.url) end it "remove files locally by default" do - S3.expects(:remove_file).never + S3Store.expects(:remove_file).never LocalStore.expects(:remove_file) Upload.remove_file(upload.url) end From 5c27dd175a9181845055e9333368aacceb9e05af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 17 Jul 2013 00:32:09 +0200 Subject: [PATCH 3/3] make sure we handle both s3 url formats --- app/models/upload.rb | 2 +- lib/s3_store.rb | 4 ++++ spec/models/upload_spec.rb | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/app/models/upload.rb b/app/models/upload.rb index d7fd2f7ae2..828eacee54 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -99,7 +99,7 @@ class Upload < ActiveRecord::Base end def self.is_on_s3?(url) - SiteSetting.enable_s3_uploads? && url.start_with?(S3Store.base_url) + SiteSetting.enable_s3_uploads? && (url.start_with?(S3Store.base_url) || url.start_with?(S3Store.base_url_old)) end def self.get_from_url(url) diff --git a/lib/s3_store.rb b/lib/s3_store.rb index 753f804597..a61d311e2c 100644 --- a/lib/s3_store.rb +++ b/lib/s3_store.rb @@ -16,6 +16,10 @@ module S3Store "//s3.amazonaws.com/#{SiteSetting.s3_upload_bucket}" end + def self.base_url_old + "//#{SiteSetting.s3_upload_bucket.downcase}.s3.amazonaws.com" + end + def self.remove_file(url) S3Store.check_missing_site_settings diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 34df67d3a0..268a52b01c 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -174,6 +174,24 @@ describe Upload do end + context ".is_on_s3?" do + + before do + SiteSetting.stubs(:enable_s3_uploads).returns(true) + SiteSetting.stubs(:s3_upload_bucket).returns("BuCkEt") + end + + it "case-insensitively matches the old subdomain format" do + Upload.is_on_s3?("//bucket.s3.amazonaws.com/1337.png").should == true + end + + it "case-sensitively matches the new folder format" do + Upload.is_on_s3?("//s3.amazonaws.com/BuCkEt/1337.png").should == true + Upload.is_on_s3?("//s3.amazonaws.com/bucket/1337.png").should == false + end + + end + context ".get_from_url" do it "works only when the file has been uploaded" do