Merge pull request #1642 from bf4/kevintyll-master

[FEATURE] Prefer object.cache_key when available.

CHANGELOG.md

@@ -3,6 +3,8 @@
 Breaking changes:
 
 Features:
+- [#1642](https://github.com/rails-api/active_model_serializers/pull/1642) Prefer object.cache_key over the generated
+  cache key. (@bf4 via #1346 by @kevintyll)
 - [#1637](https://github.com/rails-api/active_model_serializers/pull/1637) Make references to 'ActionController::Base.cache_store' explicit
   in order to avoid issues when application controllers inherit from 'ActionController::API'. (@ncuesta)
 - [#1633](https://github.com/rails-api/active_model_serializers/pull/1633) Yield 'serializer' to serializer association blocks. (@bf4)

lib/active_model/serializer/caching.rb

@@ -7,7 +7,7 @@ module ActiveModel
         with_options instance_writer: false, instance_reader: false do |serializer|
           serializer.class_attribute :_cache         # @api private : the cache store
           serializer.class_attribute :_fragmented    # @api private : @see ::fragmented
-          serializer.class_attribute :_cache_key     # @api private : when present, is first item in cache_key
+          serializer.class_attribute :_cache_key     # @api private : when present, is first item in cache_key.  Ignored if the serializable object defines #cache_key.
           serializer.class_attribute :_cache_only    # @api private : when fragment caching, whitelists cached_attributes. Cannot combine with except
           serializer.class_attribute :_cache_except  # @api private : when fragment caching, blacklists cached_attributes. Cannot combine with only
           serializer.class_attribute :_cache_options # @api private : used by CachedSerializer, passed to _cache.fetch

lib/active_model_serializers/cached_serializer.rb

@@ -1,5 +1,7 @@
 module ActiveModelSerializers
   class CachedSerializer
+    UndefinedCacheKey = Class.new(StandardError)
+
     def initialize(serializer)
       @cached_serializer = serializer
       @klass             = @cached_serializer.class
@@ -34,10 +36,18 @@ module ActiveModelSerializers
       @cache_key = parts.join('/')
     end
 
+    # Use object's cache_key if available, else derive a key from the object
+    # Pass the `key` option to the `cache` declaration or override this method to customize the cache key
     def object_cache_key
+      if @cached_serializer.object.respond_to?(:cache_key)
+        @cached_serializer.object.cache_key
+      elsif (cache_key = (@klass._cache_key || @klass._cache_options[:key]))
         object_time_safe = @cached_serializer.object.updated_at
         object_time_safe = object_time_safe.strftime('%Y%m%d%H%M%S%9N') if object_time_safe.respond_to?(:strftime)
-      @klass._cache_key ? "#{@klass._cache_key}/#{@cached_serializer.object.id}-#{object_time_safe}" : @cached_serializer.object.cache_key
+        "#{cache_key}/#{@cached_serializer.object.id}-#{object_time_safe}"
+      else
+        fail UndefinedCacheKey, "#{@cached_serializer.object.class} must define #cache_key, or the 'key:' option must be passed into '#{@klass}.cache'"
+      end
     end
 
     # find all cache_key for the collection_serializer

test/cache_test.rb

@@ -4,6 +4,21 @@ require 'tempfile'
 
 module ActiveModelSerializers
   class CacheTest < ActiveSupport::TestCase
+    UncachedAuthor = Class.new(Author) do
+      # To confirm cache_key is set using updated_at and cache_key option passed to cache
+      undef_method :cache_key
+    end
+
+    Article = Class.new(::Model) do
+      # To confirm error is raised when cache_key is not set and cache_key option not passed to cache
+      undef_method :cache_key
+    end
+
+    ArticleSerializer = Class.new(ActiveModel::Serializer) do
+      cache only: [:place], skip_digest: true
+      attributes :title
+    end
+
     InheritedRoleSerializer = Class.new(RoleSerializer) do
       cache key: 'inherited_role', only: [:name, :special_attribute]
       attribute :special_attribute
@@ -81,15 +96,27 @@ module ActiveModelSerializers
       assert_equal(nil, @comment_serializer.class._cache_key)
     end
 
-    def test_cache_key_interpolation_with_updated_at
-      render_object_with_cache(@author)
-      assert_equal(nil, cache_store.fetch(@author.cache_key))
-      assert_equal(@author_serializer.attributes.to_json, cache_store.fetch("#{@author_serializer.class._cache_key}/#{@author_serializer.object.id}-#{@author_serializer.object.updated_at.strftime("%Y%m%d%H%M%S%9N")}").to_json)
+    def test_cache_key_interpolation_with_updated_at_when_cache_key_is_not_defined_on_object
+      uncached_author            = UncachedAuthor.new(name: 'Joao M. D. Moura')
+      uncached_author_serializer = AuthorSerializer.new(uncached_author)
+
+      render_object_with_cache(uncached_author)
+      key = "#{uncached_author_serializer.class._cache_key}/#{uncached_author_serializer.object.id}-#{uncached_author_serializer.object.updated_at.strftime("%Y%m%d%H%M%S%9N")}"
+      assert_equal(uncached_author_serializer.attributes.to_json, cache_store.fetch(key).to_json)
     end
 
     def test_default_cache_key_fallback
       render_object_with_cache(@comment)
-      assert_equal(@comment_serializer.attributes.to_json, cache_store.fetch(@comment.cache_key).to_json)
+      key = @comment.cache_key
+      assert_equal(@comment_serializer.attributes.to_json, cache_store.fetch(key).to_json)
+    end
+
+    def test_error_is_raised_if_cache_key_is_not_defined_on_object_or_passed_as_cache_option
+      article = Article.new(title: 'Must Read')
+      e = assert_raises ActiveModelSerializers::CachedSerializer::UndefinedCacheKey do
+        render_object_with_cache(article)
+      end
+      assert_match(/ActiveModelSerializers::CacheTest::Article must define #cache_key, or the 'key:' option must be passed into 'CachedActiveModelSerializers_CacheTest_ArticleSerializer.cache'/, e.message)
     end
 
     def test_cache_options_definition
@@ -111,8 +138,10 @@ module ActiveModelSerializers
       Timecop.freeze(Time.current) do
         render_object_with_cache(@post)
 
-        assert_equal(@post_serializer.attributes, cache_store.fetch(@post.cache_key))
-        assert_equal(@comment_serializer.attributes, cache_store.fetch(@comment.cache_key))
+        key = @post.cache_key
+        assert_equal(@post_serializer.attributes, cache_store.fetch(key))
+        key = @comment.cache_key
+        assert_equal(@comment_serializer.attributes, cache_store.fetch(key))
       end
     end
 
@@ -122,8 +151,10 @@ module ActiveModelSerializers
         render_object_with_cache(@post)
 
         # Check if it cached the objects separately
-        assert_equal(@post_serializer.attributes,    cached_serialization(@post_serializer))
-        assert_equal(@comment_serializer.attributes, cached_serialization(@comment_serializer))
+        key = @post.cache_key
+        assert_equal(@post_serializer.attributes, cache_store.fetch(key))
+        key = @comment.cache_key
+        assert_equal(@comment_serializer.attributes, cache_store.fetch(key))
 
         # Simulating update on comments relationship with Post
         new_comment            = Comment.new(id: 2567, body: 'ZOMG A NEW COMMENT')
@@ -134,8 +165,10 @@ module ActiveModelSerializers
         render_object_with_cache(@post)
 
         # Check if the the new comment was cached
-        assert_equal(new_comment_serializer.attributes, cached_serialization(new_comment_serializer))
-        assert_equal(@post_serializer.attributes, cached_serialization(@post_serializer))
+        key = new_comment.cache_key
+        assert_equal(new_comment_serializer.attributes, cache_store.fetch(key))
+        key = @post.cache_key
+        assert_equal(@post_serializer.attributes, cache_store.fetch(key))
       end
     end
 
@@ -163,11 +196,12 @@ module ActiveModelSerializers
 
     def test_uses_file_digest_in_cache_key
       render_object_with_cache(@blog)
-      assert_equal(@blog_serializer.attributes, cache_store.fetch(@blog.cache_key_with_digest))
+      key = "#{@blog.cache_key}/#{::Model::FILE_DIGEST}"
+      assert_equal(@blog_serializer.attributes, cache_store.fetch(key))
     end
 
     def test_cache_digest_definition
-      assert_equal(FILE_DIGEST, @post_serializer.class._cache_digest)
+      assert_equal(::Model::FILE_DIGEST, @post_serializer.class._cache_digest)
     end
 
     def test_object_cache_keys
@@ -179,7 +213,7 @@ module ActiveModelSerializers
       assert_equal actual.size, 3
       assert actual.any? { |key| key == 'comment/1' }
       assert actual.any? { |key| key =~ %r{post/post-\d+} }
-      assert actual.any? { |key| key =~ %r{writer/author-\d+} }
+      assert actual.any? { |key| key =~ %r{author/author-\d+} }
     end
 
     def test_cached_attributes
@@ -196,7 +230,7 @@ module ActiveModelSerializers
         assert_equal cached_attributes[@comment.post.cache_key], Post.new(id: 'post', title: 'New Post', body: 'Body').attributes
 
         writer = @comment.post.blog.writer
-        writer_cache_key = "writer/#{writer.id}-#{writer.updated_at.strftime("%Y%m%d%H%M%S%9N")}"
+        writer_cache_key = writer.cache_key
 
         assert_equal cached_attributes[writer_cache_key], Author.new(id: 'author', name: 'Joao M. D. Moura').attributes
       end

test/fixtures/poro.rb

@@ -1,6 +1,8 @@
 verbose = $VERBOSE
 $VERBOSE = nil
 class Model < ActiveModelSerializers::Model
+  FILE_DIGEST = Digest::MD5.hexdigest(File.open(__FILE__).read)
+
   ### Helper methods, not required to be serializable
 
   # Convenience when not adding @attributes readers and writers
@@ -52,13 +54,7 @@ Post     = Class.new(Model)
 Like     = Class.new(Model)
 Author   = Class.new(Model)
 Bio      = Class.new(Model)
-Blog     = Class.new(Model) do
-  FILE_DIGEST = Digest::MD5.hexdigest(File.open(__FILE__).read)
-
-  def cache_key_with_digest
-    "#{cache_key}/#{FILE_DIGEST}"
-  end
-end
+Blog     = Class.new(Model)
 Role     = Class.new(Model)
 User     = Class.new(Model)
 Location = Class.new(Model)