Fix up caching, especially fragment_cache

jzajpt · Jun 7, 2016 · 35a7c81 · 35a7c81
1 parent d191342
commit 35a7c81
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 74 deletions.
diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb
@@ -167,7 +167,7 @@ def serializable_hash(adapter_options = nil, options = {}, adapter_instance = se
       adapter_options ||= {}
       options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(adapter_options)
       cached_attributes = adapter_options[:cached_attributes] ||= {}
-      resource = cached_attributes(options[:fields], cached_attributes, adapter_instance)
+      resource = fetch_attributes(options[:fields], cached_attributes, adapter_instance)
       relationships = resource_relationships(adapter_options, options, adapter_instance)
       resource.merge(relationships)
     end
@@ -195,6 +195,8 @@ def read_attribute_for_serialization(attr)
       if respond_to?(attr)
         send(attr)
       elsif self.class._fragmented
+        # Attribute method wasn't available on this (fragment cached) serializer,
+        # so read it from the original serializer it was based on.
         self.class._fragmented.read_attribute_for_serialization(attr)
       else
         object.read_attribute_for_serialization(attr)

diff --git a/lib/active_model/serializer/caching.rb b/lib/active_model/serializer/caching.rb
@@ -1,5 +1,3 @@
-# TODO(BF): refactor file to be smaller
-# rubocop:disable Metrics/ModuleLength
 module ActiveModel
   class Serializer
     UndefinedCacheKey = Class.new(StandardError)
@@ -9,10 +7,10 @@ module Caching
       included do
         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 :_fragmented    # @api private : Used ONLY by FragmentedSerializer to reference original serializer
           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_only    # @api private : when fragment caching, whitelists fetch_attributes. Cannot combine with except
+          serializer.class_attribute :_cache_except  # @api private : when fragment caching, blacklists fetch_attributes. Cannot combine with only
           serializer.class_attribute :_cache_options # @api private : used by CachedSerializer, passed to _cache.fetch
           #  _cache_options include:
           #    expires_in
@@ -79,13 +77,6 @@ def non_cached_attributes
           _attributes - cached_attributes
         end
 
-        # @api private
-        # Used by FragmentCache on the CachedSerializer
-        #  to call attribute methods on the fragmented cached serializer.
-        def fragmented(serializer)
-          self._fragmented = serializer
-        end
-
         # Enables a serializer to be automatically cached
         #
         # Sets +::_cache+ object to <tt>ActionController::Base.cache_store</tt>
@@ -208,46 +199,44 @@ def object_cache_key(serializer, adapter_instance)
         end
       end
 
-      def cached_attributes(fields, cached_attributes, adapter_instance)
+      ### INSTANCE METHODS
+      def fetch_attributes(fields, cached_attributes, adapter_instance)
         if self.class.cache_enabled?
           key = cache_key(adapter_instance)
           cached_attributes.fetch(key) do
-            cache_check(adapter_instance) do
+            self.class.cache_store.fetch(key, self.class._cache_options) do
               attributes(fields)
             end
           end
+        elsif self.class.fragment_cache_enabled?
+          fetch_fragment_cache(adapter_instance)
         else
-          cache_check(adapter_instance) do
-            attributes(fields)
-          end
+          attributes(fields)
         end
       end
 
-      def cache_check(adapter_instance)
-        if self.class.cache_enabled?
-          self.class.cache_store.fetch(cache_key(adapter_instance), self.class._cache_options) do
+      def fetch(adapter_instance, cache_options = self.class._cache_options)
+        if self.class.cache_store
+          self.class.cache_store.fetch(cache_key(adapter_instance), cache_options) do
             yield
           end
-        elsif self.class.fragment_cache_enabled?
-          fetch_fragment_cache(adapter_instance)
         else
           yield
         end
       end
 
-      # 1. Create a CachedSerializer and NonCachedSerializer from the serializer class
+      # 1. Create a CachedSerializer from the serializer class
       # 2. Serialize the above two with the given adapter
       # 3. Pass their serializations to the adapter +::fragment_cache+
       #
       # It will split the serializer into two, one that will be cached and one that will not
       #
       # Given a resource name
-      # 1. Dynamically creates a CachedSerializer and NonCachedSerializer
+      # 1. Dynamically creates a CachedSerializer
       #   for a given class 'name'
       # 2. Call
       #       CachedSerializer.cache(serializer._cache_options)
-      #       CachedSerializer.fragmented(serializer)
-      #       NonCachedSerializer.cache(serializer._cache_options)
+      #       CachedSerializer._fragmented = serializer
       # 3. Build a hash keyed to the +cached+ and +non_cached+ serializers
       # 4. Call +cached_attributes+ on the serializer class and the above hash
       # 5. Return the hash
@@ -256,63 +245,54 @@ def cache_check(adapter_instance)
       #   When +name+ is <tt>User::Admin</tt>
       #   creates the Serializer classes (if they don't exist).
       #     CachedUser_AdminSerializer
-      #     NonCachedUser_AdminSerializer
       #
       # Given a hash of its cached and non-cached serializers
       # 1. Determine cached attributes from serializer class options
       # 2. Add cached attributes to cached Serializer
       # 3. Add non-cached attributes to non-cached Serializer
       def fetch_fragment_cache(adapter_instance)
-        serializer_class_name = self.class.name.gsub('::'.freeze, '_'.freeze)
         self.class._cache_options ||= {}
         self.class._cache_options[:key] = self.class._cache_key if self.class._cache_key
 
-        cached_serializer = _get_or_create_fragment_cached_serializer(serializer_class_name)
+        cached_serializer = _get_or_create_fragment_cached_serializer
         cached_hash = ActiveModelSerializers::SerializableResource.new(
           object,
           serializer: cached_serializer,
           adapter: adapter_instance.class
         ).serializable_hash
 
-        non_cached_serializer = _get_or_create_fragment_non_cached_serializer(serializer_class_name)
-        non_cached_hash = ActiveModelSerializers::SerializableResource.new(
-          object,
-          serializer: non_cached_serializer,
-          adapter: adapter_instance.class
-        ).serializable_hash
+        fields = self.class.non_cached_attributes
+        non_cached_hash = attributes(fields, true)
 
         # Merge both results
         adapter_instance.fragment_cache(cached_hash, non_cached_hash)
       end
 
-      def _get_or_create_fragment_cached_serializer(serializer_class_name)
-        cached_serializer = _get_or_create_fragment_serializer "Cached#{serializer_class_name}"
+      def _get_or_create_fragment_cached_serializer
+        serializer_class_name = self.class.name.gsub('::'.freeze, '_'.freeze)
+        cached_serializer_name = "Cached#{serializer_class_name}"
+        if Object.const_defined?(cached_serializer_name)
+          cached_serializer = Object.const_get(cached_serializer_name)
+          # HACK: Test concern in production code :(
+          # But it's better than running all the cached fragment serializer
+          # code multiple times.
+          if Rails.env == 'test'.freeze
+            Object.send(:remove_const, cached_serializer_name)
+            return _get_or_create_fragment_cached_serializer
+          end
+          return cached_serializer
+        end
+        cached_serializer = Object.const_set(cached_serializer_name, Class.new(ActiveModel::Serializer))
         cached_serializer.cache(self.class._cache_options)
         cached_serializer.type(self.class._type)
-        cached_serializer.fragmented(self)
+        cached_serializer._fragmented = self
         self.class.cached_attributes.each do |attribute|
           options = self.class._attributes_keys[attribute] || {}
           cached_serializer.attribute(attribute, options)
         end
         cached_serializer
       end
 
-      def _get_or_create_fragment_non_cached_serializer(serializer_class_name)
-        non_cached_serializer = _get_or_create_fragment_serializer "NonCached#{serializer_class_name}"
-        non_cached_serializer.type(self.class._type)
-        non_cached_serializer.fragmented(self)
-        self.class.non_cached_attributes.each do |attribute|
-          options = self.class._attributes_keys[attribute] || {}
-          non_cached_serializer.attribute(attribute, options)
-        end
-        non_cached_serializer
-      end
-
-      def _get_or_create_fragment_serializer(name)
-        return Object.const_get(name) if Object.const_defined?(name)
-        Object.const_set(name, Class.new(ActiveModel::Serializer))
-      end
-
       def cache_key(adapter_instance)
         return @cache_key if defined?(@cache_key)
 
@@ -339,4 +319,3 @@ def object_cache_key
     end
   end
 end
-# rubocop:enable Metrics/ModuleLength
diff --git a/lib/active_model_serializers/adapter/json_api.rb b/lib/active_model_serializers/adapter/json_api.rb
@@ -294,7 +294,7 @@ def attributes_for(serializer, fields)
 
       # {http://jsonapi.org/format/#document-resource-objects Document Resource Objects}
       def resource_object_for(serializer)
-        resource_object = serializer.cache_check(self) do
+        resource_object = serializer.fetch(self) do
           resource_object = ResourceIdentifier.new(serializer, instance_options).as_json
 
           requested_fields = fieldset && fieldset.fields_for(resource_object[:type])

diff --git a/test/adapter/json_api/resource_identifier_test.rb b/test/adapter/json_api/resource_identifier_test.rb
@@ -42,7 +42,7 @@ def test_id_defined_on_serializer
         end
 
         def test_id_defined_on_fragmented
-          FragmentedSerializer.fragmented(WithDefinedIdSerializer.new(@model))
+          FragmentedSerializer._fragmented = WithDefinedIdSerializer.new(@model)
           test_id(FragmentedSerializer, 'special_id')
         end
 

diff --git a/test/cache_test.rb b/test/cache_test.rb
@@ -204,7 +204,7 @@ def test_uses_adapter_in_cache_key
 
     # Based on original failing test by @kevintyll
     # rubocop:disable Metrics/AbcSize
-    def test_a_serializer_rendered_by_two_adapter_returns_differently_cached_attributes
+    def test_a_serializer_rendered_by_two_adapter_returns_differently_fetch_attributes
       Object.const_set(:Alert, Class.new(ActiveModelSerializers::Model) do
         attr_accessor :id, :status, :resource, :started_at, :ended_at, :updated_at, :created_at
       end)
@@ -225,7 +225,7 @@ def test_a_serializer_rendered_by_two_adapter_returns_differently_cached_attribu
         created_at: Time.new(2016, 3, 31, 21, 37, 35, 0)
       )
 
