Skip to content

[FIX] Fetch json key from item serializer if empty collection is pass… #1537

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[FIX] Fetch json key from item serializer if empty collection is pass… #1537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 17 additions & 4 deletions lib/active_model/serializer/collection_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,14 @@ class CollectionSerializer
def initialize(resources, options = {})
@root = options[:root]
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 thinking this should really be @root = collection_root(options[:root], options[:serializer])

and something like (assuming root and json_key should behave the same... I don't remember)

private

def collection_root(explicit_root, explicit_each_serializer)
  explicit_root || derived_root(explicit_each_serializer)
end

def derived_root(explicit_each_serializer)
 serializers.first.try!(:json_key).try!(:pluralize) ||
    (resources.empty && explicit_each_serializer.try!(:allocate).try!(:json_key).try!(:pluralize)) ||
    object.try!(:name).try!(:underscore).try!(:pluralize)
end

aside: all these try make my head hurt. ( http://devblog.avdi.org/2015/10/30/activesupports-try-might-not-be-doing-what-you-think-its-doing/ )

Copy link
Member

Choose a reason for hiding this comment

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

👍 I also don't really enjoy all these try.

Copy link
Member

Choose a reason for hiding this comment

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

Right now, the only scenario tested is essentially:

@root = collection_root(options)

# condenses `root || derived_root`
def collection_root(options)
  if root = options[:root]
    root
  else
-   key = serializers.first.try(:json_key) || object.try(:name).try(:underscore)
+   key = serializers.first.try(:json_key) || options[:serializer].try!(:allocate).try!(:json_key).try!(pluralize) || object.try(:name).try(:underscore)
    key.try(:pluralize)
  end
end

(or a more thorough impl would be something like)

@root = collection_root(options)

# condenses `root || derived_root`
# might have bugs that were being hidden by the `try` code
def collection_root(options)
  if root = options[:root]
    root
  else
    # 1. get from options[:serializer] for empty resource collection
    key = object.empty? && (serializer_class = options[:serializer]) && (serializer_class.allocate.json_key.try!(:pluralize) || serializer_class._type) # shouldn't need the allocate here
    # 2. get from first serializer instance in collection
    key ||= (serializer = serializers.first) && serializer.json_key
    # 3. get from first resource in collection
    key ||= object.any? && object.name.underscore
    # 4. oh no! we should never be here. empty collection and no serializer option
    key ||= ''
    key.pluralize
  end
end

I'm also realizing we're inconsistent with object vs. resources. We should probably always refer to it as object (which is the serializer api for the thing it encapsulates).

Copy link
Member

Choose a reason for hiding this comment

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

@bf4 @NullVoxPopuli @remear I really think that allocate is not the right solution here. json_key by default use root or the object (https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer.rb#L185). Neither of these would be available and might result in a bug if the json_key method is not overidden. That might also break for polymorphic collections.

I stay on my position that the prefered solution here would be the ones described at #1536 (comment). Or even subclass the collection serializer and add its own root handling.

@object = resources

serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer)

if resources.blank? && options[:serializer]
@each_serializer = options[:serializer]
Copy link
Member

Choose a reason for hiding this comment

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

@each_serializer is too much linked to the controller option name IMO. I'd rather call it @resource_serializer or simply @serializer.

Copy link
Member

Choose a reason for hiding this comment

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

Or explicit _collection_serializer

On Fri, Feb 26, 2016 at 3:43 AM Yohan Robert [email protected]
wrote:

In lib/active_model/serializer/collection_serializer.rb
#1537 (comment)
:

@@ -10,8 +10,14 @@ class CollectionSerializer
def initialize(resources, options = {})
@root = options[:root]
@object = resources
+

  •    serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer)

  •    if resources.blank? && options[:serializer]

  •      @each_serializer = options[:serializer]

@each_serializer is too much linked to the controller option name IMO.
I'd rather call it @resource_serializer or simply @serializer.

—
Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1537/files#r54222291
.

end

@serializers = resources.map do |resource|
serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer)
serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) }

if serializer_class.nil?
Expand All @@ -23,7 +29,7 @@ def initialize(resources, options = {})
end

def json_key
root || derived_root
root || derived_root || guess_root || default_root
end

def paginated?
Expand All @@ -39,8 +45,15 @@ def paginated?
private

def derived_root
key = serializers.first.try(:json_key) || object.try(:name).try(:underscore)
key.try(:pluralize)
serializers.first.try(:json_key).try(:pluralize)
Copy link
Member

Choose a reason for hiding this comment

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

aside: we really need to replace Serializer#json_key with Serializer._type...

Copy link
Member

Choose a reason for hiding this comment

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

Here what about:

def derived_root
  return unless serializers.empty?
  serializers.first.json_key.to_s.pluralize
end

Makes it more readable IMO.

end

def default_root
object.try(:name).try(:underscore).try(:pluralize)
Copy link
Member

Choose a reason for hiding this comment

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

What about:

def default_root
  return unless object.respond_to?(:name)
  object.name.to_s.underscore.pluralize
end

end

def guess_root
@each_serializer.try(:allocate).try(:json_key).try(:pluralize)
end
end
end
Expand Down
6 changes: 6 additions & 0 deletions test/collection_serializer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ def test_json_key_with_resource_without_name_and_no_serializers
assert_nil serializer.json_key
end

def test_json_key_with_empty_resources_with_serializer
resource = []
serializer = collection_serializer.new(resource, serializer: MessagesSerializer)
assert_equal 'messages', serializer.json_key
end

def test_json_key_with_root
expected = 'custom_root'
serializer = collection_serializer.new(@resource, root: expected)
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/poro.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ def json_key
end
end

MessagesSerializer = Class.new(ActiveModel::Serializer) do
def json_key
'messages'
end
end
Copy link
Member

Choose a reason for hiding this comment

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

would you mind moving this to the test file? poro's is a little cluttered

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


AlternateBlogSerializer = Class.new(ActiveModel::Serializer) do
attribute :id
attribute :name, key: :title
Expand Down