From e8688ddb33347eb9de078843a75a075b70cc9f04 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 3 Feb 2019 22:06:38 +0800 Subject: Fix partial caching ignore repeated items issue This is because we only use hash to maintain the result. So when the key are the same, the result would be skipped. The solution is to maintain an array for tracking every item's position to restructure the result. --- .../partial_renderer/collection_caching.rb | 33 +++++++++++++--------- actionview/test/fixtures/test/_cached_set.erb | 1 + actionview/test/template/render_test.rb | 22 +++++++++++++++ 3 files changed, 43 insertions(+), 13 deletions(-) create mode 100644 actionview/test/fixtures/test/_cached_set.erb 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 ed59033e27..ca692699a6 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -17,13 +17,13 @@ module ActionView # Result is a hash with the key represents the # key used for cache lookup and the value is the item # on which the partial is being rendered - keyed_collection = collection_by_cache_keys(view, template) + keyed_collection, ordered_keys = collection_by_cache_keys(view, template) # Pull all partials from cache # Result is a hash, key matches the entry in # `keyed_collection` where the cache was retrieved and the # value is the value that was present in the cache - cached_partials = collection_cache.read_multi(*keyed_collection.keys) + cached_partials = collection_cache.read_multi(*keyed_collection.keys) instrumentation_payload[:cache_hits] = cached_partials.size # Extract the items for the keys that are not found @@ -40,11 +40,15 @@ module ActionView rendered_partials = @collection.empty? ? [] : yield index = 0 - fetch_or_cache_partial(cached_partials, template, order_by: keyed_collection.each_key) do + keyed_partials = fetch_or_cache_partial(cached_partials, template, order_by: keyed_collection.each_key) do # This block is called once # for every cache miss while preserving order. rendered_partials[index].tap { index += 1 } end + + ordered_keys.map do |key| + keyed_partials[key] + end end def callable_cache_key? @@ -56,8 +60,10 @@ module ActionView digest_path = view.digest_path_from_template(template) - @collection.each_with_object({}) do |item, hash| - hash[expanded_cache_key(seed.call(item), view, template, digest_path)] = item + @collection.each_with_object([{}, []]) do |item, (hash, ordered_keys)| + key = expanded_cache_key(seed.call(item), view, template, digest_path) + ordered_keys << key + hash[key] = item end end @@ -82,15 +88,16 @@ module ActionView # 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.map 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) - end + order_by.each_with_object({}) do |cache_key, hash| + hash[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) + end + end end - end end end end diff --git a/actionview/test/fixtures/test/_cached_set.erb b/actionview/test/fixtures/test/_cached_set.erb new file mode 100644 index 0000000000..cd492fc519 --- /dev/null +++ b/actionview/test/fixtures/test/_cached_set.erb @@ -0,0 +1 @@ +<%= cached_set.first %> | <%= cached_set.second %> | <%= cached_set.third %> | <%= cached_set.fourth %> | <%= cached_set.fifth %> diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index 15c41051de..f0b5d7d95e 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -811,6 +811,28 @@ class CachedCollectionViewRenderTest < ActiveSupport::TestCase end end + test "collection caching with repeated collection" do + sets = [ + [1, 2, 3, 4, 5], + [1, 2, 3, 4, 4], + [1, 2, 3, 4, 5], + [1, 2, 3, 4, 4], + [1, 2, 3, 4, 6] + ] + + result = @view.render(partial: "test/cached_set", collection: sets, cached: true) + + splited_result = result.split("\n") + assert_equal 5, splited_result.count + assert_equal [ + "1 | 2 | 3 | 4 | 5", + "1 | 2 | 3 | 4 | 4", + "1 | 2 | 3 | 4 | 5", + "1 | 2 | 3 | 4 | 4", + "1 | 2 | 3 | 4 | 6" + ], splited_result + end + private def cache_key(*names, virtual_path) digest = ActionView::Digestor.digest name: virtual_path, format: :html, finder: @view.lookup_context, dependencies: [] -- cgit v1.2.3