Cashier.expire should clear keys stored in that tag #19

Merged
merged 3 commits into from Mar 23, 2013

Projects

None yet

2 participants

@amoslanka
Contributor

As it currently seems to be working, if I expire a tag using cashier, after expiring all the keys associated with the tag, those keys still exist in that tag's list of keys. (I'm using Redis)

For example:

I am action caching a search page tagged with "locations" where the search string is part of the cache key. So the following would be a list of the cache keys created over a period of time:

views/localhost:3000/locations?q=1
views/localhost:3000/locations?q=2
views/localhost:3000/locations?q=3

Those three keys would be in the list under the key based on the tag "locations".

If I run Cashier.expire "locations", those three view caches are correctly expired. However, if I then visit views/localhost:3000/locations?q=4, and then run Cashier.keys, I get the following output:

views/localhost:3000/locations?q=1
views/localhost:3000/locations?q=2
views/localhost:3000/locations?q=3
views/localhost:3000/locations?q=4

This is because when the tag was expired, the "locations" entry in Redis was left as is.

Is this intended? My opinion is that any key expired should be removed from the tag's list of keys. In a production environment, if there is an infinite number of queries my users would search for, then the "locations" tag's list of keys would grow infinitely as well, and it doesn't seem reasonable to expire the tag myself after running the expire method, as this is a jump into the inner workings of Cashier that my app shouldn't expect to understand.

Owner
ahawkins commented Mar 1, 2013

Good catch! I'd be happy to accept a PR for this fix :)

On Thu, Feb 28, 2013 at 11:33 PM, Amos Lanka notifications@github.comwrote:

As it currently seems to be working, if I expire a tag using cashier,
after expiring all the keys associated with the tag, those keys still exist
in that tag's list of keys. (I'm using Redis)

For example:

I am action caching a search page tagged with "locations" where the search
string is part of the cache key. So the following would be a list of the
cache keys created over a period of time:

views/localhost:3000/locations?q=1
views/localhost:3000/locations?q=2
views/localhost:3000/locations?q=3

Those three keys would be in the list under the key based on the tag
"locations".

If I run Cashier.expire "locations", those three view caches are
correctly expired. However, if I then visit
views/localhost:3000/locations?q=4, and then run Cashier.keys, I get the
following output:

views/localhost:3000/locations?q=1
views/localhost:3000/locations?q=2
views/localhost:3000/locations?q=3
views/localhost:3000/locations?q=4

This is because when the tag was expired, the "locations" entry in Redis
was left as is.

Is this intended? My opinion is that any key expired should be removed
from the tag's list of keys. In a production environment, if there is an
infinite number of queries my users would search for, then the "locations"
tag's list of keys would grow infinitely as well, and it doesn't seem
reasonable to expire the tag myself after running the expire method, as
this is a jump into the inner workings of Cashier that my app shouldn't
expect to understand.


Reply to this email directly or view it on GitHubhttps://github.com/twinturbo/cashier/issues/19
.

Contributor

I've been digging through what I can, and am realizing that perhaps this case is already been thought of and executed, though there may somehow be some gaps. After deleting the keys that are associated with a tag, the Cashier.expire method runs adapter.delete_tag(tag), which in turn does just what it says, according to the code. Strange that this doesn't appear to actually be happening on my app. I'll be digging deeper, and will report back what I find.

Owner
ahawkins commented Mar 1, 2013

ok. Please keep me informed. This should be working correctly.

On Fri, Mar 1, 2013 at 8:12 AM, Amos Lanka notifications@github.com wrote:

I've been digging through what I can, and am realizing that perhaps this
case is already been thought of and executed, though there may somehow be
some gaps. After deleting the keys that are associated with a tag, the
Cashier.expire method runs adapter.delete_tag(tag), which in turn does
just what it says, according to the code. Strange that this doesn't appear
to actually be happening on my app. I'll be digging deeper, and will report
back what I find.


Reply to this email directly or view it on GitHubhttps://github.com/twinturbo/cashier/issues/19#issuecomment-14275868
.

Contributor

Ok what I found that I was having trouble with was not as clear as I thought.

First off, the ActionSupport::Notifications system seems to tank when using Webrick on dev. I've been working with Cashier on two seperate apps, and was able to isolate the issue in the end because I was using Thin on one app and not the other. There may need to be some messaging in the Readme about using Thin (or another server) since Cashier is dependent upon the notifications.

Second, I used the method name "expire" in describing this issue, but should have used the method "clear". Expire works as expected, #clear only removed the "cashier-tags" key. I would expect Cashier.clear to work similarly to Rails.cache.clear, being more broad in what it clears, so in my upcoming pull request I've updated #clear to delete all tagged fragments, all tags, and the "cashier-tags" key.

@ahawkins ahawkins commented on the diff Mar 6, 2013
lib/cashier.rb
adapter.clear
+ tags.count
ahawkins
ahawkins Mar 6, 2013 Owner

why return tags.count?

Owner
ahawkins commented Mar 6, 2013

Seems this PR and the other share commits. Is there any dependence here?

Contributor

I accidentally committed these to master and they ended up getting pulled into the namespaces branch that i used for that feature. That was unintentional, and I don't believe namespace stuff is dependent upon these commits.

Also, I threw in the tag count return on simply to make Cashier.clear return something similar to what Rails.cache.clear returns, which is the number of fragments cleared. No real world application yet.

Contributor

Any further changes necessary in order to accept this PR? Would like to close the issue.

@ahawkins ahawkins merged commit d63097a into ahawkins:master Mar 23, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment