From 75fa8dd309a84e125b59d01bf182d88419631eaa Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Thu, 18 May 2017 18:12:32 +0200 Subject: [PATCH] Use recyclable cache keys (#29092) --- actionmailer/test/caching_test.rb | 10 +-- actionpack/CHANGELOG.md | 13 +++ .../abstract_controller/caching/fragments.rb | 35 ++++++-- .../lib/action_controller/log_subscriber.rb | 4 +- actionpack/test/controller/caching_test.rb | 69 +++++++++----- .../formatted_fragment_cached.html.erb | 2 +- .../formatted_fragment_cached.xml.builder | 2 +- ...ragment_cached_with_variant.html+phone.erb | 2 +- .../fragment_cached.html.erb | 2 +- actionpack/test/lib/controller/fake_models.rb | 4 + .../lib/action_view/helpers/cache_helper.rb | 26 ++++-- .../partial_renderer/collection_caching.rb | 2 +- .../test/activerecord/relation_cache_test.rb | 2 +- .../test/template/log_subscriber_test.rb | 2 +- actionview/test/template/render_test.rb | 6 +- activerecord/CHANGELOG.md | 9 ++ activerecord/lib/active_record/integration.rb | 51 ++++++++--- activerecord/test/cases/cache_key_test.rb | 22 ++++- activerecord/test/cases/integration_test.rb | 48 ++++++++++ activesupport/CHANGELOG.md | 6 ++ activesupport/lib/active_support/cache.rb | 78 ++++++++++++---- .../active_support/cache/mem_cache_store.rb | 8 +- activesupport/test/caching_test.rb | 90 +++++++++++++++++++ .../lib/rails/application/configuration.rb | 8 ++ .../generators/rails/app/app_generator.rb | 10 +-- .../new_framework_defaults_5_1.rb.tt | 16 ---- .../new_framework_defaults_5_2.rb.tt | 11 +++ .../per_request_digest_cache_test.rb | 4 + .../test/generators/api_app_generator_test.rb | 1 - .../test/generators/app_generator_test.rb | 6 +- 30 files changed, 432 insertions(+), 117 deletions(-) delete mode 100644 railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_1.rb.tt create mode 100644 railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt diff --git a/actionmailer/test/caching_test.rb b/actionmailer/test/caching_test.rb index cff49c8894..5869eae7fd 100644 --- a/actionmailer/test/caching_test.rb +++ b/actionmailer/test/caching_test.rb @@ -21,10 +21,6 @@ class BaseCachingTest < ActiveSupport::TestCase @mailer.perform_caching = true @mailer.cache_store = @store end - - def test_fragment_cache_key - assert_equal "views/what a key", @mailer.fragment_cache_key("what a key") - end end class FragmentCachingTest < BaseCachingTest @@ -126,7 +122,7 @@ class FunctionalFragmentCachingTest < BaseCachingTest assert_match expected_body, email.body.encoded assert_match expected_body, - @store.read("views/caching/#{template_digest("caching_mailer/fragment_cache")}") + @store.read("views/caching_mailer/fragment_cache:#{template_digest("caching_mailer/fragment_cache")}/caching") end def test_fragment_caching_in_partials @@ -135,7 +131,7 @@ class FunctionalFragmentCachingTest < BaseCachingTest assert_match(expected_body, email.body.encoded) assert_match(expected_body, - @store.read("views/caching/#{template_digest("caching_mailer/_partial")}")) + @store.read("views/caching_mailer/_partial:#{template_digest("caching_mailer/_partial")}/caching")) end def test_skip_fragment_cache_digesting @@ -185,7 +181,7 @@ class FunctionalFragmentCachingTest < BaseCachingTest end assert_equal "caching_mailer", payload[:mailer] - assert_equal "views/caching/#{template_digest("caching_mailer/fragment_cache")}", payload[:key] + assert_equal [ :views, "caching_mailer/fragment_cache:#{template_digest("caching_mailer/fragment_cache")}", :caching ], payload[:key] ensure @mailer.enable_fragment_cache_logging = true end 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 :views, + # 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 delete_matched, 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 "

ERB

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

Builder

\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 "

PHONE

", - @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 @@ -<%= cache do %>

ERB

<% end %> +<%= cache("fragment") do %>

ERB

<% end %> 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 @@ -<%= cache do %>

PHONE

<% end %> +<%= cache("fragment") do %>

PHONE

<% end %> 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 diff --git a/actionview/lib/action_view/helpers/cache_helper.rb b/actionview/lib/action_view/helpers/cache_helper.rb index 15ab7e304f..c3aecadcd6 100644 --- a/actionview/lib/action_view/helpers/cache_helper.rb +++ b/actionview/lib/action_view/helpers/cache_helper.rb @@ -8,10 +8,9 @@ module ActionView # fragments, and so on. This method takes a block that contains # the content you wish to cache. # - # The best way to use this is by doing key-based cache expiration - # on top of a cache store like Memcached that'll automatically - # kick out old entries. For more on key-based expiration, see: - # http://signalvnoise.com/posts/3113-how-key-based-cache-expiration-works + # The best way to use this is by doing recyclable key-based cache expiration + # on top of a cache store like Memcached or Redis that'll automatically + # kick out old entries. # # When using this method, you list the cache dependency as the name of the cache, like so: # @@ -23,10 +22,14 @@ module ActionView # This approach will assume that when a new topic is added, you'll touch # the project. The cache key generated from this call will be something like: # - # views/projects/123-20120806214154/7a1156131a6928cb0026877f8b749ac9 - # ^class ^id ^updated_at ^template tree digest + # views/template/action.html.erb:7a1156131a6928cb0026877f8b749ac9/projects/123 + # ^template path ^template tree digest ^class ^id # - # The cache is thus automatically bumped whenever the project updated_at is touched. + # This cache key is stable, but it's combined with a cache version derived from the project + # record. When the project updated_at is touched, the #cache_version changes, even + # if the key stays stable. This means that unlike a traditional key-based cache expiration + # approach, you won't be generating cache trash, unused keys, simply because the dependent + # record is updated. # # If your template cache depends on multiple sources (try to avoid this to keep things simple), # you can name all these dependencies as part of an array: @@ -217,10 +220,15 @@ module ActionView def fragment_name_with_digest(name, virtual_path) virtual_path ||= @virtual_path + if virtual_path name = controller.url_for(name).split("://").last if name.is_a?(Hash) - digest = Digestor.digest name: virtual_path, finder: lookup_context, dependencies: view_cache_dependencies - [ name, digest ] + + if digest = Digestor.digest(name: virtual_path, finder: lookup_context, dependencies: view_cache_dependencies).presence + [ "#{virtual_path}:#{digest}", name ] + else + [ virtual_path, name ] + end else name end 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 1fbe209200..847256ac78 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -38,7 +38,7 @@ module ActionView end def expanded_cache_key(key) - key = @view.fragment_cache_key(@view.cache_fragment_name(key, virtual_path: @template.virtual_path)) + key = @view.combined_fragment_cache_key(@view.cache_fragment_name(key, virtual_path: @template.virtual_path)) key.frozen? ? key.dup : key # #read_multi & #write may require mutability, Dalli 2.6.0. end diff --git a/actionview/test/activerecord/relation_cache_test.rb b/actionview/test/activerecord/relation_cache_test.rb index 43f7242ee9..fbab512c41 100644 --- a/actionview/test/activerecord/relation_cache_test.rb +++ b/actionview/test/activerecord/relation_cache_test.rb @@ -10,7 +10,7 @@ class RelationCacheTest < ActionView::TestCase def test_cache_relation_other cache(Project.all) { concat("Hello World") } - assert_equal "Hello World", controller.cache_store.read("views/projects-#{Project.count}/") + assert_equal "Hello World", controller.cache_store.read("views/path/projects-#{Project.count}") end def view_cache_dependencies; end diff --git a/actionview/test/template/log_subscriber_test.rb b/actionview/test/template/log_subscriber_test.rb index 7f358add7e..584666d54b 100644 --- a/actionview/test/template/log_subscriber_test.rb +++ b/actionview/test/template/log_subscriber_test.rb @@ -39,7 +39,7 @@ class AVLogSubscriberTest < ActiveSupport::TestCase def set_view_cache_dependencies def @view.view_cache_dependencies; []; end - def @view.fragment_cache_key(*); "ahoy `controller` dependency"; end + def @view.combined_fragment_cache_key(*); "ahoy `controller` dependency"; end end def test_render_file_template diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index 3f66ab3ed3..fef78807d1 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -10,8 +10,8 @@ module RenderTestCases @view = Class.new(ActionView::Base) do def view_cache_dependencies; end - def fragment_cache_key(key) - ActiveSupport::Cache.expand_cache_key(key, :views) + def combined_fragment_cache_key(key) + [ :views, key ] end end.new(paths, @assigns) @@ -718,6 +718,6 @@ class CachedCollectionViewRenderTest < ActiveSupport::TestCase private def cache_key(*names, virtual_path) digest = ActionView::Digestor.digest name: virtual_path, finder: @view.lookup_context, dependencies: [] - @view.fragment_cache_key([ *names, digest ]) + @view.combined_fragment_cache_key([ "#{virtual_path}:#{digest}", *names ]) end end diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 4e264f5f2a..beead181f3 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,12 @@ +* Add ActiveRecord::Base#cache_version to support recyclable cache keys via the new versioned entries + in ActiveSupport::Cache. This also means that ActiveRecord::Base#cache_key will now return a stable key + that does not include a timestamp any more. + + NOTE: This feature is turned off by default, and #cache_key will still return cache keys with timestamps + until you set ActiveRecord::Base.cache_versioning = true. That's the setting for all new apps on Rails 5.2+ + + *DHH* + * Respect 'SchemaDumper.ignore_tables' in rake tasks for databases structure dump *Rusty Geldmacher*, *Guillermo Iguaran* diff --git a/activerecord/lib/active_record/integration.rb b/activerecord/lib/active_record/integration.rb index 8e71b60b29..ed652e26aa 100644 --- a/activerecord/lib/active_record/integration.rb +++ b/activerecord/lib/active_record/integration.rb @@ -13,6 +13,15 @@ module ActiveRecord # This is +:usec+, by default. class_attribute :cache_timestamp_format, instance_writer: false self.cache_timestamp_format = :usec + + ## + # :singleton-method: + # Indicates whether to use a stable #cache_key method that is accompanied + # by a changing version in the #cache_version method. + # + # This is +false+, by default until Rails 6.0. + class_attribute :cache_versioning, instance_writer: false + self.cache_versioning = false end # Returns a +String+, which Action Pack uses for constructing a URL to this @@ -52,25 +61,47 @@ module ActiveRecord # used to generate the key: # # Person.find(5).cache_key(:updated_at, :last_reviewed_at) + # + # If ActiveRecord::Base.cache_versioning is turned on, no version will be included + # in the cache key. The version will instead be supplied by #cache_version. This + # separation enables recycling of cache keys. + # + # Product.cache_versioning = true + # Product.new.cache_key # => "products/new" + # Person.find(5).cache_key # => "people/5" (even if updated_at available) def cache_key(*timestamp_names) if new_record? "#{model_name.cache_key}/new" else - timestamp = if timestamp_names.any? - max_updated_column_timestamp(timestamp_names) - else - max_updated_column_timestamp - end - - if timestamp - timestamp = timestamp.utc.to_s(cache_timestamp_format) - "#{model_name.cache_key}/#{id}-#{timestamp}" - else + if cache_version && timestamp_names.none? "#{model_name.cache_key}/#{id}" + else + timestamp = if timestamp_names.any? + max_updated_column_timestamp(timestamp_names) + else + max_updated_column_timestamp + end + + if timestamp + timestamp = timestamp.utc.to_s(cache_timestamp_format) + "#{model_name.cache_key}/#{id}-#{timestamp}" + else + "#{model_name.cache_key}/#{id}" + end end end end + # Returns a cache version that can be used together with the cache key to form + # a recyclable caching scheme. By default, the #updated_at column is used for the + # cache_version, but this method can be overwritten to return something else. + # + # Note, this method will return nil if ActiveRecord::Base.cache_versioning is set to + # +false+ (which it is by default until Rails 6.0). + def cache_version + try(:updated_at).try(:to_i) if cache_versioning + end + module ClassMethods # Defines your model's +to_param+ method to generate "pretty" URLs # using +method_name+, which can be any attribute or method that diff --git a/activerecord/test/cases/cache_key_test.rb b/activerecord/test/cases/cache_key_test.rb index 2c6a38ec35..f74cb18244 100644 --- a/activerecord/test/cases/cache_key_test.rb +++ b/activerecord/test/cases/cache_key_test.rb @@ -4,15 +4,23 @@ module ActiveRecord class CacheKeyTest < ActiveRecord::TestCase self.use_transactional_tests = false - class CacheMe < ActiveRecord::Base; end + class CacheMe < ActiveRecord::Base + self.cache_versioning = false + end + + class CacheMeWithVersion < ActiveRecord::Base + self.cache_versioning = true + end setup do @connection = ActiveRecord::Base.connection - @connection.create_table(:cache_mes) { |t| t.timestamps } + @connection.create_table(:cache_mes, force: true) { |t| t.timestamps } + @connection.create_table(:cache_me_with_versions, force: true) { |t| t.timestamps } end teardown do @connection.drop_table :cache_mes, if_exists: true + @connection.drop_table :cache_me_with_versions, if_exists: true end test "cache_key format is not too precise" do @@ -21,5 +29,15 @@ module ActiveRecord assert_equal key, record.reload.cache_key end + + test "cache_key has no version when versioning is on" do + record = CacheMeWithVersion.create + assert_equal "active_record/cache_key_test/cache_me_with_versions/#{record.id}", record.cache_key + end + + test "cache_version is only there when versioning is on" do + assert CacheMeWithVersion.create.cache_version.present? + assert_not CacheMe.create.cache_version.present? + end end end diff --git a/activerecord/test/cases/integration_test.rb b/activerecord/test/cases/integration_test.rb index 0678bb714f..7ffa86c42f 100644 --- a/activerecord/test/cases/integration_test.rb +++ b/activerecord/test/cases/integration_test.rb @@ -177,4 +177,52 @@ class IntegrationTest < ActiveRecord::TestCase owner.happy_at = nil assert_equal "owners/#{owner.id}", owner.cache_key(:happy_at) end + + def test_cache_key_is_stable_with_versioning_on + Developer.cache_versioning = true + + developer = Developer.first + first_key = developer.cache_key + + developer.touch + second_key = developer.cache_key + + assert_equal first_key, second_key + ensure + Developer.cache_versioning = false + end + + def test_cache_version_changes_with_versioning_on + Developer.cache_versioning = true + + developer = Developer.first + first_version = developer.cache_version + + travel 10.seconds do + developer.touch + end + + second_version = developer.cache_version + + assert_not_equal first_version, second_version + ensure + Developer.cache_versioning = false + end + + def test_cache_key_retains_version_when_custom_timestamp_is_used + Developer.cache_versioning = true + + developer = Developer.first + first_key = developer.cache_key(:updated_at) + + travel 10.seconds do + developer.touch + end + + second_key = developer.cache_key(:updated_at) + + assert_not_equal first_key, second_key + ensure + Developer.cache_versioning = false + end end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 3ce4a0bbab..49ff2c88d0 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,9 @@ +* Add support for versioned cache entries. This enables the cache stores to recycle cache keys, greatly saving + on storage in cases with frequent churn. Works together with the separation of #cache_key and #cache_version + in Active Record and its use in Action Pack's fragment caching. + + *DHH* + * Pass gem name and deprecation horizon to deprecation notifications. *Willem van Bergen* diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb index 4d8c2046e8..258140fe1d 100644 --- a/activesupport/lib/active_support/cache.rb +++ b/activesupport/lib/active_support/cache.rb @@ -219,6 +219,10 @@ module ActiveSupport # cache = ActiveSupport::Cache::MemoryStore.new(expires_in: 5.minutes) # cache.write(key, value, expires_in: 1.minute) # Set a lower value for one entry # + # Setting :version verifies the cache stored under name + # is of the same version. nil is returned on mismatches despite contents. + # This feature is used to support recyclable cache keys. + # # Setting :race_condition_ttl is very useful in situations where # a cache entry is used very frequently and is under heavy load. If a # cache expires and due to heavy load several different processes will try @@ -287,6 +291,7 @@ module ActiveSupport instrument(:read, name, options) do |payload| cached_entry = read_entry(key, options) unless options[:force] entry = handle_expired_entry(cached_entry, key, options) + entry = nil if entry && entry.mismatched?(normalize_version(name, options)) payload[:super_operation] = :fetch if payload payload[:hit] = !!entry if payload end @@ -303,20 +308,33 @@ module ActiveSupport end end - # Fetches data from the cache, using the given key. If there is data in + # Reads data from the cache, using the given key. If there is data in # the cache with the given key, then that data is returned. Otherwise, # +nil+ is returned. # + # Note, if data was written with the :expires_in or :version options, + # both of these conditions are applied before the data is returned. + # # Options are passed to the underlying cache implementation. def read(name, options = nil) options = merged_options(options) - key = normalize_key(name, options) + key = normalize_key(name, options) + version = normalize_version(name, options) + instrument(:read, name, options) do |payload| entry = read_entry(key, options) + if entry if entry.expired? delete_entry(key, options) payload[:hit] = false if payload + nil + elsif entry.mismatched?(version) + if payload + payload[:hit] = false + payload[:mismatch] = "#{entry.version} != #{version}" + end + nil else payload[:hit] = true if payload @@ -341,11 +359,15 @@ module ActiveSupport results = {} names.each do |name| - key = normalize_key(name, options) - entry = read_entry(key, options) + key = normalize_key(name, options) + version = normalize_version(name, options) + entry = read_entry(key, options) + if entry if entry.expired? delete_entry(key, options) + elsif entry.mismatched?(version) + # Skip mismatched versions else results[name] = entry.value end @@ -396,7 +418,7 @@ module ActiveSupport options = merged_options(options) instrument(:write, name, options) do - entry = Entry.new(value, options) + entry = Entry.new(value, options.merge(version: normalize_version(name, options))) write_entry(normalize_key(name, options), entry, options) end end @@ -420,7 +442,7 @@ module ActiveSupport instrument(:exist?, name) do entry = read_entry(normalize_key(name, options), options) - (entry && !entry.expired?) || false + (entry && !entry.expired? && !entry.mismatched?(normalize_version(name, options))) || false end end @@ -517,6 +539,17 @@ module ActiveSupport end end + + # Prefixes a key with the namespace. Namespace and key will be delimited + # with a colon. + def normalize_key(key, options) + key = expanded_key(key) + namespace = options[:namespace] if options + prefix = namespace.is_a?(Proc) ? namespace.call : namespace + key = "#{prefix}:#{key}" if prefix + key + end + # Expands key to be a consistent string value. Invokes +cache_key+ if # object responds to +cache_key+. Otherwise, +to_param+ method will be # called. If the key is a Hash, then keys will be sorted alphabetically. @@ -537,14 +570,16 @@ module ActiveSupport key.to_param end - # Prefixes a key with the namespace. Namespace and key will be delimited - # with a colon. - def normalize_key(key, options) - key = expanded_key(key) - namespace = options[:namespace] if options - prefix = namespace.is_a?(Proc) ? namespace.call : namespace - key = "#{prefix}:#{key}" if prefix - key + def normalize_version(key, options = nil) + (options && options[:version].try(:to_param)) || expanded_version(key) + end + + def expanded_version(key) + case + when key.respond_to?(:cache_version) then key.cache_version.to_param + when key.is_a?(Array) then key.map { |element| expanded_version(element) }.compact.to_param + when key.respond_to?(:to_a) then expanded_version(key.to_a) + end end def instrument(operation, key, options = nil) @@ -555,6 +590,7 @@ module ActiveSupport ActiveSupport::Notifications.instrument("cache_#{operation}.active_support", payload) { yield(payload) } end + def log return unless logger && logger.debug? && !silence? logger.debug(yield) @@ -591,13 +627,16 @@ module ActiveSupport end end - # This class is used to represent cache entries. Cache entries have a value and an optional - # expiration time. The expiration time is used to support the :race_condition_ttl option - # on the cache. + # This class is used to represent cache entries. Cache entries have a value, an optional + # expiration time, and an optional version. The expiration time is used to support the :race_condition_ttl option + # on the cache. The version is used to support the :version option on the cache for rejecting + # mismatches. # # Since cache entries in most instances will be serialized, the internals of this class are highly optimized # using short instance variable names that are lazily defined. class Entry # :nodoc: + attr_reader :version + DEFAULT_COMPRESS_LIMIT = 16.kilobytes # Creates a new cache entry for the specified value. Options supported are @@ -610,6 +649,7 @@ module ActiveSupport @value = value end + @version = options[:version] @created_at = Time.now.to_f @expires_in = options[:expires_in] @expires_in = @expires_in.to_f if @expires_in @@ -619,6 +659,10 @@ module ActiveSupport compressed? ? uncompress(@value) : @value end + def mismatched?(version) + @version && version && @version != version + end + # Checks if the entry is expired. The +expires_in+ parameter can override # the value set when the entry was created. def expired? diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb index e09cee3335..06fa9f67ad 100644 --- a/activesupport/lib/active_support/cache/mem_cache_store.rb +++ b/activesupport/lib/active_support/cache/mem_cache_store.rb @@ -97,12 +97,18 @@ module ActiveSupport options = merged_options(options) keys_to_names = Hash[names.map { |name| [normalize_key(name, options), name] }] + raw_values = @data.get_multi(keys_to_names.keys) values = {} + raw_values.each do |key, value| entry = deserialize_entry(value) - values[keys_to_names[key]] = entry.value unless entry.expired? + + unless entry.expired? || entry.mismatched?(normalize_version(keys_to_names[key], options)) + values[keys_to_names[key]] = entry.value + end end + values end diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb index dbec684ce0..f53b98c73e 100644 --- a/activesupport/test/caching_test.rb +++ b/activesupport/test/caching_test.rb @@ -579,6 +579,93 @@ module CacheStoreBehavior end end +module CacheStoreVersionBehavior + ModelWithKeyAndVersion = Struct.new(:cache_key, :cache_version) + + def test_fetch_with_right_version_should_hit + @cache.fetch("foo", version: 1) { "bar" } + assert_equal "bar", @cache.read("foo", version: 1) + end + + def test_fetch_with_wrong_version_should_miss + @cache.fetch("foo", version: 1) { "bar" } + assert_nil @cache.read("foo", version: 2) + end + + def test_read_with_right_version_should_hit + @cache.write("foo", "bar", version: 1) + assert_equal "bar", @cache.read("foo", version: 1) + end + + def test_read_with_wrong_version_should_miss + @cache.write("foo", "bar", version: 1) + assert_nil @cache.read("foo", version: 2) + end + + def test_exist_with_right_version_should_be_true + @cache.write("foo", "bar", version: 1) + assert @cache.exist?("foo", version: 1) + end + + def test_exist_with_wrong_version_should_be_false + @cache.write("foo", "bar", version: 1) + assert !@cache.exist?("foo", version: 2) + end + + def test_reading_and_writing_with_model_supporting_cache_version + m1v1 = ModelWithKeyAndVersion.new("model/1", 1) + m1v2 = ModelWithKeyAndVersion.new("model/1", 2) + + @cache.write(m1v1, "bar") + assert_equal "bar", @cache.read(m1v1) + assert_nil @cache.read(m1v2) + end + + def test_reading_and_writing_with_model_supporting_cache_version_using_nested_key + m1v1 = ModelWithKeyAndVersion.new("model/1", 1) + m1v2 = ModelWithKeyAndVersion.new("model/1", 2) + + @cache.write([ "something", m1v1 ], "bar") + assert_equal "bar", @cache.read([ "something", m1v1 ]) + assert_nil @cache.read([ "something", m1v2 ]) + end + + def test_fetching_with_model_supporting_cache_version + m1v1 = ModelWithKeyAndVersion.new("model/1", 1) + m1v2 = ModelWithKeyAndVersion.new("model/1", 2) + + @cache.fetch(m1v1) { "bar" } + assert_equal "bar", @cache.fetch(m1v1) { "bu" } + assert_equal "bu", @cache.fetch(m1v2) { "bu" } + end + + def test_exist_with_model_supporting_cache_version + m1v1 = ModelWithKeyAndVersion.new("model/1", 1) + m1v2 = ModelWithKeyAndVersion.new("model/1", 2) + + @cache.write(m1v1, "bar") + assert @cache.exist?(m1v1) + assert_not @cache.fetch(m1v2) + end + + def test_fetch_multi_with_model_supporting_cache_version + m1v1 = ModelWithKeyAndVersion.new("model/1", 1) + m2v1 = ModelWithKeyAndVersion.new("model/2", 1) + m2v2 = ModelWithKeyAndVersion.new("model/2", 2) + + first_fetch_values = @cache.fetch_multi(m1v1, m2v1) { |m| m.cache_key } + second_fetch_values = @cache.fetch_multi(m1v1, m2v2) { |m| m.cache_key + " 2nd" } + + assert_equal({ m1v1 => "model/1", m2v1 => "model/2" }, first_fetch_values) + assert_equal({ m1v1 => "model/1", m2v2 => "model/2 2nd" }, second_fetch_values) + end + + def test_version_is_normalized + @cache.write("foo", "bar", version: 1) + assert_equal "bar", @cache.read("foo", version: "1") + end +end + # https://rails.lighthouseapp.com/projects/8994/tickets/6225-memcachestore-cant-deal-with-umlauts-and-special-characters # The error is caused by character encodings that can't be compared with ASCII-8BIT regular expressions and by special # characters like the umlaut in UTF-8. @@ -822,6 +909,7 @@ class FileStoreTest < ActiveSupport::TestCase end include CacheStoreBehavior + include CacheStoreVersionBehavior include LocalCacheBehavior include CacheDeleteMatchedBehavior include CacheIncrementDecrementBehavior @@ -931,6 +1019,7 @@ class MemoryStoreTest < ActiveSupport::TestCase end include CacheStoreBehavior + include CacheStoreVersionBehavior include CacheDeleteMatchedBehavior include CacheIncrementDecrementBehavior @@ -1052,6 +1141,7 @@ class MemCacheStoreTest < ActiveSupport::TestCase end include CacheStoreBehavior + include CacheStoreVersionBehavior include LocalCacheBehavior include CacheIncrementDecrementBehavior include EncodedKeyCacheBehavior diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 27c1572357..4dc9a431f6 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -77,9 +77,17 @@ module Rails assets.unknown_asset_fallback = false end + if respond_to?(:action_view) + action_view.form_with_generates_remote_forms = true + end + when "5.2" load_defaults "5.1" + if respond_to?(:active_record) + active_record.cache_versioning = true + end + else raise "Unknown version #{target_version.to_s.inspect}" end diff --git a/railties/lib/rails/generators/rails/app/app_generator.rb b/railties/lib/rails/generators/rails/app/app_generator.rb index 669514b37e..b7bf45c522 100644 --- a/railties/lib/rails/generators/rails/app/app_generator.rb +++ b/railties/lib/rails/generators/rails/app/app_generator.rb @@ -121,7 +121,7 @@ module Rails action_cable_config_exist = File.exist?("config/cable.yml") rack_cors_config_exist = File.exist?("config/initializers/cors.rb") assets_config_exist = File.exist?("config/initializers/assets.rb") - new_framework_defaults_5_1_exist = File.exist?("config/initializers/new_framework_defaults_5_1.rb") + new_framework_defaults_5_2_exist = File.exist?("config/initializers/new_framework_defaults_5_2.rb") config @@ -145,12 +145,6 @@ module Rails unless assets_config_exist remove_file "config/initializers/assets.rb" end - - # Sprockets owns the only new default for 5.1: - # In API-only Applications, we don't want the file. - unless new_framework_defaults_5_1_exist - remove_file "config/initializers/new_framework_defaults_5_1.rb" - end end end @@ -401,7 +395,7 @@ module Rails def delete_new_framework_defaults unless options[:update] - remove_file "config/initializers/new_framework_defaults_5_1.rb" + remove_file "config/initializers/new_framework_defaults_5_2.rb" end end diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_1.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_1.rb.tt deleted file mode 100644 index a0c7f44b60..0000000000 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_1.rb.tt +++ /dev/null @@ -1,16 +0,0 @@ -# Be sure to restart your server when you modify this file. -# -# This file contains migration options to ease your Rails 5.1 upgrade. -# -# Once upgraded flip defaults one by one to migrate to the new default. -# -# Read the Guide for Upgrading Ruby on Rails for more info on each option. - -# Make `form_with` generate non-remote forms. -Rails.application.config.action_view.form_with_generates_remote_forms = false -<%- unless options[:skip_sprockets] -%> - -# Unknown asset fallback will return the path passed in when the given -# asset is not present in the asset pipeline. -# Rails.application.config.assets.unknown_asset_fallback = false -<%- end -%> diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt new file mode 100644 index 0000000000..52c08500d8 --- /dev/null +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt @@ -0,0 +1,11 @@ +# Be sure to restart your server when you modify this file. +# +# This file contains migration options to ease your Rails 5.2 upgrade. +# +# Once upgraded flip defaults one by one to migrate to the new default. +# +# Read the Guide for Upgrading Ruby on Rails for more info on each option. + +# Make Active Record use stable #cache_key alongside new #cache_version method. +# This is needed for recyclable cache keys. +# Rails.application.config.active_record.cache_versioning = true diff --git a/railties/test/application/per_request_digest_cache_test.rb b/railties/test/application/per_request_digest_cache_test.rb index 6c003e9bcc..6e6996a6ba 100644 --- a/railties/test/application/per_request_digest_cache_test.rb +++ b/railties/test/application/per_request_digest_cache_test.rb @@ -18,6 +18,10 @@ class PerRequestDigestCacheTest < ActiveSupport::TestCase class Customer < Struct.new(:name, :id) extend ActiveModel::Naming include ActiveModel::Conversion + + def cache_key + [ name, id ].join("/") + end end RUBY diff --git a/railties/test/generators/api_app_generator_test.rb b/railties/test/generators/api_app_generator_test.rb index 2edb39c8e8..a19e0f0dd8 100644 --- a/railties/test/generators/api_app_generator_test.rb +++ b/railties/test/generators/api_app_generator_test.rb @@ -70,7 +70,6 @@ class ApiAppGeneratorTest < Rails::Generators::TestCase assert_no_file "config/initializers/cookies_serializer.rb" assert_no_file "config/initializers/assets.rb" - assert_no_file "config/initializers/new_framework_defaults_5_1.rb" end def test_app_update_does_not_generate_unnecessary_bin_files diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb index ed8eea3243..ff829eb197 100644 --- a/railties/test/generators/app_generator_test.rb +++ b/railties/test/generators/app_generator_test.rb @@ -157,7 +157,7 @@ class AppGeneratorTest < Rails::Generators::TestCase end def test_new_application_doesnt_need_defaults - assert_no_file "config/initializers/new_framework_defaults_5_1.rb" + assert_no_file "config/initializers/new_framework_defaults_5_2.rb" end def test_new_application_load_defaults @@ -203,14 +203,14 @@ class AppGeneratorTest < Rails::Generators::TestCase app_root = File.join(destination_root, "myapp") run_generator [app_root] - assert_no_file "#{app_root}/config/initializers/new_framework_defaults_5_1.rb" + assert_no_file "#{app_root}/config/initializers/new_framework_defaults_5_2.rb" stub_rails_application(app_root) do generator = Rails::Generators::AppGenerator.new ["rails"], { update: true }, destination_root: app_root, shell: @shell generator.send(:app_const) quietly { generator.send(:update_config_files) } - assert_file "#{app_root}/config/initializers/new_framework_defaults_5_1.rb" + assert_file "#{app_root}/config/initializers/new_framework_defaults_5_2.rb" end end