From 04d4537cd40d0415d15af4395213632735c8683f Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 9 Aug 2009 07:14:24 -0300 Subject: This change causes some failing tests, but it should be possible to make them pass with minimal performance impact. --- actionpack/lib/action_dispatch/http/request.rb | 23 ++++++++++++----------- actionpack/lib/action_view/template/resolver.rb | 6 ++++-- 2 files changed, 16 insertions(+), 13 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 4190fa21cd..958a541436 100755 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -176,17 +176,18 @@ module ActionDispatch # Expand raw_formats by converting Mime::ALL to the Mime::SET. # def 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 + 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 end # Sets the \format by string extension, which can be used to force custom formats diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index ebfc6cc8ce..3bd2acae7a 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -41,8 +41,10 @@ module ActionView end def handler_glob - e = TemplateHandlers.extensions.map{|h| ".#{h},"}.join - "{#{e}}" + @handler_glob ||= begin + e = TemplateHandlers.extensions.map{|h| ".#{h},"}.join + "{#{e}}" + end end def formats_glob -- cgit v1.2.3 From 02d9dd900048407ef555cf09b0038a57ae924b0a Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 9 Aug 2009 09:46:50 -0300 Subject: Add some more caching to the lookup --- actionpack/lib/abstract_controller/layouts.rb | 22 ++++++++++++++---- actionpack/lib/action_view/template/resolver.rb | 30 +++++++++++++++---------- 2 files changed, 36 insertions(+), 16 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/abstract_controller/layouts.rb b/actionpack/lib/abstract_controller/layouts.rb index 0063d54149..d7317b415c 100644 --- a/actionpack/lib/abstract_controller/layouts.rb +++ b/actionpack/lib/abstract_controller/layouts.rb @@ -15,6 +15,18 @@ module AbstractController klass._write_layout_method end + def cache_layout(details) + layout = @found_layouts ||= {} + values = details.values_at(:formats, :locale) + + # Cache nil + if layout.key?(values) + return layout[values] + else + layout[values] = yield + end + end + # Specify the layout to use for this class. # # If the specified layout is a: @@ -76,10 +88,12 @@ module AbstractController when nil self.class_eval <<-ruby_eval, __FILE__, __LINE__ + 1 def _layout(details) - if view_paths.exists?("#{_implied_layout_name}", details, "layouts") - "#{_implied_layout_name}" - else - super + self.class.cache_layout(details) do + if view_paths.exists?("#{_implied_layout_name}", details, "layouts") + "#{_implied_layout_name}" + else + super + end end end ruby_eval diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index 3bd2acae7a..10f664736f 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -62,6 +62,10 @@ module ActionView class FileSystemResolver < Resolver + def self.cached_glob + @@cached_glob ||= {} + end + def initialize(path, options = {}) raise ArgumentError, "path already is a Resolver class" if path.is_a?(Resolver) super(options) @@ -107,20 +111,22 @@ module ActionView # :api: plugin def details_to_glob(name, details, prefix, partial, root) - path = "" - path << "#{prefix}/" unless prefix.empty? - path << (partial ? "_#{name}" : name) - - extensions = "" - [:locales, :formats].each do |k| - extensions << if exts = details[k] - '{' + exts.map {|e| ".#{e},"}.join + '}' - else - k == :formats ? formats_glob : '' + self.class.cached_glob[[name, prefix, partial, details, root]] ||= begin + path = "" + path << "#{prefix}/" unless prefix.empty? + path << (partial ? "_#{name}" : name) + + extensions = "" + [:locales, :formats].each do |k| + extensions << if exts = details[k] + '{' + exts.map {|e| ".#{e},"}.join + '}' + else + k == :formats ? formats_glob : '' + end end - end - "#{root}#{path}#{extensions}#{handler_glob}" + "#{root}#{path}#{extensions}#{handler_glob}" + end end # TODO: fix me -- cgit v1.2.3 From 4945d8223964d4ccb3c0a0f4107f15ae1c6c4a09 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Mon, 10 Aug 2009 02:54:17 -0400 Subject: Further experimentation. Was able to cut the cost of rendering 100 partials in a collection in half. To discuss: What are the desired semantics (if any) for layouts in a collection. There are no tests for it at present, and I'm not sure if it's needed at all. Deprecated on this branch: `object` pointing at the current object in partials. You can still use the partial name, or use :as to achieve the same thing. This is obviously up for discussion. --- actionpack/lib/action_view/render/partials.rb | 49 ++++++++++++++++++++++----- actionpack/lib/action_view/test_case.rb | 20 ++++++----- 2 files changed, 51 insertions(+), 18 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_view/render/partials.rb b/actionpack/lib/action_view/render/partials.rb index 64f08c447d..2aa3bd7db8 100644 --- a/actionpack/lib/action_view/render/partials.rb +++ b/actionpack/lib/action_view/render/partials.rb @@ -207,9 +207,7 @@ module ActionView end def render_collection - # Even if no template is rendered, this will ensure that the MIME type - # for the empty response is the same as the provided template - @options[:_template] = default_template = find_template + @options[:_template] = template = find_template return nil if collection.blank? @@ -217,15 +215,48 @@ module ActionView spacer = find_template(@options[:spacer_template]).render(@view, @locals) end - segments = [] + result = template ? collection_with_template(template) : collection_without_template + result.join(spacer) + end + + def collection_with_template(template) + options = @options + + segments, locals, as = [], @locals, options[:as] + + [].tap do |segments| + variable_name = template.variable_name + counter_name = template.counter_name + locals[counter_name] = -1 - collection.each_with_index do |object, index| - template = default_template || find_template(partial_path(object)) - @locals[template.counter_name] = index - segments << render_template(template, object) + collection.each do |object| + locals[counter_name] += 1 + locals[variable_name] = object + locals[as] = object if as + + segments << template.render(@view, locals) + end end + end - segments.join(spacer) + def collection_without_template + options = @options + + segments, locals, as = [], @locals, options[:as] + index, template = -1, nil + + [].tap do |segments| + collection.each do |object| + template = find_template(partial_path(object)) + locals[template.counter_name] = (index += 1) + locals[template.variable_name] = object + locals[as] = object if as + + segments << template.render(@view, locals) + end + + @options[:_template] = template + end end def render_template(template, object = @object) diff --git a/actionpack/lib/action_view/test_case.rb b/actionpack/lib/action_view/test_case.rb index e51744d095..b317b6dc1a 100644 --- a/actionpack/lib/action_view/test_case.rb +++ b/actionpack/lib/action_view/test_case.rb @@ -9,14 +9,16 @@ module ActionView end attr_internal :rendered - alias_method :_render_template_without_template_tracking, :_render_single_template - def _render_single_template(template, local_assigns, &block) - if template.respond_to?(:identifier) && template.present? - @_rendered[:partials][template] += 1 if template.partial? - @_rendered[:template] ||= [] - @_rendered[:template] << template - end - _render_template_without_template_tracking(template, local_assigns, &block) + end + + class Template + alias_method :render_without_tracking, :render + def render(view, locals, &blk) + rendered = view.rendered + rendered[:partials][self] += 1 if partial? + rendered[:template] ||= [] + rendered[:template] << self + render_without_tracking(view, locals, &blk) end end @@ -68,7 +70,7 @@ module ActionView def initialize @request = ActionController::TestRequest.new @response = ActionController::TestResponse.new - + @params = {} send(:initialize_current_url) end -- cgit v1.2.3 From 9e62d6d1c0c53e8b03c9659500e2b5549a1fd2ec Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Mon, 10 Aug 2009 08:57:44 -0400 Subject: Tentatively accept the ":as or :object, but not both" solution --- actionpack/lib/action_view/render/partials.rb | 43 +++++++++++++-------------- 1 file changed, 21 insertions(+), 22 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_view/render/partials.rb b/actionpack/lib/action_view/render/partials.rb index 2aa3bd7db8..83175ab4cf 100644 --- a/actionpack/lib/action_view/render/partials.rb +++ b/actionpack/lib/action_view/render/partials.rb @@ -184,6 +184,7 @@ module ActionView def initialize(view_context, options, block) partial = options[:partial] + @memo = {} @view = view_context @options = options @locals = options[:locals] || {} @@ -222,41 +223,39 @@ module ActionView def collection_with_template(template) options = @options - segments, locals, as = [], @locals, options[:as] + segments, locals, as = [], @locals, options[:as] || :object - [].tap do |segments| - variable_name = template.variable_name - counter_name = template.counter_name - locals[counter_name] = -1 + variable_name = template.variable_name + counter_name = template.counter_name + locals[counter_name] = -1 - collection.each do |object| - locals[counter_name] += 1 - locals[variable_name] = object - locals[as] = object if as + collection.each do |object| + locals[counter_name] += 1 + locals[variable_name] = object + locals[as] = object if as - segments << template.render(@view, locals) - end + segments << template.render(@view, locals) end + segments end def collection_without_template options = @options - segments, locals, as = [], @locals, options[:as] + segments, locals, as = [], @locals, options[:as] || :object index, template = -1, nil - [].tap do |segments| - collection.each do |object| - template = find_template(partial_path(object)) - locals[template.counter_name] = (index += 1) - locals[template.variable_name] = object - locals[as] = object if as - - segments << template.render(@view, locals) - end + collection.each do |object| + template = find_template(partial_path(object)) + locals[template.counter_name] = (index += 1) + locals[template.variable_name] = object + locals[as] = object if as - @options[:_template] = template + segments << template.render(@view, locals) end + + @options[:_template] = template + segments end def render_template(template, object = @object) -- cgit v1.2.3 From 0adbeeb0c92c6de2e4a148e4b54d56cd4a325800 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Mon, 10 Aug 2009 11:40:41 -0400 Subject: Got overhead down from 127 to 85. All tests pass: * Tentatively replaced HeaderHash with SimpleHeaderHash, which does not preserve case but does handle converting Arrays to Strings in to_hash. This requires further discussion. * Moved default_charset to ActionDispatch::Response to avoid having to hop over to ActionController. Ideally, this would be a constant on AD::Response, but some tests expect to be able to change it dynamically and I didn't want to change them yet. * Completely override #initialize from Rack::Response. Previously, it was creating a HeaderHash, and then we were creating an entirely new one. There is no way to call super without incurring the overhead of creating a HeaderHash. * Override #write from Rack::Response. Its implementation tracks Content-Length, and doing so adds additional overhead that could be mooted if other middleware changes the body. It is more efficiently done at the top-level server. * Change sending_file to an instance_variable instead of header inspection. In general, if a state is important, it should be set as a property of the response not reconstructed later. * Set the Etag to @body instead of .body. AS::Cache.expand_cache_key handles Arrays fine, and it's more efficient to let it handle the body parts, since it is not forced to create a joined String. * If we detect the default cache control case, just set it, rather than setting the constituent parts and then running the normal (expensive) code to generate the string. --- .../lib/action_controller/metal/compatibility.rb | 5 +- .../lib/action_controller/metal/streaming.rb | 3 +- .../lib/action_controller/testing/process.rb | 2 +- actionpack/lib/action_dispatch/http/response.rb | 92 +++++++++++++--------- 4 files changed, 62 insertions(+), 40 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/compatibility.rb b/actionpack/lib/action_controller/metal/compatibility.rb index f94d1c669c..a008b53d45 100644 --- a/actionpack/lib/action_controller/metal/compatibility.rb +++ b/actionpack/lib/action_controller/metal/compatibility.rb @@ -25,8 +25,9 @@ module ActionController cattr_accessor :relative_url_root self.relative_url_root = ENV['RAILS_RELATIVE_URL_ROOT'] - cattr_accessor :default_charset - self.default_charset = "utf-8" + class << self + delegate :default_charset=, :to => "ActionDispatch::Response" + end # cattr_reader :protected_instance_variables cattr_accessor :protected_instance_variables diff --git a/actionpack/lib/action_controller/metal/streaming.rb b/actionpack/lib/action_controller/metal/streaming.rb index 57318e8747..4761763a26 100644 --- a/actionpack/lib/action_controller/metal/streaming.rb +++ b/actionpack/lib/action_controller/metal/streaming.rb @@ -145,7 +145,6 @@ module ActionController #:nodoc: def send_data(data, options = {}) #:doc: logger.info "Sending data #{options[:filename]}" if logger send_file_headers! options.merge(:length => data.bytesize) - @performed_render = false render :status => options[:status], :text => data end @@ -175,6 +174,8 @@ module ActionController #:nodoc: 'Content-Transfer-Encoding' => 'binary' ) + response.sending_file = true + # Fix a problem with IE 6.0 on opening downloaded files: # If Cache-Control: no-cache is set (which Rails does by default), # IE removes the file it just downloaded from its cache immediately diff --git a/actionpack/lib/action_controller/testing/process.rb b/actionpack/lib/action_controller/testing/process.rb index d32d5562e8..147a7e7631 100644 --- a/actionpack/lib/action_controller/testing/process.rb +++ b/actionpack/lib/action_controller/testing/process.rb @@ -52,7 +52,7 @@ module ActionController #:nodoc: class TestResponse < ActionDispatch::TestResponse def recycle! @status = 200 - @header = Rack::Utils::HeaderHash.new + @header = ActionDispatch::Response::SimpleHeaderHash.new @writer = lambda { |x| @body << x } @block = nil @length = 0 diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 03d1780b77..1d3cf63984 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -32,18 +32,42 @@ 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_reader :cache_control - attr_writer :header + attr_writer :header, :sending_file alias_method :headers=, :header= - delegate :default_charset, :to => 'ActionController::Base' - def initialize - super + @status = 200 + @header = SimpleHeaderHash.new @cache_control = {} - @header = Rack::Utils::HeaderHash.new + + @writer = lambda { |x| @body << x } + @block = nil + @length = 0 + + @body = [] + @sending_file = false + + yield self if block_given? + end + + def write(str) + s = str.to_s + @writer.call s + str end def status=(status) @@ -128,20 +152,20 @@ module ActionDispatch # :nodoc: end end - def sending_file? - headers["Content-Transfer-Encoding"] == "binary" - end + CONTENT_TYPE = "Content-Type" + + cattr_accessor(:default_charset) { "utf-8" } def assign_default_content_type_and_charset! - return if !headers["Content-Type"].blank? + return if !headers[CONTENT_TYPE].blank? @content_type ||= Mime::HTML - @charset ||= default_charset + @charset ||= self.class.default_charset type = @content_type.to_s.dup - type << "; charset=#{@charset}" unless sending_file? + type << "; charset=#{@charset}" unless @sending_file - headers["Content-Type"] = type + headers[CONTENT_TYPE] = type end def prepare! @@ -168,17 +192,6 @@ module ActionDispatch # :nodoc: str end - def set_cookie(key, value) - if value.has_key?(:http_only) - ActiveSupport::Deprecation.warn( - "The :http_only option in ActionController::Response#set_cookie " + - "has been renamed. Please use :httponly instead.", caller) - value[:httponly] ||= value.delete(:http_only) - end - - super(key, value) - end - # Returns the response cookies, converted to a Hash of (name => value) pairs # # assert_equal 'AuthorOfNewPage', r.cookies['author'] @@ -201,7 +214,7 @@ module ActionDispatch # :nodoc: if etag? || last_modified? || !cache_control.empty? set_conditional_cache_control! elsif nonempty_ok_response? - self.etag = body + self.etag = @body if request && request.etag_matches?(etag) self.status = 304 @@ -214,30 +227,37 @@ module ActionDispatch # :nodoc: end end + EMPTY_RESPONSE = [" "] + def nonempty_ok_response? ok = !@status || @status == 200 - ok && string_body? + ok && string_body? && @body != EMPTY_RESPONSE end def string_body? !body_parts.respond_to?(:call) && body_parts.any? && body_parts.all? { |part| part.is_a?(String) } end + DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate" + def set_conditional_cache_control! - if cache_control.empty? - cache_control.merge!(:public => false, :max_age => 0, :must_revalidate => true) - end + control = cache_control - public_cache, max_age, must_revalidate, extras = - cache_control.values_at(:public, :max_age, :must_revalidate, :extras) + if control.empty? + headers["Cache-Control"] = DEFAULT_CACHE_CONTROL + else + extras = control[:extras] + max_age = control[:max_age] + + options = [] + options << "max-age=#{max_age}" if max_age + options << (control[:public] ? "public" : "private") + options << "must-revalidate" if control[:must_revalidate] + options.concat(extras) if extras - options = [] - options << "max-age=#{max_age}" if max_age - options << (public_cache ? "public" : "private") - options << "must-revalidate" if must_revalidate - options.concat(extras) if extras + headers["Cache-Control"] = options.join(", ") + end - headers["Cache-Control"] = options.join(", ") end end end -- cgit v1.2.3 From 4bf516e072f5279bdb462c6592e17b195fd9cf05 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Mon, 10 Aug 2009 15:49:33 -0700 Subject: 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! --- actionpack/lib/abstract_controller/layouts.rb | 49 ++++++++----- .../lib/action_controller/metal/compatibility.rb | 7 +- .../lib/action_controller/metal/hide_actions.rb | 14 +++- actionpack/lib/action_controller/metal/url_for.rb | 10 +-- .../lib/action_controller/testing/process.rb | 2 +- .../lib/action_controller/testing/test_case.rb | 1 - actionpack/lib/action_dispatch/http/request.rb | 37 +++------- actionpack/lib/action_dispatch/http/response.rb | 84 ++++++++++++++-------- actionpack/lib/action_view/test_case.rb | 1 - 9 files changed, 113 insertions(+), 92 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/abstract_controller/layouts.rb b/actionpack/lib/abstract_controller/layouts.rb index d7317b415c..ac2154dffc 100644 --- a/actionpack/lib/abstract_controller/layouts.rb +++ b/actionpack/lib/abstract_controller/layouts.rb @@ -6,17 +6,21 @@ module AbstractController 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 @@ module AbstractController 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 @@ module AbstractController # :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 @@ module AbstractController 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 @@ module AbstractController 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 diff --git a/actionpack/lib/action_controller/metal/compatibility.rb b/actionpack/lib/action_controller/metal/compatibility.rb index a008b53d45..5b0165f0e7 100644 --- a/actionpack/lib/action_controller/metal/compatibility.rb +++ b/actionpack/lib/action_controller/metal/compatibility.rb @@ -102,11 +102,10 @@ module ActionController 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 diff --git a/actionpack/lib/action_controller/metal/hide_actions.rb b/actionpack/lib/action_controller/metal/hide_actions.rb index af68c772b1..cdacdc40a6 100644 --- a/actionpack/lib/action_controller/metal/hide_actions.rb +++ b/actionpack/lib/action_controller/metal/hide_actions.rb @@ -13,7 +13,9 @@ module ActionController # 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 @@ module ActionController 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 diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb index 7119c14cd3..14c6523045 100644 --- a/actionpack/lib/action_controller/metal/url_for.rb +++ b/actionpack/lib/action_controller/metal/url_for.rb @@ -4,15 +4,6 @@ module ActionController 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 @@ module ActionController when String options when Hash + @url ||= UrlRewriter.new(request, params) @url.rewrite(rewrite_options(options)) else polymorphic_url(options) diff --git a/actionpack/lib/action_controller/testing/process.rb b/actionpack/lib/action_controller/testing/process.rb index 147a7e7631..09b1a59254 100644 --- a/actionpack/lib/action_controller/testing/process.rb +++ b/actionpack/lib/action_controller/testing/process.rb @@ -52,7 +52,7 @@ module ActionController #:nodoc: class TestResponse < ActionDispatch::TestResponse def recycle! @status = 200 - @header = ActionDispatch::Response::SimpleHeaderHash.new + @header = {} @writer = lambda { |x| @body << x } @block = nil @length = 0 diff --git a/actionpack/lib/action_controller/testing/test_case.rb b/actionpack/lib/action_controller/testing/test_case.rb index a11755b517..b66a4c15ff 100644 --- a/actionpack/lib/action_controller/testing/test_case.rb +++ b/actionpack/lib/action_controller/testing/test_case.rb @@ -179,7 +179,6 @@ module ActionController if @controller @controller.request = @request @controller.params = {} - @controller.send(:initialize_current_url) end end diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 958a541436..b23306af62 100755 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -173,21 +173,16 @@ module ActionDispatch 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 @@ EOM # 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 @@ EOM 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 diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 1d3cf63984..055f29a972 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/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 @@ module ActionDispatch # :nodoc: 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 @@ module ActionDispatch # :nodoc: str end + EMPTY = " " + def body=(body) + @blank = true if body == EMPTY @body = body.respond_to?(:to_str) ? [body] : body end @@ -137,19 +133,16 @@ module ActionDispatch # :nodoc: 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 @@ module ActionDispatch # :nodoc: 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 @@ module ActionDispatch # :nodoc: 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 @@ module ActionDispatch # :nodoc: # 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 @@ module ActionDispatch # :nodoc: 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 @@ module ActionDispatch # :nodoc: 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 diff --git a/actionpack/lib/action_view/test_case.rb b/actionpack/lib/action_view/test_case.rb index b317b6dc1a..c2ccd1d3a5 100644 --- a/actionpack/lib/action_view/test_case.rb +++ b/actionpack/lib/action_view/test_case.rb @@ -72,7 +72,6 @@ module ActionView @response = ActionController::TestResponse.new @params = {} - send(:initialize_current_url) end end -- cgit v1.2.3