From 3ba4a8c9b2fe1658c2cf18d644f034e0ece4fb27 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 14:17:06 -0500 Subject: [PATCH 01/14] Always return an enumerator --- lib/active_model/serializer.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index 6e1d4bfe..acb23b71 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -332,10 +332,9 @@ module ActiveModel # @param [JSONAPI::IncludeDirective] include_directive (defaults to the # +default_include_directive+ config value when not provided) # @return [Enumerator] - # def associations(include_directive = ActiveModelSerializers.default_include_directive, include_slice = nil) include_slice ||= include_directive - return unless object + return Enumerator.new unless object Enumerator.new do |y| self.class._reflections.values.each do |reflection| From 43c3c231ef8bb5d33c132e0e5f3a75018a99efe9 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 14:17:59 -0500 Subject: [PATCH 02/14] Use reflection key since we have it --- lib/active_model/serializer.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index acb23b71..c7664258 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -337,9 +337,8 @@ module ActiveModel return Enumerator.new unless object Enumerator.new do |y| - self.class._reflections.values.each do |reflection| + self.class._reflections.each do |key, reflection| next if reflection.excluded?(self) - key = reflection.options.fetch(:key, reflection.name) next unless include_directive.key?(key) y.yield reflection.build_association(self, instance_options, include_slice) From ba2aa1fdfdaf2998fbfb0208c9246c82583a8c3e Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 14:18:09 -0500 Subject: [PATCH 03/14] Remove dead comments --- lib/active_model/serializer.rb | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index c7664258..5de2ae24 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -349,31 +349,6 @@ module ActiveModel # @return [Hash] containing the attributes and first level # associations, similar to how ActiveModel::Serializers::JSON is used # in ActiveRecord::Base. - # - # TODO: Include ActiveModel::Serializers::JSON. - # So that the below is true: - # @param options [nil, Hash] The same valid options passed to `serializable_hash` - # (:only, :except, :methods, and :include). - # - # See - # https://github.com/rails/rails/blob/v5.0.0.beta2/activemodel/lib/active_model/serializers/json.rb#L17-L101 - # https://github.com/rails/rails/blob/v5.0.0.beta2/activemodel/lib/active_model/serialization.rb#L85-L123 - # https://github.com/rails/rails/blob/v5.0.0.beta2/activerecord/lib/active_record/serialization.rb#L11-L17 - # https://github.com/rails/rails/blob/v5.0.0.beta2/activesupport/lib/active_support/core_ext/object/json.rb#L147-L162 - # - # @example - # # The :only and :except options can be used to limit the attributes included, and work - # # similar to the attributes method. - # serializer.as_json(only: [:id, :name]) - # serializer.as_json(except: [:id, :created_at, :age]) - # - # # To include the result of some method calls on the model use :methods: - # serializer.as_json(methods: :permalink) - # - # # To include associations use :include: - # serializer.as_json(include: :posts) - # # Second level and higher order associations work as well: - # serializer.as_json(include: { posts: { include: { comments: { only: :body } }, only: :title } }) def serializable_hash(adapter_options = nil, options = {}, adapter_instance = self.class.serialization_adapter_instance) adapter_options ||= {} options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(adapter_options) @@ -385,13 +360,6 @@ module ActiveModel alias to_h serializable_hash # @see #serializable_hash - # TODO: When moving attributes adapter logic here, @see #serializable_hash - # So that the below is true: - # @param options [nil, Hash] The same valid options passed to `as_json` - # (:root, :only, :except, :methods, and :include). - # The default for `root` is nil. - # The default value for include_root is false. You can change it to true if the given - # JSON string includes a single root node. def as_json(adapter_opts = nil) serializable_hash(adapter_opts) end From cb16457bb35695c004c8e2324321666f3fbe02c7 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 14:18:30 -0500 Subject: [PATCH 04/14] Make reflection explicitly dependents on association --- lib/active_model/serializer/reflection.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index 96cca0be..3e476484 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -1,4 +1,5 @@ require 'active_model/serializer/field' +require 'active_model/serializer/association' module ActiveModel class Serializer From fad4ef1046fd77e5d6cbe4ce72978ed08352838f Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 14:19:04 -0500 Subject: [PATCH 05/14] Refactor reflection building of association --- lib/active_model/serializer/reflection.rb | 140 ++++++++++++++-------- test/serializers/reflection_test.rb | 12 +- 2 files changed, 98 insertions(+), 54 deletions(-) diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index 3e476484..d0ab2847 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -47,6 +47,8 @@ module ActiveModel # # So you can inspect reflections in your Adapters. class Reflection < Field + REFLECTION_OPTIONS = %i(key links polymorphic meta serializer virtual_value namespace).freeze + def initialize(*) super options[:links] = {} @@ -71,8 +73,8 @@ module ActiveModel # meta ids: ids # end # end - def link(name, value = nil, &block) - options[:links][name] = block || value + def link(name, value = nil) + options[:links][name] = block_given? ? Proc.new : value :nil end @@ -86,8 +88,8 @@ module ActiveModel # href object.blog.id.to_s # meta(id: object.blog.id) # end - def meta(value = nil, &block) - options[:meta] = block || value + def meta(value = nil) + options[:meta] = block_given? ? Proc.new : value :nil end @@ -119,23 +121,6 @@ module ActiveModel :nil end - # @param serializer [ActiveModel::Serializer] - # @yield [ActiveModel::Serializer] - # @return [:nil, associated resource or resource collection] - def value(serializer, include_slice) - @object = serializer.object - @scope = serializer.scope - - block_value = instance_exec(serializer, &block) if block - return unless include_data?(include_slice) - - if block && block_value != :nil - block_value - else - serializer.read_attribute_for_serialization(name) - end - end - # Build association. This method is used internally to # build serializer's association by its reflection. # @@ -157,35 +142,31 @@ module ActiveModel # # @api private def build_association(parent_serializer, parent_serializer_options, include_slice = {}) - reflection_options = options.dup + reflection_options = settings.merge(include_data: include_data?(include_slice)) unless block? + association_options = build_association_options(parent_serializer, parent_serializer_options[:namespace], include_slice) + association_value = association_options[:association_value] + serializer_class = association_options[:association_serializer] - # Pass the parent's namespace onto the child serializer - reflection_options[:namespace] ||= parent_serializer_options[:namespace] - - association_value = value(parent_serializer, include_slice) - serializer_class = parent_serializer.class.serializer_for(association_value, reflection_options) - reflection_options[:include_data] = include_data?(include_slice) - reflection_options[:links] = options[:links] - reflection_options[:meta] = options[:meta] + reflection_options ||= settings.merge(include_data: include_data?(include_slice)) # Needs to be after association_value is evaluated unless reflection.block.nil? if serializer_class - serializer = catch(:no_serializer) do - serializer_class.new( - association_value, - serializer_options(parent_serializer, parent_serializer_options, reflection_options) - ) - end - if serializer.nil? - reflection_options[:virtual_value] = association_value.try(:as_json) || association_value - else + if (serializer = build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class)) reflection_options[:serializer] = serializer + else + # BUG: per #2027, JSON API resource relationships are only id and type, and hence either + # *require* a serializer or we need to be a little clever about figuring out the id/type. + # In either case, returning the raw virtual value will almost always be incorrect. + # + # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do + # with an object that is non-nil and has no defined serializer. + reflection_options[:virtual_value] = association_value.try(:as_json) || association_value end elsif !association_value.nil? && !association_value.instance_of?(Object) reflection_options[:virtual_value] = association_value end - block = nil - Association.new(name, reflection_options, block) + association_block = nil + Association.new(name, reflection_options, association_block) end protected @@ -193,7 +174,34 @@ module ActiveModel # used in instance exec attr_accessor :object, :scope - private + def settings + options.dup.reject { |k, _| !REFLECTION_OPTIONS.include?(k) } + end + + # Evaluation of the reflection.block will mutate options. + # So, the settings cannot be used until the block is evaluated. + # This means that each time the block is evaluated, it may set a new + # value in the reflection instance. This is not thread-safe. + # @example + # has_many :likes do + # meta liked: object.likes.any? + # include_data: object.loaded? + # end + def block? + !block.nil? + end + + def serializer? + options.key?(:serializer) + end + + def serializer + options[:serializer] + end + + def namespace + options[:namespace] + end def include_data?(include_slice) include_data_setting = options[:include_data_setting] @@ -205,13 +213,49 @@ module ActiveModel end end - def serializer_options(parent_serializer, parent_serializer_options, reflection_options) - serializer = reflection_options.fetch(:serializer, nil) + # @param serializer [ActiveModel::Serializer] + # @yield [ActiveModel::Serializer] + # @return [:nil, associated resource or resource collection] + def value(serializer, include_slice) + @object = serializer.object + @scope = serializer.scope - serializer_options = parent_serializer_options.except(:serializer) - serializer_options[:serializer] = serializer if serializer - serializer_options[:serializer_context_class] = parent_serializer.class - serializer_options + block_value = instance_exec(serializer, &block) if block + return unless include_data?(include_slice) + + if block && block_value != :nil + block_value + else + serializer.read_attribute_for_serialization(name) + end + end + + def build_association_options(parent_serializer, parent_serializer_namespace_option, include_slice) + serializer_for_options = { + # Pass the parent's namespace onto the child serializer + namespace: namespace || parent_serializer_namespace_option + } + serializer_for_options[:serializer] = serializer if serializer? + association_value = value(parent_serializer, include_slice) + { + association_value: association_value, + association_serializer: parent_serializer.class.serializer_for(association_value, serializer_for_options) + } + end + + # NOTE(BF): This serializer throw/catch should only happen when the serializer is a collection + # serializer. This is a good reason for the reflection to have a to_many? or collection? type method. + # + # @return [ActiveModel::Serializer, nil] + def build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) + catch(:no_serializer) do + # Make all the parent serializer instance options available to associations + # except ActiveModelSerializers-specific ones we don't want. + serializer_options = parent_serializer_options.except(:serializer) + serializer_options[:serializer_context_class] = parent_serializer.class + serializer_options[:serializer] = serializer if serializer + serializer_class.new(association_value, serializer_options) + end end end end diff --git a/test/serializers/reflection_test.rb b/test/serializers/reflection_test.rb index e5932aba..4cff40a9 100644 --- a/test/serializers/reflection_test.rb +++ b/test/serializers/reflection_test.rb @@ -57,7 +57,7 @@ module ActiveModel assert_equal true, reflection.options.fetch(:include_data_setting) include_slice = :does_not_matter - assert_equal @model.blog, reflection.value(serializer_instance, include_slice) + assert_equal @model.blog, reflection.send(:value, serializer_instance, include_slice) end def test_reflection_value_block @@ -77,7 +77,7 @@ module ActiveModel assert_equal true, reflection.options.fetch(:include_data_setting) include_slice = :does_not_matter - assert_equal @model.blog, reflection.value(serializer_instance, include_slice) + assert_equal @model.blog, reflection.send(:value, serializer_instance, include_slice) end def test_reflection_value_block_with_explicit_include_data_true @@ -98,7 +98,7 @@ module ActiveModel assert_equal true, reflection.options.fetch(:include_data_setting) include_slice = :does_not_matter - assert_equal @model.blog, reflection.value(serializer_instance, include_slice) + assert_equal @model.blog, reflection.send(:value, serializer_instance, include_slice) end def test_reflection_value_block_with_include_data_false_mutates_the_reflection_include_data @@ -117,7 +117,7 @@ module ActiveModel assert_respond_to reflection.block, :call assert_equal true, reflection.options.fetch(:include_data_setting) include_slice = :does_not_matter - assert_nil reflection.value(serializer_instance, include_slice) + assert_nil reflection.send(:value, serializer_instance, include_slice) assert_equal false, reflection.options.fetch(:include_data_setting) end @@ -137,7 +137,7 @@ module ActiveModel assert_respond_to reflection.block, :call assert_equal true, reflection.options.fetch(:include_data_setting) include_slice = {} - assert_nil reflection.value(serializer_instance, include_slice) + assert_nil reflection.send(:value, serializer_instance, include_slice) assert_equal :if_sideloaded, reflection.options.fetch(:include_data_setting) end @@ -157,7 +157,7 @@ module ActiveModel assert_respond_to reflection.block, :call assert_equal true, reflection.options.fetch(:include_data_setting) include_slice = { blog: :does_not_matter } - assert_equal @model.blog, reflection.value(serializer_instance, include_slice) + assert_equal @model.blog, reflection.send(:value, serializer_instance, include_slice) assert_equal :if_sideloaded, reflection.options.fetch(:include_data_setting) end From 1bddd9fdb5342e346102e08d93439e3d8d9a1509 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 14:47:55 -0500 Subject: [PATCH 06/14] Refactor --- .../serializer/has_many_reflection.rb | 3 ++ lib/active_model/serializer/reflection.rb | 32 ++++++++++++------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/lib/active_model/serializer/has_many_reflection.rb b/lib/active_model/serializer/has_many_reflection.rb index 60ccc481..aab67a4f 100644 --- a/lib/active_model/serializer/has_many_reflection.rb +++ b/lib/active_model/serializer/has_many_reflection.rb @@ -2,6 +2,9 @@ module ActiveModel class Serializer # @api private class HasManyReflection < CollectionReflection + def to_many? + true + end end end end diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index d0ab2847..12131073 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -121,6 +121,10 @@ module ActiveModel :nil end + def to_many? + false + end + # Build association. This method is used internally to # build serializer's association by its reflection. # @@ -150,17 +154,9 @@ module ActiveModel reflection_options ||= settings.merge(include_data: include_data?(include_slice)) # Needs to be after association_value is evaluated unless reflection.block.nil? if serializer_class - if (serializer = build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class)) - reflection_options[:serializer] = serializer - else - # BUG: per #2027, JSON API resource relationships are only id and type, and hence either - # *require* a serializer or we need to be a little clever about figuring out the id/type. - # In either case, returning the raw virtual value will almost always be incorrect. - # - # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do - # with an object that is non-nil and has no defined serializer. - reflection_options[:virtual_value] = association_value.try(:as_json) || association_value - end + reflection_options.merge!( + serialize_association_value!(association_value, serializer_class, parent_serializer, parent_serializer_options) + ) elsif !association_value.nil? && !association_value.instance_of?(Object) reflection_options[:virtual_value] = association_value end @@ -230,6 +226,20 @@ module ActiveModel end end + def serialize_association_value!(association_value, serializer_class, parent_serializer, parent_serializer_options) + if (serializer = build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class)) + { serializer: serializer } + else + # BUG: per #2027, JSON API resource relationships are only id and type, and hence either + # *require* a serializer or we need to be a little clever about figuring out the id/type. + # In either case, returning the raw virtual value will almost always be incorrect. + # + # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do + # with an object that is non-nil and has no defined serializer. + { virtual_value: association_value.try(:as_json) || association_value } + end + end + def build_association_options(parent_serializer, parent_serializer_namespace_option, include_slice) serializer_for_options = { # Pass the parent's namespace onto the child serializer From 079b3d68410973a1b8d28cb80bb81538191d5c91 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 14:53:45 -0500 Subject: [PATCH 07/14] Refactor collection reflection --- lib/active_model/serializer/reflection.rb | 43 ++++++++++++++--------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index 12131073..1728347f 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -227,16 +227,20 @@ module ActiveModel end def serialize_association_value!(association_value, serializer_class, parent_serializer, parent_serializer_options) - if (serializer = build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class)) - { serializer: serializer } + if to_many? + if (serializer = build_association_collection_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class)) + { serializer: serializer } + else + # BUG: per #2027, JSON API resource relationships are only id and type, and hence either + # *require* a serializer or we need to be a little clever about figuring out the id/type. + # In either case, returning the raw virtual value will almost always be incorrect. + # + # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do + # with an object that is non-nil and has no defined serializer. + { virtual_value: association_value.try(:as_json) || association_value } + end else - # BUG: per #2027, JSON API resource relationships are only id and type, and hence either - # *require* a serializer or we need to be a little clever about figuring out the id/type. - # In either case, returning the raw virtual value will almost always be incorrect. - # - # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do - # with an object that is non-nil and has no defined serializer. - { virtual_value: association_value.try(:as_json) || association_value } + { serializer: build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) } end end @@ -254,19 +258,24 @@ module ActiveModel end # NOTE(BF): This serializer throw/catch should only happen when the serializer is a collection - # serializer. This is a good reason for the reflection to have a to_many? or collection? type method. + # serializer. # # @return [ActiveModel::Serializer, nil] - def build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) + def build_association_collection_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) catch(:no_serializer) do - # Make all the parent serializer instance options available to associations - # except ActiveModelSerializers-specific ones we don't want. - serializer_options = parent_serializer_options.except(:serializer) - serializer_options[:serializer_context_class] = parent_serializer.class - serializer_options[:serializer] = serializer if serializer - serializer_class.new(association_value, serializer_options) + build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) end end + + # @return [ActiveModel::Serializer, nil] + def build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) + # Make all the parent serializer instance options available to associations + # except ActiveModelSerializers-specific ones we don't want. + serializer_options = parent_serializer_options.except(:serializer) + serializer_options[:serializer_context_class] = parent_serializer.class + serializer_options[:serializer] = serializer if serializer + serializer_class.new(association_value, serializer_options) + end end end end From ee69293c8fb72e58d980324aab43387d819a00f2 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 15:01:58 -0500 Subject: [PATCH 08/14] Refactor reflection building serializer class --- lib/active_model/serializer/reflection.rb | 44 ++++++++++------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index 1728347f..c9c965b1 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -147,16 +147,24 @@ module ActiveModel # @api private def build_association(parent_serializer, parent_serializer_options, include_slice = {}) reflection_options = settings.merge(include_data: include_data?(include_slice)) unless block? - association_options = build_association_options(parent_serializer, parent_serializer_options[:namespace], include_slice) - association_value = association_options[:association_value] - serializer_class = association_options[:association_serializer] + + association_value = value(parent_serializer, include_slice) + serializer_class = build_serializer_class(association_value, parent_serializer, parent_serializer_options[:namespace]) reflection_options ||= settings.merge(include_data: include_data?(include_slice)) # Needs to be after association_value is evaluated unless reflection.block.nil? if serializer_class - reflection_options.merge!( - serialize_association_value!(association_value, serializer_class, parent_serializer, parent_serializer_options) - ) + if (serializer = build_serializer!(association_value, serializer_class, parent_serializer, parent_serializer_options)) + reflection_options[:serializer] = serializer + else + # BUG: per #2027, JSON API resource relationships are only id and type, and hence either + # *require* a serializer or we need to be a little clever about figuring out the id/type. + # In either case, returning the raw virtual value will almost always be incorrect. + # + # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do + # with an object that is non-nil and has no defined serializer. + reflection_options[:virtual_value] = association_value.try(:as_json) || association_value + end elsif !association_value.nil? && !association_value.instance_of?(Object) reflection_options[:virtual_value] = association_value end @@ -226,35 +234,21 @@ module ActiveModel end end - def serialize_association_value!(association_value, serializer_class, parent_serializer, parent_serializer_options) + def build_serializer!(association_value, serializer_class, parent_serializer, parent_serializer_options) if to_many? - if (serializer = build_association_collection_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class)) - { serializer: serializer } - else - # BUG: per #2027, JSON API resource relationships are only id and type, and hence either - # *require* a serializer or we need to be a little clever about figuring out the id/type. - # In either case, returning the raw virtual value will almost always be incorrect. - # - # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do - # with an object that is non-nil and has no defined serializer. - { virtual_value: association_value.try(:as_json) || association_value } - end + build_association_collection_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) else - { serializer: build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) } + build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) end end - def build_association_options(parent_serializer, parent_serializer_namespace_option, include_slice) + def build_serializer_class(association_value, parent_serializer, parent_serializer_namespace_option) serializer_for_options = { # Pass the parent's namespace onto the child serializer namespace: namespace || parent_serializer_namespace_option } serializer_for_options[:serializer] = serializer if serializer? - association_value = value(parent_serializer, include_slice) - { - association_value: association_value, - association_serializer: parent_serializer.class.serializer_for(association_value, serializer_for_options) - } + parent_serializer.class.serializer_for(association_value, serializer_for_options) end # NOTE(BF): This serializer throw/catch should only happen when the serializer is a collection From 7d8fb1606b96ca5252044720536f00b0fefbbc15 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 17:42:30 -0500 Subject: [PATCH 09/14] Cleanup --- lib/active_model/serializer.rb | 13 ++++++------- lib/active_model/serializer/reflection.rb | 14 +++++++------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index 5de2ae24..b50cb951 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -341,7 +341,8 @@ module ActiveModel next if reflection.excluded?(self) next unless include_directive.key?(key) - y.yield reflection.build_association(self, instance_options, include_slice) + association = reflection.build_association(self, instance_options, include_slice) + y.yield association end end end @@ -390,14 +391,12 @@ module ActiveModel # @api private def associations_hash(adapter_options, options, adapter_instance) - relationships = {} include_directive = options.fetch(:include_directive) - associations(include_directive).each do |association| - adapter_opts = adapter_options.merge(include_directive: include_directive[association.key]) - relationships[association.key] ||= association.serializable_hash(adapter_opts, adapter_instance) + include_slice = options[:include_slice] + associations(include_directive, include_slice).each_with_object({}) do |association, relationships| + adapter_opts = adapter_options.merge(include_directive: include_directive[association.key], adapter_instance: adapter_instance) + relationships[association.key] = association.serializable_hash(adapter_opts, adapter_instance) end - - relationships end protected diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index c9c965b1..d4f96390 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -37,13 +37,13 @@ module ActiveModel # 1) as 'comments' and named 'comments'. # 2) as 'object.comments.last(1)' and named 'last_comments'. # - # PostSerializer._reflections #=> - # # [ - # # HasOneReflection.new(:author, serializer: AuthorSerializer), - # # HasManyReflection.new(:comments) - # # HasManyReflection.new(:comments, { key: :last_comments }, #) - # # HasManyReflection.new(:secret_meta_data, { if: :is_admin? }) - # # ] + # PostSerializer._reflections # => + # # { + # # author: HasOneReflection.new(:author, serializer: AuthorSerializer), + # # comments: HasManyReflection.new(:comments) + # # last_comments: HasManyReflection.new(:comments, { key: :last_comments }, #) + # # secret_meta_data: HasManyReflection.new(:secret_meta_data, { if: :is_admin? }) + # # } # # So you can inspect reflections in your Adapters. class Reflection < Field From 34d55e47291e3352b629039ed4f8ee4c9753aed0 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 17:43:43 -0500 Subject: [PATCH 10/14] Remove extra reflection classes --- lib/active_model/serializer/belongs_to_reflection.rb | 2 +- lib/active_model/serializer/collection_reflection.rb | 7 ------- lib/active_model/serializer/has_many_reflection.rb | 4 ++-- lib/active_model/serializer/has_one_reflection.rb | 2 +- lib/active_model/serializer/reflection.rb | 4 ++-- lib/active_model/serializer/singular_reflection.rb | 7 ------- 6 files changed, 6 insertions(+), 20 deletions(-) delete mode 100644 lib/active_model/serializer/collection_reflection.rb delete mode 100644 lib/active_model/serializer/singular_reflection.rb diff --git a/lib/active_model/serializer/belongs_to_reflection.rb b/lib/active_model/serializer/belongs_to_reflection.rb index a014b7a5..67dbe79a 100644 --- a/lib/active_model/serializer/belongs_to_reflection.rb +++ b/lib/active_model/serializer/belongs_to_reflection.rb @@ -1,7 +1,7 @@ module ActiveModel class Serializer # @api private - class BelongsToReflection < SingularReflection + class BelongsToReflection < Reflection end end end diff --git a/lib/active_model/serializer/collection_reflection.rb b/lib/active_model/serializer/collection_reflection.rb deleted file mode 100644 index 3436becf..00000000 --- a/lib/active_model/serializer/collection_reflection.rb +++ /dev/null @@ -1,7 +0,0 @@ -module ActiveModel - class Serializer - # @api private - class CollectionReflection < Reflection - end - end -end diff --git a/lib/active_model/serializer/has_many_reflection.rb b/lib/active_model/serializer/has_many_reflection.rb index aab67a4f..99f6f63c 100644 --- a/lib/active_model/serializer/has_many_reflection.rb +++ b/lib/active_model/serializer/has_many_reflection.rb @@ -1,8 +1,8 @@ module ActiveModel class Serializer # @api private - class HasManyReflection < CollectionReflection - def to_many? + class HasManyReflection < Reflection + def collection? true end end diff --git a/lib/active_model/serializer/has_one_reflection.rb b/lib/active_model/serializer/has_one_reflection.rb index bf41b1d1..a385009b 100644 --- a/lib/active_model/serializer/has_one_reflection.rb +++ b/lib/active_model/serializer/has_one_reflection.rb @@ -1,7 +1,7 @@ module ActiveModel class Serializer # @api private - class HasOneReflection < SingularReflection + class HasOneReflection < Reflection end end end diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index d4f96390..12b7fcaf 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -121,7 +121,7 @@ module ActiveModel :nil end - def to_many? + def collection? false end @@ -235,7 +235,7 @@ module ActiveModel end def build_serializer!(association_value, serializer_class, parent_serializer, parent_serializer_options) - if to_many? + if collection? build_association_collection_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) else build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) diff --git a/lib/active_model/serializer/singular_reflection.rb b/lib/active_model/serializer/singular_reflection.rb deleted file mode 100644 index f90ecc21..00000000 --- a/lib/active_model/serializer/singular_reflection.rb +++ /dev/null @@ -1,7 +0,0 @@ -module ActiveModel - class Serializer - # @api private - class SingularReflection < Reflection - end - end -end From 7697d9f5ec292a483c7ca6adde99fcba1495ff83 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 16:48:05 -0500 Subject: [PATCH 11/14] Refactor: introduce lazy association --- lib/active_model/serializer/association.rb | 37 ++++++++--- .../serializer/concerns/caching.rb | 7 +- .../serializer/lazy_association.rb | 22 +++++++ lib/active_model/serializer/reflection.rb | 6 +- .../adapter/json_api.rb | 2 +- .../adapter/json_api/relationship.rb | 8 +-- test/serializers/associations_test.rb | 65 ++++++++++--------- 7 files changed, 98 insertions(+), 49 deletions(-) create mode 100644 lib/active_model/serializer/lazy_association.rb diff --git a/lib/active_model/serializer/association.rb b/lib/active_model/serializer/association.rb index 459a8186..4be64529 100644 --- a/lib/active_model/serializer/association.rb +++ b/lib/active_model/serializer/association.rb @@ -1,3 +1,5 @@ +require 'active_model/serializer/lazy_association' + module ActiveModel class Serializer # This class holds all information about serializer's association. @@ -10,14 +12,22 @@ module ActiveModel # Association.new(:comments, { serializer: CommentSummarySerializer }) # class Association < Field + attr_reader :lazy_association + delegate :include_data?, :virtual_value, to: :lazy_association + + def initialize(*) + super + @lazy_association = LazyAssociation.new(name, options, block) + end + # @return [Symbol] def key options.fetch(:key, name) end - # @return [ActiveModel::Serializer, nil] - def serializer - options[:serializer] + # @return [True,False] + def key? + options.key?(:key) end # @return [Hash] @@ -30,21 +40,30 @@ module ActiveModel options[:meta] end + def polymorphic? + true == options[:polymorphic] + end + # @api private def serializable_hash(adapter_options, adapter_instance) - return options[:virtual_value] if options[:virtual_value] - object = serializer && serializer.object - return unless object + association_serializer = lazy_association.serializer + return virtual_value if virtual_value + association_object = association_serializer && association_serializer.object + return unless association_object - serialization = serializer.serializable_hash(adapter_options, {}, adapter_instance) + serialization = association_serializer.serializable_hash(adapter_options, {}, adapter_instance) - if options[:polymorphic] && serialization - polymorphic_type = object.class.name.underscore + if polymorphic? && serialization + polymorphic_type = association_object.class.name.underscore serialization = { type: polymorphic_type, polymorphic_type.to_sym => serialization } end serialization end + + private + + delegate :reflection, to: :lazy_association end end end diff --git a/lib/active_model/serializer/concerns/caching.rb b/lib/active_model/serializer/concerns/caching.rb index f4c72468..f633447a 100644 --- a/lib/active_model/serializer/concerns/caching.rb +++ b/lib/active_model/serializer/concerns/caching.rb @@ -193,12 +193,13 @@ module ActiveModel cache_keys << object_cache_key(serializer, adapter_instance) serializer.associations(include_directive).each do |association| - if association.serializer.respond_to?(:each) - association.serializer.each do |sub_serializer| + association_serializer = association.lazy_association.serializer + if association_serializer.respond_to?(:each) + association_serializer.each do |sub_serializer| cache_keys << object_cache_key(sub_serializer, adapter_instance) end else - cache_keys << object_cache_key(association.serializer, adapter_instance) + cache_keys << object_cache_key(association_serializer, adapter_instance) end end end diff --git a/lib/active_model/serializer/lazy_association.rb b/lib/active_model/serializer/lazy_association.rb new file mode 100644 index 00000000..c23e3c15 --- /dev/null +++ b/lib/active_model/serializer/lazy_association.rb @@ -0,0 +1,22 @@ +module ActiveModel + class Serializer + class LazyAssociation < Field + + def serializer + options[:serializer] + end + + def include_data? + options[:include_data] + end + + def virtual_value + options[:virtual_value] + end + + def reflection + options[:reflection] + end + end + end +end diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index 12b7fcaf..32844962 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -170,7 +170,11 @@ module ActiveModel end association_block = nil - Association.new(name, reflection_options, association_block) + reflection_options[:reflection] = self + reflection_options[:parent_serializer] = parent_serializer + reflection_options[:parent_serializer_options] = parent_serializer_options + reflection_options[:include_slice] = include_slice + Association.new(name, reflection_options, block) end protected diff --git a/lib/active_model_serializers/adapter/json_api.rb b/lib/active_model_serializers/adapter/json_api.rb index 3d241e34..0e44ddd2 100644 --- a/lib/active_model_serializers/adapter/json_api.rb +++ b/lib/active_model_serializers/adapter/json_api.rb @@ -257,7 +257,7 @@ module ActiveModelSerializers def process_relationships(serializer, include_slice) serializer.associations(include_slice).each do |association| - process_relationship(association.serializer, include_slice[association.key]) + process_relationship(association.lazy_association.serializer, include_slice[association.key]) end end diff --git a/lib/active_model_serializers/adapter/json_api/relationship.rb b/lib/active_model_serializers/adapter/json_api/relationship.rb index 0d34cf93..e76172b4 100644 --- a/lib/active_model_serializers/adapter/json_api/relationship.rb +++ b/lib/active_model_serializers/adapter/json_api/relationship.rb @@ -15,9 +15,7 @@ module ActiveModelSerializers def as_json hash = {} - if association.options[:include_data] - hash[:data] = data_for(association) - end + hash[:data] = data_for(association) if association.include_data? links = links_for(association) hash[:links] = links if links.any? @@ -36,10 +34,10 @@ module ActiveModelSerializers private def data_for(association) - serializer = association.serializer + serializer = association.lazy_association.serializer if serializer.respond_to?(:each) serializer.map { |s| ResourceIdentifier.new(s, serializable_resource_options).as_json } - elsif (virtual_value = association.options[:virtual_value]) + elsif (virtual_value = association.virtual_value) virtual_value elsif serializer && serializer.object ResourceIdentifier.new(serializer, serializable_resource_options).as_json diff --git a/test/serializers/associations_test.rb b/test/serializers/associations_test.rb index 90d213dc..a76ddd92 100644 --- a/test/serializers/associations_test.rb +++ b/test/serializers/associations_test.rb @@ -30,18 +30,17 @@ module ActiveModel def test_has_many_and_has_one @author_serializer.associations.each do |association| key = association.key - serializer = association.serializer - options = association.options + serializer = association.lazy_association.serializer case key when :posts - assert_equal true, options.fetch(:include_data) + assert_equal true, association.include_data? assert_kind_of(ActiveModelSerializers.config.collection_serializer, serializer) when :bio - assert_equal true, options.fetch(:include_data) + assert_equal true, association.include_data? assert_nil serializer when :roles - assert_equal true, options.fetch(:include_data) + assert_equal true, association.include_data? assert_kind_of(ActiveModelSerializers.config.collection_serializer, serializer) else flunk "Unknown association: #{key}" @@ -56,12 +55,11 @@ module ActiveModel end post_serializer_class.new(@post).associations.each do |association| key = association.key - serializer = association.serializer - options = association.options + serializer = association.lazy_association.serializer assert_equal :tags, key assert_nil serializer - assert_equal [{ id: 'tagid', name: '#hashtagged' }].to_json, options[:virtual_value].to_json + assert_equal [{ id: 'tagid', name: '#hashtagged' }].to_json, association.virtual_value.to_json end end @@ -70,7 +68,7 @@ module ActiveModel .associations .detect { |assoc| assoc.key == :comments } - comment_serializer = association.serializer.first + comment_serializer = association.lazy_association.serializer.first class << comment_serializer def custom_options instance_options @@ -82,7 +80,7 @@ module ActiveModel def test_belongs_to @comment_serializer.associations.each do |association| key = association.key - serializer = association.serializer + serializer = association.lazy_association.serializer case key when :post @@ -93,7 +91,7 @@ module ActiveModel flunk "Unknown association: #{key}" end - assert_equal true, association.options.fetch(:include_data) + assert_equal true, association.include_data? end end @@ -203,11 +201,11 @@ module ActiveModel @post_serializer.associations.each do |association| case association.key when :comments - assert_instance_of(ResourceNamespace::CommentSerializer, association.serializer.first) + assert_instance_of(ResourceNamespace::CommentSerializer, association.lazy_association.serializer.first) when :author - assert_instance_of(ResourceNamespace::AuthorSerializer, association.serializer) + assert_instance_of(ResourceNamespace::AuthorSerializer, association.lazy_association.serializer) when :description - assert_instance_of(ResourceNamespace::DescriptionSerializer, association.serializer) + assert_instance_of(ResourceNamespace::DescriptionSerializer, association.lazy_association.serializer) else flunk "Unknown association: #{key}" end @@ -245,11 +243,11 @@ module ActiveModel @post_serializer.associations.each do |association| case association.key when :comments - assert_instance_of(PostSerializer::CommentSerializer, association.serializer.first) + assert_instance_of(PostSerializer::CommentSerializer, association.lazy_association.serializer.first) when :author - assert_instance_of(PostSerializer::AuthorSerializer, association.serializer) + assert_instance_of(PostSerializer::AuthorSerializer, association.lazy_association.serializer) when :description - assert_instance_of(PostSerializer::DescriptionSerializer, association.serializer) + assert_instance_of(PostSerializer::DescriptionSerializer, association.lazy_association.serializer) else flunk "Unknown association: #{key}" end @@ -260,7 +258,7 @@ module ActiveModel def test_conditional_associations model = Class.new(::Model) do attributes :true, :false - associations :association + associations :something end.new(true: true, false: false) scenarios = [ @@ -284,7 +282,7 @@ module ActiveModel scenarios.each do |s| serializer = Class.new(ActiveModel::Serializer) do - belongs_to :association, s[:options] + belongs_to :something, s[:options] def true true @@ -296,7 +294,7 @@ module ActiveModel end hash = serializable(model, serializer: serializer).serializable_hash - assert_equal(s[:included], hash.key?(:association), "Error with #{s[:options]}") + assert_equal(s[:included], hash.key?(:something), "Error with #{s[:options]}") end end @@ -341,8 +339,8 @@ module ActiveModel @author_serializer = AuthorSerializer.new(@author) @inherited_post_serializer = InheritedPostSerializer.new(@post) @inherited_author_serializer = InheritedAuthorSerializer.new(@author) - @author_associations = @author_serializer.associations.to_a - @inherited_author_associations = @inherited_author_serializer.associations.to_a + @author_associations = @author_serializer.associations.to_a.sort_by(&:name) + @inherited_author_associations = @inherited_author_serializer.associations.to_a.sort_by(&:name) @post_associations = @post_serializer.associations.to_a @inherited_post_associations = @inherited_post_serializer.associations.to_a end @@ -361,28 +359,35 @@ module ActiveModel test 'a serializer inheriting from another serializer can redefine has_many and has_one associations' do expected = [:roles, :bio].sort - result = (@inherited_author_associations - @author_associations).map(&:name).sort + result = (@inherited_author_associations.map(&:reflection) - @author_associations.map(&:reflection)).map(&:name) assert_equal(result, expected) + assert_equal [true, false, true], @inherited_author_associations.map(&:polymorphic?) + assert_equal [false, false, false], @author_associations.map(&:polymorphic?) end test 'a serializer inheriting from another serializer can redefine belongs_to associations' do assert_equal [:author, :comments, :blog], @post_associations.map(&:name) assert_equal [:author, :comments, :blog, :comments], @inherited_post_associations.map(&:name) - refute @post_associations.detect { |assoc| assoc.name == :author }.options.key?(:polymorphic) - assert_equal true, @inherited_post_associations.detect { |assoc| assoc.name == :author }.options.fetch(:polymorphic) + refute @post_associations.detect { |assoc| assoc.name == :author }.polymorphic? + assert @inherited_post_associations.detect { |assoc| assoc.name == :author }.polymorphic? - refute @post_associations.detect { |assoc| assoc.name == :comments }.options.key?(:key) + refute @post_associations.detect { |assoc| assoc.name == :comments }.key? original_comment_assoc, new_comments_assoc = @inherited_post_associations.select { |assoc| assoc.name == :comments } - refute original_comment_assoc.options.key?(:key) - assert_equal :reviews, new_comments_assoc.options.fetch(:key) + refute original_comment_assoc.key? + assert_equal :reviews, new_comments_assoc.key - assert_equal @post_associations.detect { |assoc| assoc.name == :blog }, @inherited_post_associations.detect { |assoc| assoc.name == :blog } + original_blog = @post_associations.detect { |assoc| assoc.name == :blog } + inherited_blog = @inherited_post_associations.detect { |assoc| assoc.name == :blog } + original_parent_serializer = original_blog.lazy_association.options.delete(:parent_serializer) + inherited_parent_serializer = inherited_blog.lazy_association.options.delete(:parent_serializer) + assert_equal PostSerializer, original_parent_serializer.class + assert_equal InheritedPostSerializer, inherited_parent_serializer.class end test 'a serializer inheriting from another serializer can have an additional association with the same name but with different key' do expected = [:author, :comments, :blog, :reviews].sort - result = @inherited_post_serializer.associations.map { |a| a.options.fetch(:key, a.name) }.sort + result = @inherited_post_serializer.associations.map(&:key).sort assert_equal(result, expected) end end From ff5ab21a45beece2ae01fdb5289be9d0bd6174fe Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 23 Apr 2017 18:07:33 -0500 Subject: [PATCH 12/14] Make Association totally lazy --- lib/active_model/serializer/association.rb | 30 ++-- .../serializer/lazy_association.rb | 100 +++++++++++- lib/active_model/serializer/reflection.rb | 151 ++++-------------- .../adapter/json_api/relationship.rb | 29 +++- test/serializers/associations_test.rb | 4 +- test/serializers/reflection_test.rb | 21 ++- 6 files changed, 183 insertions(+), 152 deletions(-) diff --git a/lib/active_model/serializer/association.rb b/lib/active_model/serializer/association.rb index 4be64529..767fd8a2 100644 --- a/lib/active_model/serializer/association.rb +++ b/lib/active_model/serializer/association.rb @@ -4,44 +4,42 @@ module ActiveModel class Serializer # This class holds all information about serializer's association. # - # @attr [Symbol] name - # @attr [Hash{Symbol => Object}] options - # @attr [block] - # - # @example - # Association.new(:comments, { serializer: CommentSummarySerializer }) - # - class Association < Field + # @api private + Association = Struct.new(:reflection, :association_options) do attr_reader :lazy_association - delegate :include_data?, :virtual_value, to: :lazy_association + delegate :object, :include_data?, :virtual_value, :collection?, to: :lazy_association def initialize(*) super - @lazy_association = LazyAssociation.new(name, options, block) + @lazy_association = LazyAssociation.new(reflection, association_options) end + # @return [Symbol] + delegate :name, to: :reflection + # @return [Symbol] def key - options.fetch(:key, name) + reflection_options.fetch(:key, name) end # @return [True,False] def key? - options.key?(:key) + reflection_options.key?(:key) end # @return [Hash] def links - options.fetch(:links) || {} + reflection_options.fetch(:links) || {} end # @return [Hash, nil] + # This gets mutated, so cannot use the cached reflection_options def meta - options[:meta] + reflection.options[:meta] end def polymorphic? - true == options[:polymorphic] + true == reflection_options[:polymorphic] end # @api private @@ -63,7 +61,7 @@ module ActiveModel private - delegate :reflection, to: :lazy_association + delegate :reflection_options, to: :lazy_association end end end diff --git a/lib/active_model/serializer/lazy_association.rb b/lib/active_model/serializer/lazy_association.rb index c23e3c15..1ba40f1b 100644 --- a/lib/active_model/serializer/lazy_association.rb +++ b/lib/active_model/serializer/lazy_association.rb @@ -1,21 +1,107 @@ module ActiveModel class Serializer - class LazyAssociation < Field + # @api private + LazyAssociation = Struct.new(:reflection, :association_options) do + REFLECTION_OPTIONS = %i(key links polymorphic meta serializer virtual_value namespace).freeze - def serializer - options[:serializer] + delegate :collection?, to: :reflection + + def reflection_options + @reflection_options ||= reflection.options.dup.reject { |k, _| !REFLECTION_OPTIONS.include?(k) } end + def object + @object ||= reflection.value( + association_options.fetch(:parent_serializer), + association_options.fetch(:include_slice) + ) + end + alias_method :eval_reflection_block, :object + def include_data? - options[:include_data] + eval_reflection_block if reflection.block + reflection.include_data?( + association_options.fetch(:include_slice) + ) + end + + # @return [ActiveModel::Serializer, nil] + def serializer + return @serializer if defined?(@serializer) + if serializer_class + serialize_object!(object) + elsif !object.nil? && !object.instance_of?(Object) + cached_result[:virtual_value] = object + end + @serializer = cached_result[:serializer] end def virtual_value - options[:virtual_value] + cached_result[:virtual_value] || reflection_options[:virtual_value] end - def reflection - options[:reflection] + # NOTE(BF): Kurko writes: + # 1. This class is doing a lot more than it should. It has business logic (key/meta/links) and + # it also looks like a factory (serializer/serialize_object/instantiate_serializer/serializer_class). + # It's hard to maintain classes that you can understand what it's really meant to be doing, + # so it ends up having all sorts of methods. + # Perhaps we could replace all these methods with a class called... Serializer. + # See how association is doing the job a serializer again? + # 2. I've seen code like this in many other places. + # Perhaps we should just have it all in one place: Serializer. + # We already have a class called Serializer, I know, + # and that is doing things that are not responsibility of a serializer. + def serializer_class + return @serializer_class if defined?(@serializer_class) + serializer_for_options = { namespace: namespace } + serializer_for_options[:serializer] = reflection_options[:serializer] if reflection_options.key?(:serializer) + @serializer_class = association_options.fetch(:parent_serializer).class.serializer_for(object, serializer_for_options) + end + + private + + def cached_result + @cached_result ||= {} + end + + def serialize_object!(object) + if collection? + if (serializer = instantiate_collection_serializer(object)).nil? + # BUG: per #2027, JSON API resource relationships are only id and type, and hence either + # *require* a serializer or we need to be a little clever about figuring out the id/type. + # In either case, returning the raw virtual value will almost always be incorrect. + # + # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do + # with an object that is non-nil and has no defined serializer. + cached_result[:virtual_value] = object.try(:as_json) || object + else + cached_result[:serializer] = serializer + end + else + cached_result[:serializer] = instantiate_serializer(object) + end + end + + # NOTE(BF): This serializer throw/catch should only happen when the serializer is a collection + # serializer. This is a good reason for the reflection to have a to_many? type method. + def instantiate_serializer(object) + serializer_options = association_options.fetch(:parent_serializer_options).except(:serializer) + serializer_options[:serializer_context_class] = association_options.fetch(:parent_serializer).class + serializer = reflection_options.fetch(:serializer, nil) + serializer_options[:serializer] = serializer if serializer + serializer_class.new(object, serializer_options) + end + + def instantiate_collection_serializer(object) + serializer = catch(:no_serializer) do + instantiate_serializer(object) + end + serializer + end + + def namespace + reflection_options[:namespace] || + association_options.fetch(:parent_serializer_options)[:namespace] end end end diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index 32844962..49eb7600 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -47,8 +47,6 @@ module ActiveModel # # So you can inspect reflections in your Adapters. class Reflection < Field - REFLECTION_OPTIONS = %i(key links polymorphic meta serializer virtual_value namespace).freeze - def initialize(*) super options[:links] = {} @@ -125,92 +123,6 @@ module ActiveModel false end - # Build association. This method is used internally to - # build serializer's association by its reflection. - # - # @param [Serializer] parent_serializer for given association - # @param [Hash{Symbol => Object}] parent_serializer_options - # - # @example - # # Given the following serializer defined: - # class PostSerializer < ActiveModel::Serializer - # has_many :comments, serializer: CommentSummarySerializer - # end - # - # # Then you instantiate your serializer - # post_serializer = PostSerializer.new(post, foo: 'bar') # - # # to build association for comments you need to get reflection - # comments_reflection = PostSerializer._reflections.detect { |r| r.name == :comments } - # # and #build_association - # comments_reflection.build_association(post_serializer, foo: 'bar') - # - # @api private - def build_association(parent_serializer, parent_serializer_options, include_slice = {}) - reflection_options = settings.merge(include_data: include_data?(include_slice)) unless block? - - association_value = value(parent_serializer, include_slice) - serializer_class = build_serializer_class(association_value, parent_serializer, parent_serializer_options[:namespace]) - - reflection_options ||= settings.merge(include_data: include_data?(include_slice)) # Needs to be after association_value is evaluated unless reflection.block.nil? - - if serializer_class - if (serializer = build_serializer!(association_value, serializer_class, parent_serializer, parent_serializer_options)) - reflection_options[:serializer] = serializer - else - # BUG: per #2027, JSON API resource relationships are only id and type, and hence either - # *require* a serializer or we need to be a little clever about figuring out the id/type. - # In either case, returning the raw virtual value will almost always be incorrect. - # - # Should be reflection_options[:virtual_value] or adapter needs to figure out what to do - # with an object that is non-nil and has no defined serializer. - reflection_options[:virtual_value] = association_value.try(:as_json) || association_value - end - elsif !association_value.nil? && !association_value.instance_of?(Object) - reflection_options[:virtual_value] = association_value - end - - association_block = nil - reflection_options[:reflection] = self - reflection_options[:parent_serializer] = parent_serializer - reflection_options[:parent_serializer_options] = parent_serializer_options - reflection_options[:include_slice] = include_slice - Association.new(name, reflection_options, block) - end - - protected - - # used in instance exec - attr_accessor :object, :scope - - def settings - options.dup.reject { |k, _| !REFLECTION_OPTIONS.include?(k) } - end - - # Evaluation of the reflection.block will mutate options. - # So, the settings cannot be used until the block is evaluated. - # This means that each time the block is evaluated, it may set a new - # value in the reflection instance. This is not thread-safe. - # @example - # has_many :likes do - # meta liked: object.likes.any? - # include_data: object.loaded? - # end - def block? - !block.nil? - end - - def serializer? - options.key?(:serializer) - end - - def serializer - options[:serializer] - end - - def namespace - options[:namespace] - end - def include_data?(include_slice) include_data_setting = options[:include_data_setting] case include_data_setting @@ -238,42 +150,39 @@ module ActiveModel end end - def build_serializer!(association_value, serializer_class, parent_serializer, parent_serializer_options) - if collection? - build_association_collection_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) - else - build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) - end - end - - def build_serializer_class(association_value, parent_serializer, parent_serializer_namespace_option) - serializer_for_options = { - # Pass the parent's namespace onto the child serializer - namespace: namespace || parent_serializer_namespace_option - } - serializer_for_options[:serializer] = serializer if serializer? - parent_serializer.class.serializer_for(association_value, serializer_for_options) - end - - # NOTE(BF): This serializer throw/catch should only happen when the serializer is a collection - # serializer. + # Build association. This method is used internally to + # build serializer's association by its reflection. # - # @return [ActiveModel::Serializer, nil] - def build_association_collection_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) - catch(:no_serializer) do - build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) - end + # @param [Serializer] parent_serializer for given association + # @param [Hash{Symbol => Object}] parent_serializer_options + # + # @example + # # Given the following serializer defined: + # class PostSerializer < ActiveModel::Serializer + # has_many :comments, serializer: CommentSummarySerializer + # end + # + # # Then you instantiate your serializer + # post_serializer = PostSerializer.new(post, foo: 'bar') # + # # to build association for comments you need to get reflection + # comments_reflection = PostSerializer._reflections.detect { |r| r.name == :comments } + # # and #build_association + # comments_reflection.build_association(post_serializer, foo: 'bar') + # + # @api private + def build_association(parent_serializer, parent_serializer_options, include_slice = {}) + association_options = { + parent_serializer: parent_serializer, + parent_serializer_options: parent_serializer_options, + include_slice: include_slice + } + Association.new(self, association_options) end - # @return [ActiveModel::Serializer, nil] - def build_association_serializer(parent_serializer, parent_serializer_options, association_value, serializer_class) - # Make all the parent serializer instance options available to associations - # except ActiveModelSerializers-specific ones we don't want. - serializer_options = parent_serializer_options.except(:serializer) - serializer_options[:serializer_context_class] = parent_serializer.class - serializer_options[:serializer] = serializer if serializer - serializer_class.new(association_value, serializer_options) - end + protected + + # used in instance exec + attr_accessor :object, :scope end end end diff --git a/lib/active_model_serializers/adapter/json_api/relationship.rb b/lib/active_model_serializers/adapter/json_api/relationship.rb index e76172b4..44c74878 100644 --- a/lib/active_model_serializers/adapter/json_api/relationship.rb +++ b/lib/active_model_serializers/adapter/json_api/relationship.rb @@ -34,13 +34,34 @@ module ActiveModelSerializers private def data_for(association) + if association.collection? + data_for_many(association) + else + data_for_one(association) + end + end + + def data_for_one(association) serializer = association.lazy_association.serializer - if serializer.respond_to?(:each) - serializer.map { |s| ResourceIdentifier.new(s, serializable_resource_options).as_json } + if (virtual_value = association.virtual_value) + virtual_value + elsif serializer && association.object + ResourceIdentifier.new(serializer, serializable_resource_options).as_json + else + nil + end + end + + def data_for_many(association) + collection_serializer = association.lazy_association.serializer + if collection_serializer.respond_to?(:each) + collection_serializer.map do |serializer| + ResourceIdentifier.new(serializer, serializable_resource_options).as_json + end elsif (virtual_value = association.virtual_value) virtual_value - elsif serializer && serializer.object - ResourceIdentifier.new(serializer, serializable_resource_options).as_json + else + [] end end diff --git a/test/serializers/associations_test.rb b/test/serializers/associations_test.rb index a76ddd92..f6603a8d 100644 --- a/test/serializers/associations_test.rb +++ b/test/serializers/associations_test.rb @@ -379,8 +379,8 @@ module ActiveModel original_blog = @post_associations.detect { |assoc| assoc.name == :blog } inherited_blog = @inherited_post_associations.detect { |assoc| assoc.name == :blog } - original_parent_serializer = original_blog.lazy_association.options.delete(:parent_serializer) - inherited_parent_serializer = inherited_blog.lazy_association.options.delete(:parent_serializer) + original_parent_serializer = original_blog.lazy_association.association_options.delete(:parent_serializer) + inherited_parent_serializer = inherited_blog.lazy_association.association_options.delete(:parent_serializer) assert_equal PostSerializer, original_parent_serializer.class assert_equal InheritedPostSerializer, inherited_parent_serializer.class end diff --git a/test/serializers/reflection_test.rb b/test/serializers/reflection_test.rb index 4cff40a9..fba9f354 100644 --- a/test/serializers/reflection_test.rb +++ b/test/serializers/reflection_test.rb @@ -175,6 +175,11 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) + # Assert association links empty when not yet evaluated + assert_equal @empty_links, reflection.options.fetch(:links) + assert_equal @empty_links, association.links + association.object # eager eval association + assert_equal @expected_links, association.links assert_equal @expected_links, reflection.options.fetch(:links) end @@ -195,6 +200,9 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) + # Assert association links empty when not yet evaluated + assert_equal @empty_links, association.links + association.object # eager eval association # Assert before instance_eval link link = association.links.fetch(:self) assert_respond_to link, :call @@ -218,6 +226,7 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) + association.object # eager eval required assert_equal @expected_meta, association.meta assert_equal @expected_meta, reflection.options.fetch(:meta) end @@ -239,6 +248,7 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) # Assert before instance_eval meta + association.object # eager eval required assert_respond_to association.meta, :call assert_respond_to reflection.options.fetch(:meta), :call @@ -271,6 +281,7 @@ module ActiveModel assert_nil association.meta assert_nil reflection.options.fetch(:meta) + association.object # eager eval required link = association.links.fetch(:self) assert_respond_to link, :call assert_respond_to reflection.options.fetch(:links).fetch(:self), :call @@ -279,7 +290,8 @@ module ActiveModel # Assert after instance_eval link assert_equal 'no_uri_validation', reflection.instance_eval(&link) assert_equal @expected_meta, reflection.options.fetch(:meta) - assert_nil association.meta + return # oh no, need to figure this out + assert_nil association.meta # rubocop:disable Lint/UnreachableCode end # rubocop:enable Metrics/AbcSize @@ -307,6 +319,7 @@ module ActiveModel assert_nil reflection.options.fetch(:meta) # Assert before instance_eval link + association.object # eager eval required link = association.links.fetch(:self) assert_nil reflection.options.fetch(:meta) assert_respond_to link, :call @@ -317,7 +330,8 @@ module ActiveModel assert_respond_to association.links.fetch(:self), :call # Assert before instance_eval link meta assert_respond_to reflection.options.fetch(:meta), :call - assert_nil association.meta + return # oh no, need to figure this out + assert_nil association.meta # rubocop:disable Lint/UnreachableCode # Assert after instance_eval link meta assert_equal @expected_meta, reflection.instance_eval(&reflection.options.fetch(:meta)) @@ -342,6 +356,7 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) # Assert before instance_eval link + association.object # eager eval required link = association.links.fetch(:self) assert_respond_to link, :call @@ -365,6 +380,7 @@ module ActiveModel reflection = serializer_class._reflections.fetch(:blog) assert_nil reflection.options.fetch(:meta) association = reflection.build_association(serializer_instance, @instance_options) + association.object # eager eval required assert_equal model1_meta, association.meta assert_equal model1_meta, reflection.options.fetch(:meta) @@ -380,6 +396,7 @@ module ActiveModel assert_equal model1_meta, reflection.options.fetch(:meta) association = reflection.build_association(serializer_instance, @instance_options) + association.object # eager eval required assert_equal model2_meta, association.meta assert_equal model2_meta, reflection.options.fetch(:meta) end From 5e01a93fc09685e1ca1d838aebba9ae9df6d3da0 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 30 Apr 2017 15:09:18 -0500 Subject: [PATCH 13/14] Update comments regarding lazy_association and TODOs --- lib/active_model/serializer/concerns/caching.rb | 1 + lib/active_model/serializer/lazy_association.rb | 13 ------------- lib/active_model_serializers/adapter/json_api.rb | 1 + .../adapter/json_api/relationship.rb | 3 +++ 4 files changed, 5 insertions(+), 13 deletions(-) diff --git a/lib/active_model/serializer/concerns/caching.rb b/lib/active_model/serializer/concerns/caching.rb index f633447a..1de86246 100644 --- a/lib/active_model/serializer/concerns/caching.rb +++ b/lib/active_model/serializer/concerns/caching.rb @@ -193,6 +193,7 @@ module ActiveModel cache_keys << object_cache_key(serializer, adapter_instance) serializer.associations(include_directive).each do |association| + # TODO(BF): Process relationship without evaluating lazy_association association_serializer = association.lazy_association.serializer if association_serializer.respond_to?(:each) association_serializer.each do |sub_serializer| diff --git a/lib/active_model/serializer/lazy_association.rb b/lib/active_model/serializer/lazy_association.rb index 1ba40f1b..8c4dad61 100644 --- a/lib/active_model/serializer/lazy_association.rb +++ b/lib/active_model/serializer/lazy_association.rb @@ -40,17 +40,6 @@ module ActiveModel cached_result[:virtual_value] || reflection_options[:virtual_value] end - # NOTE(BF): Kurko writes: - # 1. This class is doing a lot more than it should. It has business logic (key/meta/links) and - # it also looks like a factory (serializer/serialize_object/instantiate_serializer/serializer_class). - # It's hard to maintain classes that you can understand what it's really meant to be doing, - # so it ends up having all sorts of methods. - # Perhaps we could replace all these methods with a class called... Serializer. - # See how association is doing the job a serializer again? - # 2. I've seen code like this in many other places. - # Perhaps we should just have it all in one place: Serializer. - # We already have a class called Serializer, I know, - # and that is doing things that are not responsibility of a serializer. def serializer_class return @serializer_class if defined?(@serializer_class) serializer_for_options = { namespace: namespace } @@ -82,8 +71,6 @@ module ActiveModel end end - # NOTE(BF): This serializer throw/catch should only happen when the serializer is a collection - # serializer. This is a good reason for the reflection to have a to_many? type method. def instantiate_serializer(object) serializer_options = association_options.fetch(:parent_serializer_options).except(:serializer) serializer_options[:serializer_context_class] = association_options.fetch(:parent_serializer).class diff --git a/lib/active_model_serializers/adapter/json_api.rb b/lib/active_model_serializers/adapter/json_api.rb index 0e44ddd2..c252deaf 100644 --- a/lib/active_model_serializers/adapter/json_api.rb +++ b/lib/active_model_serializers/adapter/json_api.rb @@ -257,6 +257,7 @@ module ActiveModelSerializers def process_relationships(serializer, include_slice) serializer.associations(include_slice).each do |association| + # TODO(BF): Process relationship without evaluating lazy_association process_relationship(association.lazy_association.serializer, include_slice[association.key]) end end diff --git a/lib/active_model_serializers/adapter/json_api/relationship.rb b/lib/active_model_serializers/adapter/json_api/relationship.rb index 44c74878..fc2f116f 100644 --- a/lib/active_model_serializers/adapter/json_api/relationship.rb +++ b/lib/active_model_serializers/adapter/json_api/relationship.rb @@ -33,6 +33,7 @@ module ActiveModelSerializers private + # TODO(BF): Avoid db hit on belong_to_ releationship by using foreign_key on self def data_for(association) if association.collection? data_for_many(association) @@ -42,6 +43,7 @@ module ActiveModelSerializers end def data_for_one(association) + # TODO(BF): Process relationship without evaluating lazy_association serializer = association.lazy_association.serializer if (virtual_value = association.virtual_value) virtual_value @@ -53,6 +55,7 @@ module ActiveModelSerializers end def data_for_many(association) + # TODO(BF): Process relationship without evaluating lazy_association collection_serializer = association.lazy_association.serializer if collection_serializer.respond_to?(:each) collection_serializer.map do |serializer| From 876190440f18a0bb7004a5e401b5e5516adf36fc Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 30 Apr 2017 16:39:25 -0500 Subject: [PATCH 14/14] Update reflection tests --- test/serializers/reflection_test.rb | 49 ++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/test/serializers/reflection_test.rb b/test/serializers/reflection_test.rb index fba9f354..11cb154b 100644 --- a/test/serializers/reflection_test.rb +++ b/test/serializers/reflection_test.rb @@ -25,6 +25,10 @@ module ActiveModel @instance_options = {} end + def evaluate_association_value(association) + association.lazy_association.eval_reflection_block + end + # TODO: Remaining tests # test_reflection_value_block_with_scope # test_reflection_value_uses_serializer_instance_method @@ -175,10 +179,12 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) + # Assert association links empty when not yet evaluated assert_equal @empty_links, reflection.options.fetch(:links) assert_equal @empty_links, association.links - association.object # eager eval association + + evaluate_association_value(association) assert_equal @expected_links, association.links assert_equal @expected_links, reflection.options.fetch(:links) @@ -200,12 +206,16 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) + # Assert association links empty when not yet evaluated assert_equal @empty_links, association.links - association.object # eager eval association + + evaluate_association_value(association) + # Assert before instance_eval link link = association.links.fetch(:self) assert_respond_to link, :call + assert_respond_to reflection.options.fetch(:links).fetch(:self), :call # Assert after instance_eval link assert_equal @expected_links.fetch(:self), reflection.instance_eval(&link) @@ -226,7 +236,9 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) - association.object # eager eval required + + evaluate_association_value(association) + assert_equal @expected_meta, association.meta assert_equal @expected_meta, reflection.options.fetch(:meta) end @@ -248,7 +260,9 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) # Assert before instance_eval meta - association.object # eager eval required + + evaluate_association_value(association) + assert_respond_to association.meta, :call assert_respond_to reflection.options.fetch(:meta), :call @@ -281,7 +295,8 @@ module ActiveModel assert_nil association.meta assert_nil reflection.options.fetch(:meta) - association.object # eager eval required + evaluate_association_value(association) + link = association.links.fetch(:self) assert_respond_to link, :call assert_respond_to reflection.options.fetch(:links).fetch(:self), :call @@ -290,8 +305,7 @@ module ActiveModel # Assert after instance_eval link assert_equal 'no_uri_validation', reflection.instance_eval(&link) assert_equal @expected_meta, reflection.options.fetch(:meta) - return # oh no, need to figure this out - assert_nil association.meta # rubocop:disable Lint/UnreachableCode + assert_equal @expected_meta, association.meta end # rubocop:enable Metrics/AbcSize @@ -319,7 +333,9 @@ module ActiveModel assert_nil reflection.options.fetch(:meta) # Assert before instance_eval link - association.object # eager eval required + + evaluate_association_value(association) + link = association.links.fetch(:self) assert_nil reflection.options.fetch(:meta) assert_respond_to link, :call @@ -330,12 +346,11 @@ module ActiveModel assert_respond_to association.links.fetch(:self), :call # Assert before instance_eval link meta assert_respond_to reflection.options.fetch(:meta), :call - return # oh no, need to figure this out - assert_nil association.meta # rubocop:disable Lint/UnreachableCode + assert_respond_to association.meta, :call # Assert after instance_eval link meta assert_equal @expected_meta, reflection.instance_eval(&reflection.options.fetch(:meta)) - assert_nil association.meta + assert_respond_to association.meta, :call end # rubocop:enable Metrics/AbcSize @@ -356,7 +371,9 @@ module ActiveModel # Build Association association = reflection.build_association(serializer_instance, @instance_options) # Assert before instance_eval link - association.object # eager eval required + + evaluate_association_value(association) + link = association.links.fetch(:self) assert_respond_to link, :call @@ -380,7 +397,9 @@ module ActiveModel reflection = serializer_class._reflections.fetch(:blog) assert_nil reflection.options.fetch(:meta) association = reflection.build_association(serializer_instance, @instance_options) - association.object # eager eval required + + evaluate_association_value(association) + assert_equal model1_meta, association.meta assert_equal model1_meta, reflection.options.fetch(:meta) @@ -396,7 +415,9 @@ module ActiveModel assert_equal model1_meta, reflection.options.fetch(:meta) association = reflection.build_association(serializer_instance, @instance_options) - association.object # eager eval required + + evaluate_association_value(association) + assert_equal model2_meta, association.meta assert_equal model2_meta, reflection.options.fetch(:meta) end