Only cache GET requests with a 200 OK response. Closes #6514, #6743.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@5755 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

3 files changed, 94 insertions, 29 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 32cc8e0316..8050fb4722 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Only cache GET requests with a 200 OK response.  #6514, #6743 [RSL, anamba]
+
 * Add a 'referer' attribute to TestRequest. [Jamis Buck]
 
 * Ensure render :json => ... skips the layout.  Closes #6808 [Josh Peek]
diff --git a/actionpack/lib/action_controller/caching.rb b/actionpack/lib/action_controller/caching.rb
index 28dc45139a..c7f3e95617 100644
--- a/actionpack/lib/action_controller/caching.rb
+++ b/actionpack/lib/action_controller/caching.rb
@@ -135,7 +135,7 @@ module ActionController #:nodoc:
 
       private
         def caching_allowed
-          !request.post? && response.headers['Status'] && response.headers['Status'].to_i < 400
+          request.get? && response.headers['Status'].to_i == 200
         end
     end
 
diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb
index 1935cf5aa5..07e16896bd 100644
--- a/actionpack/test/controller/caching_test.rb
+++ b/actionpack/test/controller/caching_test.rb
@@ -5,8 +5,29 @@ CACHE_DIR = 'test_cache'
 # Don't change '/../temp/' cavalierly or you might hoze something you don't want hozed
 FILE_STORE_PATH = File.join(File.dirname(__FILE__), '/../temp/', CACHE_DIR)
 ActionController::Base.perform_caching = true
+ActionController::Base.page_cache_directory = FILE_STORE_PATH
 ActionController::Base.fragment_cache_store = :file_store, FILE_STORE_PATH
 
+class PageCachingTestController < ActionController::Base
+  caches_page :ok, :no_content, :found, :not_found
+
+  def ok
+    head :ok
+  end
+
+  def no_content
+    head :no_content
+  end
+
+  def found
+    redirect_to :action => 'ok'
+  end
+
+  def not_found
+    head :not_found
+  end
+end
+
 class PageCachingTest < Test::Unit::TestCase
   def setup
     ActionController::Routing::Routes.draw do |map|
@@ -14,11 +35,23 @@ class PageCachingTest < Test::Unit::TestCase
       map.resources :posts
       map.connect ':controller/:action/:id'
     end
-    
+
     @request = ActionController::TestRequest.new
+    @request.host = 'hostname.com'
+
+    @response   = ActionController::TestResponse.new
+    @controller = PageCachingTestController.new
+
     @params = {:controller => 'posts', :action => 'index', :only_path => true, :skip_relative_url_root => true}
     @rewriter = ActionController::UrlRewriter.new(@request, @params)
-  end 
+
+    FileUtils.rm_rf(File.dirname(FILE_STORE_PATH))
+    FileUtils.mkdir_p(FILE_STORE_PATH)
+  end
+
+  def teardown
+    FileUtils.rm_rf(File.dirname(FILE_STORE_PATH))
+  end
 
   def test_page_caching_resources_saves_to_correct_path_with_extension_even_if_default_route
     @params[:format] = 'rss'
@@ -26,35 +59,67 @@ class PageCachingTest < Test::Unit::TestCase
     @params[:format] = nil
     assert_equal '/', @rewriter.rewrite(@params)
   end
+
+  def test_should_cache_get_with_ok_status
+    get :ok
+    assert_response :ok
+    assert_page_cached :ok, "get with ok status should have been cached"
+  end
+
+  [:ok, :no_content, :found, :not_found].each do |status|
+    [:get, :post, :put, :delete].each do |method|
+      unless method == :get and status == :ok
+        define_method "test_shouldnt_cache_#{method}_with_#{status}_status" do
+          @request.env['REQUEST_METHOD'] = method.to_s.upcase
+          process status
+          assert_response status
+          assert_page_not_cached status, "#{method} with #{status} status shouldn't have been cached"
+        end
+      end
+    end
+  end
+
+  private
+    def assert_page_cached(action, message = "#{action} should have been cached")
+      assert page_cached?(action), message
+    end
+
+    def assert_page_not_cached(action, message = "#{action} shouldn't have been cached")
+      assert !page_cached?(action), message
+    end
+
+    def page_cached?(action)
+      File.exist? "#{FILE_STORE_PATH}/page_caching_test/#{action}.html"
+    end
 end
 
 class ActionCachingTestController < ActionController::Base
   caches_action :index
