Dont try to auto-set the etag based on the body if any freshness headers have already been set [DHH/José Valim]

3 files changed, 49 insertions, 17 deletions

diff --git a/actionpack/lib/action_controller/response.rb b/actionpack/lib/action_controller/response.rb
index b440065482..559c38efd0 100644
--- a/actionpack/lib/action_controller/response.rb
+++ b/actionpack/lib/action_controller/response.rb
@@ -106,8 +106,14 @@ module ActionController # :nodoc:
       headers['Last-Modified'] = utc_time.httpdate
     end
 
-    def etag; headers['ETag'] end
-    def etag?; headers.include?('ETag') end
+    def etag
+      headers['ETag']
+    end
+    
+    def etag?
+      headers.include?('ETag')
+    end
+    
     def etag=(etag)
       headers['ETag'] = %("#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(etag))}")
     end
@@ -135,16 +141,19 @@ module ActionController # :nodoc:
     end
 
     private
-      def handle_conditional_get!
-        if nonempty_ok_response?
-          self.etag ||= body
-          if request && request.etag_matches?(etag)
-            self.status = '304 Not Modified'
-            self.body = ''
-          end
-        end
-
-        set_conditional_cache_control! if etag? || last_modified?
+      def handle_conditional_get! 
+        if etag? || last_modified? 
+          set_conditional_cache_control! 
+        elsif nonempty_ok_response? 
+          self.etag = body 
+
+          if request && request.etag_matches?(etag) 
+            self.status = '304 Not Modified' 
+            self.body = '' 
+          end 
+
+          set_conditional_cache_control! 
+        end 
       end
 
       def nonempty_ok_response?
diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb
index cde1f2052b..f84c48f102 100644
--- a/actionpack/lib/action_controller/test_process.rb
+++ b/actionpack/lib/action_controller/test_process.rb
@@ -284,6 +284,11 @@ module ActionController #:nodoc:
   # See AbstractResponse for more information on controller response objects.
   class TestResponse < AbstractResponse
     include TestResponseBehavior
+    
+    def recycle!
+      headers.delete('ETag')
+      headers.delete('Last-Modified')
+    end
   end
 
   class TestSession #:nodoc:
@@ -386,6 +391,7 @@ module ActionController #:nodoc:
       end
 
       @request.recycle!
+      @response.recycle!
 
       @html_document = nil
       @request.env['REQUEST_METHOD'] ||= "GET"
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 35d9b19cc0..db2d5d885b 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -48,6 +48,16 @@ class TestController < ActionController::Base
     render :template => "test/hello_world"
   end
 
+  def render_hello_world_with_last_modified_set
+    response.last_modified = Date.new(2008, 10, 10).to_time
+    render :template => "test/hello_world"
+  end
+
+  def render_hello_world_with_etag_set
+    response.etag = "hello_world"
+    render :template => "test/hello_world"
+  end
+
   def render_hello_world_with_forward_slash
     render :template => "/test/hello_world"
   end
@@ -1338,12 +1348,21 @@ class EtagRenderTest < Test::Unit::TestCase
     assert_equal "200 OK", @response.status
     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 OK", @response.status
+    assert_not_nil @response.last_modified
+    assert_nil @response.etag
+    assert @response.body.present?
+  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 Not Modified", @response.status
@@ -1359,10 +1378,8 @@ class EtagRenderTest < Test::Unit::TestCase
   end
 
   def test_etag_should_not_be_changed_when_already_set
-    expected_etag = etag_for("hello somewhere else")
-    @response.headers["ETag"] = expected_etag
-    get :render_hello_world_from_variable
-    assert_equal expected_etag, @response.headers['ETag']
+    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