Simplifying usage of ETags and Last-Modified and conditional GET requests

7 files changed, 169 insertions, 135 deletions

diff --git a/actionpack/lib/action_controller/cgi_process.rb b/actionpack/lib/action_controller/cgi_process.rb
index 8bc5e4c3a7..0ca27b30db 100644
--- a/actionpack/lib/action_controller/cgi_process.rb
+++ b/actionpack/lib/action_controller/cgi_process.rb
@@ -43,7 +43,7 @@ module ActionController #:nodoc:
       :session_path     => "/",             # available to all paths in app
       :session_key      => "_session_id",
       :cookie_only      => true
-    } unless const_defined?(:DEFAULT_SESSION_OPTIONS)
+    }
 
     def initialize(cgi, session_options = {})
       @cgi = cgi
@@ -61,53 +61,14 @@ module ActionController #:nodoc:
       end
     end
 
-    # The request body is an IO input stream. If the RAW_POST_DATA environment
-    # variable is already set, wrap it in a StringIO.
-    def body
-      if raw_post = env['RAW_POST_DATA']
-        raw_post.force_encoding(Encoding::BINARY) if raw_post.respond_to?(:force_encoding)
-        StringIO.new(raw_post)
-      else
-        @cgi.stdinput
-      end
-    end
-
-    def query_parameters
-      @query_parameters ||= self.class.parse_query_parameters(query_string)
-    end
-
-    def request_parameters
-      @request_parameters ||= parse_formatted_request_parameters
+    def body_stream #:nodoc:
+      @cgi.stdinput
     end
 
     def cookies
       @cgi.cookies.freeze
     end
 
-    def host_with_port_without_standard_port_handling
-      if forwarded = env["HTTP_X_FORWARDED_HOST"]
-        forwarded.split(/,\s?/).last
-      elsif http_host = env['HTTP_HOST']
-        http_host
-      elsif server_name = env['SERVER_NAME']
-        server_name
-      else
-        "#{env['SERVER_ADDR']}:#{env['SERVER_PORT']}"
-      end
-    end
-
-    def host
-      host_with_port_without_standard_port_handling.sub(/:\d+$/, '')
-    end
-
-    def port
-      if host_with_port_without_standard_port_handling =~ /:(\d+)$/
-        $1.to_i
-      else
-        standard_port
-      end
-    end
-
     def session
       unless defined?(@session)
         if @session_options == false
diff --git a/actionpack/lib/action_controller/headers.rb b/actionpack/lib/action_controller/headers.rb
index 7239438c49..139669c66f 100644
--- a/actionpack/lib/action_controller/headers.rb
+++ b/actionpack/lib/action_controller/headers.rb
@@ -1,31 +1,33 @@
+require 'active_support/memoizable'
+
 module ActionController
   module Http
     class Headers < ::Hash
-      
-      def initialize(constructor = {})
-         if constructor.is_a?(Hash)
+      extend ActiveSupport::Memoizable
+
+      def initialize(*args)
+         if args.size == 1 && args[0].is_a?(Hash)
            super()
-           update(constructor)
+           update(args[0])
          else
-           super(constructor)
+           super
          end
        end
-      
+
       def [](header_name)
         if include?(header_name)
-          super 
+          super
         else
-          super(normalize_header(header_name))
+          super(env_name(header_name))
         end
       end
-      
-      
+
       private
-        # Takes an HTTP header name and returns it in the 
-        # format 
-        def normalize_header(header_name)
+        # Converts a HTTP header name to an environment variable name.
+        def env_name(header_name)
           "HTTP_#{header_name.upcase.gsub(/-/, '_')}"
         end
+        memoize :env_name
     end
   end
-end
\ No newline at end of file
+end
diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb
index c55788a531..3c1521d8b1 100644
--- a/actionpack/lib/action_controller/request.rb
+++ b/actionpack/lib/action_controller/request.rb
@@ -2,18 +2,22 @@ require 'tempfile'
 require 'stringio'
 require 'strscan'
 
