Ensure cache fragment digests include all templates

A Rails view may rely on several templates (e.g. layouts and partials)
in addition to the template for the action being rendered (e.g.
"show.html.erb"). To track which view file is currently being rendered
for the purpose of generating template tree digests used in cache
fragment keys, Action View uses a stack, the top item of which is
accessed via the @current_template variable (introduced in 1581cab).

Consider the following template:

    <!-- home.html.erb -->
    <%= render layout: "wrapper" do %>
      <%= cache "foo" %>
        HOME
      <%= end %>
    <%= end %>

Inside the block passed to the render helper, @current_template
corresponds to the wrapper.html.erb template instead of home.html.erb.
As wrapper.html.erb is then used as the root node for generating the
template tree digest used in the cache fragment key, the cache fragment
fails to expire upon changes to home.html.erb. Additionally, should a
second template use the wrapper.html.erb layout and contain a cache
fragment with the same key, the cache fragment keys for both templates
will be identical - causing cached content to "leak" from one view to
another (as described in #38984).

This commit skips adding templates to the stack when rendered as a
layout with a block via the render helper, ensuring correct and unique
cache fragment digests. Additionally, the virtual_path keyword arguments
found in CacheHelper and all references to the are removed as they no
longer possess any function. (Following the introduction of
@current_template, virtual_path is accessed via
@current_template.virtual_path rather than as a standalone variable.)

actionview/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Ensure cache fragment digests include all relevant template dependencies when
+    fragments are contained in a block passed to the render helper. Remove the
+    virtual_path keyword arguments found in CacheHelper as they no longer possess
+    any function following 1581cab.
+
+    Fixes #38984
+
+    *Aaron Lipman*
+
 *   Deprecate `config.action_view.raise_on_missing_translations` in favor of
     `config.i18n.raise_on_missing_translations`.
 

actionview/lib/action_view/base.rb

@@ -269,9 +269,9 @@ module ActionView #:nodoc:
       _prepare_context
     end
 
-    def _run(method, template, locals, buffer, &block)
+    def _run(method, template, locals, buffer, add_to_stack: true, &block)
       _old_output_buffer, _old_virtual_path, _old_template = @output_buffer, @virtual_path, @current_template
-      @current_template = template
+      @current_template = template if add_to_stack
       @output_buffer = buffer
       send(method, locals, buffer, &block)
     ensure

actionview/lib/action_view/helpers/cache_helper.rb

@@ -165,7 +165,7 @@ module ActionView
       # expire the cache.
       def cache(name = {}, options = {}, &block)
         if controller.respond_to?(:perform_caching) && controller.perform_caching
-          name_options = options.slice(:skip_digest, :virtual_path)
+          name_options = options.slice(:skip_digest)
           safe_concat(fragment_for(cache_fragment_name(name, **name_options), options, &block))
         else
           yield
@@ -205,14 +205,11 @@ module ActionView
       # 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.
-      #
-      # The digest will be generated using +virtual_path:+ if it is provided.
-      #
-      def cache_fragment_name(name = {}, skip_digest: nil, virtual_path: nil, digest_path: nil)
+      def cache_fragment_name(name = {}, skip_digest: nil, digest_path: nil)
         if skip_digest
           name
         else
-          fragment_name_with_digest(name, virtual_path, digest_path)
+          fragment_name_with_digest(name, digest_path)
         end
       end
 
@@ -227,14 +224,11 @@ module ActionView
       end
 
     private
-      def fragment_name_with_digest(name, virtual_path, digest_path)
-        virtual_path ||= @virtual_path
-
-        if virtual_path || digest_path
-          name = controller.url_for(name).split("://").last if name.is_a?(Hash)
+      def fragment_name_with_digest(name, digest_path)
+        name = controller.url_for(name).split("://").last if name.is_a?(Hash)
 
+        if @current_template&.virtual_path || digest_path
           digest_path ||= digest_path_from_template(@current_template)
-
           [ digest_path, name ]
         else
           name
@@ -243,10 +237,10 @@ module ActionView
 
       def fragment_for(name = {}, options = nil, &block)
         if content = read_fragment_for(name, options)
-          @view_renderer.cache_hits[@virtual_path] = :hit if defined?(@view_renderer)
+          @view_renderer.cache_hits[@current_template&.virtual_path] = :hit if defined?(@view_renderer)
           content
         else
-          @view_renderer.cache_hits[@virtual_path] = :miss if defined?(@view_renderer)
+          @view_renderer.cache_hits[@current_template&.virtual_path] = :miss if defined?(@view_renderer)
           write_fragment_for(name, options, &block)
         end
       end

actionview/lib/action_view/renderer/partial_renderer.rb

@@ -282,7 +282,7 @@ module ActionView
           identifier: template.identifier,
           layout: layout && layout.virtual_path
         ) do |payload|
-          content = template.render(view, locals) do |*name|
+          content = template.render(view, locals, add_to_stack: !block) do |*name|
             view._layout_for(*name, &block)
           end
 

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

@@ -68,7 +68,7 @@ module ActionView
       end
 
       def expanded_cache_key(key, view, template, digest_path)
-        key = view.combined_fragment_cache_key(view.cache_fragment_name(key, virtual_path: template.virtual_path, digest_path: digest_path))
+        key = view.combined_fragment_cache_key(view.cache_fragment_name(key, digest_path: digest_path))
         key.frozen? ? key.dup : key # #read_multi & #write may require mutability, Dalli 2.6.0.
       end
 

actionview/lib/action_view/template.rb

@@ -176,10 +176,10 @@ module ActionView
     # This method is instrumented as "!render_template.action_view". Notice that
     # we use a bang in this instrumentation because you don't want to
     # consume this in production. This is only slow if it's being listened to.
-    def render(view, locals, buffer = ActionView::OutputBuffer.new, &block)
+    def render(view, locals, buffer = ActionView::OutputBuffer.new, add_to_stack: true, &block)
       instrument_render_template do
         compile!(view)
-        view._run(method_name, self, locals, buffer, &block)
+        view._run(method_name, self, locals, buffer, add_to_stack: add_to_stack, &block)
       end
     rescue => e
       handle_render_error(view, e)

actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_1.html.erb

@@ -0,0 +1 @@
+<%= render layout: "layouts/yield_only" do %><%= cache "foo" do %>CAT<% end %><% end %>

actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_2.html.erb

@@ -0,0 +1 @@
+<%= render layout: "layouts/yield_only" do %><%= cache "foo" do %>DOG<% end %><% end %>

actionview/test/template/render_test.rb

@@ -22,6 +22,7 @@ module RenderTestCases
 
     controller = TestController.new
     controller.perform_caching = true
+    controller.cache_store = :memory_store
     @view.controller = controller
 
     @controller_view = controller.view_context_class.with_empty_template_cache.new(
@@ -705,6 +706,13 @@ class CachedViewRenderTest < ActiveSupport::TestCase
     assert_equal ActionView::OptimizedFileSystemResolver, view_paths.first.class
     setup_view(view_paths)
   end
+
+  def test_cache_fragments_inside_render_layout_call_with_block
+    cat = @view.render(template: "test/cache_fragment_inside_render_layout_block_1")
+    dog = @view.render(template: "test/cache_fragment_inside_render_layout_block_2")
+
+    assert_not_equal cat, dog
+  end
 end
 
 class LazyViewRenderTest < ActiveSupport::TestCase