From 20e394d5123c58b241b29c84efddeb77445ecb1c Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Wed, 31 Aug 2016 08:35:41 -0500 Subject: [PATCH] Refactor Association into Field like everything else (#1897) * Make assocations asserts easier to understand * Refactor Association into Field like everything else * Make assocation serializer/links/meta lazier * Push association deeper into relationship * Simplify association usage in relationships * Better naming of reflection parent serializer * Easier to read association method --- lib/active_model/serializer/association.rb | 21 ++++++-- .../serializer/concerns/associations.rb | 4 +- lib/active_model/serializer/reflection.rb | 21 ++++---- .../adapter/json_api/relationship.rb | 48 +++++++++++-------- test/adapter/json_api/relationship_test.rb | 12 +++-- test/serializers/associations_test.rb | 35 +++++++++++--- 6 files changed, 94 insertions(+), 47 deletions(-) diff --git a/lib/active_model/serializer/association.rb b/lib/active_model/serializer/association.rb index 02ac7606..d7bb5e89 100644 --- a/lib/active_model/serializer/association.rb +++ b/lib/active_model/serializer/association.rb @@ -3,17 +3,32 @@ module ActiveModel # This class hold all information about serializer's association. # # @attr [Symbol] name - # @attr [ActiveModel::Serializer] serializer # @attr [Hash{Symbol => Object}] options + # @attr [block] # # @example - # Association.new(:comments, CommentSummarySerializer) + # Association.new(:comments, { serializer: CommentSummarySerializer }) # - Association = Struct.new(:name, :serializer, :options, :links, :meta) do + class Association < Field # @return [Symbol] def key options.fetch(:key, name) end + + # @return [ActiveModel::Serializer, nil] + def serializer + options[:serializer] + end + + # @return [Hash] + def links + options.fetch(:links) || {} + end + + # @return [Hash, nil] + def meta + options[:meta] + end end end end diff --git a/lib/active_model/serializer/concerns/associations.rb b/lib/active_model/serializer/concerns/associations.rb index c5c6f8b3..c27dfeb8 100644 --- a/lib/active_model/serializer/concerns/associations.rb +++ b/lib/active_model/serializer/concerns/associations.rb @@ -74,8 +74,8 @@ module ActiveModel # @api private # def associate(reflection) - key = reflection.options[:key] - key ? self._reflections[key] = reflection : self._reflections[reflection.name] = reflection + key = reflection.options[:key] || reflection.name + self._reflections[key] = reflection end end diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index fbc421f7..a64a849a 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -88,7 +88,7 @@ module ActiveModel # Build association. This method is used internally to # build serializer's association by its reflection. # - # @param [Serializer] subject is a parent serializer for given association + # @param [Serializer] parent_serializer for given association # @param [Hash{Symbol => Object}] parent_serializer_options # # @example @@ -106,17 +106,19 @@ module ActiveModel # # @api private # - def build_association(subject, parent_serializer_options) - association_value = value(subject) + def build_association(parent_serializer, parent_serializer_options) + association_value = value(parent_serializer) reflection_options = options.dup - serializer_class = subject.class.serializer_for(association_value, reflection_options) + serializer_class = parent_serializer.class.serializer_for(association_value, reflection_options) reflection_options[:include_data] = @_include_data + reflection_options[:links] = @_links + reflection_options[:meta] = @_meta if serializer_class begin - serializer = serializer_class.new( + reflection_options[:serializer] = serializer_class.new( association_value, - serializer_options(subject, parent_serializer_options, reflection_options) + serializer_options(parent_serializer, parent_serializer_options, reflection_options) ) rescue ActiveModel::Serializer::CollectionSerializer::NoSerializerError reflection_options[:virtual_value] = association_value.try(:as_json) || association_value @@ -125,7 +127,8 @@ module ActiveModel reflection_options[:virtual_value] = association_value end - Association.new(name, serializer, reflection_options, @_links, @_meta) + block = nil + Association.new(name, reflection_options, block) end protected @@ -134,12 +137,12 @@ module ActiveModel private - def serializer_options(subject, parent_serializer_options, reflection_options) + def serializer_options(parent_serializer, parent_serializer_options, reflection_options) serializer = reflection_options.fetch(:serializer, nil) serializer_options = parent_serializer_options.except(:serializer) serializer_options[:serializer] = serializer if serializer - serializer_options[:serializer_context_class] = subject.class + serializer_options[:serializer_context_class] = parent_serializer.class serializer_options 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 7c597314..8cff36ef 100644 --- a/lib/active_model_serializers/adapter/json_api/relationship.rb +++ b/lib/active_model_serializers/adapter/json_api/relationship.rb @@ -7,28 +7,22 @@ module ActiveModelSerializers # {http://jsonapi.org/format/#document-resource-object-linkage Document Resource Relationship Linkage} # {http://jsonapi.org/format/#document-meta Document Meta} def initialize(parent_serializer, serializable_resource_options, association) - serializer = association.serializer - options = association.options - links = association.links - meta = association.meta - @object = parent_serializer.object - @scope = parent_serializer.scope - @association_options = options || {} + @parent_serializer = parent_serializer + @association = association @serializable_resource_options = serializable_resource_options - @data = data_for(serializer) - @links = (links || {}).each_with_object({}) do |(key, value), hash| - result = Link.new(parent_serializer, value).as_json - hash[key] = result if result - end - @meta = meta.respond_to?(:call) ? parent_serializer.instance_eval(&meta) : meta end def as_json hash = {} - hash[:data] = data if association_options[:include_data] - links = self.links + + if association.options[:include_data] + hash[:data] = data_for(association) + end + + links = links_for(association) hash[:links] = links if links.any? - meta = self.meta + + meta = meta_for(association) hash[:meta] = meta if meta hash @@ -36,20 +30,32 @@ module ActiveModelSerializers protected - attr_reader :object, :scope, :data, :serializable_resource_options, - :association_options, :links, :meta + attr_reader :parent_serializer, :serializable_resource_options, :association private - def data_for(serializer) + def data_for(association) + serializer = association.serializer if serializer.respond_to?(:each) serializer.map { |s| ResourceIdentifier.new(s, serializable_resource_options).as_json } - elsif association_options[:virtual_value] - association_options[:virtual_value] + elsif (virtual_value = association.options[:virtual_value]) + virtual_value elsif serializer && serializer.object ResourceIdentifier.new(serializer, serializable_resource_options).as_json end end + + def links_for(association) + association.links.each_with_object({}) do |(key, value), hash| + result = Link.new(parent_serializer, value).as_json + hash[key] = result if result + end + end + + def meta_for(association) + meta = association.meta + meta.respond_to?(:call) ? parent_serializer.instance_eval(&meta) : meta + end end end end diff --git a/test/adapter/json_api/relationship_test.rb b/test/adapter/json_api/relationship_test.rb index 8b443df6..5e4d016e 100644 --- a/test/adapter/json_api/relationship_test.rb +++ b/test/adapter/json_api/relationship_test.rb @@ -155,15 +155,17 @@ module ActiveModelSerializers serializable_resource_options = {} # adapter.instance_options - meta = test_options.delete(:meta) - options = test_options.delete(:options) - links = test_options.delete(:links) + options = test_options.delete(:options) || {} + options[:links] = test_options.delete(:links) + options[:meta] = test_options.delete(:meta) association_serializer = @serializer if association_serializer && association_serializer.object association_name = association_serializer.json_key.to_sym - association = ::ActiveModel::Serializer::Association.new(association_name, association_serializer, options, links, meta) + options[:serializer] = association_serializer + association = ::ActiveModel::Serializer::Association.new(association_name, options, nil) else - association = ::ActiveModel::Serializer::Association.new(:association_name_not_used, association, options, links, meta) + options[:serializer] = association + association = ::ActiveModel::Serializer::Association.new(:association_name_not_used, options, nil) end relationship = Relationship.new(parent_serializer, serializable_resource_options, association) diff --git a/test/serializers/associations_test.rb b/test/serializers/associations_test.rb index 59d957be..ee470385 100644 --- a/test/serializers/associations_test.rb +++ b/test/serializers/associations_test.rb @@ -31,13 +31,13 @@ module ActiveModel case key when :posts - assert_equal({ include_data: true }, options) + assert_equal true, options.fetch(:include_data) assert_kind_of(ActiveModelSerializers.config.collection_serializer, serializer) when :bio - assert_equal({ include_data: true }, options) + assert_equal true, options.fetch(:include_data) assert_nil serializer when :roles - assert_equal({ include_data: true }, options) + assert_equal true, options.fetch(:include_data) assert_kind_of(ActiveModelSerializers.config.collection_serializer, serializer) else flunk "Unknown association: #{key}" @@ -79,7 +79,7 @@ module ActiveModel flunk "Unknown association: #{key}" end - assert_equal({ include_data: true }, association.options) + assert_equal true, association.options.fetch(:include_data) end end @@ -291,11 +291,23 @@ module ActiveModel end class InheritedSerializerTest < ActiveSupport::TestCase + class PostSerializer < ActiveModel::Serializer + belongs_to :author + has_many :comments + belongs_to :blog + end + class InheritedPostSerializer < PostSerializer belongs_to :author, polymorphic: true has_many :comments, key: :reviews end + class AuthorSerializer < ActiveModel::Serializer + has_many :posts + has_many :roles + has_one :bio + end + class InheritedAuthorSerializer < AuthorSerializer has_many :roles, polymorphic: true has_one :bio, polymorphic: true @@ -333,9 +345,18 @@ module ActiveModel end test 'a serializer inheriting from another serializer can redefine belongs_to associations' do - expected = [:author, :comments, :blog].sort - result = (@inherited_post_associations - @post_associations).map(&:name).sort - assert_equal(result, expected) + 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 == :comments }.options.key?(: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) + + assert_equal @post_associations.detect { |assoc| assoc.name == :blog }, @inherited_post_associations.detect { |assoc| assoc.name == :blog } end test 'a serializer inheriting from another serializer can have an additional association with the same name but with different key' do