-module ActionController
-  # HTTP methods which are accepted by default.
-  ACCEPTED_HTTP_METHODS = Set.new(%w( get head put post delete options ))
+require 'active_support/memoizable'
 
+module ActionController
   # CgiRequest and TestRequest provide concrete implementations.
   class AbstractRequest
+    extend ActiveSupport::Memoizable
+
     def self.relative_url_root=(*args)
       ActiveSupport::Deprecation.warn(
         "ActionController::AbstractRequest.relative_url_root= has been renamed." +
         "You can now set it with config.action_controller.relative_url_root=", caller)
     end
 
+    HTTP_METHODS = %w(get head put post delete options)
+    HTTP_METHOD_LOOKUP = HTTP_METHODS.inject({}) { |h, m| h[m] = h[m.upcase] = m.to_sym; h }
+
     # The hash of environment variables for this request,
     # such as { 'RAILS_ENV' => 'production' }.
     attr_reader :env
@@ -21,15 +25,12 @@ module ActionController
     # The true HTTP request method as a lowercase symbol, such as <tt>:get</tt>.
     # UnknownHttpMethod is raised for invalid methods not listed in ACCEPTED_HTTP_METHODS.
     def request_method
-      @request_method ||= begin
-        method = ((@env['REQUEST_METHOD'] == 'POST' && !parameters[:_method].blank?) ? parameters[:_method].to_s : @env['REQUEST_METHOD']).downcase
-        if ACCEPTED_HTTP_METHODS.include?(method)
-          method.to_sym
-        else
-          raise UnknownHttpMethod, "#{method}, accepted HTTP methods are #{ACCEPTED_HTTP_METHODS.to_a.to_sentence}"
-        end
-      end
+      method = @env['REQUEST_METHOD']
+      method = parameters[:_method] if method == 'POST' && !parameters[:_method].blank?
+
+      HTTP_METHOD_LOOKUP[method] || raise(UnknownHttpMethod, "#{method}, accepted HTTP methods are #{HTTP_METHODS.to_sentence}")
     end
+    memoize :request_method
 
     # The HTTP request method as a lowercase symbol, such as <tt>:get</tt>.
     # Note, HEAD is returned as <tt>:get</tt> since the two are functionally
@@ -67,33 +68,59 @@ module ActionController
     # Provides access to the request's HTTP headers, for example:
     #  request.headers["Content-Type"] # => "text/plain"
     def headers
-      @headers ||= ActionController::Http::Headers.new(@env)
+      ActionController::Http::Headers.new(@env)
     end
+    memoize :headers
 
     def content_length
-      @content_length ||= env['CONTENT_LENGTH'].to_i
+      @env['CONTENT_LENGTH'].to_i
     end
+    memoize :content_length
 
     # The MIME type of the HTTP request, such as Mime::XML.
     #
     # For backward compatibility, the post format is extracted from the
     # X-Post-Data-Format HTTP header if present.
     def content_type
-      @content_type ||= Mime::Type.lookup(content_type_without_parameters)
+      Mime::Type.lookup(content_type_without_parameters)
     end
+    memoize :content_type
 
     # Returns the accepted MIME type for the request
     def accepts
-      @accepts ||=
-        begin
-          header = @env['HTTP_ACCEPT'].to_s.strip
+      header = @env['HTTP_ACCEPT'].to_s.strip
 
-          if header.empty?
-            [content_type, Mime::ALL].compact
-          else
-            Mime::Type.parse(header)
-          end
-        end
+      if header.empty?
+        [content_type, Mime::ALL].compact
+      else
+        Mime::Type.parse(header)
+      end
+    end
+    memoize :accepts
+
+    def if_modified_since
+      if since = env['HTTP_IF_MODIFIED_SINCE']
+        Time.rfc2822(since)
+      end
+    end
+    memoize :if_modified_since
+
+    def if_none_match
+      env['HTTP_IF_NONE_MATCH']
+    end
+
+    def not_modified?(modified_at)
+      if_modified_since && modified_at && if_modified_since >= modified_at
+    end
+
+    def etag_matches?(etag)
+      if_none_match && if_none_match == etag
+    end
+
+    # Check response freshness (Last-Modified and ETag) against request
+    # If-Modified-Since and If-None-Match conditions.
+    def fresh?(response)
+      not_modified?(response.last_modified) || etag_matches?(response.etag)
     end
 
     # Returns the Mime type for the format used in the request.
