Use recyclable cache keys (#29092)

9 files changed, 96 insertions, 37 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index cc908dcc43..c83e101619 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,16 @@
+*  Change the cache key format for fragments to make it easier to debug key churn. The new format is:
+
+      views/template/action.html.erb:7a1156131a6928cb0026877f8b749ac9/projects/123
+            ^template path           ^template tree digest            ^class   ^id
+
+   *DHH*
+
+*  Add support for recyclable cache keys with fragment caching. This uses the new versioned entries in the
+   ActiveSupport::Cache stores and relies on the fact that Active Record has split #cache_key and #cache_version
+   to support it.
+   
+   *DHH*
+
 *  Add `action_controller_api` and `action_controller_base` load hooks to be called in `ActiveSupport.on_load`
 
     `ActionController::Base` and `ActionController::API` have differing implementations. This means that
diff --git a/actionpack/lib/abstract_controller/caching/fragments.rb b/actionpack/lib/abstract_controller/caching/fragments.rb
index c85b4adba1..14e4a82523 100644
--- a/actionpack/lib/abstract_controller/caching/fragments.rb
+++ b/actionpack/lib/abstract_controller/caching/fragments.rb
@@ -25,7 +25,10 @@ module AbstractController
 
         self.fragment_cache_keys = []
 
-        helper_method :fragment_cache_key if respond_to?(:helper_method)
+        if respond_to?(:helper_method)
+          helper_method :fragment_cache_key
+          helper_method :combined_fragment_cache_key
+        end
       end
 
       module ClassMethods
@@ -62,17 +65,36 @@ module AbstractController
       # with the specified +key+ value. The key is expanded using
       # ActiveSupport::Cache.expand_cache_key.
       def fragment_cache_key(key)
+        ActiveSupport::Deprecation.warn(<<-MSG.squish)
+          Calling fragment_cache_key directly is deprecated and will be removed in Rails 6.0.
+          All fragment accessors now use the combined_fragment_cache_key method that retains the key as an array,
+          such that the caching stores can interrogate the parts for cache versions used in
+          recyclable cache keys.
+        MSG
+
         head = self.class.fragment_cache_keys.map { |k| instance_exec(&k) }
         tail = key.is_a?(Hash) ? url_for(key).split("://").last : key
         ActiveSupport::Cache.expand_cache_key([*head, *tail], :views)
       end
 
+      # Given a key (as described in +expire_fragment+), returns
+      # a key array suitable for use in reading, writing, or expiring a
+      # cached fragment. All keys begin with <tt>:views</tt>,
+      # followed by ENV["RAILS_CACHE_ID"] or ENV["RAILS_APP_VERSION"] if set,
+      # followed by any controller-wide key prefix values, ending
+      # with the specified +key+ value.
+      def combined_fragment_cache_key(key)
+        head = self.class.fragment_cache_keys.map { |k| instance_exec(&k) }
+        tail = key.is_a?(Hash) ? url_for(key).split("://").last : key
+        [ :views, (ENV["RAILS_CACHE_ID"] || ENV["RAILS_APP_VERSION"]), *head, *tail ].compact
+      end
+
       # Writes +content+ to the location signified by
       # +key+ (see +expire_fragment+ for acceptable formats).
       def write_fragment(key, content, options = nil)
         return content unless cache_configured?
 
-        key = fragment_cache_key(key)
+        key = combined_fragment_cache_key(key)
         instrument_fragment_cache :write_fragment, key do
           content = content.to_str
           cache_store.write(key, content, options)
@@ -85,7 +107,7 @@ module AbstractController
       def read_fragment(key, options = nil)
         return unless cache_configured?
 
-        key = fragment_cache_key(key)
+        key = combined_fragment_cache_key(key)
         instrument_fragment_cache :read_fragment, key do
           result = cache_store.read(key, options)
           result.respond_to?(:html_safe) ? result.html_safe : result
@@ -96,7 +118,7 @@ module AbstractController
       # +key+ exists (see +expire_fragment+ for acceptable formats).
       def fragment_exist?(key, options = nil)
         return unless cache_configured?
-        key = fragment_cache_key(key)
+        key = combined_fragment_cache_key(key)
 
         instrument_fragment_cache :exist_fragment?, key do
           cache_store.exist?(key, options)
@@ -123,7 +145,7 @@ module AbstractController
       # method (or <tt>delete_matched</tt>, for Regexp keys).
       def expire_fragment(key, options = nil)
         return unless cache_configured?
-        key = fragment_cache_key(key) unless key.is_a?(Regexp)
+        key = combined_fragment_cache_key(key) unless key.is_a?(Regexp)
 
         instrument_fragment_cache :expire_fragment, key do
           if key.is_a?(Regexp)
@@ -135,8 +157,7 @@ module AbstractController
       end
 
       def instrument_fragment_cache(name, key) # :nodoc:
-        payload = instrument_payload(key)
-        ActiveSupport::Notifications.instrument("#{name}.#{instrument_name}", payload) { yield }
+        ActiveSupport::Notifications.instrument("#{name}.#{instrument_name}", instrument_payload(key)) { yield }
       end
     end
   end
diff --git a/actionpack/lib/action_controller/log_subscriber.rb b/actionpack/lib/action_controller/log_subscriber.rb
index d29a5fe68f..d00fcbcd13 100644
--- a/actionpack/lib/action_controller/log_subscriber.rb
+++ b/actionpack/lib/action_controller/log_subscriber.rb
@@ -60,9 +60,9 @@ module ActionController
       class_eval <<-METHOD, __FILE__, __LINE__ + 1
         def #{method}(event)
           return unless logger.info? && ActionController::Base.enable_fragment_cache_logging
-          key_or_path = event.payload[:key] || event.payload[:path]
+          key         = ActiveSupport::Cache.expand_cache_key(event.payload[:key] || event.payload[:path])
           human_name  = #{method.to_s.humanize.inspect}
-          info("\#{human_name} \#{key_or_path} (\#{event.duration.round(1)}ms)")
+          info("\#{human_name} \#{key} (\#{event.duration.round(1)}ms)")
         end
       METHOD
     end
diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb
index fa8d9dc09a..dac5861c4b 100644
--- a/actionpack/test/controller/caching_test.rb
+++ b/actionpack/test/controller/caching_test.rb
@@ -26,10 +26,6 @@ class FragmentCachingMetalTest < ActionController::TestCase
     @controller.request = @request
     @controller.response = @response
   end
-
-  def test_fragment_cache_key
-    assert_equal "views/what a key", @controller.fragment_cache_key("what a key")
-  end
 end
 
 class CachingController < ActionController::Base
@@ -43,6 +39,8 @@ class FragmentCachingTestController < CachingController
 end
 
 class FragmentCachingTest < ActionController::TestCase
+  ModelWithKeyAndVersion = Struct.new(:cache_key, :cache_version)
+
   def setup
     super
     @store = ActiveSupport::Cache::MemoryStore.new
@@ -53,12 +51,25 @@ class FragmentCachingTest < ActionController::TestCase
     @controller.params = @params
     @controller.request = @request
     @controller.response = @response
+
+    @m1v1 = ModelWithKeyAndVersion.new("model/1", "1")
+    @m1v2 = ModelWithKeyAndVersion.new("model/1", "2")
+    @m2v1 = ModelWithKeyAndVersion.new("model/2", "1")
+    @m2v2 = ModelWithKeyAndVersion.new("model/2", "2")
   end
 
   def test_fragment_cache_key
-    assert_equal "views/what a key", @controller.fragment_cache_key("what a key")
-    assert_equal "views/test.host/fragment_caching_test/some_action",
-                  @controller.fragment_cache_key(controller: "fragment_caching_test", action: "some_action")
+    assert_deprecated do
+      assert_equal "views/what a key", @controller.fragment_cache_key("what a key")
+      assert_equal "views/test.host/fragment_caching_test/some_action",
+        @controller.fragment_cache_key(controller: "fragment_caching_test", action: "some_action")
+    end
+  end
+
+  def test_combined_fragment_cache_key
+    assert_equal [ :views, "what a key" ], @controller.combined_fragment_cache_key("what a key")
+    assert_equal [ :views, "test.host/fragment_caching_test/some_action" ],
+      @controller.combined_fragment_cache_key(controller: "fragment_caching_test", action: "some_action")
   end
 
   def test_read_fragment_with_caching_enabled
@@ -72,6 +83,12 @@ class FragmentCachingTest < ActionController::TestCase
     assert_nil @controller.read_fragment("name")
   end
 
+  def test_read_fragment_with_versioned_model
+    @controller.write_fragment([ "stuff", @m1v1 ], "hello")
+    assert_equal "hello", @controller.read_fragment([ "stuff", @m1v1 ])
+    assert_nil @controller.read_fragment([ "stuff", @m1v2 ])
+  end
+
   def test_fragment_exist_with_caching_enabled
     @store.write("views/name", "value")
     assert @controller.fragment_exist?("name")
@@ -198,7 +215,7 @@ CACHED
     assert_equal expected_body, @response.body
 
     assert_equal "This bit's fragment cached",
-      @store.read("views/test.host/functional_caching/fragment_cached/#{template_digest("functional_caching/fragment_cached")}")
+      @store.read("views/functional_caching/fragment_cached:#{template_digest("functional_caching/fragment_cached")}/fragment")
   end
 
   def test_fragment_caching_in_partials
@@ -207,7 +224,7 @@ CACHED
     assert_match(/Old fragment caching in a partial/, @response.body)
 
     assert_match("Old fragment caching in a partial",
-      @store.read("views/test.host/functional_caching/html_fragment_cached_with_partial/#{template_digest("functional_caching/_partial")}"))
+      @store.read("views/functional_caching/_partial:#{template_digest("functional_caching/_partial")}/test.host/functional_caching/html_fragment_cached_with_partial"))
   end
 
   def test_skipping_fragment_cache_digesting
