Merge pull request rails#35145 from st0012/fix-35114

Fix partial caching ignore repeated items issue

Author: rafaelfranca

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

@@ -17,13 +17,13 @@ def cache_collection_render(instrumentation_payload, view, template)
         # 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 @@ def cache_collection_render(instrumentation_payload, view, template)
         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 @@ def collection_by_cache_keys(view, template)
 
         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 @@ 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.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

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 %>

actionview/test/template/render_test.rb

@@ -811,6 +811,28 @@ def b.to_partial_path; "test/partial_iteration_2"; end
     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: []