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
Add #cache_key to ActiveRecord::Relation. #20884
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
module ActiveRecord | ||
module CollectionCacheKey | ||
|
||
def collection_cache_key(collection = all, timestamp_column = :updated_at) # :nodoc: | ||
query_signature = Digest::MD5.hexdigest(collection.to_sql) | ||
key = "#{collection.model_name.cache_key}/query-#{query_signature}" | ||
|
||
if collection.loaded? | ||
size = collection.size | ||
timestamp = collection.max_by(×tamp_column).public_send(timestamp_column) | ||
else | ||
column_type = type_for_attribute(timestamp_column.to_s) | ||
column = "#{connection.quote_table_name(collection.table_name)}.#{connection.quote_column_name(timestamp_column)}" | ||
|
||
query = collection.select("COUNT(*) AS size", "MAX(#{column}) AS timestamp") | ||
result = connection.select_one(query) | ||
|
||
size = result["size"] | ||
timestamp = column_type.deserialize(result["timestamp"]) | ||
end | ||
|
||
if timestamp | ||
"#{key}-#{size}-#{timestamp.utc.to_s(cache_timestamp_format)}" | ||
else | ||
"#{key}-#{size}" | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,6 +298,32 @@ def many? | |
limit_value ? to_a.many? : size > 1 | ||
end | ||
|
||
# Returns a cache key that can be used to identify the records fetched by | ||
# this query. The cache key is built with a fingerprint of the sql query, | ||
# the number of records matched by the query and a timestamp of the last | ||
# updated record. When a new record comes to match the query, or any of | ||
# the existing records is updated or deleted, the cache key changes. | ||
# | ||
# Product.where("name like ?", "%Cosmic Encounter%").cache_key | ||
# => "products/query-1850ab3d302391b85b8693e941286659-1-20150714212553907087000" | ||
# | ||
# If the collection is loaded, the method will iterate through the records | ||
# to generate the timestamp, otherwise it will trigger one SQL query like: | ||
# | ||
# SELECT COUNT(*), MAX("products"."updated_at") FROM "products" WHERE (name like '%Cosmic Encounter%') | ||
# | ||
# You can also pass a custom timestamp column to fetch the timestamp of the | ||
# last updated record. | ||
# | ||
# Product.where("name like ?", "%Game%").cache_key(:last_reviewed_at) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to document that the user can override this behavior by implementing |
||
# | ||
# You can customize the strategy to generate the key on a per model basis | ||
# overriding ActiveRecord::Base#collection_cache_key. | ||
def cache_key(timestamp_column = :updated_at) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name is a bit misleading, it may be how the reason you want to use this data, but not what the data actually is (e.g. it doesn't refer to any actual cache stored by Rails). Not really sure what would be a better name, though... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm following the same name as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for clarity perhaps rename def cache_key
super(:id)
end On other hand this would introduce discrepancy between definition of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I'm not sure if |
||
@cache_keys ||= {} | ||
@cache_keys[timestamp_column] ||= @klass.collection_cache_key(self, timestamp_column) | ||
end | ||
|
||
# Scope all queries to the current scope. | ||
# | ||
# Comment.where(post_id: 1).scoping do | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
require "cases/helper" | ||
require "models/computer" | ||
require "models/developer" | ||
require "models/project" | ||
require "models/topic" | ||
require "models/post" | ||
require "models/comment" | ||
|
||
module ActiveRecord | ||
class CollectionCacheKeyTest < ActiveRecord::TestCase | ||
fixtures :developers, :projects, :developers_projects, :topics, :comments, :posts | ||
|
||
test "collection_cache_key on model" do | ||
assert_match(/\Adevelopers\/query-(\h+)-(\d+)-(\d+)\Z/, Developer.collection_cache_key) | ||
end | ||
|
||
test "cache_key for relation" do | ||
developers = Developer.where(name: "David") | ||
last_developer_timestamp = developers.order(updated_at: :desc).first.updated_at | ||
|
||
assert_match /\Adevelopers\/query-(\h+)-(\d+)-(\d+)\Z/, developers.cache_key | ||
|
||
/\Adevelopers\/query-(\h+)-(\d+)-(\d+)\Z/ =~ developers.cache_key | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
assert_equal Digest::MD5.hexdigest(developers.to_sql), $1 | ||
assert_equal developers.count.to_s, $2 | ||
assert_equal last_developer_timestamp.to_s(ActiveRecord::Base.cache_timestamp_format), $3 | ||
end | ||
|
||
test "it triggers at most one query" do | ||
developers = Developer.where(name: "David") | ||
|
||
assert_queries(1) { developers.cache_key } | ||
assert_queries(0) { developers.cache_key } | ||
end | ||
|
||
test "it doesn't trigger any query if the relation is already loaded" do | ||
developers = Developer.where(name: "David").load | ||
assert_queries(0) { developers.cache_key } | ||
end | ||
|
||
test "relation cache_key changes when the sql query changes" do | ||
developers = Developer.where(name: "David") | ||
other_relation = Developer.where(name: "David").where("1 = 1") | ||
|
||
assert_not_equal developers.cache_key, other_relation.cache_key | ||
end | ||
|
||
test "cache_key for empty relation" do | ||
developers = Developer.where(name: "Non Existent Developer") | ||
assert_match(/\Adevelopers\/query-(\h+)-0\Z/, developers.cache_key) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is great, but can we add a test case that shows that the count portion is a specific expected value for a cache key, both with and without a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added these, to check each part of the cache key. Is that ok? |
||
|
||
test "cache_key with custom timestamp column" do | ||
topics = Topic.where("title like ?", "%Topic%") | ||
last_topic_timestamp = topics(:fifth).written_on.utc.to_s(:nsec) | ||
assert_match(last_topic_timestamp, topics.cache_key(:written_on)) | ||
end | ||
|
||
test "cache_key with unknown timestamp column" do | ||
topics = Topic.where("title like ?", "%Topic%") | ||
assert_raises(ActiveRecord::StatementInvalid) { topics.cache_key(:published_at) } | ||
end | ||
|
||
test "collection proxy provides a cache_key" do | ||
developers = projects(:active_record).developers | ||
assert_match(/\Adevelopers\/query-(\h+)-(\d+)-(\d+)\Z/, developers.cache_key) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where we would have no value for
timestamp
, butsize
is some value other than 0?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
timestamp
column could haveNULL
values. In that case, it probably wouldn't be a good idea to create a cache key like this, but at least the method behaves somewhat better in that situation.