@@ -237,7 +254,7 @@ CACHED
     assert_match(/Some inline content/, @response.body)
     assert_match(/Some cached content/, @response.body)
     assert_match("Some cached content",
-      @store.read("views/test.host/functional_caching/inline_fragment_cached/#{template_digest("functional_caching/inline_fragment_cached")}"))
+      @store.read("views/functional_caching/inline_fragment_cached:#{template_digest("functional_caching/inline_fragment_cached")}/test.host/functional_caching/inline_fragment_cached"))
   end
 
   def test_fragment_cache_instrumentation
@@ -264,7 +281,7 @@ CACHED
     assert_equal expected_body, @response.body
 
     assert_equal "<p>ERB</p>",
-      @store.read("views/test.host/functional_caching/formatted_fragment_cached/#{template_digest("functional_caching/formatted_fragment_cached")}")
+      @store.read("views/functional_caching/formatted_fragment_cached:#{template_digest("functional_caching/formatted_fragment_cached")}/fragment")
   end
 
   def test_xml_formatted_fragment_caching
@@ -275,7 +292,7 @@ CACHED
     assert_equal expected_body, @response.body
 
     assert_equal "  <p>Builder</p>\n",
-      @store.read("views/test.host/functional_caching/formatted_fragment_cached/#{template_digest("functional_caching/formatted_fragment_cached")}")
+      @store.read("views/functional_caching/formatted_fragment_cached:#{template_digest("functional_caching/formatted_fragment_cached")}/fragment")
   end
 
   def test_fragment_caching_with_variant
@@ -286,7 +303,7 @@ CACHED
     assert_equal expected_body, @response.body
 
     assert_equal "<p>PHONE</p>",
-      @store.read("views/test.host/functional_caching/formatted_fragment_cached_with_variant/#{template_digest("functional_caching/formatted_fragment_cached_with_variant")}")
+      @store.read("views/functional_caching/formatted_fragment_cached_with_variant:#{template_digest("functional_caching/formatted_fragment_cached_with_variant")}/fragment")
   end
 
   private
@@ -412,7 +429,7 @@ class CollectionCacheTest < ActionController::TestCase
   def test_collection_fetches_cached_views
     get :index
     assert_equal 1, @controller.partial_rendered_times
-    assert_customer_cached "david/1", "david, 1"
+    assert_match "david, 1", ActionView::PartialRenderer.collection_cache.read("views/customers/_customer:7c228ab609f0baf0b1f2367469210937/david/1")
 
     get :index
     assert_equal 1, @controller.partial_rendered_times
@@ -444,14 +461,8 @@ class CollectionCacheTest < ActionController::TestCase
 
   def test_caching_with_callable_cache_key
     get :index_with_callable_cache_key
-    assert_customer_cached "cached_david", "david, 1"
+    assert_match "david, 1", ActionView::PartialRenderer.collection_cache.read("views/customers/_customer:7c228ab609f0baf0b1f2367469210937/cached_david")
   end