@@ -102,7 +129,7 @@ module ActionController
     #   GET /posts/5.xhtml | request.format => Mime::HTML
     #   GET /posts/5       | request.format => Mime::HTML or MIME::JS, or request.accepts.first depending on the value of <tt>ActionController::Base.use_accept_header</tt>
     def format
-      @format ||= begin
+      @format ||=
         if parameters[:format]
           Mime::Type.lookup_by_extension(parameters[:format])
         elsif ActionController::Base.use_accept_header
@@ -112,7 +139,6 @@ module ActionController
         else
           Mime::Type.lookup_by_extension("html")
         end
-      end
     end
 
 
@@ -200,42 +226,63 @@ EOM
 
       @env['REMOTE_ADDR']
     end
+    memoize :remote_ip
 
     # Returns the lowercase name of the HTTP server software.
     def server_software
       (@env['SERVER_SOFTWARE'] && /^([a-zA-Z]+)/ =~ @env['SERVER_SOFTWARE']) ? $1.downcase : nil
     end
+    memoize :server_software
 
 
     # Returns the complete URL used for this request
     def url
       protocol + host_with_port + request_uri
     end
+    memoize :url
 
     # Return 'https://' if this is an SSL request and 'http://' otherwise.
     def protocol
       ssl? ? 'https://' : 'http://'
     end
+    memoize :protocol
 
     # Is this an SSL request?
     def ssl?
       @env['HTTPS'] == 'on' || @env['HTTP_X_FORWARDED_PROTO'] == 'https'
     end
 
+    def host_with_port_without_standard_port_handling
+      if forwarded = env["HTTP_X_FORWARDED_HOST"]
+        forwarded.split(/,\s?/).last
+      else
+        env['HTTP_HOST'] || env['SERVER_NAME'] || "#{env['SERVER_ADDR']}:#{env['SERVER_PORT']}"
+      end
+    end
+    memoize :host_with_port_without_standard_port_handling
+
     # Returns the host for this request, such as example.com.
     def host
+      host_with_port_without_standard_port_handling.sub(/:\d+\Z/, '')
     end
+    memoize :host
 
     # Returns a host:port string for this request, such as example.com or
     # example.com:8080.
     def host_with_port
-      @host_with_port ||= host + port_string
+      "#{host}#{port_string}"
     end
+    memoize :host_with_port
 
     # Returns the port number of this request as an integer.
     def port
-      @port_as_int ||= @env['SERVER_PORT'].to_i
+      if host_with_port_without_standard_port_handling =~ /:(\d+)$/
+        $1.to_i
+      else
+        standard_port
+      end
     end
+    memoize :port
 
     # Returns the standard port number for this request's protocol
     def standard_port
@@ -248,7 +295,7 @@ EOM
     # Returns a port suffix like ":8080" if the port number of this request
     # is not the default HTTP port 80 or HTTPS port 443.
     def port_string
-      (port == standard_port) ? '' : ":#{port}"
+      ":#{port}" unless port == standard_port
     end
 
     # Returns the domain part of a host, such as rubyonrails.org in "www.rubyonrails.org". You can specify
@@ -276,6 +323,7 @@ EOM
         @env['QUERY_STRING'] || ''
       end
     end
+    memoize :query_string
 
     # Return the request URI, accounting for server idiosyncrasies.
     # WEBrick includes the full URL. IIS leaves REQUEST_URI blank.
@@ -300,6 +348,7 @@ EOM
         end
       end
     end
+    memoize :request_uri
 
     # Returns the interpreted path to requested resource after all the installation directory of this application was taken into account
     def path
@@ -345,19 +394,41 @@ EOM
       @path_parameters ||= {}
     end
 
