Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Added :use_accelerate_endpoint option when using S3 #2291

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions 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
Expand Down
10 changes: 10 additions & 0 deletions lib/paperclip/storage/s3.rb
Expand Up @@ -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 => {
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| false is the same as nothing (falsy). We can drop that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I updated :)

end

Paperclip.interpolates(:s3_alias_url) do |attachment, style|
Expand Down Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config[:use_accelerate_endpoint] = true if use_accelerate_endpoint?

is the same as

config[:use_accelerate_endpoint] = 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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion paperclip.gemspec
Expand Up @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this bump necessary? I wouldn't require it to everyone; the exception takes care of it for us. CI should anyway install latest version, as that's what we are specifying here.

s.add_development_dependency('bourne')
s.add_development_dependency('cucumber', '~> 1.3.18')
s.add_development_dependency('aruba', '~> 0.9.0')
Expand Down
47 changes: 47 additions & 0 deletions spec/paperclip/storage/s3_spec.rb
Expand Up @@ -282,6 +282,53 @@ class << @dummy
end
end

context "use_accelerate_endpoint" do
context "defaults to false" do
before do
rebuild_model storage: :s3,
s3_credentials: {},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what Hound wants here:

rebuild_model(
  storage: :s3,
  s3_credentials: {},
  bucket: "bucket",
  # ...
)

bucket: "bucket",
path: ":attachment/:basename:dotextension",
s3_host_name: "s3-ap-northeast-1.amazonaws.com",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

s3_region: "ap-northeast-1"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last parameter of a multiline method call.

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [103/80]

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [90/80]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [90/80]

end
end

context "set to true" do
before do
rebuild_model storage: :s3,
s3_credentials: {},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

bucket: "bucket",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

path: ":attachment/:basename:dotextension",
s3_host_name: "s3-accelerate.amazonaws.com",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

s3_region: "ap-northeast-1",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

use_accelerate_endpoint: true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last parameter of a multiline method call.

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [99/80]

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [89/80]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [89/80]

end
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,
Expand Down