-
-  private
-    def assert_customer_cached(key, content)
-      assert_match content,
-        ActionView::PartialRenderer.collection_cache.read("views/#{key}/7c228ab609f0baf0b1f2367469210937")
-    end
 end
 
 class FragmentCacheKeyTestController < CachingController
@@ -470,11 +481,21 @@ class FragmentCacheKeyTest < ActionController::TestCase
     @controller.cache_store = @store
   end
 
-  def test_fragment_cache_key
+  def test_combined_fragment_cache_key
     @controller.account_id = "123"
-    assert_equal "views/v1/123/what a key", @controller.fragment_cache_key("what a key")
+    assert_equal [ :views, "v1", "123", "what a key" ], @controller.combined_fragment_cache_key("what a key")
 
     @controller.account_id = nil
-    assert_equal "views/v1//what a key", @controller.fragment_cache_key("what a key")
+    assert_equal [ :views, "v1", "what a key" ], @controller.combined_fragment_cache_key("what a key")
+  end
+
+  def test_combined_fragment_cache_key_with_envs
+    ENV["RAILS_APP_VERSION"] = "55"
+    assert_equal [ :views, "55", "v1", "what a key" ], @controller.combined_fragment_cache_key("what a key")
+
+    ENV["RAILS_CACHE_ID"] = "66"
+    assert_equal [ :views, "66", "v1", "what a key" ], @controller.combined_fragment_cache_key("what a key")
+  ensure
+    ENV["RAILS_CACHE_ID"] = ENV["RAILS_APP_VERSION"] = nil
   end
 end
diff --git a/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.html.erb b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.html.erb
index 9b88fa1f5a..dfcd423978 100644
--- a/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.html.erb
+++ b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.html.erb
@@ -1,3 +1,3 @@
 <body>
-<%= cache do %><p>ERB</p><% end %>
+<%= cache("fragment") do %><p>ERB</p><% end %>
 </body>
diff --git a/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.xml.builder b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.xml.builder
index efdcc28e0f..6599579740 100644
--- a/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.xml.builder
+++ b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached.xml.builder
@@ -1,5 +1,5 @@
 xml.body do
-  cache do
+  cache("fragment") do
     xml.p "Builder"
   end
 end
diff --git a/actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb
index e523b74ae3..abf7017ce6 100644
--- a/actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb
+++ b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb
@@ -1,3 +1,3 @@
 <body>
-<%= cache do %><p>PHONE</p><% end %>
+<%= cache("fragment") do %><p>PHONE</p><% end %>
 </body>
diff --git a/actionpack/test/fixtures/functional_caching/fragment_cached.html.erb b/actionpack/test/fixtures/functional_caching/fragment_cached.html.erb
index fa5e6bd318..1148d83ad7 100644
--- a/actionpack/test/fixtures/functional_caching/fragment_cached.html.erb
+++ b/actionpack/test/fixtures/functional_caching/fragment_cached.html.erb
@@ -1,3 +1,3 @@
 Hello
-<%= cache do %>This bit's fragment cached<% end %>
+<%= cache "fragment" do %>This bit's fragment cached<% end %>
 <%= 'Ciao' %>
diff --git a/actionpack/test/lib/controller/fake_models.rb b/actionpack/test/lib/controller/fake_models.rb
index b768553e7a..ff37d85ed8 100644
--- a/actionpack/test/lib/controller/fake_models.rb
+++ b/actionpack/test/lib/controller/fake_models.rb
@@ -26,6 +26,10 @@ Customer = Struct.new(:name, :id) do
   def persisted?
     id.present?
   end
+
+  def cache_key
+    "#{name}/#{id}"
+  end
 end
 
 Post = Struct.new(:title, :author_name, :body, :secret, :persisted, :written_on, :cost) do