-
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 for Nested Relationships for the Json Adapter #1114
Changes from 9 commits
edfec6c
8df52d5
4f2ad3e
a8e8515
05ec32a
8a6ee84
ce75e78
de2d9b4
2fd748e
85406f9
75d7a95
670c724
9f8f7fc
f11fa95
4339749
608741f
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 | ||||
---|---|---|---|---|---|---|
|
@@ -7,37 +7,13 @@ class Json < Adapter | |||||
def serializable_hash(options = nil) | ||||||
options ||= {} | ||||||
if serializer.respond_to?(:each) | ||||||
@result = serializer.map { |s| FlattenJson.new(s).serializable_hash(options) } | ||||||
@result = serialize_array_without_root(serializer, options) | ||||||
else | ||||||
@hash = {} | ||||||
@result = resource_object_for(serializer) | ||||||
@result = add_resource_relationships(@result, serializer) | ||||||
|
||||||
@core = cache_check(serializer) do | ||||||
serializer.attributes(options) | ||||||
end | ||||||
|
||||||
serializer.associations.each do |association| | ||||||
serializer = association.serializer | ||||||
opts = association.options | ||||||
|
||||||
if serializer.respond_to?(:each) | ||||||
array_serializer = serializer | ||||||
@hash[association.key] = array_serializer.map do |item| | ||||||
cache_check(item) do | ||||||
item.attributes(opts) | ||||||
end | ||||||
end | ||||||
else | ||||||
@hash[association.key] = | ||||||
if serializer && serializer.object | ||||||
cache_check(serializer) do | ||||||
serializer.attributes(options) | ||||||
end | ||||||
elsif opts[:virtual_value] | ||||||
opts[:virtual_value] | ||||||
end | ||||||
end | ||||||
end | ||||||
@result = @core.merge @hash | ||||||
@result | ||||||
end | ||||||
|
||||||
{ root => @result } | ||||||
|
@@ -47,6 +23,147 @@ def fragment_cache(cached_hash, non_cached_hash) | |||||
Json::FragmentCache.new().fragment_cache(cached_hash, non_cached_hash) | ||||||
end | ||||||
|
||||||
private | ||||||
|
||||||
# iterate through the associations on the serializer, | ||||||
# adding them to the parent as needed (as singular or plural) | ||||||
# | ||||||
# nested_associations is a list of symbols that governs what | ||||||
# associations on the passed in seralizer to include | ||||||
def add_resource_relationships(parent, serializer, nested_associations = []) | ||||||
# have the include array normalized | ||||||
nested_associations = include_array_to_hash(nested_associations) | ||||||
|
||||||
included_associations = if nested_associations.present? | ||||||
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. so Eg. so Initially I thought this was to check if it's an already rendered relationship and avoid render it again. but it seems it is used here to loop the associations. What I presume is the the code inside this if might never be executed unless you have something like (probably): Eg. then you will find 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. The if body actually decides what nested association to render. Cause, lets say a post has a ton of associations, but you only want the author. (The tests also cover this scenario) |
||||||
serializer.associations.select{ |association| | ||||||
# nested_associations is a hash of: | ||||||
# key => nested association to include | ||||||
nested_associations.has_key?(association.name) | ||||||
} | ||||||
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 probably be 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 dig it. |
||||||
else | ||||||
serializer.associations | ||||||
end | ||||||
|
||||||
included_associations.each do |association| | ||||||
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 think you can loop straight through |
||||||
serializer = association.serializer | ||||||
opts = association.options | ||||||
key = association.key | ||||||
|
||||||
# sanity check if the association has nesting data | ||||||
has_nesting = nested_associations[key].present? | ||||||
if has_nesting | ||||||
include_options_from_parent = { include: nested_associations[key] } | ||||||
opts = opts.merge(include_options_from_parent) | ||||||
end | ||||||
|
||||||
if serializer.respond_to?(:each) | ||||||
parent[key] = add_relationships(serializer, opts) | ||||||
else | ||||||
parent[key] = add_relationship(serializer, opts) | ||||||
end | ||||||
end | ||||||
|
||||||
parent | ||||||
end | ||||||
|
||||||
# add a singular relationship | ||||||
# the options should always belong to the serializer | ||||||
def add_relationship(serializer, options) | ||||||
serialized_relationship = serialized_or_virtual_of(serializer, options) | ||||||
|
||||||
nested_associations_to_include = options[:include] | ||||||
if nested_associations_to_include.present? | ||||||
serialized_relationship = add_resource_relationships( | ||||||
serialized_relationship, | ||||||
serializer, | ||||||
nested_associations_to_include) | ||||||
end | ||||||
|
||||||
serialized_relationship | ||||||
end | ||||||
|
||||||
# add a many relationship | ||||||
def add_relationships(serializer, options) | ||||||
serialize_array(serializer, options) do |serialized_item, item_serializer| | ||||||
nested_associations_to_include = options[:include] | ||||||
|
||||||
if nested_associations_to_include.present? | ||||||
serialized_item = add_resource_relationships( | ||||||
serialized_item, | ||||||
item_serializer, | ||||||
nested_associations_to_include) | ||||||
end | ||||||
|
||||||
serialized_item | ||||||
end | ||||||
end | ||||||
|
||||||
|
||||||
def serialize_array_without_root(serializer, options) | ||||||
serializer.map { |s| FlattenJson.new(s).serializable_hash(options) } | ||||||
end | ||||||
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 don't think the serializer should be calling the adapter... but I guess you didn't add that 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. the json_api adapter
if serializer.respond_to?(:each)
serializer.each do |s|
result = self.class.new(s, @options.merge(fieldset: @fieldset)).serializable_hash(options)
@hash[:data] << result[:data] 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. that doesn't look very nice to look at. :-\ aren't there json_api refactors in the prs? I'm curious how that cleans things up. |
||||||
|
||||||
# a virtual value is something that doesn't need a serializer, | ||||||
# such as a ruby array, or any other raw value | ||||||
def serialized_or_virtual_of(serializer, options) | ||||||
if serializer && serializer.object | ||||||
resource_object_for(serializer) | ||||||
elsif options[:virtual_value] | ||||||
options[:virtual_value] | ||||||
end | ||||||
end | ||||||
|
||||||
def serialize_array(serializer, options) | ||||||
serializer.map do |item| | ||||||
serialized_item = resource_object_for(item) | ||||||
serialized_item = yield(serialized_item, item) if block_given? | ||||||
serialized_item | ||||||
end | ||||||
end | ||||||
|
||||||
def resource_object_for(serializer) | ||||||
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. Nice abstraction! 👍 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. thanks! |
||||||
cache_check(serializer) do | ||||||
serializer.attributes(serializer.options) | ||||||
end | ||||||
end | ||||||
|
||||||
# converts the include hash to a standard format | ||||||
# for constrained serialization recursion | ||||||
# | ||||||
# converts | ||||||
# [:author, :comments => [:author]] to | ||||||
# {:author => [], :comments => [:author]} | ||||||
# | ||||||
# and | ||||||
# [:author, :comments => {:author => :bio}, :posts => [:comments]] to | ||||||
# {:author => [], :comments => {:author => :bio}, :posts => [:comments]} | ||||||
# | ||||||
# The data passed in to this method should be an array where the last | ||||||
# parameter is a hash | ||||||
# | ||||||
# the point of this method is to normalize the include | ||||||
# options for the child relationships. | ||||||
# if a sub inclusion is still an array after this method, | ||||||
# it will get converted during the next iteration | ||||||
def include_array_to_hash(include_array) | ||||||
# still don't trust input | ||||||
# but this also allows | ||||||
# include: :author syntax | ||||||
include_array = Array[*include_array].compact | ||||||
|
||||||
result = {} | ||||||
|
||||||
hashes = include_array.select{|a| a.is_a?(Hash)} | ||||||
non_hashes = include_array - hashes | ||||||
|
||||||
hashes += non_hashes.map{ |association_name| { association_name => [] } } | ||||||
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. better to mutate 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. how come? 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 seems more like a function than a (private) property of the JSON Adapter.. What would you think about adding a module ActiveModel::Serializer::Utils
module_function
def include_array_to_hash(include_array)
# still don't trust input
# but this also allows
# include: :author syntax
include_array = Array[*include_array].compact
result = {}
hashes = include_array.select{|a| a.is_a?(Hash)}
non_hashes = include_array - hashes
hashes += non_hashes.map{ |association_name| { association_name => [] } }
# now merge all the hashes
hashes.each{|hash| result.merge!(hash) }
result
end
end and then 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 like that. Calling it as a class metho would be my preference. |
||||||
|
||||||
# now merge all the hashes | ||||||
hashes.each{|hash| result.merge!(hash) } | ||||||
|
||||||
result | ||||||
end | ||||||
|
||||||
end | ||||||
end | ||||||
end | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,203 @@ | ||
require 'test_helper' | ||
|
||
NestedPostSerializer = Class.new(ActiveModel::Serializer) do | ||
attributes :id, :title | ||
|
||
has_many :comments, include: :author | ||
end | ||
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 know this is the pattern in the poros.. but can we just do regular Ruby class declarations here, unless there's a good reason and we can comment that? 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'd prefer normal class definitions. I was just following existing patterns. |
||
|
||
NestedCommentBelongsToSerializer = Class.new(ActiveModel::Serializer) do | ||
attributes :id, :body | ||
|
||
belongs_to :author, include: [:posts] | ||
end | ||
|
||
NestedAuthorSerializer = Class.new(ActiveModel::Serializer) do | ||
attributes :id, :name | ||
|
||
has_many :posts, include: [:comments] | ||
end | ||
|
||
ComplexNestedAuthorSerializer = Class.new(ActiveModel::Serializer) do | ||
attributes :id, :name | ||
|
||
# it would normally be silly to have this in production code, cause a post's | ||
# author in this case is always going to be your root object | ||
has_many :posts, include: [:author, comments: [:author]] | ||
end | ||
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. Would be nice to cover scenarios using multiple association declarations as well 👍 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'll add a test for that real quick, just to be sure |
||
|
||
module ActiveModel | ||
class Serializer | ||
class Adapter | ||
class Json | ||
class NestedRelationShipsTestTest < Minitest::Test | ||
def setup | ||
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] | ||
end | ||
|
||
def test_complex_nested_has_many | ||
@first_comment.author = @author | ||
@second_comment.author = @author | ||
|
||
serializer = ComplexNestedAuthorSerializer.new(@author) | ||
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. loving the name |
||
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer) | ||
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 be 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 didn't know you could do that. huh 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 I change it? 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's the way the controller does it.. so if that's what you're mimicing... 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.e. given a resource with serializer and adapter options, seralize it... 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 also highlights the current inability to use adapters outside of the AMS namespace :) 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. personally, I feel that that what the controller does should be in separate tests. So that way if something breaks, if your tests are all separated by responsibility, it'll be easier to track down the cause. |
||
|
||
expected = { | ||
author: { | ||
id: 1, | ||
name: 'Steve K.', | ||
posts: [ | ||
{ | ||
id: 42, title: 'New Post', body: 'Body', | ||
author: { | ||
id: 1, | ||
name: 'Steve K.' | ||
}, | ||
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.' | ||
} | ||
} | ||
] | ||
} | ||
] | ||
} | ||
} | ||
|
||
actual = adapter.serializable_hash | ||
|
||
assert_equal(expected, actual) | ||
end | ||
|
||
def test_nested_has_many | ||
serializer = NestedAuthorSerializer.new(@author) | ||
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer) | ||
|
||
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' | ||
} | ||
] | ||
} | ||
] | ||
} | ||
} | ||
|
||
actual = adapter.serializable_hash | ||
|
||
assert_equal(expected, actual) | ||
end | ||
|
||
def test_belongs_to_on_a_has_many | ||
@first_comment.author = @author | ||
@second_comment.author = @author | ||
|
||
serializer = NestedPostSerializer.new(@post) | ||
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer) | ||
|
||
expected = { | ||
post: { | ||
id: 42, title: 'New Post', | ||
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.' | ||
} | ||
} | ||
] | ||
}, | ||
} | ||
|
||
actual = adapter.serializable_hash | ||
|
||
assert_equal(expected, actual) | ||
end | ||
|
||
def test_belongs_to_with_a_has_many | ||
@author.roles = [] | ||
@author.bio = {} | ||
@first_comment.author = @author | ||
@second_comment.author = @author | ||
|
||
serializer = NestedCommentBelongsToSerializer.new(@first_comment) | ||
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer) | ||
|
||
expected = { | ||
comment: { | ||
id: 1, body: 'ZOMG A COMMENT', | ||
author: { | ||
id: 1, | ||
name: 'Steve K.', | ||
posts: [ | ||
{ | ||
id: 42, title: 'New Post', body: 'Body' | ||
} | ||
] | ||
} | ||
} | ||
} | ||
|
||
actual = adapter.serializable_hash | ||
|
||
assert_equal(expected, actual) | ||
end | ||
|
||
def test_include_array_to_hash | ||
serializer = NestedCommentBelongsToSerializer.new(@first_comment) | ||
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer) | ||
|
||
expected = {author: [], comments: {author: :bio}, posts: [:comments]} | ||
input = [:author, comments: {author: :bio}, posts: [:comments]] | ||
actual = adapter.send(:include_array_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.
👍