Merge pull request #23695 from kaspth/remove-automatic-collection-caching

Make collection caching explicit.

actionpack/test/controller/caching_test.rb

@@ -381,19 +381,14 @@ def index_ordered
     render 'index'
   end
 
-  def index_explicit_render
+  def index_explicit_render_in_controller
     @customers = [Customer.new('david', 1)]
-    render partial: 'customers/customer', collection: @customers
+    render partial: 'customers/customer', collection: @customers, cached: true
   end
 
   def index_with_comment
     @customers = [Customer.new('david', 1)]
-    render partial: 'customers/commented_customer', collection: @customers, as: :customer
-  end
-
-  def index_with_callable_cache_key
-    @customers = [Customer.new('david', 1)]
-    render @customers, cache: -> customer { 'cached_david' }
+    render partial: 'customers/commented_customer', collection: @customers, as: :customer, cached: true
   end
 end
 
@@ -404,7 +399,7 @@ def setup
     @controller.perform_caching = true
     @controller.partial_rendered_times = 0
     @controller.cache_store = ActiveSupport::Cache::MemoryStore.new
-    ActionView::PartialRenderer.collection_cache = @controller.cache_store
+    ActionView::PartialRenderer.collection_cache = ActiveSupport::Cache::MemoryStore.new
   end
 
   def test_collection_fetches_cached_views
@@ -427,7 +422,7 @@ def test_preserves_order_when_reading_from_cache_plus_rendering
   end
 
   def test_explicit_render_call_with_options
-    get :index_explicit_render
+    get :index_explicit_render_in_controller
 
     assert_select ':root', "david, 1"
   end
@@ -440,12 +435,6 @@ def test_caching_works_with_beginning_comment
     assert_equal 1, @controller.partial_rendered_times
   end
 
-  def test_caching_with_callable_cache_key
-    get :index_with_callable_cache_key
-    assert_customer_cached 'cached_david', 'david, 1'
-    assert_customer_cached 'david/1', 'david, 1'
-  end
-
   private
     def assert_customer_cached(key, content)
       assert_match content,

actionpack/test/fixtures/collection_cache/index.html.erb

-<%= render @customers %>
\ No newline at end of file
+<%= render partial: 'customers/customer', collection: @customers, cached: true %>

actionview/lib/action_view/helpers/cache_helper.rb

@@ -126,44 +126,16 @@ module CacheHelper
       #
       # 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)

actionview/lib/action_view/log_subscriber.rb

@@ -20,7 +20,15 @@ def render_template(event)
       end
     end
     alias :render_partial :render_template
-    alias :render_collection :render_template
+
+    def render_collection(event)
+      identifier = event.payload[:identifier] || 'templates'
+
+      info do
+        "  Rendered collection of #{from_rails_root(identifier)}" \
+        " #{render_count(event.payload)} (#{event.duration.round(1)}ms)"
+      end
+    end
 
     def logger
       ActionView::Base.logger
@@ -38,6 +46,14 @@ def from_rails_root(string)
     def rails_root
       @root ||= "#{Rails.root}/"
     end
+
+    def render_count(payload)
+      if payload[:cache_hits]
+        "[#{payload[:cache_hits]} / #{payload[:count]} cache hits]"
+      else
+        "[#{payload[:count]} times]"
+      end
+    end
   end
 end
 

actionview/lib/action_view/renderer/abstract_renderer.rb

@@ -35,8 +35,12 @@ def extract_details(options)
       end
     end
 
-    def instrument(name, options={})
-      ActiveSupport::Notifications.instrument("render_#{name}.action_view", options){ yield }
+    def instrument(name, **options)
+      options[:identifier] ||= (@template && @template.identifier) || @path
+
+      ActiveSupport::Notifications.instrument("render_#{name}.action_view", options) do |payload|
+        yield payload
+      end
     end
 
     def prepend_formats(formats)

actionview/lib/action_view/renderer/partial_renderer.rb

@@ -294,7 +294,7 @@ def initialize(*)
 
     def render(context, options, block)
       setup(context, options, block)
