-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you add some docs to this? |
||
case included | ||
when Symbol | ||
{ included => {} } | ||
when Hash | ||
included.each_with_object({}) { |(key, value), hash| hash[key] = include_args_to_hash(value) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bf4: How about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are the only tests involving There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did these tests break? if so, that's worrysome There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
|
||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = { | ||
|
@@ -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]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, I meant in the adapter tests when I said There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = [ | ||
|
@@ -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 = [ | ||
|
@@ -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 = { | ||
|
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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add some tests for invalid input? such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or 'posts.comments.author.invalid' There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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? :-)