Make collection caching explicit.

Having collection caching that wraps templates and automatically tries
to infer if they are cachable proved to be too much of a hassle.

We'd rather have it be something you explicitly turn on.

This removes much of the code and docs to explain the previous automatic
behavior.

This change also removes scoped cache keys and passing cache_options.

7 files changed, 47 insertions, 174 deletions

diff --git a/actionview/lib/action_view/helpers/cache_helper.rb b/actionview/lib/action_view/helpers/cache_helper.rb
index 401f398721..e842d8e31c 100644
--- a/actionview/lib/action_view/helpers/cache_helper.rb
+++ b/actionview/lib/action_view/helpers/cache_helper.rb
@@ -126,44 +126,16 @@ module ActionView
       #
       # Now all you have to do is change that timestamp when the helper method changes.
       #
-      # === Automatic Collection Caching
+      # === Collection Caching
       #
-      # When rendering collections such as:
+      # When rendering a collection of objects that each use the same partial, a `cached`
+      # option can be passed.
+      # For collections rendered such:
       #
-      #   <%= render @notifications %>
-      #   <%= render partial: 'notifications/notification', collection: @notifications %>
+      #   <%= render partial: 'notifications/notification', collection: @notifications, cached: true %>
       #
-      # If the notifications/_notification partial starts with a cache call as:
-      #
-      #   <% cache notification do %>
-      #     <%= notification.name %>
-      #   <% end %>
-      #
-      # The collection can then automatically use any cached renders for that
-      # template by reading them at once instead of one by one.
-      #
-      # See ActionView::Template::Handlers::ERB.resource_cache_call_pattern for
-      # more information on what cache calls make a template eligible for this
-      # collection caching.
-      #
-      # The automatic cache multi read can be turned off like so:
-      #
-      #   <%= render @notifications, cache: false %>
-      #
-      # === Explicit Collection Caching
-      #
-      # If the partial template doesn't start with a clean cache call as
-      # mentioned above, you can still benefit from collection caching by
-      # adding a special comment format anywhere in the template, like:
-      #
-      #   <%# Template Collection: notification %>
-      #   <% my_helper_that_calls_cache(some_arg, notification) do %>
-      #     <%= notification.name %>
-      #   <% end %>
-      #
-      # The pattern used to match these is <tt>/# Template Collection: (\S+)/</tt>,
-      # so it's important that you type it out just so.
-      # You can only declare one collection in a partial template file.
+      # The `cached: true` will make Action Views rendering issue a `read_multi` to
+      # the cache store instead of reading from it for every partial.
       def cache(name = {}, options = {}, &block)
         if controller.respond_to?(:perform_caching) && controller.perform_caching
           name_options = options.slice(:skip_digest, :virtual_path)
diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb
index a9bd257e76..bdbf03191a 100644
--- a/actionview/lib/action_view/renderer/partial_renderer.rb
+++ b/actionview/lib/action_view/renderer/partial_renderer.rb
@@ -428,8 +428,6 @@ module ActionView
         layout = find_template(layout, @template_keys)
       end
 
-      collection_cache_eligible = automatic_cache_eligible?
-
       partial_iteration = PartialIteration.new(@collection.size)
       locals[iteration] = partial_iteration
 
@@ -438,11 +436,6 @@ module ActionView
         locals[counter]   = partial_iteration.index
 
         content = template.render(view, locals)
-
-        if collection_cache_eligible
-          collection_cache_rendered_partial(content, object)
-        end
-
         content = layout.render(view, locals) { content } if layout
         partial_iteration.iterate!
         content
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 4860f00243..debfcae870 100644
--- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb
+++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb
@@ -1,5 +1,3 @@
-require 'active_support/core_ext/object/try'
-
 module ActionView
   module CollectionCaching # :nodoc:
     extend ActiveSupport::Concern
@@ -12,7 +10,7 @@ module ActionView
 
     private
       def cache_collection_render
-        return yield unless cache_collection?
+        return yield unless @options[:cached]
 
         keyed_collection = collection_by_cache_keys
         cached_partials = collection_cache.read_multi(*keyed_collection.keys)
@@ -21,30 +19,14 @@ module ActionView
         rendered_partials = @collection.empty? ? [] : yield
 
         index = 0
-        keyed_collection.map do |cache_key, _|
-          cached_partials.fetch(cache_key) do
-            rendered_partials[index].tap { index += 1 }
-          end
+        fetch_or_cache_partial(cached_partials, order_by: keyed_collection.each_key) do
+          rendered_partials[index].tap { index += 1 }
         end
       end
 
-      def cache_collection?
-        @options.fetch(:cache, automatic_cache_eligible?)
-      end
-
-      def automatic_cache_eligible?
-        @template && @template.eligible_for_collection_caching?(as: @options[:as])
-      end
-
-      def callable_cache_key?
-        @options[:cache].respond_to?(:call)
-      end
-
       def collection_by_cache_keys
