Skip to content

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

Merged
merged 1 commit into from
Oct 9, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Breaking changes:

Features:

- [#1225](https://github.com/rails-api/active_model_serializers/pull/1125) Better serializer lookup, use nested serializer when it exists (@beauby)
- [#1172](https://github.com/rails-api/active_model_serializers/pull/1172) Better serializer registration, get more than just the first module (@bf4)
- [#1158](https://github.com/rails-api/active_model_serializers/pull/1158) Add support for wildcards in `include` option (@beauby)
- [#1127](https://github.com/rails-api/active_model_serializers/pull/1127) Add support for nested
Expand Down
31 changes: 31 additions & 0 deletions docs/general/getting_started.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,37 @@ class CommentSerializer < ActiveModel::Serializer
end
```

### Namespaced Models

When serializing a model inside a namespace, such as `Api::V1::Post`, AMS will expect the corresponding serializer to be inside the same namespace (namely `Api::V1::PostSerializer`).

### Model Associations and Nested Serializers

When declaring a serializer for a model with associations, such as:
```ruby
class PostSerializer < ActiveModel::Serializer
has_many :comments
end
```
AMS will look for `PostSerializer::CommentSerializer` in priority, and fall back to `::CommentSerializer` in case the former does not exist. This allows for more control over the way a model gets serialized as an association of an other model.

For example, in the following situation:

```ruby
class CommentSerializer < ActiveModel::Serializer
attributes :body, :date, :nb_likes
end

class PostSerializer < ActiveModel::Serializer
has_many :comments
class CommentSerializer < ActiveModel::Serializer
attributes :body_short
end
end
```

AMS will use `PostSerializer::CommentSerializer` (thus including only the `:body_short` attribute) when serializing a `Comment` as part of a `Post`, but use `::CommentSerializer` when serializing a `Comment` directly (thus including `:body, :date, :nb_likes`).

Copy link
Member

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 to PostSerializer

## Rails Integration

AMS will automatically integrate with you Rails app, you won't need to update your controller, this is a example of how it will look like:
Expand Down
19 changes: 17 additions & 2 deletions lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this extraction.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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) :-\

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but serializer_lookup_chain_for is part of the private API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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 }
Copy link
Member

Choose a reason for hiding this comment

The 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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
this line really should be its own multi-line method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NullVoxPopuli Plus, there already are serializer_for and get_serializer_for, so if we extract this line into its own method, what would it be? get_actual_serializer_for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serializer from lookup chain?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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 }

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

serializer_class = serializer_lookup_chain_for(klass).find{ |x| s = x.safe_constantize; break s if s}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 serializer_lookup_chain_for return a lazy iterator. Map is not syntactic sugar, it is one of the base constructs of functional programming. Plus, it really describes what we're doing here: iterate over the lookup chain, apply safe_constantize, and return the first one for which the result is not nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beauby seriously, I meant the symbol to proc, not map

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
5 changes: 2 additions & 3 deletions lib/active_model/serializer/array_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential 🐛 serializer_context_class is a required option. Would you mind adding a comment to the initializer to that effect? and if it's absent, it'll throw a KeyError. How do we want to handle that?

Maybe just say "don't use ArraySerializer directly, Just use SerializableResource" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous line(serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer)) ensures it is not required. Am I misunderstanding?

Copy link
Member

Choose a reason for hiding this comment

The 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}"
Expand Down
7 changes: 4 additions & 3 deletions lib/active_model/serializer/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serializer_context_class/parent_serializer (however we may call it) is not a reflection option but a serializer option. I agree that there is room for improvement though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified the code to set the serializer_context_class option inside the serializer_options method.


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
Expand All @@ -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
Expand Down
82 changes: 82 additions & 0 deletions test/serializers/associations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never seen that, hmm

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the order matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
23 changes: 23 additions & 0 deletions test/serializers/serializer_for_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's missing from the tests is when ResourceNamespace::PostSerializer.serializer_for is called. the tests don't confirm this is hooked up anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. ResourceNamespace::PostSerializer.serializer_for is called just above?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a test to test/serializers/associations_test.rb to make sure the corresponding serializer gets used. This will be doable in a decent way once the test infrastructure refactor has been done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests.

end
end
end
end
Expand Down