Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add explicit opt-out for fragment cache digesting #8317

Merged
merged 1 commit into from

5 participants

@latortuga

This add support for sending an explicit opt-out of the "Russian-doll" cache digest feature on a case-by-case basis. This is useful when cache-expiration needs to be performed manually and it would be otherwise difficult to know the exact name of a digested cache key.

@dhh requested I submit this PR, more information can be found here: rails/cache_digests#16

Note that the cache_digests gem also checks to see if a cache fragment is explicitly versioned using the following heuristic:

  1. The cache key is an array
  2. The first item in that array is in the format of "v{digits}" (e.g. ["v2",project] would be considered explicitly versioned)

I have not added support for this feature in this PR but I can add it fairly easily if you would like it. Adding the "explicitly versioned key" check would bring the rails master functionality in-line with gem's behavior.

@latortuga latortuga Add explicit opt-out for fragment cache digesting
This add support for sending an explicit opt-out of the "Russian-doll"
cache digest feature on a case-by-case basis. This is useful when cache-
expiration needs to be performed manually and it would be otherwise
difficult to know the exact name of a digested cache key.

More information: rails/cache_digests#16
7fb8c67
@dhh
Owner
@dhh dhh merged commit 1081ae2 into rails:master
@sobrinho

I'm not sure if it will be a problem but this is mutating the options hash.

@latortuga

@sobrinho Yes, I put it in there so that the option would not be co-opted for any other use and so that the intention of the option flag would be clear. If you would like I can change it to merely check the key's value and leave options unchanged.

@sobrinho

@latortuga I've never been affected by this type of mutability but I've seen it be fixed in some places on rails.

Maybe someone else can confirm if we need to fix it :)

@rafaelfranca rafaelfranca commented on the diff
actionpack/lib/action_view/helpers/cache_helper.rb
@@ -113,6 +120,21 @@ def cache(name = {}, options = nil, &block)
nil
end
+ # This helper returns the name of a cache key for a given fragment cache
+ # call. By supplying skip_digest: true to cache, the digestion of cache
+ # fragments can be manually bypassed. This is useful when cache fragments
+ # cannot be manually expired unless you know the exact key which is the
+ # case when using memcached.
+ def cache_fragment_name(name = {}, options = nil)
+ skip_digest = options && options.delete(:skip_digest)
+
+ if skip_digest
+ name
+ else
+ fragment_name_with_digest(name)
+ end
+ end
+
def fragment_name_with_digest(name) #:nodoc:
@rafaelfranca Owner

Should not this method be private?

@rafaelfranca Thanks for the feedback. I made it public due to the existing fragment_name_with_digest being public. My reasoning was that, because fragment_name_with_digest is public, it must be useful to be able to get the fragment name for a given key. Given that, it would also be useful to get a fragment key given a name and set of options. Let me know if you'd like it changed and/or covered with further unit tests.

@rafaelfranca Owner

Since it is public we should remove the #:nodoc: and add real documentation to this method.

I think the other method that was added should solve the case of this one, since you have the option to have both the result of this method, or give :skip_digest. I think that should be public, this one, since it's nodoc, could be private. Thoughts?

@rafaelfranca Owner

Yes, @carlosantoniodasilva is right. I prefer to leave it private

Alright so to summarize I will make the following alterations/fixes:

  • Change fragment_name_with_digest to private because cached_fragment_name accomplishes its functionality
  • Switch away from deleting the option key in favor of merely checking the value of it
  • Add a CHANGELOG entry

Would you like me to open a new PR? Thanks for all your feedback @carlosantoniodasilva @rafaelfranca

@rafaelfranca Owner

Right. Please fo ahead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca

@latortuga yes, @sobrinho is right. We avoid this kind of mutation in all the methods. Is not a good practice.

Also we need a CHANGELOG entry

@sgerrand sgerrand referenced this pull request from a commit in sgerrand/rails
@latortuga latortuga Cleanup CacheHelper changes allowing opt-out of cache digests
Instead of deleting the skip_digest option flag, this changes the method to merely check the
key. This change is because of a discussion in this thread:

rails#8317

This commit also makes #fragment_name_with_digest private due to its
functionality being subsumed by #cache_fragment_name.
e105adc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 26, 2012
  1. @latortuga

    Add explicit opt-out for fragment cache digesting

    latortuga authored
    This add support for sending an explicit opt-out of the "Russian-doll"
    cache digest feature on a case-by-case basis. This is useful when cache-
    expiration needs to be performed manually and it would be otherwise
    difficult to know the exact name of a digested cache key.
    
    More information: rails/cache_digests#16
This page is out of date. Refresh to see the latest.
View
24 actionpack/lib/action_view/helpers/cache_helper.rb
@@ -52,6 +52,13 @@ module CacheHelper
# Additionally, the digestor will automatically look through your template file for
# explicit and implicit dependencies, and include those as part of the digest.
#
+ # The digestor can be bypassed by passing skip_digest: true as an option to the cache call:
+ #
+ # <% cache project, skip_digest: true do %>
+ # <b>All the topics on this project</b>
+ # <%= render project.topics %>
+ # <% end %>
+ #
# ==== Implicit dependencies
#
# Most template dependencies can be derived from calls to render in the template itself.
@@ -105,7 +112,7 @@ module CacheHelper
# Now all you'll have to do is change that timestamp when the helper method changes.
def cache(name = {}, options = nil, &block)
if controller.perform_caching
- safe_concat(fragment_for(fragment_name_with_digest(name), options, &block))
+ safe_concat(fragment_for(cache_fragment_name(name, options), options, &block))
else
yield
end
@@ -113,6 +120,21 @@ def cache(name = {}, options = nil, &block)
nil
end
+ # This helper returns the name of a cache key for a given fragment cache
+ # call. By supplying skip_digest: true to cache, the digestion of cache
+ # fragments can be manually bypassed. This is useful when cache fragments
+ # cannot be manually expired unless you know the exact key which is the
+ # case when using memcached.
+ def cache_fragment_name(name = {}, options = nil)
+ skip_digest = options && options.delete(:skip_digest)
+
+ if skip_digest
+ name
+ else
+ fragment_name_with_digest(name)
+ end
+ end
+
def fragment_name_with_digest(name) #:nodoc:
@rafaelfranca Owner

Should not this method be private?

@rafaelfranca Thanks for the feedback. I made it public due to the existing fragment_name_with_digest being public. My reasoning was that, because fragment_name_with_digest is public, it must be useful to be able to get the fragment name for a given key. Given that, it would also be useful to get a fragment key given a name and set of options. Let me know if you'd like it changed and/or covered with further unit tests.

@rafaelfranca Owner

Since it is public we should remove the #:nodoc: and add real documentation to this method.

I think the other method that was added should solve the case of this one, since you have the option to have both the result of this method, or give :skip_digest. I think that should be public, this one, since it's nodoc, could be private. Thoughts?

@rafaelfranca Owner

Yes, @carlosantoniodasilva is right. I prefer to leave it private

Alright so to summarize I will make the following alterations/fixes:

  • Change fragment_name_with_digest to private because cached_fragment_name accomplishes its functionality
  • Switch away from deleting the option key in favor of merely checking the value of it
  • Add a CHANGELOG entry

Would you like me to open a new PR? Thanks for all your feedback @carlosantoniodasilva @rafaelfranca

@rafaelfranca Owner

Right. Please fo ahead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if @virtual_path
[
View
12 actionpack/test/controller/caching_test.rb
@@ -164,6 +164,9 @@ def formatted_fragment_cached
format.xml
end
end
+
+ def fragment_cached_without_digest
+ end
end
class FunctionalFragmentCachingTest < ActionController::TestCase
@@ -200,6 +203,15 @@ def test_fragment_caching_in_partials
@store.read("views/test.host/functional_caching/html_fragment_cached_with_partial/#{template_digest("functional_caching/_partial", "html")}"))
end
+ def test_skipping_fragment_cache_digesting
+ get :fragment_cached_without_digest, :format => "html"
+ assert_response :success
+ expected_body = "<body>\n<p>ERB</p>\n</body>\n"
+
+ assert_equal expected_body, @response.body
+ assert_equal "<p>ERB</p>", @store.read("views/nodigest")
+ end
+
def test_render_inline_before_fragment_caching
get :inline_fragment_cached
assert_response :success
View
3  actionpack/test/fixtures/functional_caching/fragment_cached_without_digest.html.erb
@@ -0,0 +1,3 @@
+<body>
+<%= cache 'nodigest', skip_digest: true do %><p>ERB</p><% end %>
+</body>
Something went wrong with that request. Please try again.