Changeset 5755

Timestamp:

12/19/06 20:25:52 (2 years ago)

Author:

bitsweat

Message:

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

Files:

• trunk/actionpack/CHANGELOG (modified) (1 diff)
• trunk/actionpack/lib/action_controller/caching.rb (modified) (1 diff)
• trunk/actionpack/test/controller/caching_test.rb (modified) (10 diffs)

trunk/actionpack/CHANGELOG

@@ -1 +1 @@
-*SVN*
+
+* Only cache GET requests with a 200 OK response.  #6514, #6743 [RSL, anamba]
-
-* Add a 'referer' attribute to TestRequest. [Jamis Buck]

trunk/actionpack/lib/action_controller/caching.rb

@@ -136 +136 @@
-      private
-        def caching_allowed
-!request.post? && response.headers['Status'] && response.headers['Status'].to_i < 4
+request.get? && response.headers['Status'].to_i == 2
-        end
-    end

trunk/actionpack/test/controller/caching_test.rb

@@ -6 +6 @@
-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
@@ -15 +36 @@
-      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)
-
+
+
+
+
+
+
+
+
-
-  def test_page_caching_resources_saves_to_correct_path_with_extension_even_if_default_route
@@ -27 +60 @@
-    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
-
@@ -47 +112 @@
-  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
@@ -72 +137 @@
-    @mock_controller = ActionCachingMockController.new
-  end
-
+
-  def teardown
-    FileUtils.rm_rf(File.dirname(FILE_STORE_PATH))
-  end
-
+
-  def test_simple_action_cache
-    get :index
@@ -82 +147 @@
-    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
@@ -98 +163 @@
-    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/'
@@ -135 +200 @@
-    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/'
@@ -142 +207 @@
-    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
@@ -162 +226 @@
-      @request.host = 'hostname.com'
-    end
-
-
+