Merge pull request #1781 from bf4/serializer_cleanup_6

Fix up caching, especially fragment_cache

Author: bf4

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
@@ -194,8 +194,6 @@ def json_key
     def read_attribute_for_serialization(attr)
       if respond_to?(attr)
         send(attr)
-      elsif self.class._fragmented
-        self.class._fragmented.read_attribute_for_serialization(attr)
       else
         object.read_attribute_for_serialization(attr)
       end

lib/active_model/serializer/adapter/base.rb

@@ -7,9 +7,11 @@ class << self
           deprecate :inherited, 'ActiveModelSerializers::Adapter::Base.'
         end
 
+        # :nocov:
         def initialize(serializer, options = {})
           super(ActiveModelSerializers::Adapter::Base.new(serializer, options))
         end
+        # :nocov:
       end
     end
   end

lib/active_model/serializer/belongs_to_reflection.rb

@@ -2,9 +2,6 @@ module ActiveModel
   class Serializer
     # @api private
     class BelongsToReflection < SingularReflection
-      def macro
-        :belongs_to
-      end
     end
   end
 end

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,9 @@ 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 :_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
@@ -21,7 +18,7 @@ module Caching
           #    race_condition_ttl
           #  Passed to ::_cache as
           #    serializer.cache_store.fetch(cache_key, @klass._cache_options)
-          #  Passed as second argument to serializer.cache_store.fetch(cache_key, self.class._cache_options)
+          #  Passed as second argument to serializer.cache_store.fetch(cache_key, serializer_class._cache_options)
           serializer.class_attribute :_cache_digest_file_path # @api private : Derived at inheritance
         end
       end
@@ -71,19 +68,15 @@ def _skip_digest?
           _cache_options && _cache_options[:skip_digest]
         end
 
-        def cached_attributes
-          _cache_only ? _cache_only : _attributes - _cache_except
-        end
-
-        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
+        def fragmented_attributes
+          cached = _cache_only ? _cache_only : _attributes - _cache_except
+          cached = cached.map! { |field| _attributes_keys.fetch(field, field) }
+          non_cached = _attributes - cached
+          non_cached = non_cached.map! { |field| _attributes_keys.fetch(field, field) }
+          {
+            cached: cached,
+            non_cached: non_cached
+          }
         end
 
         # Enables a serializer to be automatically cached
@@ -208,118 +201,65 @@ def object_cache_key(serializer, adapter_instance)
         end
       end
 
-      def cached_attributes(fields, cached_attributes, adapter_instance)
-        if self.class.cache_enabled?
+      ### INSTANCE METHODS
+      def fetch_attributes(fields, cached_attributes, adapter_instance)
+        if serializer_class.cache_enabled?
           key = cache_key(adapter_instance)
           cached_attributes.fetch(key) do
-            cache_check(adapter_instance) do
-              attributes(fields)
+            serializer_class.cache_store.fetch(key, serializer_class._cache_options) do
+              attributes(fields, true)
             end
           end
+        elsif serializer_class.fragment_cache_enabled?
+          fetch_attributes_fragment(adapter_instance)
         else
-          cache_check(adapter_instance) do
-            attributes(fields)
-          end
+          attributes(fields, true)
         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 = serializer_class._cache_options)
+        if serializer_class.cache_store
+          serializer_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
-      # 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
-      #   for a given class 'name'
-      # 2. Call
-      #       CachedSerializer.cache(serializer._cache_options)
-      #       CachedSerializer.fragmented(serializer)
-      #       NonCachedSerializer.cache(serializer._cache_options)
-      # 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
-      #
-      # @example
-      #   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_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
+      # 1. Determine cached fields from serializer class options
+      # 2. Get non_cached_fields and fetch cache_fields
+      # 3. Merge the two hashes using adapter_instance#fragment_cache
+      def fetch_attributes_fragment(adapter_instance)
+        serializer_class._cache_options ||= {}
+        serializer_class._cache_options[:key] = serializer_class._cache_key if serializer_class._cache_key
+        fields = serializer_class.fragmented_attributes
+
+        non_cached_fields = fields[:non_cached].dup
+        non_cached_hash = attributes(non_cached_fields, true)
+        include_directive = JSONAPI::IncludeDirective.new(non_cached_fields - non_cached_hash.keys)
+        non_cached_hash.merge! resource_relationships({}, { include_directive: include_directive }, adapter_instance)
+
+        cached_fields = fields[:cached].dup
+        key = cache_key(adapter_instance)
+        cached_hash =
+          serializer_class.cache_store.fetch(key, serializer_class._cache_options) do
+            hash = attributes(cached_fields, true)
+            include_directive = JSONAPI::IncludeDirective.new(cached_fields - hash.keys)
+            hash.merge! resource_relationships({}, { include_directive: include_directive }, adapter_instance)
+          end
 
         # 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}"
