Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

More perf work:

  * Move #set_cookie and #delete_cookie inline to optimize. These optimizations should
    almost certainly be sent back upstream to Rack. The optimization involves using
    an ivar for cookies instead of indexing into the headers each time.
  * Was able to use a bare Hash for headers now that cookies have their own joining
    semantics (some code assumed that the raw cookies were an Array).
  * Cache blankness of body on body=
  * Improve expand_cache_key for Arrays of a single element (common in our case)
  * Use a simple layout condition check unless conditions are used
  * Cache visible actions
  * Lazily load the UrlRewriter
  * Make etag an ivar that is set on prepare!
  • Loading branch information...
commit 4bf516e072f5279bdb462c6592e17b195fd9cf05 1 parent 0adbeeb
@wycats wycats authored
View
49 actionpack/lib/abstract_controller/layouts.rb
@@ -6,17 +6,21 @@ module Layouts
included do
extlib_inheritable_accessor(:_layout_conditions) { Hash.new }
+ extlib_inheritable_accessor(:_action_has_layout) { Hash.new }
_write_layout_method
end
module ClassMethods
def inherited(klass)
super
- klass._write_layout_method
+ klass.class_eval do
+ _write_layout_method
+ @found_layouts = {}
+ end
end
def cache_layout(details)
- layout = @found_layouts ||= {}
+ layout = @found_layouts
values = details.values_at(:formats, :locale)
# Cache nil
@@ -27,6 +31,28 @@ def cache_layout(details)
end
end
+ # This module is mixed in if layout conditions are provided. This means
+ # that if no layout conditions are used, this method is not used
+ module LayoutConditions
+ # Determines whether the current action has a layout by checking the
+ # action name against the :only and :except conditions set on the
+ # layout.
+ #
+ # ==== Returns
+ # Boolean:: True if the action has a layout, false otherwise.
+ def _action_has_layout?
+ conditions = _layout_conditions
+
+ if only = conditions[:only]
+ only.include?(action_name)
+ elsif except = conditions[:except]
+ !except.include?(action_name)
+ else
+ true
+ end
+ end
+ end
+
# Specify the layout to use for this class.
#
# If the specified layout is a:
@@ -43,6 +69,8 @@ def cache_layout(details)
# :only<#to_s, Array[#to_s]>:: A list of actions to apply this layout to.
# :except<#to_s, Array[#to_s]>:: Apply this layout to all actions but this one
def layout(layout, conditions = {})
+ include LayoutConditions unless conditions.empty?
+
conditions.each {|k, v| conditions[k] = Array(v).map {|a| a.to_s} }
self._layout_conditions = conditions
@@ -150,7 +178,7 @@ def _find_layout(name, details)
view_paths.find(name, details, prefix)
end
- # Returns the default layout for this controller and a given set of details.
+ # Returns the default layout for this controller and a given set of details.
# Optionally raises an exception if the layout could not be found.
#
# ==== Parameters
@@ -176,21 +204,8 @@ def _default_layout(details, require_layout = false)
end
end
- # Determines whether the current action has a layout by checking the
- # action name against the :only and :except conditions set on the
- # layout.
- #
- # ==== Returns
- # Boolean:: True if the action has a layout, false otherwise.
def _action_has_layout?
- conditions = _layout_conditions
- if only = conditions[:only]
- only.include?(action_name)
- elsif except = conditions[:except]
- !except.include?(action_name)
- else
- true
- end
+ true
end
end
end
View
7 actionpack/lib/action_controller/metal/compatibility.rb
@@ -102,11 +102,10 @@ def render_to_body(options)
options[:template].sub!(/^\//, '')
end
- options[:text] = nil if options[:nothing] == true
+ options[:text] = nil if options.delete(:nothing) == true
+ options[:text] = " " if options.key?(:text) && options[:text].nil?
- body = super
- body = [' '] if body.blank?
- body
+ super || " "
end
def _handle_method_missing
View
14 actionpack/lib/action_controller/metal/hide_actions.rb
@@ -13,7 +13,9 @@ module HideActions
# Overrides AbstractController::Base#action_method? to return false if the
# action name is in the list of hidden actions.
def action_method?(action_name)
- !hidden_actions.include?(action_name) && super
+ self.class.visible_action?(action_name) do
+ !hidden_actions.include?(action_name) && super
+ end
end
module ClassMethods
@@ -25,6 +27,16 @@ def hide_action(*args)
hidden_actions.merge(args.map! {|a| a.to_s })
end
+ def inherited(klass)
+ klass.instance_variable_set("@visible_actions", {})
+ super
+ end
+
+ def visible_action?(action_name)
+ return @visible_actions[action_name] if @visible_actions.key?(action_name)
+ @visible_actions[action_name] = yield
+ end
+
# Overrides AbstractController::Base#action_methods to remove any methods
# that are listed as hidden methods.
def action_methods
View
10 actionpack/lib/action_controller/metal/url_for.rb
@@ -4,15 +4,6 @@ module UrlFor
include RackConvenience
- def process_action(*)
- initialize_current_url
- super
- end
-
- def initialize_current_url
- @url = UrlRewriter.new(request, params.clone)
- end
-
# Overwrite to implement a number of default options that all url_for-based methods will use. The default options should come in
# the form of a hash, just like the one you would use for url_for directly. Example:
#
@@ -40,6 +31,7 @@ def url_for(options = {})
when String
options
when Hash
+ @url ||= UrlRewriter.new(request, params)
@url.rewrite(rewrite_options(options))
else
polymorphic_url(options)
View
2  actionpack/lib/action_controller/testing/process.rb
@@ -52,7 +52,7 @@ def recycle!
class TestResponse < ActionDispatch::TestResponse
def recycle!
@status = 200
- @header = ActionDispatch::Response::SimpleHeaderHash.new
+ @header = {}
@writer = lambda { |x| @body << x }
@block = nil
@length = 0
View
1  actionpack/lib/action_controller/testing/test_case.rb
@@ -179,7 +179,6 @@ def setup_controller_request_and_response
if @controller
@controller.request = @request
@controller.params = {}
- @controller.send(:initialize_current_url)
end
end
View
37 actionpack/lib/action_dispatch/http/request.rb
@@ -173,21 +173,16 @@ def format(view_path = [])
end
end
- # Expand raw_formats by converting Mime::ALL to the Mime::SET.
- #
def formats
- return raw_formats
- # if ActionController::Base.use_accept_header
- # raw_formats.tap do |ret|
- # if ret == ONLY_ALL
- # ret.replace Mime::SET
- # elsif all = ret.index(Mime::ALL)
- # ret.delete_at(all) && ret.insert(all, *Mime::SET)
- # end
- # end
- # else
- # raw_formats + Mime::SET
- # end
+ if ActionController::Base.use_accept_header
+ if param = parameters[:format]
+ Array.wrap(Mime[param])
+ else
+ accepts.dup
+ end
+ else
+ [format]
+ end
end
# Sets the \format by string extension, which can be used to force custom formats
@@ -488,7 +483,7 @@ def flash
# matches the order array.
#
def negotiate_mime(order)
- raw_formats.each do |priority|
+ formats.each do |priority|
if priority == Mime::ALL
return order.first
elsif order.include?(priority)
@@ -501,18 +496,6 @@ def negotiate_mime(order)
private
- def raw_formats
- if ActionController::Base.use_accept_header
- if param = parameters[:format]
- Array.wrap(Mime[param])
- else
- accepts.dup
- end
- else
- [format]
- end
- end
-
def named_host?(host)
!(host.nil? || /\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.match(host))
end
View
84 actionpack/lib/action_dispatch/http/response.rb
@@ -32,18 +32,7 @@ module ActionDispatch # :nodoc:
# end
# end
class Response < Rack::Response
- class SimpleHeaderHash < Hash
- def to_hash
- result = {}
- each do |k,v|
- v = v.join("\n") if v.is_a?(Array)
- result[k] = v
- end
- result
- end
- end
-
- attr_accessor :request
+ attr_accessor :request, :blank
attr_reader :cache_control
attr_writer :header, :sending_file
@@ -51,19 +40,23 @@ def to_hash
def initialize
@status = 200
- @header = SimpleHeaderHash.new
+ @header = {}
@cache_control = {}
@writer = lambda { |x| @body << x }
@block = nil
@length = 0
- @body = []
+ @body, @cookie = [], []
@sending_file = false
yield self if block_given?
end
+ def cache_control
+ @cache_control ||= {}
+ end
+
def write(str)
s = str.to_s
@writer.call s
@@ -95,7 +88,10 @@ def body
str
end
+ EMPTY = " "
+
def body=(body)
+ @blank = true if body == EMPTY
@body = body.respond_to?(:to_str) ? [body] : body
end
@@ -137,19 +133,16 @@ def last_modified=(utc_time)
end
def etag
- headers['ETag']
+ @etag
end
def etag?
- headers.include?('ETag')
+ @etag
end
def etag=(etag)
- if etag.blank?
- headers.delete('ETag')
- else
- headers['ETag'] = %("#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(etag))}")
- end
+ key = ActiveSupport::Cache.expand_cache_key(etag)
+ @etag = %("#{Digest::MD5.hexdigest(key)}")
end
CONTENT_TYPE = "Content-Type"
@@ -157,7 +150,7 @@ def etag=(etag)
cattr_accessor(:default_charset) { "utf-8" }
def assign_default_content_type_and_charset!
- return if !headers[CONTENT_TYPE].blank?
+ return if headers[CONTENT_TYPE].present?
@content_type ||= Mime::HTML
@charset ||= self.class.default_charset
@@ -171,7 +164,8 @@ def assign_default_content_type_and_charset!
def prepare!
assign_default_content_type_and_charset!
handle_conditional_get!
- self["Set-Cookie"] ||= ""
+ self["Set-Cookie"] = @cookie.join("\n")
+ self["ETag"] = @etag if @etag
end
def each(&callback)
@@ -197,7 +191,7 @@ def write(str)
# assert_equal 'AuthorOfNewPage', r.cookies['author']
def cookies
cookies = {}
- if header = headers['Set-Cookie']
+ if header = @cookie
header = header.split("\n") if header.respond_to?(:to_str)
header.each do |cookie|
if pair = cookie.split(';').first
@@ -209,9 +203,40 @@ def cookies
cookies
end
+ def set_cookie(key, value)
+ case value
+ when Hash
+ domain = "; domain=" + value[:domain] if value[:domain]
+ path = "; path=" + value[:path] if value[:path]
+ # According to RFC 2109, we need dashes here.
+ # N.B.: cgi.rb uses spaces...
+ expires = "; expires=" + value[:expires].clone.gmtime.
+ strftime("%a, %d-%b-%Y %H:%M:%S GMT") if value[:expires]
+ secure = "; secure" if value[:secure]
+ httponly = "; HttpOnly" if value[:httponly]
+ value = value[:value]
+ end
+ value = [value] unless Array === value
+ cookie = Rack::Utils.escape(key) + "=" +
+ value.map { |v| Rack::Utils.escape v }.join("&") +
+ "#{domain}#{path}#{expires}#{secure}#{httponly}"
+
+ @cookie << cookie
+ end
+
+ def delete_cookie(key, value={})
+ @cookie.reject! { |cookie|
+ cookie =~ /\A#{Rack::Utils.escape(key)}=/
+ }
+
+ set_cookie(key,
+ {:value => '', :path => nil, :domain => nil,
+ :expires => Time.at(0) }.merge(value))
+ end
+
private
def handle_conditional_get!
- if etag? || last_modified? || !cache_control.empty?
+ if etag? || last_modified? || !@cache_control.empty?
set_conditional_cache_control!
elsif nonempty_ok_response?
self.etag = @body
@@ -227,21 +252,18 @@ def handle_conditional_get!
end
end
- EMPTY_RESPONSE = [" "]
-
def nonempty_ok_response?
- ok = !@status || @status == 200
- ok && string_body? && @body != EMPTY_RESPONSE
+ @status == 200 && string_body?
end
def string_body?
- !body_parts.respond_to?(:call) && body_parts.any? && body_parts.all? { |part| part.is_a?(String) }
+ !@blank && @body.respond_to?(:all?) && @body.all? { |part| part.is_a?(String) }
end
DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate"
def set_conditional_cache_control!
- control = cache_control
+ control = @cache_control
if control.empty?
headers["Cache-Control"] = DEFAULT_CACHE_CONTROL
View
1  actionpack/lib/action_view/test_case.rb
@@ -72,7 +72,6 @@ def initialize
@response = ActionController::TestResponse.new
@params = {}
- send(:initialize_current_url)
end
end
View
1  actionpack/test/controller/caching_test.rb
@@ -536,7 +536,6 @@ def setup
@controller.params = @params
@controller.request = @request
@controller.response = @response
- @controller.send(:initialize_current_url)
@controller.send(:initialize_template_class, @response)
@controller.send(:assign_shortcuts, @request, @response)
end
View
22 activesupport/lib/active_support/cache.rb
@@ -62,19 +62,27 @@ def self.lookup_store(*store_option)
end
end
+ RAILS_CACHE_ID = ENV["RAILS_CACHE_ID"]
+ RAILS_APP_VERION = ENV["RAILS_APP_VERION"]
+ EXPANDED_CACHE = RAILS_CACHE_ID || RAILS_APP_VERION
+
def self.expand_cache_key(key, namespace = nil)
expanded_cache_key = namespace ? "#{namespace}/" : ""
- if ENV["RAILS_CACHE_ID"] || ENV["RAILS_APP_VERSION"]
- expanded_cache_key << "#{ENV["RAILS_CACHE_ID"] || ENV["RAILS_APP_VERSION"]}/"
+ if EXPANDED_CACHE
+ expanded_cache_key << "#{RAILS_CACHE_ID || RAILS_APP_VERION}/"
end
- expanded_cache_key << case
- when key.respond_to?(:cache_key)
+ expanded_cache_key <<
+ if key.respond_to?(:cache_key)
key.cache_key
- when key.is_a?(Array)
- key.collect { |element| expand_cache_key(element) }.to_param
- when key
+ elsif key.is_a?(Array)
+ if key.size > 1
+ key.collect { |element| expand_cache_key(element) }.to_param
+ else
+ key.first.to_param
@exviva
exviva added a note

Not 100% sure yet, but I think this causes a bug where the cache_key method is never called if key is a 1-element array of something that does respond to cache_key.

Will provide a patch later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+ elsif key
key.to_param
end.to_s
Please sign in to comment.
Something went wrong with that request. Please try again.