Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a more precise generated cache key #939

Merged
merged 1 commit into from
Jun 26, 2015

Conversation

aaronlerch
Copy link
Contributor

This change updates the default generated cache key (when the key: 'foo' option is specified and the object's #cache_key implementation isn't used) to use a more precise version of updated_at. By default, it was invoking updated_at.to_s which ends up with a cache key value like cache-key-prefix/807130-2015-05-27 18:11:05 UTC

This effectively means, if someone specifies a cache key in the serializer (which is really just a cache key prefix value -- I'll save that discussion for another PR perhaps ;) ) it will only be cached with a precision to the minute. So if I have a model and it gets cached, and I update it within the same minute, the cache will not be considered invalid and my model changes will not be reflected.

The ActiveRecord implementation of the default cache key, as you can see here https://github.com/rails/rails/blob/master/activerecord/lib/active_record/integration.rb#L55 , allows some customization but defaults to updated_at.utc.to_s(:nsec) which gives a much greater precision. In the example I gave above, it would be cache-key-prefix/807130-20150527181105000000000.
Since this gem doesn't have the dependencies to leverage the :nsec format helper, I leveraged the same format string to use here. (Currently using a magic string, since the only duplicate copies are in tests.)

I specifically did not update the cache_key to use #utc, but if that's desired I can update it to use that as well.

I also fixed up some of the tests. The test harnesses were written specifically using updated_at.to_i which allowed some implicit assumptions about cache invalidation to be encoded into the tests, which failed (rightly so) when I made the change to use a higher precision value.

✨ 🕒 ✨

Thanks!
Aaron

@@ -63,7 +63,7 @@ def is_fragment_cached?
end

def cache_key
(@klass._cache_key) ? "#{@klass._cache_key}/#{@cached_serializer.object.id}-#{@cached_serializer.object.updated_at}" : @cached_serializer.object.cache_key
(@klass._cache_key) ? "#{@klass._cache_key}/#{@cached_serializer.object.id}-#{@cached_serializer.object.updated_at.strftime("%Y%m%d%H%M%S%9N")}" : @cached_serializer.object.cache_key
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this means if updated_at is nil, you're gonna have a Bad Time. If that's something we should consider, I can update this method to be a bit more resilient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be awesome if we could make it more resilient 😄

@joaomdmoura
Copy link
Member

@aaronlerch sorry for taking so long to review it, I was rushing with some new feature myself, but I'm really in favor of this PR.
Could you just try to avoid threat the updated_at as nil thing and rebase it with master? 😄

@aaronlerch
Copy link
Contributor Author

No problem -- yeah, I'll make the change and update the pull request. I'm travelling for a few days so I'll get to it as soon as I'm able. Thanks!

@joaomdmoura
Copy link
Member

Great! Thank you! ❤️

@aaronlerch
Copy link
Contributor Author

Hold on, I think I messed up my attempt to rebase off master.

@aaronlerch
Copy link
Contributor Author

Okay, assuming Travis likes my pull request, it should be ready now @joaomdmoura -- including the change to make the use of #updated_at more resilient. Thanks for your patience!

@joaomdmoura
Copy link
Member

Amazing @aaronlerch! I'm merging it! ❤️

joaomdmoura added a commit that referenced this pull request Jun 26, 2015
Use a more precise generated cache key
@joaomdmoura joaomdmoura merged commit 01a225f into rails-api:master Jun 26, 2015
@aaronlerch aaronlerch deleted the default-cache-key branch June 26, 2015 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants