Move ETag and ConditionalGet logic from AD::Response to the middleware stack.

7 files changed, 51 insertions, 216 deletions

diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb
index 047fab006e..4061222d11 100644
--- a/actionpack/lib/action_dispatch/http/cache.rb
+++ b/actionpack/lib/action_dispatch/http/cache.rb
@@ -50,8 +50,7 @@ module ActionDispatch
           if cache_control = self["Cache-Control"]
             cache_control.split(/,\s*/).each do |segment|
               first, last = segment.split("=")
-              last ||= true
-              @cache_control[first.to_sym] = last
+              @cache_control[first.to_sym] = last || true
             end
           end
         end
@@ -88,28 +87,9 @@ module ActionDispatch
         def handle_conditional_get!
           if etag? || last_modified? || !@cache_control.empty?
             set_conditional_cache_control!
-          elsif nonempty_ok_response?
-            self.etag = body
-
-            if request && request.etag_matches?(etag)
-              self.status = 304
-              self.body = []
-            end
-
-            set_conditional_cache_control!
-          else
-            headers["Cache-Control"] = "no-cache"
           end
         end
 
-        def nonempty_ok_response?
-          @status == 200 && string_body?
-        end
-
-        def string_body?
-          !@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!
diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb
index 151c90167b..72871e328a 100644
--- a/actionpack/lib/action_dispatch/http/response.rb
+++ b/actionpack/lib/action_dispatch/http/response.rb
@@ -132,7 +132,7 @@ module ActionDispatch # :nodoc:
     # information.
     attr_accessor :charset, :content_type
 
-    CONTENT_TYPE    = "Content-Type"
+    CONTENT_TYPE = "Content-Type"
 
     cattr_accessor(:default_charset) { "utf-8" }
 
diff --git a/actionpack/test/controller/new_base/etag_test.rb b/actionpack/test/controller/new_base/etag_test.rb
deleted file mode 100644
index 2bca5aec6a..0000000000
--- a/actionpack/test/controller/new_base/etag_test.rb
+++ /dev/null
@@ -1,46 +0,0 @@
-require 'abstract_unit'
-
-module Etags
-  class BasicController < ActionController::Base
-    self.view_paths = [ActionView::FixtureResolver.new(
-      "etags/basic/base.html.erb" => "Hello from without_layout.html.erb",
-      "layouts/etags.html.erb"    => "teh <%= yield %> tagz"
-    )]
-
-    def without_layout
-      render :action => "base"
-    end
-
-    def with_layout
-      render :action => "base", :layout => "etags"
-    end
-  end
-
-  class EtagTest < Rack::TestCase
-    describe "Rendering without any special etag options returns an etag that is an MD5 hash of its text"
-
-    test "an action without a layout" do
-      get "/etags/basic/without_layout"
-
-      body = "Hello from without_layout.html.erb"
-      assert_body body
-      assert_header "Etag", etag_for(body)
-      assert_status 200
-    end
-
-    test "an action with a layout" do
-      get "/etags/basic/with_layout"
-
-      body = "teh Hello from without_layout.html.erb tagz"
-      assert_body body
-      assert_header "Etag", etag_for(body)
-      assert_status 200
-    end
-
-    private
-
-    def etag_for(text)
-      %("#{Digest::MD5.hexdigest(text)}")
-    end
-  end
-end
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 7ca784c467..fca8de60bc 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -99,11 +99,6 @@ class TestController < ActionController::Base
     render :template => "test/hello_world"
   end
 
-  def render_hello_world_with_etag_set
-    response.etag = "hello_world"
-    render :template => "test/hello_world"
-  end
-
   # :ported: compatibility
   def render_hello_world_with_forward_slash
     render :template => "/test/hello_world"
@@ -1386,119 +1381,6 @@ class ExpiresInRenderTest < ActionController::TestCase
   end
 end
 
