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 2 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 `Enumerable#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
34 changes: 34 additions & 0 deletions test/cache_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,26 @@

module ActiveModelSerializers
class CacheTest < ActiveSupport::TestCase
class AuthorWithCustomCacheKey < Author
Copy link
Member

Choose a reason for hiding this comment

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

maybe AuthorWithCacheKeyObjects ?

Copy link
Member

Choose a reason for hiding this comment

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

or AuthorWithExpandableCacheKeyElements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that last one, I'll change pr to use that

On Aug 12, 2016 8:25 PM, "Benjamin Fleischer" [email protected]
wrote:

In test/cache_test.rb
#1878 (comment)
:

@@ -4,6 +4,26 @@

module ActiveModelSerializers
class CacheTest < ActiveSupport::TestCase

  • class AuthorWithCustomCacheKey < Author

or AuthorWithExpandableCacheKeyElements?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1878/files/4ce7dc06280eb3b9580fab21956ce1202919ca5c#r74628088,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADRa4mGlYcKqJZ9u1mlwJC-vfdol4u_ks5qfKyFgaJpZM4Ji8cj
.

Copy link
Contributor Author

@markiz markiz Aug 13, 2016

Choose a reason for hiding this comment

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

class DerivedCacheKey
def initialize(object, method)
@object = object
@method = method
end

def cache_key
@object.__send__(@method)
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
[
DerivedCacheKey.new(self, :name),
DerivedCacheKey.new(self, :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 +126,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_with_non_primitive_values
author = AuthorWithCustomCacheKey.new(id: 10, name: 'hello')
same_author = AuthorWithCustomCacheKey.new(id: 10, name: 'hello')
diff_author = AuthorWithCustomCacheKey.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