TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Make collection caching explicit.

Browse Source 
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.
This commit is contained in:
Kasper Timm Hansen 2016-02-15 22:47:44 +01:00
parent 454bc1deab
commit b4558c10fb
10 changed files with 58 additions and 194 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
actionpack/test/controller/caching_test.rb
View File

@ -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,

2
actionpack/test/fixtures/collection_cache/index.html.erb vendored
View File

@ -1 +1 @@
<%= render @customers %>
<%= render partial: 'customers/customer', collection: @customers, cached: true %>

42
actionview/lib/action_view/helpers/cache_helper.rb
View File

@ -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)

7
actionview/lib/action_view/renderer/partial_renderer.rb
View File

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

37
actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb
View File

@ -1,5 +1,3 @@
require 'active_support/core_ext/object/try'
module ActionView
module CollectionCaching # :nodoc:
extend ActiveSupport::Concern
@ -12,7 +10,7 @@ module CollectionCaching # :nodoc:
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 @@ def cache_collection_render
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 @@ 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

23
actionview/lib/action_view/template.rb
View File

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

25
actionview/lib/action_view/template/handlers/erb.rb
View File

@ -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)

61
actionview/test/template/render_test.rb
View File

@ -628,56 +628,59 @@ def with_external_encoding(encoding)
end
end
class CachedCollectionViewRenderTest < CachedViewRenderTest
class CachedCollectionViewRenderTest < ActiveSupport::TestCase
class CachedCustomer < Customer; end
include RenderTestCases
# 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 = ActiveSupport::Cache::MemoryStore.new
setup_view(view_paths)
end
teardown do
ActionView::PartialRenderer.collection_cache.clear
GC.start
I18n.reload!
end
test "with custom key" do
customer = Customer.new("david")
key = cache_key([customer, 'key'], "test/_customer")
test "collection caching does not cache by default" do
customer = Customer.new("david", 1)
key = cache_key(customer, "test/_customer")
ActionView::PartialRenderer.collection_cache.write(key, 'Hello')
ActionView::PartialRenderer.collection_cache.write(key, 'Cached')
assert_equal "Hello",
@view.render(partial: "test/customer", collection: [customer], cache: ->(item) { [item, 'key'] })
assert_not_equal "Cached",
@view.render(partial: "test/customer", collection: [customer])
end
test "with caching with custom key and rendering with different key" do
customer = Customer.new("david")
key = cache_key([customer, 'key'], "test/_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, 'Hello')
ActionView::PartialRenderer.collection_cache.write(key, 'Cached')
assert_equal "Hello: david",
@view.render(partial: "test/customer", collection: [customer], cache: ->(item) { [item, 'another_key'] })
assert_equal "Cached",
@view.render(partial: "test/customer", collection: [customer], cached: true)
end
test "automatic caching with inferred cache name" do
customer = CachedCustomer.new("david")
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", collection: [customer])
end
test "automatic caching with as name" do
customer = CachedCustomer.new("david")
key = cache_key(customer, "test/_cached_customer_as")
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

32
actionview/test/template/template_test.rb
View File

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

2
railties/test/application/per_request_digest_cache_test.rb
View File

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