-
-class EtagRenderTest < ActionController::TestCase
-  tests TestController
-
-  def setup
-    super
-    @request.host = "www.nextangle.com"
-    @expected_bang_etag = etag_for(expand_key([:foo, 123]))
-  end
-
-  def test_render_blank_body_shouldnt_set_etag
-    get :blank_response
-    assert !@response.etag?
-  end
-
-  def test_render_200_should_set_etag
-    get :render_hello_world_from_variable
-    assert_equal etag_for("hello david"), @response.headers['ETag']
-    assert_equal "max-age=0, private, must-revalidate", @response.headers['Cache-Control']
-  end
-
-  def test_render_against_etag_request_should_304_when_match
-    @request.if_none_match = etag_for("hello david")
-    get :render_hello_world_from_variable
-    assert_equal 304, @response.status.to_i
-    assert @response.body.empty?
-  end
-
-  def test_render_against_etag_request_should_have_no_content_length_when_match
-    @request.if_none_match = etag_for("hello david")
-    get :render_hello_world_from_variable
-    assert !@response.headers.has_key?("Content-Length")
-  end
-
-  def test_render_against_etag_request_should_200_when_no_match
-    @request.if_none_match = etag_for("hello somewhere else")
-    get :render_hello_world_from_variable
-    assert_equal 200, @response.status.to_i
-    assert !@response.body.empty?
-  end
-
-  def test_render_should_not_set_etag_when_last_modified_has_been_specified
-    get :render_hello_world_with_last_modified_set
-    assert_equal 200, @response.status.to_i
-    assert_not_nil @response.last_modified
-    assert_nil @response.etag
-    assert_present @response.body
-  end
-
-  def test_render_with_etag
-    get :render_hello_world_from_variable
-    expected_etag = etag_for('hello david')
-    assert_equal expected_etag, @response.headers['ETag']
-    @response = ActionController::TestResponse.new
-
-    @request.if_none_match = expected_etag
-    get :render_hello_world_from_variable
-    assert_equal 304, @response.status.to_i
-
-    @response = ActionController::TestResponse.new
-    @request.if_none_match = "\"diftag\""
-    get :render_hello_world_from_variable
-    assert_equal 200, @response.status.to_i
-  end
-
-  def render_with_404_shouldnt_have_etag
-    get :render_custom_code
-    assert_nil @response.headers['ETag']
-  end
-
-  def test_etag_should_not_be_changed_when_already_set
-    get :render_hello_world_with_etag_set
-    assert_equal etag_for("hello_world"), @response.headers['ETag']
-  end
-
-  def test_etag_should_govern_renders_with_layouts_too
-    get :builder_layout_test
-    assert_equal "<wrapper>\n<html>\n  <p>Hello </p>\n<p>This is grand!</p>\n</html>\n</wrapper>\n", @response.body
-    assert_equal etag_for("<wrapper>\n<html>\n  <p>Hello </p>\n<p>This is grand!</p>\n</html>\n</wrapper>\n"), @response.headers['ETag']
-  end
-
-  def test_etag_with_bang_should_set_etag
-    get :conditional_hello_with_bangs
-    assert_equal @expected_bang_etag, @response.headers["ETag"]
-    assert_response :success
-  end
-
-  def test_etag_with_bang_should_obey_if_none_match
-    @request.if_none_match = @expected_bang_etag
-    get :conditional_hello_with_bangs
-    assert_response :not_modified
-  end
-
-  def test_etag_with_public_true_should_set_header
-    get :conditional_hello_with_public_header
-    assert_equal "public", @response.headers['Cache-Control']
-  end
-
-  def test_etag_with_public_true_should_set_header_and_retain_other_headers
-    get :conditional_hello_with_public_header_and_expires_at
-    assert_equal "max-age=60, public", @response.headers['Cache-Control']
-  end
-
-  protected
-    def etag_for(text)
-      %("#{Digest::MD5.hexdigest(text)}")
-    end
-
-    def expand_key(args)
-      ActiveSupport::Cache.expand_cache_key(args)
-    end
-end
-
 class LastModifiedRenderTest < ActionController::TestCase
   tests TestController
 
diff --git a/actionpack/test/dispatch/response_test.rb b/actionpack/test/dispatch/response_test.rb
index cd0418c338..be6398fead 100644
--- a/actionpack/test/dispatch/response_test.rb
+++ b/actionpack/test/dispatch/response_test.rb
@@ -11,9 +11,7 @@ class ResponseTest < ActiveSupport::TestCase
     status, headers, body = @response.to_a
     assert_equal 200, status
     assert_equal({
-      "Content-Type" => "text/html; charset=utf-8",
-      "Cache-Control" => "max-age=0, private, must-revalidate",
-      "ETag" => '"65a8e27d8879283831b664bd8b7f0ad4"'
+      "Content-Type" => "text/html; charset=utf-8"
     }, headers)
 
     parts = []
@@ -27,9 +25,7 @@ class ResponseTest < ActiveSupport::TestCase
     status, headers, body = @response.to_a
     assert_equal 200, status
     assert_equal({
-      "Content-Type" => "text/html; charset=utf-8",
-      "Cache-Control" => "max-age=0, private, must-revalidate",
-      "ETag" => '"ebb5e89e8a94e9dd22abf5d915d112b2"'
+      "Content-Type" => "text/html; charset=utf-8"
     }, headers)
   end
 
@@ -41,8 +37,7 @@ class ResponseTest < ActiveSupport::TestCase
     status, headers, body = @response.to_a
     assert_equal 200, status
     assert_equal({
-      "Content-Type" => "text/html; charset=utf-8",
-      "Cache-Control" => "no-cache"
+      "Content-Type" => "text/html; charset=utf-8"
     }, headers)
 
     parts = []
diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb
index 4d04184b20..075e3c5692 100644
--- a/railties/lib/rails/application.rb
+++ b/railties/lib/rails/application.rb
@@ -145,8 +145,8 @@ module Rails
         rack_cache = config.action_controller.perform_caching && config.action_dispatch.rack_cache
 
         require "action_dispatch/http/rack_cache" if rack_cache
+        middleware.use ::Rack::Cache, rack_cache  if rack_cache
 
-        middleware.use ::Rack::Cache, rack_cache if rack_cache
         middleware.use ::ActionDispatch::Static, config.static_asset_paths if config.serve_static_assets
         middleware.use ::Rack::Lock if !config.allow_concurrency
         middleware.use ::Rack::Runtime
@@ -165,6 +165,8 @@ module Rails
         middleware.use ::ActionDispatch::ParamsParser
         middleware.use ::Rack::MethodOverride
         middleware.use ::ActionDispatch::Head
+        middleware.use ::Rack::ConditionalGet
+        middleware.use ::Rack::ETag, "no-cache"
         middleware.use ::ActionDispatch::BestStandardsSupport, config.action_dispatch.best_standards_support if config.action_dispatch.best_standards_support
       end
     end
diff --git a/railties/test/application/middleware_test.rb b/railties/test/application/middleware_test.rb
index 2372ad85b8..173ac40b12 100644
--- a/railties/test/application/middleware_test.rb
+++ b/railties/test/application/middleware_test.rb
@@ -36,6 +36,8 @@ module ApplicationTests
         "ActionDispatch::ParamsParser",
         "Rack::MethodOverride",
         "ActionDispatch::Head",
+        "Rack::ConditionalGet",
+        "Rack::ETag",
         "ActionDispatch::BestStandardsSupport"
       ], middleware
     end
@@ -45,27 +47,7 @@ module ApplicationTests
 
       boot!
 
-      assert_equal [
-        "Rack::Cache",
-        "ActionDispatch::Static",
-        "Rack::Lock",
-        "ActiveSupport::Cache::Strategy::LocalCache",
-        "Rack::Runtime",
-        "Rails::Rack::Logger",
-        "ActionDispatch::ShowExceptions",
-        "ActionDispatch::RemoteIp",
-        "Rack::Sendfile",
-        "ActionDispatch::Callbacks",
-        "ActiveRecord::ConnectionAdapters::ConnectionManagement",
-        "ActiveRecord::QueryCache",
-        "ActionDispatch::Cookies",
-        "ActionDispatch::Session::CookieStore",
-        "ActionDispatch::Flash",
-        "ActionDispatch::ParamsParser",
-        "Rack::MethodOverride",
-        "ActionDispatch::Head",
-        "ActionDispatch::BestStandardsSupport"
-      ], middleware
+      assert_equal "Rack::Cache", middleware.first
     end
 
     test "removing Active Record omits its middleware" do
@@ -129,6 +111,46 @@ module ApplicationTests
       assert_equal "Rack::Config", middleware.first
     end
 
+    # ConditionalGet + Etag
+    test "conditional get + etag middlewares handle http caching based on body" do
+      make_basic_app
+
+      class ::OmgController < ActionController::Base
+        def index
+          if params[:nothing]
+            render :text => ""
+          else
+            render :text => "OMG"
+          end
+        end
+      end
+
+      etag = "5af83e3196bf99f440f31f2e1a6c9afe".inspect
+
+      get "/"
+      assert_equal 200, last_response.status
+      assert_equal "OMG", last_response.body
+      assert_equal "text/html; charset=utf-8", last_response.headers["Content-Type"]
+      assert_equal "max-age=0, private, must-revalidate", last_response.headers["Cache-Control"]
+      assert_equal etag, last_response.headers["Etag"]
+
+      get "/", {}, "HTTP_IF_NONE_MATCH" => etag
+      assert_equal 304, last_response.status
+      assert_equal "", last_response.body
+      assert_equal nil, last_response.headers["Content-Type"]
+      assert_equal "max-age=0, private, must-revalidate", last_response.headers["Cache-Control"]
+      assert_equal etag, last_response.headers["Etag"]
+
+      get "/?nothing=true"
+      puts last_response.body
+      assert_equal 200, last_response.status
+      assert_equal "", last_response.body
+      assert_equal "text/html; charset=utf-8", last_response.headers["Content-Type"]
+      assert_equal "no-cache", last_response.headers["Cache-Control"]
+      assert_equal nil, last_response.headers["Etag"]
+    end
+
+    # Show exceptions middleware
     test "show exceptions middleware filter backtrace before logging" do
       my_middleware = Struct.new(:app) do
         def call(env)