-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for nested serializers #1225
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 |
---|---|---|
|
@@ -108,10 +108,25 @@ def self.digest_caller_file(caller_line) | |
Digest::MD5.hexdigest(serializer_file_contents) | ||
end | ||
|
||
# @api private | ||
def self.serializer_lookup_chain_for(klass) | ||
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 extraction. 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. some documentation on how this works (when to use, expected input/output, etc) would be fantabulous 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. Looks pretty much self-explanatory to me. What kind of comment would you add that would not be direct paraphrasing of the code? 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. me too, but I would like to prefer to over document than to under document. Like, look at how rails documents everything with plenty of examples: https://github.com/rails/rails/blob/master/activemodel/lib/active_model/errors.rb 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 guess that is more or less direct paraphrasing in some cases though) :-\ 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 but 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. fair point 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. |
||
chain = [] | ||
|
||
resource_class_name = klass.name.demodulize | ||
resource_namespace = klass.name.deconstantize | ||
serializer_class_name = "#{resource_class_name}Serializer" | ||
|
||
chain.push("#{name}::#{serializer_class_name}") if self != ActiveModel::Serializer | ||
chain.push("#{resource_namespace}::#{serializer_class_name}") | ||
|
||
chain | ||
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. @bf4 So if you were referring to this part of code saying it was not easily understandable, then I agree. Suggestions? I could add comments, but I don't see how to make the code itself clearer. |
||
|
||
# @api private | ||
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. giving us the license to change it :) 👍 |
||
def self.get_serializer_for(klass) | ||
serializers_cache.fetch_or_store(klass) do | ||
serializer_class_name = "#{klass.name}Serializer" | ||
serializer_class = serializer_class_name.safe_constantize | ||
# NOTE(beauby): When we drop 1.9.3 support we can lazify the map for perfs. | ||
serializer_class = serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x } | ||
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'm going to block this PR till the code is more readable behavior looks 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. 👍 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 line simply does try every constant in the lookup chain until one that exists is found. How would you make it more readable? 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. @NullVoxPopuli Plus, there already are 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. Serializer from lookup chain? 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. serializer_class = serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x } ran be rewritten as def class_from_serializer_lookup_chain(klass)
serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x }
end I think extracting the method is possibly half way between what you have, @beauby and @bf4's struct idea 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.
but that's going to unnecessarily iterate over every element when it doesn't need to. and it's only shorter because of syntactic sugar and laconic 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. Once we wrop support for 1.9.3, which we all agreed on, we can just 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. @beauby seriously, I meant the symbol to proc, not map 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 bad. Then it is, indeed syntactic sugar for serializer_lookup_chain_for(klass).map { |x| x.safe_constantize }.find { |x| x } |
||
|
||
if serializer_class | ||
serializer_class | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,8 @@ def initialize(resources, options = {}) | |
@root = options[:root] | ||
@object = resources | ||
@serializers = resources.map do |resource| | ||
serializer_class = options.fetch(:serializer) do | ||
ActiveModel::Serializer.serializer_for(resource) | ||
end | ||
serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer) | ||
serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) } | ||
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. Potential 🐛 Maybe just say "don't use ArraySerializer directly, Just use SerializableResource" :) 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 previous line( 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. nope, I'm 👊 (facepalm?) oops never mind me |
||
|
||
if serializer_class.nil? | ||
fail NoSerializerError, "No serializer found for resource: #{resource.inspect}" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,13 +42,13 @@ class Serializer | |
def build_association(subject, parent_serializer_options) | ||
association_value = subject.send(name) | ||
reflection_options = options.dup | ||
serializer_class = ActiveModel::Serializer.serializer_for(association_value, reflection_options) | ||
serializer_class = subject.class.serializer_for(association_value, reflection_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. 👍 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. serializer_context_class = subject.class
reflection_options = options.dup.merge!(serializer_context_class: serializer_context_class)
serializer_class = serializer_context_class.serializer_for(association_value, reflection_options)
# etc, no changes needed below 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. Modified the code to set the |
||
|
||
if serializer_class | ||
begin | ||
serializer = serializer_class.new( | ||
association_value, | ||
serializer_options(parent_serializer_options, reflection_options) | ||
serializer_options(subject, parent_serializer_options, reflection_options) | ||
) | ||
rescue ActiveModel::Serializer::ArraySerializer::NoSerializerError | ||
reflection_options[:virtual_value] = association_value.try(:as_json) || association_value | ||
|
@@ -62,11 +62,12 @@ def build_association(subject, parent_serializer_options) | |
|
||
private | ||
|
||
def serializer_options(parent_serializer_options, reflection_options) | ||
def serializer_options(subject, parent_serializer_options, reflection_options) | ||
serializer = reflection_options.fetch(:serializer, nil) | ||
|
||
serializer_options = parent_serializer_options.except(:serializer) | ||
serializer_options[:serializer] = serializer if serializer | ||
serializer_options[:serializer_context_class] = subject.class | ||
serializer_options | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,6 +125,88 @@ def test_associations_custom_keys | |
assert expected_association_keys.include? :writer | ||
assert expected_association_keys.include? :site | ||
end | ||
|
||
class NamespacedResourcesTest < Minitest::Test | ||
class ResourceNamespace | ||
Post = Class.new(::Model) | ||
Comment = Class.new(::Model) | ||
Author = Class.new(::Model) | ||
Description = Class.new(::Model) | ||
class PostSerializer < ActiveModel::Serializer | ||
has_many :comments | ||
belongs_to :author | ||
has_one :description | ||
end | ||
CommentSerializer = Class.new(ActiveModel::Serializer) | ||
AuthorSerializer = Class.new(ActiveModel::Serializer) | ||
DescriptionSerializer = Class.new(ActiveModel::Serializer) | ||
end | ||
|
||
def setup | ||
@comment = ResourceNamespace::Comment.new | ||
@author = ResourceNamespace::Author.new | ||
@description = ResourceNamespace::Description.new | ||
@post = ResourceNamespace::Post.new(comments: [@comment], | ||
author: @author, | ||
description: @description) | ||
@post_serializer = ResourceNamespace::PostSerializer.new(@post) | ||
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 add a serializer in global scope for completeness (unless I'm just not seeing 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. So the previous behavior was not to look for a serializer in the global namespace unless the model itself is in the global namespace, which this PR preserves. 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, per se, those tests were not breaking before this PR, but nonetheless I think we should have such tests to ensure the behavior. 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 want to add one? else I'll merge it |
||
end | ||
|
||
def test_associations_namespaced_resources | ||
@post_serializer.associations.each do |association| | ||
case association.key | ||
when :comments | ||
assert_instance_of(ResourceNamespace::CommentSerializer, association.serializer.first) | ||
when :author | ||
assert_instance_of(ResourceNamespace::AuthorSerializer, association.serializer) | ||
when :description | ||
assert_instance_of(ResourceNamespace::DescriptionSerializer, association.serializer) | ||
else | ||
flunk "Unknown association: #{key}" | ||
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. never seen that, hmm 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 was all around so I sticked with the current convention. |
||
end | ||
end | ||
end | ||
end | ||
|
||
class NestedSerializersTest < Minitest::Test | ||
Post = Class.new(::Model) | ||
Comment = Class.new(::Model) | ||
Author = Class.new(::Model) | ||
Description = Class.new(::Model) | ||
class PostSerializer < ActiveModel::Serializer | ||
has_many :comments | ||
CommentSerializer = Class.new(ActiveModel::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. does the order matter? 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, but it felt natural to group stuff by "feature". |
||
belongs_to :author | ||
AuthorSerializer = Class.new(ActiveModel::Serializer) | ||
has_one :description | ||
DescriptionSerializer = Class.new(ActiveModel::Serializer) | ||
end | ||
|
||
def setup | ||
@comment = Comment.new | ||
@author = Author.new | ||
@description = Description.new | ||
@post = Post.new(comments: [@comment], | ||
author: @author, | ||
description: @description) | ||
@post_serializer = PostSerializer.new(@post) | ||
end | ||
|
||
def test_associations_namespaced_resources | ||
@post_serializer.associations.each do |association| | ||
case association.key | ||
when :comments | ||
assert_instance_of(PostSerializer::CommentSerializer, association.serializer.first) | ||
when :author | ||
assert_instance_of(PostSerializer::AuthorSerializer, association.serializer) | ||
when :description | ||
assert_instance_of(PostSerializer::DescriptionSerializer, association.serializer) | ||
else | ||
flunk "Unknown association: #{key}" | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,8 +27,19 @@ def test_overwritten_serializer_for_array | |
end | ||
|
||
class SerializerTest < Minitest::Test | ||
module ResourceNamespace | ||
Post = Class.new(::Model) | ||
Comment = Class.new(::Model) | ||
|
||
class PostSerializer < ActiveModel::Serializer | ||
class CommentSerializer < ActiveModel::Serializer | ||
end | ||
end | ||
end | ||
|
||
class MyProfile < Profile | ||
end | ||
|
||
class CustomProfile | ||
def serializer_class; ProfileSerializer; end | ||
end | ||
|
@@ -59,6 +70,18 @@ def test_serializer_custom_serializer | |
serializer = ActiveModel::Serializer.serializer_for(@custom_profile) | ||
assert_equal ProfileSerializer, serializer | ||
end | ||
|
||
def test_serializer_for_namespaced_resource | ||
post = ResourceNamespace::Post.new | ||
serializer = ActiveModel::Serializer.serializer_for(post) | ||
assert_equal(ResourceNamespace::PostSerializer, serializer) | ||
end | ||
|
||
def test_serializer_for_nested_resource | ||
comment = ResourceNamespace::Comment.new | ||
serializer = ResourceNamespace::PostSerializer.serializer_for(comment) | ||
assert_equal(ResourceNamespace::PostSerializer::CommentSerializer, 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. what's missing from the tests is when 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 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. What I see is: ActiveModel::Serializer.serializer_for(post) #=> ResourceNamespace::PostSerializer
ResourceNamespace::PostSerializer.serializer_for(comment) #=> ResourceNamespace::PostSerializer::CommentSerializer which tests serialization from an AM::S subclass, but not that the subclass is ever used.. maybe my comment just means there should be another test somewhere else 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. Got 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. We need to add a test to 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. Added tests. |
||
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.
outside of this PR, but this doesn't include the superclass scenario and it might be interesting to compare
Api::V1::PostSerializer
toPostSerializer