-        seed = callable_cache_key? ? @options[:cache] : ->(i) { i }
-
         @collection.each_with_object({}) do |item, hash|
-          hash[expanded_cache_key(seed.call(item))] = item
+          hash[expanded_cache_key(item)] = item
         end
       end
 
@@ -53,10 +35,13 @@ module ActionView
         key.frozen? ? key.dup : key # #read_multi & #write may require mutability, Dalli 2.6.0.
       end
 
-      def collection_cache_rendered_partial(rendered_partial, key_by)
-        if callable_cache_key?
-          key = expanded_cache_key(@options[:cache].call(key_by))
-          collection_cache.write(key, rendered_partial, @options[:cache_options])
+      def fetch_or_cache_partial(cached_partials, order_by:)
+        order_by.map do |cache_key|
+          cached_partials.fetch(cache_key) do
+            yield.tap do |rendered_partial|
+              collection_cache.write(cache_key, rendered_partial)
+            end
+          end
         end
       end
   end
diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb
index 15fc2b71a3..169ee55fdc 100644
--- a/actionview/lib/action_view/template.rb
+++ b/actionview/lib/action_view/template.rb
@@ -130,7 +130,6 @@ module ActionView
       @source            = source
       @identifier        = identifier
       @handler           = handler
-      @cache_name        = extract_resource_cache_name
       @compiled          = false
       @original_encoding = nil
       @locals            = details[:locals] || []
@@ -166,10 +165,6 @@ module ActionView
       @type ||= Types[@formats.first] if @formats.first
     end
 
-    def eligible_for_collection_caching?(as: nil)
-      @cache_name == (as || inferred_cache_name).to_s
-    end
-
     # Receives a view object and return a template similar to self by using @virtual_path.
     #
     # This method is useful if you have a template object but it does not contain its source
@@ -355,23 +350,5 @@ module ActionView
           ActiveSupport::Notifications.instrument("#{action}.action_view".freeze, payload, &block)
         end
       end
-
-      EXPLICIT_COLLECTION = /# Template Collection: (?<resource_name>\w+)/
-
-      def extract_resource_cache_name
-        if match = @source.match(EXPLICIT_COLLECTION) || resource_cache_call_match
-          match[:resource_name]
-        end
-      end
-
-      def resource_cache_call_match
-        if @handler.respond_to?(:resource_cache_call_pattern)
-          @source.match(@handler.resource_cache_call_pattern)
-        end
-      end
-
-      def inferred_cache_name
-        @inferred_cache_name ||= @virtual_path.split('/'.freeze).last.sub('_'.freeze, ''.freeze)
-      end
   end
 end
diff --git a/actionview/lib/action_view/template/handlers/erb.rb b/actionview/lib/action_view/template/handlers/erb.rb
index 1f8459c24b..85a100ed4c 100644
--- a/actionview/lib/action_view/template/handlers/erb.rb
+++ b/actionview/lib/action_view/template/handlers/erb.rb
@@ -123,31 +123,6 @@ module ActionView
           ).src
         end
 
