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>

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]

actionpack/lib/action_controller/caching/actions.rb

@@ -67,10 +67,10 @@ def expire_action(options = {})
 
           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 @@ class ActionCachePath
         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 @@ def normalize!(path)
           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

actionpack/test/controller/caching_test.rb

@@ -189,6 +189,10 @@ def expire
     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 @@ def request
     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 @@ def test_cache_expiration
     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 @@ def test_forbidden_is_not_cached
   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