Improve ActionCaching's format-handling

Make ActionCaching more aware of different mimetype formats.
It will now use request.format to look up the cache type, in addition to the path extension.
When expiring caches, the request format no longer affects which cache is expired.

Signed-off-by: Pratik Naik <pratiknaik@gmail.com>

3 files changed, 79 insertions, 17 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 3096716dba..8d1acb265f 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *Edge*
 
+* Make caching more aware of mime types. Ensure request format is not considered while expiring cache.  [Jonathan del Strother]
+
 * Drop ActionController::Base.allow_concurrency flag [Josh Peek]
 
 * More efficient concat and capture helpers. Remove ActionView::Base.erb_variable.  [Jeremy Kemper]
diff --git a/actionpack/lib/action_controller/caching/actions.rb b/actionpack/lib/action_controller/caching/actions.rb
index c4b0a97a33..65a36f7f98 100644
--- a/actionpack/lib/action_controller/caching/actions.rb
+++ b/actionpack/lib/action_controller/caching/actions.rb
@@ -67,10 +67,10 @@ module ActionController #:nodoc:
 
           if options[:action].is_a?(Array)
             options[:action].dup.each do |action|
-              expire_fragment(ActionCachePath.path_for(self, options.merge({ :action => action })))
+              expire_fragment(ActionCachePath.path_for(self, options.merge({ :action => action }), false))
             end
           else
-            expire_fragment(ActionCachePath.path_for(self, options))
+            expire_fragment(ActionCachePath.path_for(self, options, false))
           end
         end
 
@@ -125,16 +125,24 @@ module ActionController #:nodoc:
         attr_reader :path, :extension
 
         class << self
-          def path_for(controller, options)
-            new(controller, options).path
+          def path_for(controller, options, infer_extension=true)
+            new(controller, options, infer_extension).path
           end
         end
-
-        def initialize(controller, options = {})
-          @extension = extract_extension(controller.request.path)
+        
+        # When true, infer_extension will look up the cache path extension from the request's path & format.
+        # This is desirable when reading and writing the cache, but not when expiring the cache -  expire_action should expire the same files regardless of the request format.
+        def initialize(controller, options = {}, infer_extension=true)
+          if infer_extension and options.is_a? Hash
+            request_extension = extract_extension(controller.request)
+            options = options.reverse_merge(:format => request_extension)
+          end
           path = controller.url_for(options).split('://').last
           normalize!(path)
-          add_extension!(path, @extension)
+          if infer_extension
+            @extension = request_extension
+            add_extension!(path, @extension)
+          end
           @path = URI.unescape(path)
         end
 
@@ -144,13 +152,22 @@ module ActionController #:nodoc:
           end
 
           def add_extension!(path, extension)
-            path << ".#{extension}" if extension
+            path << ".#{extension}" if extension and !path.ends_with?(extension)
           end
-
-          def extract_extension(file_path)
+          
+          def extract_extension(request)
             # Don't want just what comes after the last '.' to accommodate multi part extensions
             # such as tar.gz.
-            file_path[/^[^.]+\.(.+)$/, 1]
+            extension = request.path[/^[^.]+\.(.+)$/, 1]
+
+            # If there's no extension in the path, check request.format
+            if extension.nil?
+              extension = request.format.to_sym.to_s
+              if extension=='all'
+                extension = nil
+              end
+            end
+            extension
           end
       end
     end
diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb
index 4fb2397e5b..89e12ddae3 100644
--- a/actionpack/test/controller/caching_test.rb
+++ b/actionpack/test/controller/caching_test.rb
@@ -189,6 +189,10 @@ class ActionCachingTestController < ActionController::Base
     expire_action :controller => 'action_caching_test', :action => 'index'
     render :nothing => true
   end
+  def expire_xml
+    expire_action :controller => 'action_caching_test', :action => 'index', :format => 'xml'
+    render :nothing => true
+  end
 end
 
 class MockTime < Time
@@ -214,6 +218,7 @@ class ActionCachingMockController
     mocked_path = @mock_path
     Object.new.instance_eval(<<-EVAL)
       def path; '#{@mock_path}' end
+      def format; 'all' end
       self
     EVAL
   end
@@ -327,6 +332,20 @@ class ActionCacheTest < Test::Unit::TestCase
     assert_equal new_cached_time, @response.body
   end
 
+  def test_cache_expiration_isnt_affected_by_request_format
+    get :index
+    cached_time = content_to_cache
+    reset!
+
+    @request.set_REQUEST_URI "/action_caching_test/expire.xml"
+    get :expire, :format => :xml
+    reset!
+
+    get :index
+    new_cached_time = content_to_cache
+    assert_not_equal cached_time, @response.body
+  end
+
   def test_cache_is_scoped_by_subdomain
     @request.host = 'jamis.hostname.com'
     get :index
@@ -371,11 +390,35 @@ class ActionCacheTest < Test::Unit::TestCase
   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'
-    path_object = @path_class.new(@mock_controller, {})
-    assert_equal 'xml', path_object.extension
-    assert_equal 'example.org/posts/index.xml', path_object.path
+    with_routing do |set|
+      ActionController::Routing::Routes.draw do |map|
+        map.connect ':controller/:action.:format'
+        map.connect ':controller/:action'
+      end
+
+      get :index, :format => 'xml'
+      cached_time = content_to_cache
+      assert_equal cached_time, @response.body
+      assert fragment_exist?('hostname.com/action_caching_test/index.xml')
+      reset!
+
+      get :index, :format => 'xml'
+      assert_equal cached_time, @response.body
+      assert_equal 'application/xml', @response.content_type
+      reset!
+
+      @request.env['HTTP_ACCEPT'] = "application/xml"
+      get :index
+      assert_equal cached_time, @response.body
+      assert_equal 'application/xml', @response.content_type
+      reset!
+
+      get :expire_xml
+      reset!
+
+      get :index, :format => 'xml'
+      assert_not_equal cached_time, @response.body
+    end
   end
 
   def test_correct_content_type_is_returned_for_cache_hit