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 ActiveSupport::Cache.expand_cache_key for cache key expansions #1878

Merged
merged 3 commits into from
Aug 13, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ Fixes:

Misc:

- [#1878](https://github.com/rails-api/active_model_serializers/pull/1878) Cache key generation for serializers now uses `ActiveSupport::Cache.expand_cache_key` instead of `Array#join` by default and is also overridable. This change should be backward-compatible. (@markiz)

### [v0.10.2 (2016-07-05)](https://github.com/rails-api/active_model_serializers/compare/v0.10.1...v0.10.2)

Fixes:
Expand Down
6 changes: 5 additions & 1 deletion lib/active_model/serializer/caching.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,11 @@ def cache_key(adapter_instance)
parts << object_cache_key
parts << adapter_instance.cache_key
parts << serializer_class._cache_digest unless serializer_class._skip_digest?
@cache_key = parts.join('/')
@cache_key = expand_cache_key(parts)
end

def expand_cache_key(parts)
ActiveSupport::Cache.expand_cache_key(parts)
end

# Use object's cache_key if available, else derive a key from the object
Expand Down
38 changes: 38 additions & 0 deletions test/cache_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,30 @@

module ActiveModelSerializers
class CacheTest < ActiveSupport::TestCase
# Instead of a primitive cache key (i.e. a string), this class
# returns a list of objects that require to be expanded themselves.
class AuthorWithExpandableCacheElements < Author
# For the test purposes it's important that #to_s for HasCacheKey differs
# between instances, hence not a Struct.
class HasCacheKey
attr_reader :cache_key
def initialize(cache_key)
@cache_key = cache_key
end

def to_s
"HasCacheKey##{object_id}"
end
end
Copy link
Member

@bf4 bf4 Aug 12, 2016

Choose a reason for hiding this comment

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

Maybe

class AuthorWithCustomCacheKey < Author
  HasCacheKeyMethod = Struct.new(:cache_key)

  # Rather than `[name, id]` as the cache key, demonstrate that `:cache_key` is called on
  # any element that responds to it.
  def cache_key
    [HasCacheKeyMethod.new(name), HasCacheKeyMethod.new(id)]
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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


def cache_key
[
HasCacheKey.new(name),
HasCacheKey.new(id)
]
Copy link
Member

@bf4 bf4 Aug 12, 2016

Choose a reason for hiding this comment

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

@markiz my only real concern is that these classes are really weird-looking. Would you mind adding some comments why you designed it this way? Be nice to future devs :)

Copy link
Member

Choose a reason for hiding this comment

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

end
end

class UncachedAuthor < Author
# To confirm cache_key is set using updated_at and cache_key option passed to cache
undef_method :cache_key
Expand Down Expand Up @@ -106,6 +130,20 @@ def test_cache_key_interpolation_with_updated_at_when_cache_key_is_not_defined_o
assert_equal(uncached_author_serializer.attributes.to_json, cache_store.fetch(key).to_json)
end

def test_cache_key_expansion
author = AuthorWithExpandableCacheElements.new(id: 10, name: 'hello')
same_author = AuthorWithExpandableCacheElements.new(id: 10, name: 'hello')
diff_author = AuthorWithExpandableCacheElements.new(id: 11, name: 'hello')

author_serializer = AuthorSerializer.new(author)
same_author_serializer = AuthorSerializer.new(same_author)
diff_author_serializer = AuthorSerializer.new(diff_author)
adapter = AuthorSerializer.serialization_adapter_instance

assert_equal(author_serializer.cache_key(adapter), same_author_serializer.cache_key(adapter))
refute_equal(author_serializer.cache_key(adapter), diff_author_serializer.cache_key(adapter))
end

def test_default_cache_key_fallback
render_object_with_cache(@comment)
key = "#{@comment.cache_key}/#{adapter.cache_key}"
Expand Down