-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Cache interpolator methods and reduce memory allocations #1888
Changes from all commits
8a405d3
6faafc4
ce3681e
56ad18a
aaaffb6
b524cac
adba458
c77ca63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,7 +69,8 @@ def self.default_options | |
# +url_generator+ - the object used to generate URLs, using the interpolator. Defaults to Paperclip::UrlGenerator | ||
# +escape_url+ - Perform URI escaping to URLs. Defaults to true | ||
def initialize(name, instance, options = {}) | ||
@name = name | ||
@name = name.to_sym | ||
@name_string = name.to_s | ||
@instance = instance | ||
|
||
options = self.class.default_options.deep_merge(options) | ||
|
@@ -366,7 +367,7 @@ def instance_respond_to?(attr) | |
# instance_write(:file_name, "me.jpg") will write "me.jpg" to the instance's | ||
# "avatar_file_name" field (assuming the attachment is called avatar). | ||
def instance_write(attr, value) | ||
setter = :"#{name}_#{attr}=" | ||
setter = :"#{@name_string}_#{attr}=" | ||
if instance.respond_to?(setter) | ||
instance.send(setter, value) | ||
end | ||
|
@@ -375,7 +376,7 @@ def instance_write(attr, value) | |
# Reads the attachment-specific attribute on the instance. See instance_write | ||
# for more details. | ||
def instance_read(attr) | ||
getter = :"#{name}_#{attr}" | ||
getter = :"#{@name_string}_#{attr}" | ||
if instance.respond_to?(getter) | ||
instance.send(getter) | ||
end | ||
|
@@ -403,8 +404,8 @@ def ensure_required_validations! | |
|
||
def ensure_required_accessors! #:nodoc: | ||
%w(file_name).each do |field| | ||
unless @instance.respond_to?("#{name}_#{field}") && @instance.respond_to?("#{name}_#{field}=") | ||
raise Paperclip::Error.new("#{@instance.class} model missing required attr_accessor for '#{name}_#{field}'") | ||
unless @instance.respond_to?("#{@name_string}_#{field}") && @instance.respond_to?("#{@name_string}_#{field}=") | ||
raise Paperclip::Error.new("#{@instance.class} model missing required attr_accessor for '#{@name_string}_#{field}'") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [126/80] |
||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ module Interpolations | |
# and is not intended for normal use. | ||
def self.[]= name, block | ||
define_method(name, &block) | ||
@interpolators_cache = nil | ||
end | ||
|
||
# Hash access of interpolations. Included only for compatibility, | ||
|
@@ -20,7 +21,7 @@ def self.[] name | |
|
||
# Returns a sorted list of all interpolations. | ||
def self.all | ||
self.instance_methods(false).sort | ||
self.instance_methods(false).sort! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant |
||
end | ||
|
||
# Perform the actual interpolation. Takes the pattern to interpolate | ||
|
@@ -29,11 +30,15 @@ def self.all | |
# an interpolation pattern for Paperclip to use. | ||
def self.interpolate pattern, *args | ||
pattern = args.first.instance.send(pattern) if pattern.kind_of? Symbol | ||
all.reverse.inject(pattern) do |result, tag| | ||
result.gsub(/:#{tag}/) do |match| | ||
send( tag, *args ) | ||
end | ||
result = pattern.dup | ||
interpolators_cache.each do |method, token| | ||
result.gsub!(token) { send(method, *args) } if result.include?(token) | ||
end | ||
result | ||
end | ||
|
||
def self.interpolators_cache | ||
@interpolators_cache ||= all.reverse!.map! { |method| [method, ":#{method}"] } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [84/80] |
||
end | ||
|
||
def self.plural_cache | ||
|
@@ -42,7 +47,7 @@ def self.plural_cache | |
|
||
# Returns the filename, the same way as ":basename.:extension" would. | ||
def filename attachment, style_name | ||
[ basename(attachment, style_name), extension(attachment, style_name) ].reject(&:blank?).join(".") | ||
[ basename(attachment, style_name), extension(attachment, style_name) ].delete_if(&:empty?).join(".".freeze) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [114/80] |
||
end | ||
|
||
# Returns the interpolated URL. Will raise an error if the url itself | ||
|
@@ -85,28 +90,28 @@ def rails_env attachment, style_name | |
# all class names. Calling #class will return the expected class. | ||
def class attachment = nil, style_name = nil | ||
return super() if attachment.nil? && style_name.nil? | ||
plural_cache.underscore_and_pluralize(attachment.instance.class.to_s) | ||
plural_cache.underscore_and_pluralize_class(attachment.instance.class) | ||
end | ||
|
||
# Returns the basename of the file. e.g. "file" for "file.jpg" | ||
def basename attachment, style_name | ||
attachment.original_filename.gsub(/#{Regexp.escape(File.extname(attachment.original_filename))}\Z/, "") | ||
File.basename(attachment.original_filename, ".*".freeze) | ||
end | ||
|
||
# Returns the extension of the file. e.g. "jpg" for "file.jpg" | ||
# If the style has a format defined, it will return the format instead | ||
# of the actual extension. | ||
def extension attachment, style_name | ||
((style = attachment.styles[style_name.to_s.to_sym]) && style[:format]) || | ||
File.extname(attachment.original_filename).gsub(/\A\.+/, "") | ||
File.extname(attachment.original_filename).sub(/\A\.+/, "".freeze) | ||
end | ||
|
||
# Returns the dot+extension of the file. e.g. ".jpg" for "file.jpg" | ||
# If the style has a format defined, it will return the format instead | ||
# of the actual extension. If the extension is empty, no dot is added. | ||
def dotextension attachment, style_name | ||
ext = extension(attachment, style_name) | ||
ext.empty? ? "" : ".#{ext}" | ||
ext.empty? ? ext : ".#{ext}" | ||
end | ||
|
||
# Returns an extension based on the content type. e.g. "jpeg" for | ||
|
@@ -170,9 +175,9 @@ def hash attachment=nil, style_name=nil | |
def id_partition attachment, style_name | ||
case id = attachment.instance.id | ||
when Integer | ||
("%09d" % id).scan(/\d{3}/).join("/") | ||
("%09d".freeze % id).scan(/\d{3}/).join("/".freeze) | ||
when String | ||
id.scan(/.{3}/).first(3).join("/") | ||
id.scan(/.{3}/).first(3).join("/".freeze) | ||
else | ||
nil | ||
end | ||
|
@@ -181,7 +186,7 @@ def id_partition attachment, style_name | |
# Returns the pluralized form of the attachment name. e.g. | ||
# "avatars" for an attachment of :avatar | ||
def attachment attachment, style_name | ||
plural_cache.pluralize(attachment.name.to_s.downcase) | ||
plural_cache.pluralize_symbol(attachment.name) | ||
end | ||
|
||
# Returns the style, or the default style if nil is supplied. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,7 +153,7 @@ def sanitize_hash(hash) | |
Proc.new do |style, attachment| | ||
permission = (@s3_permissions[style.to_s.to_sym] || @s3_permissions[:default]) | ||
permission = permission.call(attachment, style) if permission.respond_to?(:call) | ||
(permission == DEFAULT_PERMISSION) ? 'http' : 'https' | ||
(permission == DEFAULT_PERMISSION) ? 'http'.freeze : 'https'.freeze | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [81/80] |
||
end | ||
@s3_metadata = @options[:s3_metadata] || {} | ||
@s3_headers = {} | ||
|
@@ -169,26 +169,26 @@ def sanitize_hash(hash) | |
@s3_server_side_encryption = @options[:s3_server_side_encryption] | ||
end | ||
|
||
unless @options[:url].to_s.match(/\A:s3.*url\Z/) || @options[:url] == ":asset_host" | ||
@options[:path] = path_option.gsub(/:url/, @options[:url]).gsub(/\A:rails_root\/public\/system/, '') | ||
@options[:url] = ":s3_path_url" | ||
unless @options[:url].to_s.match(/\A:s3.*url\Z/) || @options[:url] == ":asset_host".freeze | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [100/80] |
||
@options[:path] = path_option.gsub(/:url/, @options[:url]).sub(/\A:rails_root\/public\/system/, "".freeze) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [118/80] |
||
@options[:url] = ":s3_path_url".freeze | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary spacing detected. |
||
end | ||
@options[:url] = @options[:url].inspect if @options[:url].is_a?(Symbol) | ||
|
||
@http_proxy = @options[:http_proxy] || nil | ||
end | ||
|
||
Paperclip.interpolates(:s3_alias_url) do |attachment, style| | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_alias}/#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_alias}/#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [129/80] |
||
end unless Paperclip::Interpolations.respond_to? :s3_alias_url | ||
Paperclip.interpolates(:s3_path_url) do |attachment, style| | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [154/80] |
||
end unless Paperclip::Interpolations.respond_to? :s3_path_url | ||
Paperclip.interpolates(:s3_domain_url) do |attachment, style| | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [154/80] |
||
end unless Paperclip::Interpolations.respond_to? :s3_domain_url | ||
Paperclip.interpolates(:asset_host) do |attachment, style| | ||
"#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer single-quoted strings inside interpolations. |
||
end unless Paperclip::Interpolations.respond_to? :asset_host | ||
end | ||
|
||
|
@@ -214,7 +214,7 @@ def s3_host_name | |
host_name = @options[:s3_host_name] | ||
host_name = host_name.call(self) if host_name.is_a?(Proc) | ||
|
||
host_name || s3_credentials[:s3_host_name] || "s3.amazonaws.com" | ||
host_name || s3_credentials[:s3_host_name] || "s3.amazonaws.com".freeze | ||
end | ||
|
||
def s3_region | ||
|
@@ -330,7 +330,7 @@ def set_storage_class(storage_class) | |
end | ||
|
||
def parse_credentials creds | ||
creds = creds.respond_to?('call') ? creds.call(self) : creds | ||
creds = creds.respond_to?(:call) ? creds.call(self) : creds | ||
creds = find_credentials(creds).stringify_keys | ||
(creds[RailsEnvironment.get] || creds).symbolize_keys | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,15 +24,16 @@ | |
end | ||
|
||
it "returns the class of the instance" do | ||
class Thing ; end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space found before semicolon. |
||
attachment = mock | ||
attachment.expects(:instance).returns(attachment) | ||
attachment.expects(:class).returns("Thing") | ||
attachment.expects(:class).returns(Thing) | ||
assert_equal "things", Paperclip::Interpolations.class(attachment, :style) | ||
end | ||
|
||
it "returns the basename of the file" do | ||
attachment = mock | ||
attachment.expects(:original_filename).returns("one.jpg").times(2) | ||
attachment.expects(:original_filename).returns("one.jpg").times(1) | ||
assert_equal "one", Paperclip::Interpolations.basename(attachment, :style) | ||
end | ||
|
||
|
@@ -187,14 +188,14 @@ def url(*args) | |
it "returns the filename as basename.extension" do | ||
attachment = mock | ||
attachment.expects(:styles).returns({}) | ||
attachment.expects(:original_filename).returns("one.jpg").times(3) | ||
attachment.expects(:original_filename).returns("one.jpg").times(2) | ||
assert_equal "one.jpg", Paperclip::Interpolations.filename(attachment, :style) | ||
end | ||
|
||
it "returns the filename as basename.extension when format supplied" do | ||
attachment = mock | ||
attachment.expects(:styles).returns({style: {format: :png}}) | ||
attachment.expects(:original_filename).returns("one.jpg").times(2) | ||
attachment.expects(:original_filename).returns("one.jpg").times(1) | ||
assert_equal "one.png", Paperclip::Interpolations.filename(attachment, :style) | ||
end | ||
|
||
|
@@ -249,4 +250,13 @@ def url(*args) | |
value = Paperclip::Interpolations.interpolate(":notreal/:id/:attachment", :attachment, :style) | ||
assert_equal ":notreal/1234/attachments", value | ||
end | ||
|
||
it "handles question marks" do | ||
Paperclip.interpolates :foo? do | ||
"bar" | ||
end | ||
Paperclip::Interpolations.expects(:fool).never | ||
value = Paperclip::Interpolations.interpolate(":fo/:foo?") | ||
assert_equal ":fo/bar", value | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,34 +3,35 @@ | |
describe 'Plural cache' do | ||
it 'caches pluralizations' do | ||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "box" | ||
symbol = :box | ||
|
||
word.expects(:pluralize).returns("boxes").once | ||
|
||
cache.pluralize(word) | ||
cache.pluralize(word) | ||
first = cache.pluralize_symbol(symbol) | ||
second = cache.pluralize_symbol(symbol) | ||
expect(first).to equal(second) | ||
end | ||
|
||
it 'caches pluralizations and underscores' do | ||
class BigBox ; end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space found before semicolon. |
||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "BigBox" | ||
|
||
word.expects(:pluralize).returns(word).once | ||
word.expects(:underscore).returns(word).once | ||
klass = BigBox | ||
|
||
cache.underscore_and_pluralize(word) | ||
cache.underscore_and_pluralize(word) | ||
first = cache.underscore_and_pluralize_class(klass) | ||
second = cache.underscore_and_pluralize_class(klass) | ||
expect(first).to equal(second) | ||
end | ||
|
||
it 'pluralizes words' do | ||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "box" | ||
assert_equal "boxes", cache.pluralize(word) | ||
symbol = :box | ||
|
||
expect(cache.pluralize_symbol(symbol)).to eq("boxes") | ||
end | ||
|
||
it 'pluralizes and underscore words' do | ||
it 'pluralizes and underscore class names' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. |
||
class BigBox ; end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space found before semicolon. |
||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "BigBox" | ||
assert_equal "big_boxes", cache.underscore_and_pluralize(word) | ||
klass = BigBox | ||
|
||
expect(cache.underscore_and_pluralize_class(klass)).to eq("big_boxes") | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [118/80]