Skip to content

Optional fields in associations (priority on serializer) #1628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Optional fields in associations (priority on serializer) #1628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/active_model/serializer/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ def config.array_serializer
end

config.adapter = :attributes
config.attributes_adapter_include_default = '*'
config.jsonapi_resource_type = :plural
config.jsonapi_version = '1.0'
config.jsonapi_toplevel_meta = {}
Expand Down
6 changes: 5 additions & 1 deletion lib/active_model/serializer/reflection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ def build_association(subject, parent_serializer_options)
begin
serializer = serializer_class.new(
association_value,
serializer_options(subject, parent_serializer_options, reflection_options)
serializer_options(
subject,
parent_serializer_options,
reflection_options.merge(options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing we should rename this variable to field_options so it's clear where it's coming from. It took me a moment.

)
)
rescue ActiveModel::Serializer::CollectionSerializer::NoSerializerError
reflection_options[:virtual_value] = association_value.try(:as_json) || association_value
Expand Down
24 changes: 21 additions & 3 deletions lib/active_model_serializers/adapter/attributes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ module Adapter
class Attributes < Base
def initialize(serializer, options = {})
super
@include_tree = ActiveModel::Serializer::IncludeTree.from_include_args(options[:include] || '*')
@include_tree = ActiveModel::Serializer::IncludeTree.from_include_args(
options[:include] || ActiveModel::Serializer.config.attributes_adapter_include_default
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a separate feature, no?

@cached_attributes = options[:cache_attributes] || {}
end

Expand Down Expand Up @@ -60,8 +62,11 @@ def serializable_hash_for_single_resource(options)

def resource_relationships(options)
relationships = {}

serializer.associations(@include_tree).each do |association|
relationships[association.key] = relationship_value_for(association, options)
relationships[association.key] = relationship_value_for(
association, options
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is just cosmetic, am curious what you like about it

end

relationships
Expand All @@ -71,8 +76,21 @@ def relationship_value_for(association, options)
return association.options[:virtual_value] if association.options[:virtual_value]
return unless association.serializer && association.serializer.object

if options[:fields].is_a? Array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this snippet of code is somewhat problematic.. I believe what it says is

  1. If options fields is a collection of class Array (bug: should checking if respond_to?(:to_ary))
  2. collect all elements of class Hash (or responds to keys is probably fine)
  3. If the collection responds to first, call first (bug: it will always respond to first. and if we only want the first item in the collection, we should just use detect or better lazy and take)
  4. If the previous step's result responds to :[], i.e. the collection had a hash in it, get the value keyed by the association name

I think this could be rewritten as

fields = Array(options[:fields]).lazy.select {|elem| elem.respond_to?(:keys) }.take(1).map {|elem| association.name }.force.compact.first

Would be nice for this to be tested separately or have a descriptive name so it's easier to follow what it's doing, as well

:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might also be able to use the ActiveModel::Serializer::Fieldset.new(options[:fields]).fields_for(association.name) like the JSON API adapter does

fields = options[:fields].select do |k| #I would prefer using {...} here but RuboCop fails :/
k.is_a? Hash
end.try(:first).try(:[], association.name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#I would prefer using {...} here but RuboCop fails :/

Yeah, we're using Rails-style block semantics :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the following:

fields = options[:fields]
  .select { |k| k.is_a? Hash }
  .try(:[], association.name)

end

opts = instance_options.merge(include: @include_tree[association.key])
Attributes.new(association.serializer, opts).serializable_hash(options)
options = options.merge(fields: fields)

Attributes.new(
association.serializer,
opts.merge(association.options)
).serializable_hash(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to rename internal apis like serializable_hash here to as_json. it's just confusing when it's not part of the public serialization api. e.g. #1643

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just thinking aloud. not actionable here)

options.merge(association.options)
)
end

# no-op: Attributes adapter does not include meta data, because it does not support root.
Expand Down
66 changes: 66 additions & 0 deletions test/adapter/json/collection_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,72 @@ def test_include_option

assert_equal(expected, actual)
end

def test_fields_option
serializer = ActiveModel::Serializer::CollectionSerializer.new([@first_post, @second_post])
adapter = ActiveModelSerializers::Adapter::Json.new(serializer)
actual = adapter.serializable_hash(fields: [:id])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these options should be passed into the adapter. most conventional way to do it is

actual = serializable([@first_post, @second_post], adapter: :json, fields: [:id]).serializable_hash

see #1643

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since fields is part of JSON API, we need to make sure fields behave properly there as well. (and the adapter already handles fields)


expected = { posts: [{
id: 1,
comments: [],
author: {
id: 1,
name: 'Steve K.'
},
blog: {
id: 999,
name: 'Custom blog'
}
}, {
id: 2,
comments: [],
author: {
id: 1,
name: 'Steve K.'
},
blog: {
id: 999,
name: 'Custom blog'
}
}] }

assert_equal(expected, actual)
end

def test_fields_with_no_associations_include_option
serializer = ActiveModel::Serializer::CollectionSerializer.new([@first_post, @second_post])
adapter = ActiveModelSerializers::Adapter::Json.new(serializer, include: [])
actual = adapter.serializable_hash(fields: [:id])

expected = { posts: [{
id: 1
}, {
id: 2
}] }

assert_equal(expected, actual)
end

def test_fields_with_associations_fields_option
serializer = ActiveModel::Serializer::CollectionSerializer.new([@first_post, @second_post])
adapter = ActiveModelSerializers::Adapter::Json.new(serializer, include: [:author])
actual = adapter.serializable_hash(fields: [:id, author: [:name]])

expected = { posts: [{
id: 1,
author: {
name: 'Steve K.'
}
}, {
id: 2,
author: {
name: 'Steve K.'
}
}] }

assert_equal(expected, actual)
end
end
end
end
Expand Down