Merge pull request rails#20538 from repinel/fix-render-caching-issue

Fix cache issue when different partials use the same collection

Author: kaspth

actionview/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Always attach the template digest to the cache key for collection caching
+    even when `virtual_path` is not available from the view context.
+    Which could happen if the rendering was done directly in the controller
+    and not in a template.
+
+    Fixes #20535
+
+    *Roque Pinel*
+
 *   Improve detection of partial templates eligible for collection caching,
     now allowing multi-line comments at the beginning of the template file.
 

actionview/lib/action_view/helpers/cache_helper.rb

@@ -137,7 +137,7 @@ module CacheHelper
       # The automatic cache multi read can be turned off like so:
       #
       #   <%= render @notifications, cache: false %>
-      def cache(name = {}, options = nil, &block)
+      def cache(name = {}, options = {}, &block)
         if controller.respond_to?(:perform_caching) && controller.perform_caching
           safe_concat(fragment_for(cache_fragment_name(name, options), options, &block))
         else
@@ -153,7 +153,7 @@ def cache(name = {}, options = nil, &block)
       #     <b>All the topics on this project</b>
       #     <%= render project.topics %>
       #   <% end %>
-      def cache_if(condition, name = {}, options = nil, &block)
+      def cache_if(condition, name = {}, options = {}, &block)
         if condition
           cache(name, options, &block)
         else
@@ -169,22 +169,23 @@ def cache_if(condition, name = {}, options = nil, &block)
       #     <b>All the topics on this project</b>
       #     <%= render project.topics %>
       #   <% end %>
-      def cache_unless(condition, name = {}, options = nil, &block)
+      def cache_unless(condition, name = {}, options = {}, &block)
         cache_if !condition, name, options, &block
       end
 
       # This helper returns the name of a cache key for a given fragment cache
-      # call. By supplying skip_digest: true to cache, the digestion of cache
+      # call. By supplying +skip_digest:+ true to cache, the digestion of cache
       # fragments can be manually bypassed. This is useful when cache fragments
       # cannot be manually expired unless you know the exact key which is the
       # case when using memcached.
-      def cache_fragment_name(name = {}, options = nil)
-        skip_digest = options && options[:skip_digest]
-
+      #
+      # The digest will be generated using +virtual_path:+ if it is provided.
+      #
+      def cache_fragment_name(name = {}, skip_digest: nil, virtual_path: nil)
         if skip_digest
           name
         else
-          fragment_name_with_digest(name)
+          fragment_name_with_digest(name, virtual_path)
         end
       end
 
@@ -198,10 +199,11 @@ def fragment_cache_key(key)
 
     private
 
-      def fragment_name_with_digest(name) #:nodoc:
-        if @virtual_path
+      def fragment_name_with_digest(name, virtual_path) #:nodoc:
+        virtual_path ||= @virtual_path
+        if virtual_path
           names  = Array(name.is_a?(Hash) ? controller.url_for(name).split("://").last : name)
-          digest = Digestor.digest name: @virtual_path, finder: lookup_context, dependencies: view_cache_dependencies
+          digest = Digestor.digest name: virtual_path, finder: lookup_context, dependencies: view_cache_dependencies
 
           [ *names, digest ]
         else

actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb

@@ -51,7 +51,7 @@ def collection_by_cache_keys
       end
 
       def expanded_cache_key(key)
-        key = @view.fragment_cache_key(@view.cache_fragment_name(key))
+        key = @view.fragment_cache_key(@view.cache_fragment_name(key, virtual_path: @template.virtual_path))
         key.frozen? ? key.dup : key # #read_multi & #write may require mutability, Dalli 2.6.0.
       end
 

actionview/test/template/render_test.rb

@@ -7,7 +7,10 @@ class TestController < ActionController::Base
 module RenderTestCases
   def setup_view(paths)
     @assigns = { :secret => 'in the sauce' }
-    @view = ActionView::Base.new(paths, @assigns)
+    @view = Class.new(ActionView::Base) do
+      def view_cache_dependencies; end
+    end.new(paths, @assigns)
+
     @controller_view = TestController.new.view_context
 
     # Reload and register danish language for testing
@@ -616,7 +619,7 @@ class CachedCustomer < Customer; end
 
   test "with custom key" do
     customer = Customer.new("david")
-    key = ActionController::Base.new.fragment_cache_key([customer, 'key'])
+    key = cache_key([customer, 'key'], "test/_customer")
 
     ActionView::PartialRenderer.collection_cache.write(key, 'Hello')
 
@@ -626,7 +629,7 @@ class CachedCustomer < Customer; end
 
   test "automatic caching with inferred cache name" do
     customer = CachedCustomer.new("david")
-    key = ActionController::Base.new.fragment_cache_key(customer)
+    key = cache_key(customer, "test/_cached_customer")
 
     ActionView::PartialRenderer.collection_cache.write(key, 'Cached')
 
@@ -636,11 +639,17 @@ class CachedCustomer < Customer; end
 
   test "automatic caching with as name" do
     customer = CachedCustomer.new("david")
-    key = ActionController::Base.new.fragment_cache_key(customer)
+    key = cache_key(customer, "test/_cached_customer_as")
 
     ActionView::PartialRenderer.collection_cache.write(key, 'Cached')
 
     assert_equal "Cached",
       @view.render(partial: "test/cached_customer_as", collection: [customer], as: :buyer)
   end
+
+  private
+    def cache_key(names, virtual_path)
+      digest = ActionView::Digestor.digest name: virtual_path, finder: @view.lookup_context, dependencies: []
+      @view.fragment_cache_key([ *Array(names), digest ])
+    end
 end