-      identifier = (@template = find_partial) ? @template.identifier : @path
+      @template = find_partial
 
       @lookup_context.rendered_format ||= begin
         if @template && @template.formats.present?
@@ -305,11 +305,9 @@ def render(context, options, block)
       end
 
       if @collection
-        instrument(:collection, :identifier => identifier || "collection", :count => @collection.size) do
-          render_collection
-        end
+        render_collection
       else
-        instrument(:partial, :identifier => identifier) do
+        instrument(:partial) do
           render_partial
         end
       end
@@ -318,15 +316,17 @@ def render(context, options, block)
     private
 
     def render_collection
-      return nil if @collection.blank?
+      instrument(:collection, count: @collection.size) do |payload|
+        return nil if @collection.blank?
 
-      if @options.key?(:spacer_template)
-        spacer = find_template(@options[:spacer_template], @locals.keys).render(@view, @locals)
-      end
+        if @options.key?(:spacer_template)
+          spacer = find_template(@options[:spacer_template], @locals.keys).render(@view, @locals)
+        end
 
-      cache_collection_render do
-        @template ? collection_with_template : collection_without_template
-      end.join(spacer).html_safe
+        cache_collection_render(payload) do
+          @template ? collection_with_template : collection_without_template
+        end.join(spacer).html_safe
+      end
     end
 
     def render_partial
@@ -428,8 +428,6 @@ def collection_with_template
         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 @@ def collection_with_template
         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

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

-require 'active_support/core_ext/object/try'
-
 module ActionView
   module CollectionCaching # :nodoc:
     extend ActiveSupport::Concern
@@ -11,40 +9,25 @@ module CollectionCaching # :nodoc:
     end
 
     private
-      def cache_collection_render
-        return yield unless cache_collection?
+      def cache_collection_render(instrumentation_payload)
+        return yield unless @options[:cached]
 
         keyed_collection = collection_by_cache_keys
-        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
 
         @collection = keyed_collection.reject { |key, _| cached_partials.key?(key) }.values
         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 +36,13 @@ def expanded_cache_key(key)
         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

actionview/lib/action_view/template.rb

@@ -130,7 +130,6 @@ def initialize(source, identifier, handler, details)
       @source            = source
       @identifier        = identifier
       @handler           = handler
-      @cache_name        = extract_resource_cache_name
       @compiled          = false
       @original_encoding = nil
       @locals            = details[:locals] || []
@@ -166,10 +165,6 @@ def type
       @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 @@ def instrument(action, &block)
           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

actionview/lib/action_view/template/handlers/erb.rb

@@ -123,31 +123,6 @@ def call(template)
           ).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)

actionview/test/template/log_subscriber_test.rb

@@ -86,7 +86,7 @@ def test_render_collection_template
       wait
 
       assert_equal 1, @logger.logged(:info).size
-      assert_match(/Rendered test\/_customer.erb/, @logger.logged(:info).last)
+      assert_match(/Rendered collection of test\/_customer.erb \[2 times\]/, @logger.logged(:info).last)
     end
   end
 
@@ -96,7 +96,7 @@ def test_render_collection_with_implicit_path
       wait
 
       assert_equal 1, @logger.logged(:info).size
-      assert_match(/Rendered customers\/_customer\.html\.erb/, @logger.logged(:info).last)
+      assert_match(/Rendered collection of customers\/_customer\.html\.erb \[2 times\]/, @logger.logged(:info).last)
     end
   end
 
@@ -106,7 +106,21 @@ def test_render_collection_template_without_path
       wait
 
       assert_equal 1, @logger.logged(:info).size
-      assert_match(/Rendered collection/, @logger.logged(:info).last)
+      assert_match(/Rendered collection of templates/, @logger.logged(:info).last)
+    end
+  end
+
+  def test_render_collection_with_cached_set
+    Rails.stub(:root, File.expand_path(FIXTURE_LOAD_PATH)) do
+      def @view.view_cache_dependencies; []; end
+      def @view.fragment_cache_key(*); 'ahoy `controller` dependency'; end
+
+      @view.render(partial: 'customers/customer', collection: [ Customer.new('david'), Customer.new('mary') ], cached: true,
+        locals: { greeting: 'hi' })
+      wait
+
+      assert_equal 1, @logger.logged(:info).size
+      assert_match(/Rendered collection of customers\/_customer\.html\.erb \[0 \/ 2 cache hits\]/, @logger.logged(:info).last)
     end
   end
 end

