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

Make collection caching explicit.

3 files changed, 46 insertions, 61 deletions

diff --git a/actionview/test/template/log_subscriber_test.rb b/actionview/test/template/log_subscriber_test.rb
index 4776c18b0b..93a0701dcc 100644
--- a/actionview/test/template/log_subscriber_test.rb
+++ b/actionview/test/template/log_subscriber_test.rb
@@ -86,7 +86,7 @@ class AVLogSubscriberTest < ActiveSupport::TestCase
       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 @@ class AVLogSubscriberTest < ActiveSupport::TestCase
       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 @@ class AVLogSubscriberTest < ActiveSupport::TestCase
       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
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