Skip to content
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

Replace fail/rescue CollectionSerializer::NoSerializerError with throw/catch :no_serializer #1767

Merged
merged 1 commit into from
Sep 26, 2016
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Fixes:
- [#1881](https://github.com/rails-api/active_model_serializers/pull/1881) ActiveModelSerializers::Model correctly works with string keys (@yevhene)

Misc:
- [#1767](https://github.com/rails-api/active_model_serializers/pull/1767) Replace raising/rescuing `CollectionSerializer::NoSerializerError`,
throw/catch `:no_serializer`. (@bf4)
- [#1839](https://github.com/rails-api/active_model_serializers/pull/1839) `fields` tests demonstrating usage for both attributes and relationships. (@NullVoxPopuli)
- [#1812](https://github.com/rails-api/active_model_serializers/pull/1812) add a code of conduct (@corainchicago)

Expand Down
13 changes: 6 additions & 7 deletions docs/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ It requires an adapter to transform its attributes into a JSON document; it cann
It may be useful to think of it as a
[presenter](http://blog.steveklabnik.com/posts/2011-09-09-better-ruby-presenters).

The **`ActiveModel::ArraySerializer`** represent a collection of resources as serializers
The **`ActiveModel::CollectionSerializer`** represents a collection of resources as serializers
and, if there is no serializer, primitives.

The **`ActiveModel::Adapter`** describes the structure of the JSON document generated from a
Expand All @@ -42,10 +42,9 @@ it is not modified.
Internally, if no serializer can be found in the controller, the resource is not decorated by
ActiveModelSerializers.

If the collection serializer (ArraySerializer) cannot
identify a serializer for a resource in its collection, it raises [`NoSerializerError`](https://github.com/rails-api/active_model_serializers/issues/1191#issuecomment-142327128)
which is rescued in `ActiveModel::Serializer::Reflection#build_association` which sets
the association value directly:
If the collection serializer (CollectionSerializer) cannot
identify a serializer for a resource in its collection, it throws [`:no_serializer`](https://github.com/rails-api/active_model_serializers/issues/1191#issuecomment-142327128).
For example, when caught by `Reflection#build_association`, the association value is set directly:

```ruby
reflection_options[:virtual_value] = association_value.try(:as_json) || association_value
Expand Down Expand Up @@ -85,8 +84,8 @@ Details:
1. The serializer and adapter are created as
1. `serializer_instance = serializer.new(resource, serializer_opts)`
2. `adapter_instance = ActiveModel::Serializer::Adapter.create(serializer_instance, adapter_opts)`
1. **ActiveModel::Serializer::ArraySerializer#new**
1. If the `serializer_instance` was a `ArraySerializer` and the `:serializer` serializer_opts
1. **ActiveModel::Serializer::CollectionSerializer#new**
1. If the `serializer_instance` was a `CollectionSerializer` and the `:serializer` serializer_opts
is present, then [that serializer is passed into each resource](https://github.com/rails-api/active_model_serializers/blob/a54d237e2828fe6bab1ea5dfe6360d4ecc8214cd/lib/active_model/serializer/array_serializer.rb#L14-L16).
1. **ActiveModel::Serializer#attributes** is used by the adapter to get the attributes for
resource as defined by the serializer.
Expand Down
6 changes: 3 additions & 3 deletions lib/active_model/serializer/collection_serializer.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module ActiveModel
class Serializer
class CollectionSerializer
NoSerializerError = Class.new(StandardError)
include Enumerable
delegate :each, to: :@serializers

Expand Down Expand Up @@ -74,8 +73,9 @@ def serializers_from_resources
def serializer_from_resource(resource, serializer_context_class, options)
serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) }

if serializer_class.nil? # rubocop:disable Style/GuardClause
fail NoSerializerError, "No serializer found for resource: #{resource.inspect}"
if serializer_class.nil?
ActiveModelSerializers.logger.debug "No serializer found for resource: #{resource.inspect}"
Copy link
Member Author

Choose a reason for hiding this comment

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

If we merge this, it fixes #1693 I believe cc @beauby

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it does, although we should also warn when we can't find a serializer for something that is not inside a collection.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean at serializer lookup on rendered resource but on associations? Or just always debug?

B mobile phone

On Jun 9, 2016, at 4:09 AM, Lucas Hosseini notifications@github.com wrote:

In lib/active_model/serializer/collection_serializer.rb:

@@ -73,8 +72,9 @@ def serializers_from_resources
def serializer_from_resource(resource, serializer_context_class, options)
serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) }

  •    if serializer_class.nil? # rubocop:disable Style/GuardClause
    
  •      fail NoSerializerError, "No serializer found for resource: #{resource.inspect}"
    
  •    if serializer_class.nil?
    
  •      ActiveModelSerializers.logger.debug "No serializer found for resource: #{resource.inspect}"
    
    I think it does, although we should also warn when we can't find a serializer for something that is not inside a collection.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

throw :no_serializer
else
serializer_class.new(resource, options.except(:serializer))
end
Expand Down
11 changes: 7 additions & 4 deletions lib/active_model/serializer/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,21 +105,24 @@ def value(serializer, include_slice)
# @api private
#
def build_association(parent_serializer, parent_serializer_options, include_slice = {})
association_value = value(parent_serializer, include_slice)
reflection_options = options.dup
association_value = value(parent_serializer, include_slice)
serializer_class = parent_serializer.class.serializer_for(association_value, reflection_options)
reflection_options[:include_data] = include_data?(include_slice)
reflection_options[:links] = @_links
reflection_options[:meta] = @_meta

if serializer_class
begin
reflection_options[:serializer] = serializer_class.new(
serializer = catch(:no_serializer) do
serializer_class.new(
association_value,
serializer_options(parent_serializer, parent_serializer_options, reflection_options)
)
rescue ActiveModel::Serializer::CollectionSerializer::NoSerializerError
end
if serializer.nil?
reflection_options[:virtual_value] = association_value.try(:as_json) || association_value
else
reflection_options[:serializer] = serializer
end
elsif !association_value.nil? && !association_value.instance_of?(Object)
reflection_options[:virtual_value] = association_value
Expand Down
7 changes: 4 additions & 3 deletions lib/active_model_serializers/serializable_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ def adapter

def find_adapter
return resource unless serializer?
ActiveModelSerializers::Adapter.create(serializer_instance, adapter_opts)
rescue ActiveModel::Serializer::CollectionSerializer::NoSerializerError
resource
adapter = catch :no_serializer do
ActiveModelSerializers::Adapter.create(serializer_instance, adapter_opts)
end
adapter || resource
end

def serializer_instance
Expand Down