-        # Returns Regexp to extract a cached resource's name from a cache call at the
-        # first line of a template.
-        # The extracted cache name is captured as :resource_name.
-        #
-        #   <% cache notification do %> # => notification
-        #
-        # The pattern should support templates with a beginning comment:
-        #
-        #   <%# Still extractable even though there's a comment %>
-        #   <% cache notification do %> # => notification
-        #
-        # But fail to extract a name if a resource association is cached.
-        #
-        #   <% cache notification.event do %> # => nil
-        def resource_cache_call_pattern
-          /\A
-            (?:<%\#.*%>)*                 # optional initial comment
-            \s*                           # followed by optional spaces or newlines
-            <%\s*cache[\(\s]              # followed by an ERB call to cache
-            \s*                           # followed by optional spaces or newlines
-            (?<resource_name>\w+)         # capture the cache call argument as :resource_name
-            [\s\)]                        # followed by a space or close paren
-          /xm
-        end
-
       private
 
         def valid_encoding(string, encoding)
diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb
index bf811abdd0..3561114d90 100644
--- a/actionview/test/template/render_test.rb
+++ b/actionview/test/template/render_test.rb
@@ -628,56 +628,59 @@ class LazyViewRenderTest < ActiveSupport::TestCase
   end
 end
 
-class CachedCollectionViewRenderTest < CachedViewRenderTest
+class CachedCollectionViewRenderTest < ActiveSupport::TestCase
   class CachedCustomer < Customer; end
 
-  teardown do
-    ActionView::PartialRenderer.collection_cache.clear
-  end
+  include RenderTestCases
 
-  test "with custom key" do
-    customer = Customer.new("david")
-    key = cache_key([customer, 'key'], "test/_customer")
+  # Ensure view path cache is primed
+  setup do
+    view_paths = ActionController::Base.view_paths
+    assert_equal ActionView::OptimizedFileSystemResolver, view_paths.first.class
 
-    ActionView::PartialRenderer.collection_cache.write(key, 'Hello')
+    ActionView::PartialRenderer.collection_cache = ActiveSupport::Cache::MemoryStore.new
 
-    assert_equal "Hello",
-      @view.render(partial: "test/customer", collection: [customer], cache: ->(item) { [item, 'key'] })
+    setup_view(view_paths)
   end
 
-  test "with caching with custom key and rendering with different key" do
-    customer = Customer.new("david")
-    key = cache_key([customer, 'key'], "test/_customer")
+  teardown do
+    GC.start
+    I18n.reload!
+  end
 
-    ActionView::PartialRenderer.collection_cache.write(key, 'Hello')
+  test "collection caching does not cache by default" do
+    customer = Customer.new("david", 1)
+    key = cache_key(customer, "test/_customer")
 
-    assert_equal "Hello: david",
-      @view.render(partial: "test/customer", collection: [customer], cache: ->(item) { [item, 'another_key'] })
+    ActionView::PartialRenderer.collection_cache.write(key, 'Cached')
+
+    assert_not_equal "Cached",
+      @view.render(partial: "test/customer", collection: [customer])
   end
 
-  test "automatic caching with inferred cache name" do
-    customer = CachedCustomer.new("david")
-    key = cache_key(customer, "test/_cached_customer")
+  test "collection caching with partial that doesn't use fragment caching" do
+    customer = Customer.new("david", 1)
+    key = cache_key(customer, "test/_customer")
 
     ActionView::PartialRenderer.collection_cache.write(key, 'Cached')
 
     assert_equal "Cached",
-      @view.render(partial: "test/cached_customer", collection: [customer])
+      @view.render(partial: "test/customer", collection: [customer], cached: true)
   end
 
-  test "automatic caching with as name" do
-    customer = CachedCustomer.new("david")
-    key = cache_key(customer, "test/_cached_customer_as")
+  test "collection caching with cached true" do
+    customer = CachedCustomer.new("david", 1)
+    key = cache_key(customer, "test/_cached_customer")
 
     ActionView::PartialRenderer.collection_cache.write(key, 'Cached')
 
     assert_equal "Cached",
-      @view.render(partial: "test/cached_customer_as", collection: [customer], as: :buyer)
+      @view.render(partial: "test/cached_customer", collection: [customer], cached: true)
   end
 
   private
-    def cache_key(names, virtual_path)
+    def cache_key(*names, virtual_path)
       digest = ActionView::Digestor.digest name: virtual_path, finder: @view.lookup_context, dependencies: []
-      @view.fragment_cache_key([ *Array(names), digest ])
+      @view.fragment_cache_key([ *names, digest ])
     end
 end
diff --git a/actionview/test/template/template_test.rb b/actionview/test/template/template_test.rb
index 921011b073..533c1c3219 100644
--- a/actionview/test/template/template_test.rb
+++ b/actionview/test/template/template_test.rb
@@ -192,38 +192,6 @@ class TestERBTemplate < ActiveSupport::TestCase
     assert_match(/\xFC/, e.message)
   end
 
-  def test_not_eligible_for_collection_caching_without_cache_call
-    [
-      "<%= 'Hello' %>",
-      "<% cache_customer = 42 %>",
-      "<% cache customer.name do %><% end %>",
-      "<% my_cache customer do %><% end %>"
-    ].each do |body|
-      template = new_template(body, virtual_path: "test/foo/_customer")
-      assert_not template.eligible_for_collection_caching?, "Template #{body.inspect} should not be eligible for collection caching"
-    end
-  end
-
-  def test_eligible_for_collection_caching_with_cache_call_or_explicit
-    [
-      "<% cache customer do %><% end %>",
-      "<% cache(customer) do %><% end %>",
-      "<% cache( customer) do %><% end %>",
-      "<% cache( customer ) do %><% end %>",
-      "<%cache customer do %><% end %>",
-      "<%   cache   customer    do %><% end %>",
-      "  <% cache customer do %>\n<% end %>\n",
-      "<%# comment %><% cache customer do %><% end %>",
-      "<%# comment %>\n<% cache customer do %><% end %>",
-      "<%# comment\n line 2\n line 3 %>\n<% cache customer do %><% end %>",
-      "<%# comment 1 %>\n<%# comment 2 %>\n<% cache customer do %><% end %>",
-      "<%# comment 1 %>\n<%# Template Collection: customer %>\n<% my_cache customer do %><% end %>"
-    ].each do |body|
-      template = new_template(body, virtual_path: "test/foo/_customer")
-      assert template.eligible_for_collection_caching?, "Template #{body.inspect} should be eligible for collection caching"
-    end
-  end
-
   def with_external_encoding(encoding)
     old = Encoding.default_external
     Encoding::Converter.new old, encoding if old != encoding