Skip to content

Nicer syntax for jsonapi include #1131

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.

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

Merged
merged 1 commit into from
Sep 13, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@
* remove root key option and split JSON adapter [@joaomdmoura]
* adds FlattenJSON as default adapter [@joaomdmoura]
* adds support for `pagination links` at top level of JsonApi adapter [@bacarini]
* adds extended format for `include` option to JSONAPI adapter [@beauby]
2 changes: 2 additions & 0 deletions docs/general/adapters.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ resources in the `"included"` member when the resource names are included in the
render @posts, include: 'authors,comments'
```

The format of the `include` option can be either a String composed of a comma-separated list of [relationship paths](http://jsonapi.org/format/#fetching-includes), an Array of Symbols and Hashes, or a mix of both.

## Choosing an adapter

If you want to use a specify a default adapter, such as JsonApi, you can change this in an initializer:
Expand Down
1 change: 1 addition & 0 deletions lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class Serializer
autoload :Lint
autoload :Associations
autoload :Fieldset
autoload :Utils
include Configuration
include Associations

Expand Down
56 changes: 21 additions & 35 deletions lib/active_model/serializer/adapter/json_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ def initialize(serializer, options = {})
super
@hash = { data: [] }

@options[:include] ||= []
if @options[:include].is_a?(String)
@options[:include] = @options[:include].split(',')
end

@included = ActiveModel::Serializer::Utils.include_args_to_hash(@options[:include])
fields = options.delete(:fields)
if fields
@fieldset = ActiveModel::Serializer::Fieldset.new(fields, serializer.json_key)
Expand Down Expand Up @@ -117,46 +113,36 @@ def relationships_for(serializer)
end

def included_for(serializer)
serializer.associations.flat_map { |assoc| _included_for(assoc.key, assoc.serializer) }.uniq
included = @included.flat_map do |inc|
association = serializer.associations.find { |assoc| assoc.key == inc.first }
_included_for(association.serializer, inc.second) if association
end

included.uniq
end

def _included_for(resource_name, serializer, parent = nil)
def _included_for(serializer, includes)
if serializer.respond_to?(:each)
serializer.flat_map { |s| _included_for(resource_name, s, parent) }.uniq
serializer.flat_map { |s| _included_for(s, includes) }.uniq
else
return [] unless serializer && serializer.object
result = []
resource_path = [parent, resource_name].compact.join('.')

if include_assoc?(resource_path)
primary_data = primary_data_for(serializer, @options)
relationships = relationships_for(serializer)
primary_data[:relationships] = relationships if relationships.any?
result.push(primary_data)
end

if include_nested_assoc?(resource_path)
non_empty_associations = serializer.associations.select(&:serializer)
primary_data = primary_data_for(serializer, @options)
relationships = relationships_for(serializer)
primary_data[:relationships] = relationships if relationships.any?

included = [primary_data]

non_empty_associations.each do |association|
result.concat(_included_for(association.key, association.serializer, resource_path))
result.uniq!
includes.each do |inc|
association = serializer.associations.find { |assoc| assoc.key == inc.first }
if association
included.concat(_included_for(association.serializer, inc.second))
included.uniq!
end
end
result
end
end

def include_assoc?(assoc)
check_assoc("#{assoc}$")
end

def include_nested_assoc?(assoc)
check_assoc("#{assoc}.")
end

def check_assoc(assoc)
@options[:include].any? { |s| s.match(/^#{assoc.gsub('.', '\.')}/) }
included
end
end

def add_links(options)
Expand Down
35 changes: 35 additions & 0 deletions lib/active_model/serializer/utils.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module ActiveModel::Serializer::Utils
module_function

# Translates a comma separated list of dot separated paths (JSONAPI format) into a Hash.
# Example: `'posts.author, posts.comments.upvotes, posts.comments.author'` would become `{ posts: { author: {}, comments: { author: {}, upvotes: {} } } }`.
#
# @param [String] included
# @return [Hash] a Hash representing the same tree structure
def include_string_to_hash(included)
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add some docs to this? :-)

included.delete(' ').split(',').inject({}) do |hash, path|
hash.deep_merge!(path.split('.').reverse_each.inject({}) { |a, e| { e.to_sym => a } })
end
end

# Translates the arguments passed to the include option into a Hash. The format can be either
# a String (see #include_string_to_hash), an Array of Symbols and Hashes, or a mix of both.
# Example: `posts: [:author, comments: [:author, :upvotes]]` would become `{ posts: { author: {}, comments: { author: {}, upvotes: {} } } }`.
#
# @param [Symbol, Hash, Array, String] included
# @return [Hash] a Hash representing the same tree structure
def include_args_to_hash(included)
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add some docs to this?
you could take from my PR if you want, there just needs to be something explaining what this does. :)

case included
when Symbol
{ included => {} }
when Hash
included.each_with_object({}) { |(key, value), hash| hash[key] = include_args_to_hash(value) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bf4: How about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

this aligns well with https://github.com/rails-api/active_model_serializers/pull/1127/files#diff-2e4bf15928c11c2f4eff9285340b602d

comments would be nice though.

Have you tried seeing what happens if you somehow pass an invalid string as an include option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would pretty much do what it's supposed to. What do you have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly just adding tests for if someone specified 'authr' instead of 'author' stuff like that.

strings can be a little difficult to catch errors in sometimes, due to silent failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. Currently, the way it works, it should silently ignore any included relationship that was not defined on the serializer. Moreover, I believe there already are some tests on the adapter for trying to include a missing relationship.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there should be a logger output for when that happens? maybe not in this PR, but in another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it would make sense. Also, there should be a logger output when attempting to serialize a related relationship for which there exists no serializer.

when Array
included.inject({}) { |a, e| a.merge!(include_args_to_hash(e)) }
when String
include_string_to_hash(included)
else
{}
end
end
end
15 changes: 7 additions & 8 deletions test/action_controller/json_api/linked_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,29 +43,29 @@ def render_resource_without_include

def render_resource_with_include
setup_post
render json: @post, include: 'author', adapter: :json_api
render json: @post, include: [:author], adapter: :json_api
Copy link
Member

Choose a reason for hiding this comment

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

These are the only tests involving action_controller + include would be nice to have some of them or new ones covering the string behavior in to make sure we con't break it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted some to the string syntax.

end

def render_resource_with_nested_include
setup_post
render json: @post, include: 'comments.author', adapter: :json_api
render json: @post, include: [comments: [:author]], adapter: :json_api
Copy link
Member

Choose a reason for hiding this comment

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

did these tests break? if so, that's worrysome

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They broke when I removed the string syntax, and stopped breaking when I added it back.

end

def render_resource_with_nested_has_many_include
setup_post
render json: @post, include: ['author', 'author.roles'], adapter: :json_api
render json: @post, include: 'author.roles', adapter: :json_api
end

def render_resource_with_missing_nested_has_many_include
setup_post
@post.author = @author2 # author2 has no roles.
render json: @post, include: 'author,author.roles', adapter: :json_api
render json: @post, include: [author: [:roles]], adapter: :json_api
end

def render_collection_with_missing_nested_has_many_include
setup_post
@post.author = @author2
render json: [@post, @post2], include: 'author,author.roles', adapter: :json_api
render json: [@post, @post2], include: [author: [:roles]], adapter: :json_api
end

def render_collection_without_include
Expand All @@ -75,7 +75,7 @@ def render_collection_without_include

def render_collection_with_include
setup_post
render json: [@post], include: %w(author comments), adapter: :json_api
render json: [@post], include: 'author, comments', adapter: :json_api
end
end

Expand Down Expand Up @@ -141,8 +141,7 @@ def test_render_resource_with_nested_include
get :render_resource_with_nested_include
response = JSON.parse(@response.body)
assert response.key? 'included'
assert_equal 1, response['included'].size
assert_equal 'Anonymous', response['included'].first['attributes']['name']
assert_equal 3, response['included'].size
end

def test_render_collection_without_include
Expand Down
6 changes: 3 additions & 3 deletions test/adapter/json_api/belongs_to_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_includes_post_id
end

def test_includes_linked_post
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: 'post')
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:post])
expected = [{
id: '42',
type: 'posts',
Expand All @@ -56,7 +56,7 @@ def test_includes_linked_post
end

def test_limiting_linked_post_fields
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: 'post', fields: { post: [:title] })
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:post], fields: { post: [:title] })
expected = [{
id: '42',
type: 'posts',
Expand Down Expand Up @@ -108,7 +108,7 @@ def test_include_type_for_association_when_different_than_name

def test_include_linked_resources_with_type_name
serializer = BlogSerializer.new(@blog)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, include: %w(writer articles))
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, include: [:writer, :articles])
linked = adapter.serializable_hash[:included]
expected = [
{
Expand Down
2 changes: 1 addition & 1 deletion test/adapter/json_api/has_many_explicit_serializer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def setup
@serializer = PostPreviewSerializer.new(@post)
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(
@serializer,
include: %w(comments author)
include: [:comments, :author]
)
end

Expand Down
4 changes: 2 additions & 2 deletions test/adapter/json_api/has_many_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def test_includes_comment_ids
end

def test_includes_linked_comments
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: 'comments')
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:comments])
expected = [{
id: '1',
type: 'comments',
Expand All @@ -68,7 +68,7 @@ def test_includes_linked_comments
end

def test_limit_fields_of_linked_comments
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: 'comments', fields: { comment: [:id] })
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:comments], fields: { comment: [:id] })
expected = [{
id: '1',
type: 'comments',
Expand Down
4 changes: 2 additions & 2 deletions test/adapter/json_api/has_one_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def setup
@virtual_value = VirtualValue.new(id: 1)

@serializer = AuthorSerializer.new(@author)
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: 'bio,posts')
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:bio, :posts])
end

def test_includes_bio_id
Expand All @@ -38,7 +38,7 @@ def test_includes_bio_id
end

def test_includes_linked_bio
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: 'bio')
@adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:bio])

expected = [
{
Expand Down
12 changes: 6 additions & 6 deletions test/adapter/json_api/linked_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ def test_include_multiple_posts_and_linked_array
serializer = ArraySerializer.new([@first_post, @second_post])
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(
serializer,
include: ['author', 'author.bio', 'comments']
include: [:comments, author: [:bio]]
)
alt_adapter = ActiveModel::Serializer::Adapter::JsonApi.new(
serializer,
include: 'author,author.bio,comments'
include: [:comments, author: [:bio]]
)

expected = {
Expand Down Expand Up @@ -153,11 +153,11 @@ def test_include_multiple_posts_and_linked
serializer = BioSerializer.new @bio1
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(
serializer,
include: ['author', 'author.posts']
include: [author: [:posts]]
Copy link
Contributor

Choose a reason for hiding this comment

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

should we still have tests for the string equivalent of all these? just to be thorough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left some of the tests with the string syntax. We could add more, although I think the right place to test this is in the include_args_to_hash tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I meant in the adapter tests when I said include_args_to_hash!

Copy link
Contributor

Choose a reason for hiding this comment

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

I gotchyou :-)

)
alt_adapter = ActiveModel::Serializer::Adapter::JsonApi.new(
serializer,
include: 'author,author.posts'
include: [author: [:posts]]
)

expected = [
Expand Down Expand Up @@ -224,7 +224,7 @@ def test_multiple_references_to_same_resource
serializer = ArraySerializer.new([@first_comment, @second_comment])
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(
serializer,
include: ['post']
include: [:post]
)

expected = [
Expand Down Expand Up @@ -257,7 +257,7 @@ def test_nil_link_with_specified_serializer
serializer = PostPreviewSerializer.new(@first_post)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(
serializer,
include: ['author']
include: [:author]
)

expected = {
Expand Down
79 changes: 79 additions & 0 deletions test/utils/include_args_to_hash_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
require 'test_helper'

module ActiveModel
class Serializer
module Utils
class IncludeArgsToHashTest < Minitest::Test
def test_nil
input = nil
expected = {}
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_empty_string
input = ''
expected = {}
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_single_string
input = 'author'
expected = { author: {} }
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_multiple_strings
input = 'author,comments'
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some tests for invalid input? such as authr?

Copy link
Contributor

Choose a reason for hiding this comment

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

or 'posts.comments.author.invalid'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would produce the expected output at this level, since this is just building a hash.

expected = { author: {}, comments: {} }
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_multiple_strings_with_space
input = 'author, comments'
expected = { author: {}, comments: {} }
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_nested_string
input = 'posts.author'
expected = { posts: { author: {} } }
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_multiple_nested_string
input = 'posts.author,posts.comments.author,comments'
expected = { posts: { author: {}, comments: { author: {} } }, comments: {} }
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_empty_array
input = []
expected = {}
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_simple_array
input = [:comments, :author]
expected = { author: {}, comments: {} }
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end

def test_nested_array
input = [:comments, posts: [:author, comments: [:author]]]
expected = { posts: { author: {}, comments: { author: {} } }, comments: {} }
actual = ActiveModel::Serializer::Utils.include_args_to_hash(input)
assert_equal(expected, actual)
end
end
end
end
end