-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for wildcards in nested includes #1158
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 |
---|---|---|
|
@@ -2,6 +2,11 @@ module ActiveModel | |
class Serializer | ||
module Adapter | ||
class Attributes < Base | ||
def initialize(serializer, options = {}) | ||
super | ||
@include_tree = IncludeTree.from_include_args(options[:include] || '*') | ||
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 just options[:include] be passed, and then in the method, do 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 be more specific? If you're referring to doing 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. maybe like this:
def from_include_args(included)
new(include_args_to_hash(included || '*'))
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. Doesn't work for the reasons specified somewhere else: we need the flexibility of different fallbacks depending on the 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. gotchya. What if the class-level method to get the include associations tree per-adapter? adapters could just override that method if they need to, and we could have a generic one in base? 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. Not sure I understand. Could you be more specific? 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. maybe something like this? def self.included_associations(options)
IncludeTree.from_include_args(options[:include] || '*')
end and then idk 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. Not sure it adds much value at this stage, but I am not opposed to 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. Let's keep it like that for now, and refactor it later if it makes stuff easier? |
||
end | ||
|
||
def serializable_hash(options = nil) | ||
options ||= {} | ||
if serializer.respond_to?(:each) | ||
|
@@ -13,7 +18,7 @@ def serializable_hash(options = nil) | |
serializer.attributes(options) | ||
end | ||
|
||
serializer.associations.each do |association| | ||
serializer.associations(@include_tree).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 like 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. sorta railsy |
||
serializer = association.serializer | ||
association_options = association.options | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,8 @@ class JsonApi < Base | |
|
||
def initialize(serializer, options = {}) | ||
super | ||
@included = ActiveModel::Serializer::Utils.include_args_to_hash(instance_options[:include]) | ||
@include_tree = IncludeTree.from_include_args(options[:include]) | ||
|
||
fields = options.delete(:fields) | ||
if fields | ||
@fieldset = ActiveModel::Serializer::Fieldset.new(fields, serializer.json_key) | ||
|
@@ -19,10 +20,11 @@ def initialize(serializer, options = {}) | |
|
||
def serializable_hash(options = nil) | ||
options ||= {} | ||
|
||
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. Not required for this PR, but passing the serializer reference along is useless here as it is available via an 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. because of the new associations @include stuff :-) 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. What do you mean? 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 kind of like to simplify defining 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. scratch that, I thought I was looking at a different 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. @bf4: Not sure I understand. 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. Basically just avoid the def serializable_hash(options = nil)
options ||= {} boilerplate 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. I'm all up for that, although I don't think it falls within the scope of this PR. Moreover, I think we should have 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. pretty much the same as what I'm thinking. Yes, outside of scope |
||
if serializer.respond_to?(:each) | ||
serializable_hash_for_collection(serializer, options) | ||
serializable_hash_for_collection(options) | ||
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. Not required for this PR, but passing the serializer reference along is useless here as it is available via an |
||
else | ||
serializable_hash_for_single_resource(serializer, options) | ||
serializable_hash_for_single_resource(options) | ||
end | ||
end | ||
|
||
|
@@ -34,10 +36,10 @@ def fragment_cache(cached_hash, non_cached_hash) | |
private | ||
|
||
ActiveModel.silence_warnings do | ||
attr_reader :included, :fieldset | ||
attr_reader :fieldset | ||
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. Not required for this PR, but passing the serializer reference along is useless here as it is available via an |
||
|
||
def serializable_hash_for_collection(serializer, options) | ||
def serializable_hash_for_collection(options) | ||
hash = { data: [] } | ||
serializer.each do |s| | ||
result = self.class.new(s, instance_options.merge(fieldset: fieldset)).serializable_hash(options) | ||
|
@@ -57,10 +59,10 @@ def serializable_hash_for_collection(serializer, options) | |
hash | ||
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. Not required for this PR, but passing the serializer reference along is useless here as it is available via an |
||
|
||
def serializable_hash_for_single_resource(serializer, options) | ||
def serializable_hash_for_single_resource(options) | ||
primary_data = primary_data_for(serializer, options) | ||
relationships = relationships_for(serializer) | ||
included = included_for(serializer) | ||
included = included_resources(@include_tree) | ||
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 there's a trend towards avoiding instance variables, but I think they make the situation really clear, compared to private accessors. 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. my concern with ivars is that they're more buggy. If I type 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 is true - 👍 @bf4 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. My concern is not that much towards performance than clarity: it's easy to define a local variable with the same name as a getter/setter. Plus |
||
hash = { data: primary_data } | ||
hash[:data][:relationships] = relationships if relationships.any? | ||
hash[:included] = included if included.any? | ||
|
@@ -123,37 +125,37 @@ def relationship_value_for(serializer, options = {}) | |
end | ||
|
||
def relationships_for(serializer) | ||
Hash[serializer.associations.map { |association| [association.key, { data: relationship_value_for(association.serializer, association.options) }] }] | ||
serializer.associations.each_with_object({}) do |association, hash| | ||
hash[association.key] = { data: relationship_value_for(association.serializer, association.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. Not required for this PR, but it feels nicer this way. |
||
end | ||
|
||
def included_for(serializer) | ||
included.flat_map { |inc| | ||
association = serializer.associations.find { |assoc| assoc.key == inc.first } | ||
_included_for(association.serializer, inc.second) if association | ||
}.uniq | ||
def included_resources(include_tree) | ||
included = [] | ||
|
||
serializer.associations(include_tree).each do |association| | ||
add_included_resources_for(association.serializer, include_tree[association.key], included) | ||
end | ||
|
||
included | ||
end | ||
|
||
def _included_for(serializer, includes) | ||
def add_included_resources_for(serializer, include_tree, included) | ||
if serializer.respond_to?(:each) | ||
serializer.flat_map { |s| _included_for(s, includes) }.uniq | ||
serializer.each { |s| add_included_resources_for(s, include_tree, included) } | ||
else | ||
return [] unless serializer && serializer.object | ||
return unless serializer && serializer.object | ||
|
||
primary_data = primary_data_for(serializer, instance_options) | ||
relationships = relationships_for(serializer) | ||
primary_data[:relationships] = relationships if relationships.any? | ||
|
||
included = [primary_data] | ||
return if included.include?(primary_data) | ||
included.push(primary_data) | ||
|
||
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 | ||
serializer.associations(include_tree).each do |association| | ||
add_included_resources_for(association.serializer, include_tree[association.key], included) | ||
end | ||
|
||
included | ||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
module ActiveModel | ||
class Serializer | ||
class IncludeTree | ||
module Parsing | ||
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 is the same 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. Yes. |
||
module_function | ||
|
||
def include_string_to_hash(included) | ||
included.delete(' ').split(',').reduce({}) do |hash, path| | ||
include_tree = path.split('.').reverse_each.reduce({}) { |a, e| { e.to_sym => a } } | ||
hash.deep_merge!(include_tree) | ||
end | ||
end | ||
|
||
def include_args_to_hash(included) | ||
case included | ||
when Symbol | ||
{ included => {} } | ||
when Hash | ||
included.each_with_object({}) { |(key, value), hash| | ||
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 end for multiline block? 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 I'm not too sure since it's not technically a multiline block (there's only one line in the block), and this format is present in the codebase already. I don't mind changing it, but let's make it a lasting decision 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. yeah, idk. according to the styleguide, this should all be on one line -- but it's so long. I wonder if this should be extracted to a method, like you did with the string 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. Two private class methods all taking 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.
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. naming? 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 could basically be renamed 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. Sure. Mind you, I'm kind of tired, so anything that makes me think less sounds good :) |
||
hash[key] = include_args_to_hash(value) | ||
} | ||
when Array | ||
included.reduce({}) { |a, e| a.merge!(include_args_to_hash(e)) } | ||
when String | ||
include_string_to_hash(included) | ||
else | ||
{} | ||
end | ||
end | ||
end | ||
|
||
# Builds an IncludeTree from a comma separated list of dot separated paths (JSON API format). | ||
# @example `'posts.author, posts.comments.upvotes, posts.comments.author'` | ||
# | ||
# @param [String] included | ||
# @return [IncludeTree] | ||
# | ||
def self.from_string(included) | ||
new(Parsing.include_string_to_hash(included)) | ||
end | ||
|
||
# Translates the arguments passed to the include option into an IncludeTree. | ||
# The format can be either a String (see #from_string), an Array of Symbols and Hashes, or a mix of both. | ||
# @example `posts: [:author, comments: [:author, :upvotes]]` | ||
# | ||
# @param [Symbol, Hash, Array, String] included | ||
# @return [IncludeTree] | ||
# | ||
def self.from_include_args(included) | ||
new(Parsing.include_args_to_hash(included)) | ||
end | ||
|
||
# @param [Hash] hash | ||
def initialize(hash = {}) | ||
@hash = hash | ||
end | ||
|
||
def key?(key) | ||
@hash.key?(key) || @hash.key?(:*) || @hash.key?(:**) | ||
end | ||
|
||
def [](key) | ||
# TODO(beauby): Adopt a lazy caching strategy for generating subtrees. | ||
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. 👍 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. does this mean the PR merging should wait on this being done? 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. No, it does not. Just that it's something we will probably want to do in a subsequent PR. 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. cool |
||
case | ||
when @hash.key?(key) | ||
self.class.new(@hash[key]) | ||
when @hash.key?(:*) | ||
self.class.new(@hash[:*]) | ||
when @hash.key?(:**) | ||
self.class.new(:** => {}) | ||
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. A 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. According to the Ruby Style Guide, one should avoid redundant else-clauses. 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. Link? I don't see it, nor do I think it's a style issue 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. is the intent to return nil for the else case? if so, a method yardoc would be great :-) 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. My mistake, it was just my linter. Will update immediately. 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. Fixed. |
||
end | ||
end | ||
end | ||
end | ||
end |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,9 +51,9 @@ def render_resource_with_nested_include | |
render json: @post, include: [comments: [:author]], adapter: :json_api | ||
end | ||
|
||
def render_resource_with_nested_has_many_include | ||
def render_resource_with_nested_has_many_include_wildcard | ||
setup_post | ||
render json: @post, include: 'author.roles', adapter: :json_api | ||
render json: @post, include: 'author.*', adapter: :json_api | ||
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. should there be an additional test instead of changing an existing one? 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. There should be many more tests, indeed. 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. Actually, I don't believe there should be more integration tests for this PR. We should just thoroughly test 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.
@beauby Yes, moar 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 method does not exist anymore. |
||
|
||
def render_resource_with_missing_nested_has_many_include | ||
|
@@ -96,7 +96,7 @@ def test_render_resource_with_include | |
end | ||
|
||
def test_render_resource_with_nested_has_many_include | ||
get :render_resource_with_nested_has_many_include | ||
get :render_resource_with_nested_has_many_include_wildcard | ||
response = JSON.parse(@response.body) | ||
expected_linked = [ | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class Serializer | ||
class IncludeTree | ||
class FromStringTest < Minitest::Test | ||
def test_simple_array | ||
input = [:comments, :author] | ||
actual = ActiveModel::Serializer::IncludeTree.from_include_args(input) | ||
assert(actual.key?(:author)) | ||
assert(actual.key?(:comments)) | ||
end | ||
|
||
def test_nested_array | ||
input = [:comments, posts: [:author, comments: [:author]]] | ||
actual = ActiveModel::Serializer::IncludeTree.from_include_args(input) | ||
assert(actual.key?(:posts)) | ||
assert(actual[:posts].key?(:author)) | ||
assert(actual[:posts].key?(:comments)) | ||
assert(actual[:posts][:comments].key?(:author)) | ||
assert(actual.key?(:comments)) | ||
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.
It has to be before
associations
, because we use it in there.