actionview/test/template/render_test.rb

@@ -628,56 +628,59 @@ def with_external_encoding(encoding)
   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

actionview/test/template/template_test.rb

@@ -192,38 +192,6 @@ def test_error_when_template_isnt_valid_utf8
     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

activesupport/lib/active_support/cache.rb

@@ -333,21 +333,19 @@ def read_multi(*names)
         options = names.extract_options!
         options = merged_options(options)
 
-        instrument_multi(:read, names, options) do |payload|
-          results = {}
-          names.each do |name|
-            key = normalize_key(name, options)
-            entry = read_entry(key, options)
-            if entry
-              if entry.expired?
-                delete_entry(key, options)
-              else
-                results[name] = entry.value
-              end
+        results = {}
+        names.each do |name|
+          key = normalize_key(name, options)
+          entry = read_entry(key, options)
+          if entry
+            if entry.expired?
+              delete_entry(key, options)
+            else
+              results[name] = entry.value
             end
           end
-          results
         end
+        results
       end
 
       # Fetches data from the cache, using the given keys. If there is data in
@@ -555,17 +553,6 @@ def instrument(operation, key, options = nil)
           ActiveSupport::Notifications.instrument("cache_#{operation}.active_support", payload){ yield(payload) }
         end
 
-        def instrument_multi(operation, keys, options = nil)
-          log do
-            formatted_keys = keys.map { |k| "- #{k}" }.join("\n")
-            "Caches multi #{operation}:\n#{formatted_keys}#{options.blank? ? "" : " (#{options.inspect})"}"
-          end
-
-          payload = { key: keys }
-          payload.merge!(options) if options.is_a?(Hash)
-          ActiveSupport::Notifications.instrument("cache_#{operation}_multi.active_support", payload) { yield(payload) }
-        end
-
         def log
           return unless logger && logger.debug? && !silence?
           logger.debug(yield)

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -96,16 +96,14 @@ def read_multi(*names)
         options = names.extract_options!
         options = merged_options(options)
 
-        instrument_multi(:read, names, options) do
-          keys_to_names = Hash[names.map{|name| [normalize_key(name, options), name]}]
-          raw_values = @data.get_multi(keys_to_names.keys, :raw => true)
-          values = {}
-          raw_values.each do |key, value|
-            entry = deserialize_entry(value)
-            values[keys_to_names[key]] = entry.value unless entry.expired?
-          end
-          values
+        keys_to_names = Hash[names.map{|name| [normalize_key(name, options), name]}]
+        raw_values = @data.get_multi(keys_to_names.keys, :raw => true)
+        values = {}
+        raw_values.each do |key, value|
+          entry = deserialize_entry(value)
+          values[keys_to_names[key]] = entry.value unless entry.expired?
         end
+        values
       end
 
       # Increment a cached value. This method uses the memcached incr atomic

activesupport/test/caching_test.rb

@@ -1151,15 +1151,6 @@ def test_mute_logging
     @cache.mute { @cache.fetch('foo') { 'bar' } }
     assert @buffer.string.blank?
   end
-
-  def test_multi_read_loggin
-    @cache.write 'hello', 'goodbye'
-    @cache.write 'world', 'earth'
-
-    @cache.read_multi('hello', 'world')
-
-    assert_match "Caches multi read:\n- hello\n- world", @buffer.string
-  end
 end
 
 class CacheEntryTest < ActiveSupport::TestCase

railties/test/application/per_request_digest_cache_test.rb

@@ -29,6 +29,8 @@ class Customer < Struct.new(:name, :id)
 
     app_file 'app/controllers/customers_controller.rb', <<-RUBY
       class CustomersController < ApplicationController
+        self.perform_caching = true
+
         def index
           render [ Customer.new('david', 1), Customer.new('dingus', 2) ]
         end