-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support nested associations for Json and Attributes adapters + Refactor Attributes adapter #1127
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 |
---|---|---|
|
@@ -9,40 +9,12 @@ def initialize(serializer, options = {}) | |
|
||
def serializable_hash(options = nil) | ||
options ||= {} | ||
|
||
if serializer.respond_to?(:each) | ||
result = serializer.map { |s| Attributes.new(s, instance_options).serializable_hash(options) } | ||
serializable_hash_for_collection(options) | ||
else | ||
hash = {} | ||
|
||
core = cache_check(serializer) do | ||
serializer.attributes(options) | ||
end | ||
|
||
serializer.associations(@include_tree).each do |association| | ||
serializer = association.serializer | ||
association_options = association.options | ||
|
||
if serializer.respond_to?(:each) | ||
array_serializer = serializer | ||
hash[association.key] = array_serializer.map do |item| | ||
cache_check(item) do | ||
item.attributes(association_options) | ||
end | ||
end | ||
else | ||
hash[association.key] = | ||
if serializer && serializer.object | ||
cache_check(serializer) do | ||
serializer.attributes(options) | ||
end | ||
elsif association_options[:virtual_value] | ||
association_options[:virtual_value] | ||
end | ||
end | ||
end | ||
result = core.merge hash | ||
serializable_hash_for_single_resource(options) | ||
end | ||
result | ||
end | ||
|
||
def fragment_cache(cached_hash, non_cached_hash) | ||
|
@@ -51,10 +23,43 @@ def fragment_cache(cached_hash, non_cached_hash) | |
|
||
private | ||
|
||
def serializable_hash_for_collection(options) | ||
serializer.map { |s| Attributes.new(s, instance_options).serializable_hash(options) } | ||
end | ||
|
||
def serializable_hash_for_single_resource(options) | ||
resource = resource_object_for(options) | ||
relationships = resource_relationships(options) | ||
resource.merge!(relationships) | ||
end | ||
|
||
def resource_relationships(options) | ||
relationships = {} | ||
serializer.associations(@include_tree).each do |association| | ||
relationships[association.key] = relationship_value_for(association, options) | ||
end | ||
|
||
relationships | ||
end | ||
|
||
def relationship_value_for(association, options) | ||
return association.options[:virtual_value] if association.options[:virtual_value] | ||
return unless association.serializer && association.serializer.object | ||
|
||
opts = instance_options.merge(include: @include_tree[association.key]) | ||
Attributes.new(association.serializer, opts).serializable_hash(options) | ||
end | ||
|
||
# no-op: Attributes adapter does not include meta data, because it does not support root. | ||
def include_meta(json) | ||
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 was here before. What is it used for? 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. guess :) def include_meta(json)
json[meta_key] = meta if meta
json
end So, really, if we do a better job of making this a no-op by testing for an attribute, we wouldn't need it in the Attributes adapter. 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. ah. |
||
json | ||
end | ||
|
||
def resource_object_for(options) | ||
cache_check(serializer) do | ||
serializer.attributes(options) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,167 @@ | ||
require 'test_helper' | ||
|
||
module ActionController | ||
module Serialization | ||
class Json | ||
class IncludeTest < ActionController::TestCase | ||
class IncludeTestController < ActionController::Base | ||
def setup_data | ||
ActionController::Base.cache_store.clear | ||
|
||
@author = Author.new(id: 1, name: 'Steve K.') | ||
|
||
@post = Post.new(id: 42, title: 'New Post', body: 'Body') | ||
@first_comment = Comment.new(id: 1, body: 'ZOMG A COMMENT') | ||
@second_comment = Comment.new(id: 2, body: 'ZOMG ANOTHER COMMENT') | ||
|
||
@post.comments = [@first_comment, @second_comment] | ||
@post.author = @author | ||
|
||
@first_comment.post = @post | ||
@second_comment.post = @post | ||
|
||
@blog = Blog.new(id: 1, name: 'My Blog!!') | ||
@post.blog = @blog | ||
@author.posts = [@post] | ||
|
||
@first_comment.author = @author | ||
@second_comment.author = @author | ||
@author.comments = [@first_comment, @second_comment] | ||
@author.roles = [] | ||
@author.bio = {} | ||
end | ||
|
||
def render_without_include | ||
setup_data | ||
render json: @author, adapter: :json | ||
end | ||
|
||
def render_resource_with_include_hash | ||
setup_data | ||
render json: @author, include: { posts: :comments }, adapter: :json | ||
end | ||
|
||
def render_resource_with_include_string | ||
setup_data | ||
render json: @author, include: 'posts.comments', adapter: :json | ||
end | ||
|
||
def render_resource_with_deep_include | ||
setup_data | ||
render json: @author, include: 'posts.comments.author', adapter: :json | ||
end | ||
end | ||
|
||
tests IncludeTestController | ||
|
||
def test_render_without_include | ||
get :render_without_include | ||
response = JSON.parse(@response.body) | ||
expected = { | ||
'author' => { | ||
'id' => 1, | ||
'name' => 'Steve K.', | ||
'posts' => [ | ||
{ | ||
'id' => 42, 'title' => 'New Post', 'body' => 'Body' | ||
} | ||
], | ||
'roles' => [], | ||
'bio' => {} | ||
} | ||
} | ||
|
||
assert_equal(expected, response) | ||
end | ||
|
||
def test_render_resource_with_include_hash | ||
get :render_resource_with_include_hash | ||
response = JSON.parse(@response.body) | ||
expected = { | ||
'author' => { | ||
'id' => 1, | ||
'name' => 'Steve K.', | ||
'posts' => [ | ||
{ | ||
'id' => 42, 'title' => 'New Post', 'body' => 'Body', | ||
'comments' => [ | ||
{ | ||
'id' => 1, 'body' => 'ZOMG A COMMENT' | ||
}, | ||
{ | ||
'id' => 2, 'body' => 'ZOMG ANOTHER COMMENT' | ||
} | ||
] | ||
} | ||
] | ||
} | ||
} | ||
|
||
assert_equal(expected, response) | ||
end | ||
|
||
def test_render_resource_with_include_string | ||
get :render_resource_with_include_string | ||
|
||
response = JSON.parse(@response.body) | ||
expected = { | ||
'author' => { | ||
'id' => 1, | ||
'name' => 'Steve K.', | ||
'posts' => [ | ||
{ | ||
'id' => 42, 'title' => 'New Post', 'body' => 'Body', | ||
'comments' => [ | ||
{ | ||
'id' => 1, 'body' => 'ZOMG A COMMENT' | ||
}, | ||
{ | ||
'id' => 2, 'body' => 'ZOMG ANOTHER COMMENT' | ||
} | ||
] | ||
} | ||
] | ||
} | ||
} | ||
|
||
assert_equal(expected, response) | ||
end | ||
|
||
def test_render_resource_with_deep_include | ||
get :render_resource_with_deep_include | ||
|
||
response = JSON.parse(@response.body) | ||
expected = { | ||
'author' => { | ||
'id' => 1, | ||
'name' => 'Steve K.', | ||
'posts' => [ | ||
{ | ||
'id' => 42, 'title' => 'New Post', 'body' => 'Body', | ||
'comments' => [ | ||
{ | ||
'id' => 1, 'body' => 'ZOMG A COMMENT', | ||
'author' => { | ||
'id' => 1, | ||
'name' => 'Steve K.' | ||
} | ||
}, | ||
{ | ||
'id' => 2, 'body' => 'ZOMG ANOTHER COMMENT', | ||
'author' => { | ||
'id' => 1, | ||
'name' => 'Steve K.' | ||
} | ||
} | ||
] | ||
} | ||
] | ||
} | ||
} | ||
|
||
assert_equal(expected, response) | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
require 'test_helper' | ||
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. 👍 |
||
|
||
module ActiveModel | ||
class Serializer | ||
class IncludeTree | ||
module Parsing | ||
class IncludeArgsToHashTest < MiniTest::Test | ||
def test_include_args_to_hash_from_symbol | ||
expected = { author: {} } | ||
input = :author | ||
actual = Parsing.include_args_to_hash(input) | ||
|
||
assert_equal(expected, actual) | ||
end | ||
|
||
def test_include_args_to_hash_from_array | ||
expected = { author: {}, comments: {} } | ||
input = [:author, :comments] | ||
actual = Parsing.include_args_to_hash(input) | ||
|
||
assert_equal(expected, actual) | ||
end | ||
|
||
def test_include_args_to_hash_from_nested_array | ||
expected = { author: {}, comments: { author: {} } } | ||
input = [:author, comments: [:author]] | ||
actual = Parsing.include_args_to_hash(input) | ||
|
||
assert_equal(expected, actual) | ||
end | ||
|
||
def test_include_args_to_hash_from_array_of_hashes | ||
expected = { | ||
author: {}, | ||
blogs: { posts: { contributors: {} } }, | ||
comments: { author: { blogs: { posts: {} } } } | ||
} | ||
input = [ | ||
:author, | ||
blogs: [posts: :contributors], | ||
comments: { author: { blogs: :posts } } | ||
] | ||
actual = Parsing.include_args_to_hash(input) | ||
|
||
assert_equal(expected, actual) | ||
end | ||
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.
How about
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.
I guess if this isn't the kind of method that is ever going to be overridden when it moves to Adapter::Base, I can get behind this. It at least looks a little better
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.
There's very little chance, as the JsonApi adapter works in a very different way wrt relationships inclusion.
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.
'fixed'
rubocop through a weird thing at me - and got around it by turning the if into a guard clause.
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.
Oh yeah, right, rubocop favors guard clauses. Works as well.