Prefer object.cache_key when available.

rails-api · Apr 1, 2016 · 9cb0841 · 9cb0841
1 parent e6b056a
commit 9cb0841
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 70 deletions.
diff --git a/CHANGELOG.md b/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)

diff --git a/lib/active_model/serializer/caching.rb b/lib/active_model/serializer/caching.rb
@@ -58,10 +58,6 @@ def digest_caller_file(caller_line)
           ''.freeze
         end
 
-        def _skip_digest?
-          _cache_options && _cache_options[:skip_digest]
-        end
-
         # @api private
         # Used by FragmentCache on the CachedSerializer
         #  to call attribute methods on the fragmented cached serializer.
@@ -146,18 +142,6 @@ def fragment_cache_enabled?
             (_cache_only && !_cache_except || !_cache_only && _cache_except)
         end
       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 cache_key
-        if object.respond_to?(:cache_key)
-          object.cache_key
-        else
-          object_time_safe = object.updated_at
-          object_time_safe = object_time_safe.strftime('%Y%m%d%H%M%S%9N') if object_time_safe.respond_to?(:strftime)
-          "#{self.class._cache_key}/#{object.id}-#{object_time_safe}"
-        end
-      end
     end
   end
 end
diff --git a/lib/active_model_serializers/adapter/base.rb b/lib/active_model_serializers/adapter/base.rb
@@ -8,21 +8,13 @@ def self.inherited(subclass)
         ActiveModelSerializers::Adapter.register(subclass)
       end
 
-      def self.name
-        to_s.demodulize
-      end
-
       attr_reader :serializer, :instance_options
 
       def initialize(serializer, options = {})
         @serializer = serializer
         @instance_options = options
       end
 
-      def name
-        self.class.name
-      end
-
       def serializable_hash(_options = nil)
         fail NotImplementedError, 'This is an abstract method. Should be implemented at the concrete adapter.'
       end

diff --git a/lib/active_model_serializers/cached_serializer.rb b/lib/active_model_serializers/cached_serializer.rb
@@ -4,13 +4,12 @@ class CachedSerializer
 
     def initialize(serializer)
       @cached_serializer = serializer
-      return unless cached? && !@cached_serializer.object.respond_to?(:cache_key) && @klass._cache_key.blank?
-      fail(UndefinedCacheKey, "#{@cached_serializer.object} must define #cache_key, or the cache_key option must be passed into cache on #{@cached_serializer}")
+      @klass             = @cached_serializer.class
     end
 
     def cache_check(adapter_instance)
       if cached?
-        @klass._cache.fetch(cache_key(adapter_instance), @klass._cache_options) do
+        @klass._cache.fetch(cache_key, @klass._cache_options) do
           yield
         end
       elsif fragment_cached?
@@ -28,16 +27,29 @@ def fragment_cached?
       @klass.fragment_cache_enabled?
     end
 
-    def cache_key(adapter_instance)
+    def cache_key
       return @cache_key if defined?(@cache_key)
 
       parts = []
-      parts << @cached_serializer.cache_key
-      parts << adapter_instance.name.underscore
-      parts << @klass._cache_digest unless @klass._skip_digest?
+      parts << object_cache_key
+      parts << @klass._cache_digest unless @klass._cache_options && @klass._cache_options[:skip_digest]
       @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)
+        "#{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
     # @param collection_serializer
     # @param include_tree

diff --git a/test/cache_test.rb b/test/cache_test.rb
@@ -96,26 +96,27 @@ def test_cache_key_definition
       assert_equal(nil, @comment_serializer.class._cache_key)
     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')
-      assert_raises ActiveModel::Serializer::Adapter::CachedSerializer::UndefinedCacheKey do
+      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_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 = cache_key_with_adapter("#{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, ActionController::Base.cache_store.fetch(key).to_json)
+      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, ActionController::Base.cache_store.fetch(cache_key_with_adapter(@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_cache_options_definition
@@ -137,8 +138,10 @@ def test_associations_separately_cache
       Timecop.freeze(Time.current) do
         render_object_with_cache(@post)
 
-        assert_equal(@post_serializer.attributes, ActionController::Base.cache_store.fetch(cache_key_with_adapter(@post.cache_key)))
-        assert_equal(@comment_serializer.attributes, ActionController::Base.cache_store.fetch(cache_key_with_adapter(@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
 
@@ -148,9 +151,10 @@ def test_associations_cache_when_updated
         render_object_with_cache(@post)
 
         # Check if it cached the objects separately
-        assert_equal(@post_serializer.attributes, ActionController::Base.cache_store.fetch(cache_key_with_adapter(@post.cache_key)))
-        assert_equal(@comment_serializer.attributes, ActionController::Base.cache_store.fetch(cache_key_with_adapter(@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))
 
         # Simulating update on comments relationship with Post
         new_comment            = Comment.new(id: 2567, body: 'ZOMG A NEW COMMENT')
@@ -161,8 +165,10 @@ def test_associations_cache_when_updated
         render_object_with_cache(@post)
 
         # Check if the the new comment was cached
-        assert_equal(new_comment_serializer.attributes, ActionController::Base.cache_store.fetch(cache_key_with_adapter(new_comment.cache_key)))
-        assert_equal(@post_serializer.attributes, ActionController::Base.cache_store.fetch(cache_key_with_adapter(@post.cache_key)))
+        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
 
@@ -177,7 +183,7 @@ def test_fragment_fetch_with_virtual_associations
       hash = render_object_with_cache(@location)
 
       assert_equal(hash, expected_result)
-      assert_equal({ place: 'Nowhere' }, ActionController::Base.cache_store.fetch(cache_key_with_adapter(@location.cache_key)))
+      assert_equal({ place: 'Nowhere' }, cache_store.fetch(@location.cache_key))
     end
 
     def test_fragment_cache_with_inheritance
@@ -188,18 +194,14 @@ def test_fragment_cache_with_inheritance
       refute_includes(base.keys, :special_attribute)
     end
 
-    def test_uses_adapter_in_cache_key
-      render_object_with_cache(@post)
-      assert_equal(@post_serializer.attributes, ActionController::Base.cache_store.fetch("#{@post.cache_key}/#{adapter.class.to_s.demodulize.underscore}"))
-    end
-
     def test_uses_file_digest_in_cache_key
       render_object_with_cache(@blog)
-      assert_equal(@blog_serializer.attributes, ActionController::Base.cache_store.fetch("#{cache_key_with_adapter(@blog.cache_key)}/#{@blog.class::FILE_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
@@ -211,7 +213,7 @@ def test_object_cache_keys
       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
@@ -228,7 +230,7 @@ def test_cached_attributes
         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
@@ -286,16 +288,7 @@ def test_warn_on_serializer_not_defined_in_file
     private
 
     def render_object_with_cache(obj, options = {})
-      @serializable_resource = serializable(obj, options).serializable_hash
-      @serializable_resource.serializable_hash
-    end
-
-    def adapter
-      @serializable_resource.adapter
-    end
-
-    def cache_key_with_adapter(key)
-      "#{key}/#{adapter.name.underscore}"
+      serializable(obj, options).serializable_hash
     end
 
     def cache_store

diff --git a/test/fixtures/poro.rb b/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 @@ class ProfilePreviewSerializer < ActiveModel::Serializer
 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 digest
-    FILE_DIGEST
-  end
-end
+Blog     = Class.new(Model)
 Role     = Class.new(Model)
 User     = Class.new(Model)
 Location = Class.new(Model)