+    # The request body is an IO input stream. If the RAW_POST_DATA environment
+    # variable is already set, wrap it in a StringIO.
+    def body
+      if raw_post = env['RAW_POST_DATA']
+        raw_post.force_encoding(Encoding::BINARY) if raw_post.respond_to?(:force_encoding)
+        StringIO.new(raw_post)
+      else
+        body_stream
+      end
+    end
 
-    #--
-    # Must be implemented in the concrete request
-    #++
+    def remote_addr
+      @env['REMOTE_ADDR']
+    end
 
-    # The request body is an IO input stream.
-    def body
+    def referrer
+      @env['HTTP_REFERER']
+    end
+    alias referer referrer
+
+
+    def query_parameters
+      @query_parameters ||= self.class.parse_query_parameters(query_string)
     end
 
-    def query_parameters #:nodoc:
+    def request_parameters
+      @request_parameters ||= parse_formatted_request_parameters
     end
 
-    def request_parameters #:nodoc:
+
+    #--
+    # Must be implemented in the concrete request
+    #++
+
+    def body_stream #:nodoc:
     end
 
     def cookies #:nodoc:
@@ -384,8 +455,9 @@ EOM
 
       # The raw content type string with its parameters stripped off.
       def content_type_without_parameters
-        @content_type_without_parameters ||= self.class.extract_content_type_without_parameters(content_type_with_parameters)
+        self.class.extract_content_type_without_parameters(content_type_with_parameters)
       end
+      memoize :content_type_without_parameters
 
     private
       def content_type_from_legacy_post_data_format_header
diff --git a/actionpack/lib/action_controller/response.rb b/actionpack/lib/action_controller/response.rb
index da352b6993..a85fad0d39 100644
--- a/actionpack/lib/action_controller/response.rb
+++ b/actionpack/lib/action_controller/response.rb
@@ -37,12 +37,20 @@ module ActionController # :nodoc:
     attr_accessor :body
     # The headers of the response, as a Hash. It maps header names to header values.
     attr_accessor :headers
-    attr_accessor :session, :cookies, :assigns, :template, :redirected_to, :redirected_to_method_params, :layout
+    attr_accessor :session, :cookies, :assigns, :template, :layout
+    attr_accessor :redirected_to, :redirected_to_method_params
 
     def initialize
       @body, @headers, @session, @assigns = "", DEFAULT_HEADERS.merge("cookie" => []), [], []
     end
 
+    def status; headers['Status'] end
+    def status=(status) headers['Status'] = status end
+
+    def location; headers['Location'] end
+    def location=(url) headers['Location'] = url end
+
+
     # Sets the HTTP response's content MIME type. For example, in the controller
     # you could write this:
     #
@@ -70,35 +78,29 @@ module ActionController # :nodoc:
       charset.blank? ? nil : charset.strip.split("=")[1]
     end
 
-    def redirect(to_url, response_status)
-      self.headers["Status"] = response_status
-      self.headers["Location"] = to_url
+    def last_modified
+      Time.rfc2822(headers['Last-Modified'])
+    end
 
-      self.body = "<html><body>You are being <a href=\"#{to_url}\">redirected</a>.</body></html>"
+    def last_modified=(utc_time)
+      headers['Last-Modified'] = utc_time.httpdate
     end
 
-    def prepare!
-      handle_conditional_get!
-      convert_content_type!
-      set_content_length!
+    def etag; headers['ETag'] end
+    def etag=(etag)
+      headers['ETag'] = %("#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(etag))}")
     end
 
-    # Sets the Last-Modified response header. Returns whether it's older than
-    # the If-Modified-Since request header.
-    def last_modified!(utc_time)
-      headers['Last-Modified'] ||= utc_time.httpdate
-      if request && since = request.headers['HTTP_IF_MODIFIED_SINCE']
-        utc_time <= Time.rfc2822(since)
-      end
+    def redirect(url, status)
+      self.status = status
+      self.location = url
+      self.body = "<html><body>You are being <a href=\"#{url}\">redirected</a>.</body></html>"
     end
 