-        cached_serializer.cache(self.class._cache_options)
-        cached_serializer.type(self.class._type)
-        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)
 
         parts = []
         parts << object_cache_key
         parts << adapter_instance.cache_key
-        parts << self.class._cache_digest unless self.class._skip_digest?
+        parts << serializer_class._cache_digest unless serializer_class._skip_digest?
         @cache_key = parts.join('/')
       end
 
@@ -328,15 +268,18 @@ def cache_key(adapter_instance)
       def object_cache_key
         if object.respond_to?(:cache_key)
           object.cache_key
-        elsif (serializer_cache_key = (self.class._cache_key || self.class._cache_options[:key]))
+        elsif (serializer_cache_key = (serializer_class._cache_key || serializer_class._cache_options[:key]))
           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)
           "#{serializer_cache_key}/#{object.id}-#{object_time_safe}"
         else
-          fail UndefinedCacheKey, "#{object.class} must define #cache_key, or the 'key:' option must be passed into '#{self.class}.cache'"
+          fail UndefinedCacheKey, "#{object.class} must define #cache_key, or the 'key:' option must be passed into '#{serializer_class}.cache'"
         end
       end
+
+      def serializer_class
+        @serializer_class ||= self.class
+      end
     end
   end
 end
-# rubocop:enable Metrics/ModuleLength

lib/active_model/serializer/has_many_reflection.rb

@@ -2,9 +2,6 @@ module ActiveModel
   class Serializer
     # @api private
     class HasManyReflection < CollectionReflection
-      def macro
-        :has_many
-      end
     end
   end
 end

lib/active_model/serializer/has_one_reflection.rb

@@ -2,9 +2,6 @@ module ActiveModel
   class Serializer
     # @api private
     class HasOneReflection < SingularReflection
-      def macro
-        :has_one
-      end
     end
   end
 end

lib/active_model_serializers/adapter.rb

@@ -5,11 +5,13 @@ module Adapter
     private_constant :ADAPTER_MAP if defined?(private_constant)
 
     class << self # All methods are class functions
+      # :nocov:
       def new(*args)
         fail ArgumentError, 'Adapters inherit from Adapter::Base.' \
           "Adapter.new called with args: '#{args.inspect}', from" \
           "'caller[0]'."
       end
+      # :nocov:
 
       def configured_adapter
         lookup(ActiveModelSerializers.config.adapter)

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])

lib/active_model_serializers/deserialization.rb

@@ -6,8 +6,10 @@ def jsonapi_parse(*args)
       Adapter::JsonApi::Deserialization.parse(*args)
     end
 
+    # :nocov:
     def jsonapi_parse!(*args)
       Adapter::JsonApi::Deserialization.parse!(*args)
     end
+    # :nocov:
   end
 end

lib/active_model_serializers/model.rb

@@ -38,12 +38,14 @@ def read_attribute_for_serialization(key)
     end
 
     # The following methods are needed to be minimally implemented for ActiveModel::Errors
+    # :nocov:
     def self.human_attribute_name(attr, _options = {})
       attr
     end
 
     def self.lookup_ancestors
       [self]
     end
+    # :nocov:
   end
 end

lib/active_model_serializers/railtie.rb

@@ -32,11 +32,13 @@ class Railtie < Rails::Railtie
       end
     end
 
+    # :nocov:
     generators do |app|
       Rails::Generators.configure!(app.config.generators)
       Rails::Generators.hidden_namespaces.uniq!
       require 'generators/rails/resource_override'
     end
+    # :nocov:
 
     if Rails.env.test?
       ActionController::TestCase.send(:include, ActiveModelSerializers::Test::Schema)

test/action_controller/explicit_serializer_test.rb

@@ -123,7 +123,7 @@ def test_render_using_explicit_each_serializer
               id: 42,
               lat: '-23.550520',
               lng: '-46.633309',
-              place: 'Nowhere' # is a virtual attribute on LocationSerializer
+              address: 'Nowhere' # is a virtual attribute on LocationSerializer
             }
           ]
         }

test/adapter/json_api/resource_identifier_test.rb

@@ -14,7 +14,13 @@ def id
           end
         end
 
-        class FragmentedSerializer < ActiveModel::Serializer; end
+        class FragmentedSerializer < ActiveModel::Serializer
+          cache only: :id
+
+          def id
+            'special_id'
+          end
+        end
 
         setup do
           @model = Author.new(id: 1, name: 'Steve K.')
@@ -42,7 +48,6 @@ def test_id_defined_on_serializer
         end
 
         def test_id_defined_on_fragmented
-          FragmentedSerializer.fragmented(WithDefinedIdSerializer.new(@model))
           test_id(FragmentedSerializer, 'special_id')
         end