From 4671fe20403770b8e932eb08d1f1c02d4b66213a Mon Sep 17 00:00:00 2001 From: Aaron Lipman Date: Fri, 24 Apr 2020 17:29:00 -0400 Subject: [PATCH] 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: <%= 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 | 9 ++++++++ actionview/lib/action_view/base.rb | 4 ++-- .../lib/action_view/helpers/cache_helper.rb | 22 +++++++------------ .../action_view/renderer/partial_renderer.rb | 2 +- .../partial_renderer/collection_caching.rb | 2 +- actionview/lib/action_view/template.rb | 4 ++-- ...ment_inside_render_layout_block_1.html.erb | 1 + ...ment_inside_render_layout_block_2.html.erb | 1 + actionview/test/template/render_test.rb | 8 +++++++ 9 files changed, 33 insertions(+), 20 deletions(-) create mode 100644 actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_1.html.erb create mode 100644 actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_2.html.erb diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 4197c4c021..50b9e5be44 100644 --- a/actionview/CHANGELOG.md +++ b/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`. diff --git a/actionview/lib/action_view/base.rb b/actionview/lib/action_view/base.rb index e9002e9b8b..87141413d6 100644 --- a/actionview/lib/action_view/base.rb +++ b/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 diff --git a/actionview/lib/action_view/helpers/cache_helper.rb b/actionview/lib/action_view/helpers/cache_helper.rb index 3f396da288..3f87bcf9e1 100644 --- a/actionview/lib/action_view/helpers/cache_helper.rb +++ b/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 diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 1503ba734d..39d24bd4f8 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/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 diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index 788d171a90..ea02ffcae9 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/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 diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index cf0ef74959..d2d24299fe 100644 --- a/actionview/lib/action_view/template.rb +++ b/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) diff --git a/actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_1.html.erb b/actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_1.html.erb new file mode 100644 index 0000000000..60ec279ff4 --- /dev/null +++ b/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 %> \ No newline at end of file diff --git a/actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_2.html.erb b/actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_2.html.erb new file mode 100644 index 0000000000..49df88f56c --- /dev/null +++ b/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 %> \ No newline at end of file diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index e315d6e4e6..56f2674eb0 100644 --- a/actionview/test/template/render_test.rb +++ b/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