From 64a6513791adac8d60e6c97401250bf0f54961b2 Mon Sep 17 00:00:00 2001 From: Dat Date: Fri, 26 Aug 2016 13:54:56 +0700 Subject: [PATCH 1/4] allow S3 use accelerate_endpoint --- lib/paperclip/storage/s3.rb | 10 ++++++++++ paperclip.gemspec | 2 +- spec/paperclip/storage/s3_spec.rb | 24 ++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lib/paperclip/storage/s3.rb b/lib/paperclip/storage/s3.rb index d0b1755ca..bfda1c69c 100644 --- a/lib/paperclip/storage/s3.rb +++ b/lib/paperclip/storage/s3.rb @@ -102,6 +102,8 @@ module Storage # Redundancy Storage. RRS enables customers to reduce their # costs by storing non-critical, reproducible data at lower # levels of redundancy than Amazon S3's standard storage. + # * +use_accelerate_endpoint+: Use accelerate endpoint + # http://docs.aws.amazon.com/AmazonS3/latest/dev/transfer-acceleration.html # # You can set storage class on a per style bases by doing the following: # :s3_storage_class => { @@ -152,6 +154,8 @@ def self.extended base @options[:url] = @options[:url].inspect if @options[:url].is_a?(Symbol) @http_proxy = @options[:http_proxy] || nil + + @use_accelerate_endpoint = @options[:use_accelerate_endpoint] || false end Paperclip.interpolates(:s3_alias_url) do |attachment, style| @@ -232,6 +236,8 @@ def s3_interface config[:proxy_uri] = URI::HTTP.build(proxy_opts) end + config[:use_accelerate_endpoint] = true if use_accelerate_endpoint? + [:access_key_id, :secret_access_key, :credential_provider, :credentials].each do |opt| config[opt] = s3_credentials[opt] if s3_credentials[opt] end @@ -257,6 +263,10 @@ def s3_object style_name = default_style s3_bucket.object style_name_as_path(style_name) end + def use_accelerate_endpoint? + @use_accelerate_endpoint + end + def using_http_proxy? !!@http_proxy end diff --git a/paperclip.gemspec b/paperclip.gemspec index 4587fbaa1..a71955bd3 100644 --- a/paperclip.gemspec +++ b/paperclip.gemspec @@ -35,7 +35,7 @@ Gem::Specification.new do |s| s.add_development_dependency('rspec', '~> 3.0') s.add_development_dependency('appraisal') s.add_development_dependency('mocha') - s.add_development_dependency('aws-sdk', '>= 2.0.34', '< 3.0') + s.add_development_dependency('aws-sdk', '>= 2.3.0', '< 3.0') s.add_development_dependency('bourne') s.add_development_dependency('cucumber', '~> 1.3.18') s.add_development_dependency('aruba', '~> 0.9.0') diff --git a/spec/paperclip/storage/s3_spec.rb b/spec/paperclip/storage/s3_spec.rb index 036c98aca..63727ff35 100644 --- a/spec/paperclip/storage/s3_spec.rb +++ b/spec/paperclip/storage/s3_spec.rb @@ -282,6 +282,30 @@ class << @dummy end end + context "use_accelerate_endpoint" do + before do + rebuild_model storage: :s3, + s3_credentials: {}, + bucket: "bucket", + path: ":attachment/:basename:dotextension", + s3_host_name: "s3-accelerate.amazonaws.com", + s3_region: "ap-northeast-1", + use_accelerate_endpoint: true + @dummy = Dummy.new + @dummy.avatar = stringy_file + @dummy.stubs(:new_record?).returns(false) + end + + it "returns a url based on an :s3_host_name path" do + assert_match %r{^//s3-accelerate.amazonaws.com/bucket/avatars/data[^\.]}, @dummy.avatar.url + end + + it "uses the S3 bucket with the correct host name" do + assert_equal "s3-accelerate.amazonaws.com", + @dummy.avatar.s3_bucket.client.config.endpoint.host + end + end + context "An attachment that uses S3 for storage and has styles that return different file types" do before do rebuild_model (aws2_add_region).merge storage: :s3, From 7712cd86ee9b91e73be77260892e3c3507465d78 Mon Sep 17 00:00:00 2001 From: gagoit Date: Sat, 27 Aug 2016 20:44:52 +0700 Subject: [PATCH 2/4] Mention new changes in NEWS.md and add more specs --- NEWS | 1 + spec/paperclip/storage/s3_spec.rb | 57 ++++++++++++++++++++++--------- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/NEWS b/NEWS index 04a846dbb..781dd200f 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ master: +* Improvement: Added `:use_accelerate_endpoint` option when using S3 to enable [Amazon S3 Transfer Acceleration](http://docs.aws.amazon.com/AmazonS3/latest/dev/transfer-acceleration.html) * Improvement: make the fingerprint digest configurable per attachment. The default remains MD5 but this will change in a future version because it is not considered secure anymore against intentional file corruption. For more diff --git a/spec/paperclip/storage/s3_spec.rb b/spec/paperclip/storage/s3_spec.rb index 63727ff35..58b87ba41 100644 --- a/spec/paperclip/storage/s3_spec.rb +++ b/spec/paperclip/storage/s3_spec.rb @@ -283,26 +283,49 @@ class << @dummy end context "use_accelerate_endpoint" do - before do - rebuild_model storage: :s3, - s3_credentials: {}, - bucket: "bucket", - path: ":attachment/:basename:dotextension", - s3_host_name: "s3-accelerate.amazonaws.com", - s3_region: "ap-northeast-1", - use_accelerate_endpoint: true - @dummy = Dummy.new - @dummy.avatar = stringy_file - @dummy.stubs(:new_record?).returns(false) - end + context "defaults to false" do + before do + rebuild_model storage: :s3, + s3_credentials: {}, + bucket: "bucket", + path: ":attachment/:basename:dotextension", + s3_host_name: "s3-ap-northeast-1.amazonaws.com", + s3_region: "ap-northeast-1" + @dummy = Dummy.new + @dummy.avatar = stringy_file + @dummy.stubs(:new_record?).returns(false) + end - it "returns a url based on an :s3_host_name path" do - assert_match %r{^//s3-accelerate.amazonaws.com/bucket/avatars/data[^\.]}, @dummy.avatar.url + it "returns a url based on an :s3_host_name path" do + assert_match %r{^//s3-ap-northeast-1.amazonaws.com/bucket/avatars/data[^\.]}, @dummy.avatar.url + end + + it "uses the S3 client with the use_accelerate_endpoint config is true" do + expect(@dummy.avatar.s3_bucket.client.config.use_accelerate_endpoint).to be(false) + end end - it "uses the S3 bucket with the correct host name" do - assert_equal "s3-accelerate.amazonaws.com", - @dummy.avatar.s3_bucket.client.config.endpoint.host + context "set to true" do + before do + rebuild_model storage: :s3, + s3_credentials: {}, + bucket: "bucket", + path: ":attachment/:basename:dotextension", + s3_host_name: "s3-accelerate.amazonaws.com", + s3_region: "ap-northeast-1", + use_accelerate_endpoint: true + @dummy = Dummy.new + @dummy.avatar = stringy_file + @dummy.stubs(:new_record?).returns(false) + end + + it "returns a url based on an :s3_host_name path" do + assert_match %r{^//s3-accelerate.amazonaws.com/bucket/avatars/data[^\.]}, @dummy.avatar.url + end + + it "uses the S3 client with the use_accelerate_endpoint config is true" do + expect(@dummy.avatar.s3_bucket.client.config.use_accelerate_endpoint).to be(true) + end end end From 217d4c264202ee1ebe9941a1ba4cbdbd6a5ab51b Mon Sep 17 00:00:00 2001 From: gagoit Date: Sat, 27 Aug 2016 21:22:00 +0700 Subject: [PATCH 3/4] text correction --- spec/paperclip/storage/s3_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/paperclip/storage/s3_spec.rb b/spec/paperclip/storage/s3_spec.rb index 58b87ba41..cd8829310 100644 --- a/spec/paperclip/storage/s3_spec.rb +++ b/spec/paperclip/storage/s3_spec.rb @@ -300,7 +300,7 @@ class << @dummy assert_match %r{^//s3-ap-northeast-1.amazonaws.com/bucket/avatars/data[^\.]}, @dummy.avatar.url end - it "uses the S3 client with the use_accelerate_endpoint config is true" do + it "uses the S3 client with the use_accelerate_endpoint config is false" do expect(@dummy.avatar.s3_bucket.client.config.use_accelerate_endpoint).to be(false) end end From 5743e01aea8cec7a2b14b274a1eea23759939509 Mon Sep 17 00:00:00 2001 From: gagoit Date: Sat, 27 Aug 2016 23:22:59 +0700 Subject: [PATCH 4/4] Add checking aws-sdk version when using use_accelerate_endpoint --- lib/paperclip/storage/s3.rb | 9 +++++++-- spec/paperclip/storage/s3_spec.rb | 18 ++++++++++++------ spec/spec_helper.rb | 1 + spec/support/conditional_filter_helper.rb | 5 +++++ 4 files changed, 25 insertions(+), 8 deletions(-) create mode 100644 spec/support/conditional_filter_helper.rb diff --git a/lib/paperclip/storage/s3.rb b/lib/paperclip/storage/s3.rb index bfda1c69c..e9f5597eb 100644 --- a/lib/paperclip/storage/s3.rb +++ b/lib/paperclip/storage/s3.rb @@ -155,7 +155,12 @@ def self.extended base @http_proxy = @options[:http_proxy] || nil - @use_accelerate_endpoint = @options[:use_accelerate_endpoint] || false + if @options.has_key?(:use_accelerate_endpoint) && + Gem::Version.new(Aws::VERSION) < Gem::Version.new("2.3.0") + raise LoadError, ":use_accelerate_endpoint is only available from aws-sdk version 2.3.0. Please upgrade aws-sdk to a newer version." + end + + @use_accelerate_endpoint = @options[:use_accelerate_endpoint] end Paperclip.interpolates(:s3_alias_url) do |attachment, style| @@ -264,7 +269,7 @@ def s3_object style_name = default_style end def use_accelerate_endpoint? - @use_accelerate_endpoint + !!@use_accelerate_endpoint end def using_http_proxy? diff --git a/spec/paperclip/storage/s3_spec.rb b/spec/paperclip/storage/s3_spec.rb index cd8829310..a3adb37b5 100644 --- a/spec/paperclip/storage/s3_spec.rb +++ b/spec/paperclip/storage/s3_spec.rb @@ -282,22 +282,25 @@ class << @dummy end end - context "use_accelerate_endpoint" do + context "use_accelerate_endpoint", :if => use_accelerate_endpoint_option_is_available_in_aws_sdk? do context "defaults to false" do before do - rebuild_model storage: :s3, + rebuild_model( + storage: :s3, s3_credentials: {}, bucket: "bucket", path: ":attachment/:basename:dotextension", s3_host_name: "s3-ap-northeast-1.amazonaws.com", s3_region: "ap-northeast-1" + ) @dummy = Dummy.new @dummy.avatar = stringy_file @dummy.stubs(:new_record?).returns(false) end it "returns a url based on an :s3_host_name path" do - assert_match %r{^//s3-ap-northeast-1.amazonaws.com/bucket/avatars/data[^\.]}, @dummy.avatar.url + assert_match %r{^//s3-ap-northeast-1.amazonaws.com/bucket/avatars/data[^\.]}, + @dummy.avatar.url end it "uses the S3 client with the use_accelerate_endpoint config is false" do @@ -305,22 +308,25 @@ class << @dummy end end - context "set to true" do + context "set to true", :if => use_accelerate_endpoint_option_is_available_in_aws_sdk? do before do - rebuild_model storage: :s3, + rebuild_model( + storage: :s3, s3_credentials: {}, bucket: "bucket", path: ":attachment/:basename:dotextension", s3_host_name: "s3-accelerate.amazonaws.com", s3_region: "ap-northeast-1", use_accelerate_endpoint: true + ) @dummy = Dummy.new @dummy.avatar = stringy_file @dummy.stubs(:new_record?).returns(false) end it "returns a url based on an :s3_host_name path" do - assert_match %r{^//s3-accelerate.amazonaws.com/bucket/avatars/data[^\.]}, @dummy.avatar.url + assert_match %r{^//s3-accelerate.amazonaws.com/bucket/avatars/data[^\.]}, + @dummy.avatar.url end it "uses the S3 client with the use_accelerate_endpoint config is true" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 92e7f431f..eed5d6313 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -38,6 +38,7 @@ config.include TestData config.include Reporting config.extend VersionHelper + config.extend ConditionalFilterHelper config.mock_framework = :mocha config.before(:all) do rebuild_model diff --git a/spec/support/conditional_filter_helper.rb b/spec/support/conditional_filter_helper.rb new file mode 100644 index 000000000..77e160ae7 --- /dev/null +++ b/spec/support/conditional_filter_helper.rb @@ -0,0 +1,5 @@ +module ConditionalFilterHelper + def use_accelerate_endpoint_option_is_available_in_aws_sdk? + (Gem::Version.new(Aws::VERSION) >= Gem::Version.new("2.3.0")) + end +end