- 
+
   def index
     @cache_this = Time.now.to_f.to_s
     render :text => @cache_this
   end
-  
+
   def expire
     expire_action :controller => 'action_caching_test', :action => 'index'
     render :nothing => true
   end
-  
+
 end
 
 class ActionCachingMockController
   attr_accessor :mock_url_for
   attr_accessor :mock_path
-  
+
   def initialize
     yield self if block_given?
   end
-  
+
   def url_for(*args)
     @mock_url_for
   end
-  
+
   def request
     mocked_path = @mock_path
     Object.new.instance_eval(<<-EVAL)
@@ -71,62 +136,62 @@ class ActionCacheTest < Test::Unit::TestCase
     @path_class = ActionController::Caching::Actions::ActionCachePath
     @mock_controller = ActionCachingMockController.new
   end
-  
+
   def teardown
     FileUtils.rm_rf(File.dirname(FILE_STORE_PATH))
   end
-  
+
   def test_simple_action_cache
     get :index
     cached_time = content_to_cache
     assert_equal cached_time, @response.body
     reset!
-    
+
     get :index
     assert_equal cached_time, @response.body
   end
-  
+
   def test_cache_expiration
     get :index
     cached_time = content_to_cache
     reset!
-        
+
     get :index
     assert_equal cached_time, @response.body
     reset!
 
     get :expire
     reset!
-    
+
     get :index
     new_cached_time = content_to_cache
     assert_not_equal cached_time, @response.body
     reset!
-    
+
     get :index
     assert_response :success
     assert_equal new_cached_time, @response.body
   end
-  
+
   def test_cache_is_scoped_by_subdomain
     @request.host = 'jamis.hostname.com'
     get :index
     jamis_cache = content_to_cache
-    
+
     @request.host = 'david.hostname.com'
     get :index
     david_cache = content_to_cache
     assert_not_equal jamis_cache, @response.body
-    
+
     @request.host = 'jamis.hostname.com'
     get :index
     assert_equal jamis_cache, @response.body
-    
+
     @request.host = 'david.hostname.com'
     get :index
     assert_equal david_cache, @response.body
   end
-  
+
   def test_xml_version_of_resource_is_treated_as_different_cache
     @mock_controller.mock_url_for = 'http://example.org/posts/'
     @mock_controller.mock_path    = '/posts/index.xml'
@@ -134,32 +199,30 @@ class ActionCacheTest < Test::Unit::TestCase
     assert_equal 'xml', path_object.extension
     assert_equal 'example.org/posts/index.xml', path_object.path
   end
-  
+
   def test_empty_path_is_normalized
     @mock_controller.mock_url_for = 'http://example.org/'
     @mock_controller.mock_path    = '/'
 
     assert_equal 'example.org/index', @path_class.path_for(@mock_controller)
   end
-  
+
   def test_file_extensions
-    get :index, :id => 'kitten.jpg' 
+    get :index, :id => 'kitten.jpg'
     get :index, :id => 'kitten.jpg'
 
     assert_response :success
   end
-  
+
   private
-  
     def content_to_cache
       assigns(:cache_this)
     end
-    
+
     def reset!
       @request    = ActionController::TestRequest.new
       @response   = ActionController::TestResponse.new
       @controller = ActionCachingTestController.new
       @request.host = 'hostname.com'
     end
-  
-end
\ No newline at end of file
+end