Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Action View collection caching to store fragments as bare strings #48645

Merged
merged 1 commit into from Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -89,15 +89,25 @@ def expanded_cache_key(key, view, template, digest_path)
# If the partial is not already cached it will also be
# written back to the underlying cache store.
def fetch_or_cache_partial(cached_partials, template, order_by:)
order_by.index_with do |cache_key|
entries_to_write = {}

keyed_partials = order_by.index_with do |cache_key|
if content = cached_partials[cache_key]
build_rendered_template(content, template)
else
yield.tap do |rendered_partial|
collection_cache.write(cache_key, rendered_partial.body)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was at it, I converted this write loop into a single write_multi.

rendered_partial = yield
if fragment = rendered_partial.body&.to_str
entries_to_write[cache_key] = fragment
end
rendered_partial
end
end

unless entries_to_write.empty?
collection_cache.write_multi(entries_to_write)
end

keyed_partials
end
end
end
21 changes: 21 additions & 0 deletions actionview/test/activerecord/multifetch_cache_test.rb
Expand Up @@ -30,6 +30,12 @@ class MultifetchCacheTest < ActiveRecordTestCase

setup do
Topic.update_all(updated_at: Time.now)
@cache_store_was = ActionView::PartialRenderer.collection_cache
ActionView::PartialRenderer.collection_cache = ActiveSupport::Cache::MemoryStore.new
end

teardown do
ActionView::PartialRenderer.collection_cache = @cache_store_was
end

def test_only_preloading_for_records_that_miss_the_cache
Expand Down Expand Up @@ -77,4 +83,19 @@ def test_preloads_all_records_if_using_cached_proc
assert_equal first_req.first, second_req.first
assert_includes second_req.last, %(WHERE "replies"."topic_id" IN (?, ?, ?))
end

class InspectableStore < ActiveSupport::Cache::MemoryStore
attr_reader :data
end

def test_fragments_are_stored_as_bare_strings
cache = ActionView::PartialRenderer.collection_cache = InspectableStore.new
Topic.update_all(title: "title")
get :cached_true

assert_not_predicate cache.data, :empty?
cache.data.each_value do |entry|
assert_equal String, entry.value.class
end
end
end