-      expected_cached_attributes = {
+      expected_fetch_attributes = {
         id: 1,
         status: 'fail',
         resource: 'resource-1',
@@ -250,7 +250,7 @@ def test_a_serializer_rendered_by_two_adapter_returns_differently_cached_attribu
       # Assert attributes are serialized correctly
       serializable_alert = serializable(alert, serializer: AlertSerializer, adapter: :attributes)
       attributes_serialization = serializable_alert.as_json
-      assert_equal expected_cached_attributes, alert.attributes
+      assert_equal expected_fetch_attributes, alert.attributes
       assert_equal alert.attributes, attributes_serialization
       attributes_cache_key = serializable_alert.adapter.serializer.cache_key(serializable_alert.adapter)
       assert_equal attributes_serialization, cache_store.fetch(attributes_cache_key)
@@ -296,23 +296,28 @@ def test_object_cache_keys
       assert actual.any? { |key| key =~ %r{author/author-\d+} }
     end
 
-    def test_cached_attributes
-      serializer = ActiveModel::Serializer::CollectionSerializer.new([@comment, @comment])
+    def test_fetch_attributes_from_cache
+      serializers = ActiveModel::Serializer::CollectionSerializer.new([@comment, @comment])
 
       Timecop.freeze(Time.current) do
         render_object_with_cache(@comment)
 
-        attributes = Adapter::Attributes.new(serializer)
+        options = {}
+        adapter_options = {}
+        adapter_instance = ActiveModelSerializers::Adapter::Attributes.new(serializers, adapter_options)
+        serializers.serializable_hash(adapter_options, options, adapter_instance)
+        cached_attributes = adapter_options.fetch(:cached_attributes)
+
         include_directive = ActiveModelSerializers.default_include_directive
-        cached_attributes = ActiveModel::Serializer.cache_read_multi(serializer, attributes, include_directive)
+        manual_cached_attributes = ActiveModel::Serializer.cache_read_multi(serializers, adapter_instance, include_directive)
+        assert_equal manual_cached_attributes, cached_attributes
 
-        assert_equal cached_attributes["#{@comment.cache_key}/#{attributes.cache_key}"], Comment.new(id: 1, body: 'ZOMG A COMMENT').attributes
-        assert_equal cached_attributes["#{@comment.post.cache_key}/#{attributes.cache_key}"], Post.new(id: 'post', title: 'New Post', body: 'Body').attributes
+        assert_equal cached_attributes["#{@comment.cache_key}/#{adapter_instance.cache_key}"], Comment.new(id: 1, body: 'ZOMG A COMMENT').attributes
+        assert_equal cached_attributes["#{@comment.post.cache_key}/#{adapter_instance.cache_key}"], Post.new(id: 'post', title: 'New Post', body: 'Body').attributes
 
         writer = @comment.post.blog.writer
         writer_cache_key = writer.cache_key
-
-        assert_equal cached_attributes["#{writer_cache_key}/#{attributes.cache_key}"], Author.new(id: 'author', name: 'Joao M. D. Moura').attributes
+        assert_equal cached_attributes["#{writer_cache_key}/#{adapter_instance.cache_key}"], Author.new(id: 'author', name: 'Joao M. D. Moura').attributes
       end
     end
 
@@ -442,6 +447,9 @@ def test_fragment_fetch_with_virtual_attributes
         name: @role.name
       }
       assert_equal(@role_hash, expected_result)
+    ensure
+      fragmented_serializer = @role_serializer
+      Object.send(:remove_const, fragmented_serializer._get_or_create_fragment_cached_serializer.name)
     end
 
     def test_fragment_fetch_with_namespaced_object
@@ -452,6 +460,9 @@ def test_fragment_fetch_with_namespaced_object
         id: @spam.id
       }
       assert_equal(@spam_hash, expected_result)
+    ensure
+      fragmented_serializer = @spam_serializer
+      Object.send(:remove_const, fragmented_serializer._get_or_create_fragment_cached_serializer.name)
     end
 
     private
@@ -476,10 +487,5 @@ def render_object_with_cache(obj, options = {})
     def adapter
       @serializable_resource.adapter
     end
-
-    def cached_serialization(serializer)
-      cache_key = serializer.cache_key(adapter)
-      cache_store.fetch(cache_key)
-    end
   end
 end