-    # Sets the ETag response header. Returns whether it matches the
-    # If-None-Match request header.
-    def etag!(tag)
-      headers['ETag'] ||= %("#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(tag))}")
-      if request && request.headers['HTTP_IF_NONE_MATCH'] == headers['ETag']
-        true
-      end
+    def prepare!
+      handle_conditional_get!
+      convert_content_type!
+      set_content_length!
     end
 
     private
@@ -106,15 +108,15 @@ module ActionController # :nodoc:
         if nonempty_ok_response?
           set_conditional_cache_control!
 
-          if etag!(body)
-            headers['Status'] = '304 Not Modified'
+          self.etag ||= body
+          if request && request.etag_matches?(etag)
+            self.status = '304 Not Modified'
             self.body = ''
           end
         end
       end
 
       def nonempty_ok_response?
-        status = headers['Status']
         ok = !status || status[0..2] == '200'
         ok && body.is_a?(String) && !body.empty?
       end
diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb
index 66675aaa13..42e79fe819 100644
--- a/actionpack/lib/action_controller/test_process.rb
+++ b/actionpack/lib/action_controller/test_process.rb
@@ -23,7 +23,7 @@ module ActionController #:nodoc:
 
   class TestRequest < AbstractRequest #:nodoc:
     attr_accessor :cookies, :session_options
-    attr_accessor :query_parameters, :request_parameters, :path, :session, :env
+    attr_accessor :query_parameters, :request_parameters, :path, :session
     attr_accessor :host, :user_agent
 
     def initialize(query_parameters = nil, request_parameters = nil, session = nil)
@@ -42,7 +42,7 @@ module ActionController #:nodoc:
     end
 
     # Wraps raw_post in a StringIO.
-    def body
+    def body_stream #:nodoc:
       StringIO.new(raw_post)
     end
 
@@ -54,7 +54,7 @@ module ActionController #:nodoc:
 
     def port=(number)
       @env["SERVER_PORT"] = number.to_i
-      @port_as_int = nil
+      port(true)
     end
 
     def action=(action_name)
@@ -83,10 +83,6 @@ module ActionController #:nodoc:
       @env['REMOTE_ADDR'] = addr
     end
 
-    def remote_addr
-      @env['REMOTE_ADDR']
-    end
-
     def request_uri
       @request_uri || super
     end
@@ -120,10 +116,6 @@ module ActionController #:nodoc:
       self.query_parameters   = {}
       self.path_parameters    = {}
       @request_method, @accepts, @content_type = nil, nil, nil
-    end    
-
-    def referer
-      @env["HTTP_REFERER"]
     end
 
     private
diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb
index 5fe1ca86f3..89ac500717 100644
--- a/actionpack/lib/action_view/renderable.rb
+++ b/actionpack/lib/action_view/renderable.rb
@@ -31,10 +31,10 @@ module ActionView
 
       view.send(:evaluate_assigns)
       view.send(:set_controller_content_type, mime_type) if respond_to?(:mime_type)
-      view.send(:execute, method(local_assigns), local_assigns)
+      view.send(:execute, method_name(local_assigns), local_assigns)
     end
 
-    def method(local_assigns)
+    def method_name(local_assigns)
       if local_assigns && local_assigns.any?
         local_assigns_keys = "locals_#{local_assigns.keys.map { |k| k.to_s }.sort.join('_')}"
       end
@@ -44,7 +44,7 @@ module ActionView
     private
       # Compile and evaluate the template's code (if necessary)
       def compile(local_assigns)
-        render_symbol = method(local_assigns)
+        render_symbol = method_name(local_assigns)
 
         @@mutex.synchronize do
           if recompile?(render_symbol)
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 76832f5713..fd042794fa 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -15,9 +15,14 @@ class TestController < ActionController::Base
   end
 
   def conditional_hello
-    etag! [:foo, 123]
-    last_modified! Time.now.utc.beginning_of_day
-    render :action => 'hello_world' unless performed?
+    response.last_modified = Time.now.utc.beginning_of_day
+    response.etag = [:foo, 123]
+
+    if request.fresh?(response)
+      head :not_modified
+    else
+      render :action => 'hello_world'
+    end
   end
 
   def render_hello_world