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

Adds support for S3 scheme-less URL generation #1183

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions lib/paperclip/storage/s3.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ module Storage
# :s3_permissions => :private
#
# * +s3_protocol+: The protocol for the URLs generated to your S3 assets. Can be either
# 'http' or 'https'. Defaults to 'http' when your :s3_permissions are :public_read (the
# default), and 'https' when your :s3_permissions are anything else.
# 'http', 'https', or an empty string to generate scheme-less URLs. Defaults to 'http'
Copy link
Contributor

Choose a reason for hiding this comment

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

"schema-less"

Copy link

Choose a reason for hiding this comment

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

No, it's "scheme"

# when your :s3_permissions are :public_read (the default), and 'https' when your
# :s3_permissions are anything else.
# * +s3_headers+: A hash of headers or a Proc. You may specify a hash such as
# {'Expires' => 1.year.from_now.httpdate}. If you use a Proc, headers are determined at
# runtime. Paperclip will call that Proc with attachment as the only argument.
Expand Down Expand Up @@ -125,14 +126,18 @@ def self.extended base

@http_proxy = @options[:http_proxy] || nil
end

Paperclip.interpolates(:s3_alias_url) do |attachment, style|
"#{attachment.s3_protocol(style)}://#{attachment.s3_host_alias}/#{attachment.path(style).gsub(%r{^/}, "")}"
url_prefix = (s3_protocol = attachment.s3_protocol(style)).present? ? "#{s3_protocol}:" : ""
Copy link
Contributor

Choose a reason for hiding this comment

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

We're probably guilty of breaking this rule all over the place in paperclip, but the non-ternary longer form of this is a bit more legible.

Copy link

Choose a reason for hiding this comment

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

Similarly, since this is used in a few places, I would rather see it extracted into a helper.

"#{url_prefix}//#{attachment.s3_host_alias}/#{attachment.path(style).gsub(%r{^/}, "")}"
end unless Paperclip::Interpolations.respond_to? :s3_alias_url
Paperclip.interpolates(:s3_path_url) do |attachment, style|
"#{attachment.s3_protocol(style)}://#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
url_prefix = (s3_protocol = attachment.s3_protocol(style)).present? ? "#{s3_protocol}:" : ""
Copy link
Contributor

Choose a reason for hiding this comment

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

same as line 131, ternaries lead to very dense lines that're hard to understand immediately.

"#{url_prefix}//#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
end unless Paperclip::Interpolations.respond_to? :s3_path_url
Paperclip.interpolates(:s3_domain_url) do |attachment, style|
"#{attachment.s3_protocol(style)}://#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
url_prefix = (s3_protocol = attachment.s3_protocol(style)).present? ? "#{s3_protocol}:" : ""
"#{url_prefix}//#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
end unless Paperclip::Interpolations.respond_to? :s3_domain_url
Paperclip.interpolates(:asset_host) do |attachment, style|
"#{attachment.path(style).gsub(%r{^/}, "")}"
Expand Down
32 changes: 32 additions & 0 deletions test/storage/s3_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,38 @@ def teardown

end

context ":s3_protocol => 'https'" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test for when s3_protocol is explictly set to "http" and when it's not set at all?

setup do
rebuild_model :storage => :s3,
:s3_credentials => {},
:s3_protocol => 'https',
:bucket => "bucket",
:path => ":attachment/:basename.:extension"
@dummy = Dummy.new
@dummy.avatar = StringIO.new(".")
end

should "return a url based on an S3 path" do
assert_match %r{^https://s3.amazonaws.com/bucket/avatars/stringio.txt}, @dummy.avatar.url
end
end

context ":s3_protocol => ''" do
setup do
rebuild_model :storage => :s3,
:s3_credentials => {},
:s3_protocol => '',
:bucket => "bucket",
:path => ":attachment/:basename.:extension"
@dummy = Dummy.new
@dummy.avatar = StringIO.new(".")
end

should "return a url based on an S3 path" do
assert_match %r{^//s3.amazonaws.com/bucket/avatars/stringio.txt}, @dummy.avatar.url
end
end

context "An attachment that uses S3 for storage and has the style in the path" do
setup do
rebuild_model :storage => :s3,
Expand Down