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.

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

Fix root/meta for ArraySerializer #915

Closed
wants to merge 1 commit into from
Closed

Fix root/meta for ArraySerializer #915

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
5 changes: 5 additions & 0 deletions lib/active_model/serializer/adapter/json.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ module ActiveModel
class Serializer
class Adapter
class Json < Adapter
def initialize(serializer, options = {})
super
serializer.root = true if @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.

this logic doesn't make sense to me. why are we only checking if the value of options[:root] is truthy?

end

def serializable_hash(options = {})
if serializer.respond_to?(:each)
@result = serializer.map{|s| self.class.new(s).serializable_hash }
Expand Down
13 changes: 7 additions & 6 deletions lib/active_model/serializer/array_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ class ArraySerializer
include Enumerable
delegate :each, to: :@objects

attr_reader :meta, :meta_key
attr_accessor :meta, :meta_key, :root
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 the read to change the reader to an accessor?


def initialize(objects, options = {})
@root = options[:root]
options.merge!(root: nil)

@objects = objects.map do |object|
Expand All @@ -21,11 +22,11 @@ def initialize(objects, options = {})
end

def json_key
@objects.first.json_key if @objects.first
end

def root=(root)
@objects.first.root = root if @objects.first
if root == true && @objects.first
@objects.first.class.root_name
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 not sure about using the root_name here. ArraySerializer is pretty abstract, take a lott at this piece of code for example:

@objects = objects.map do |object|
  serializer_class = options.fetch(
    :serializer,
    ActiveModel::Serializer.serializer_for(object)
  )
  serializer_class.new(object, options.except(:serializer))
end

By checking this code we can presume that the objects might not be instances from the same class. Wrap them by the class name from its first object doesn't would be right.

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 problem here is the jsonapi adapter. It sets serializer.root = true in its initializer. I'm not sure how to deal with heterogenous arrays.

When all of the elements are the same, it makes sense to autodetect the json_key from the first element. When they're not the same, there's not a default that makes sense so we'd need to require a string/symbol to be passed in as the root. How do we handle the case where the root is true but we have a heterogenous array?

Copy link
Member

Choose a reason for hiding this comment

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

We could define a default data root by default is there isn't one defined. But I'm not convinced yet about the root implementation on ArraySerializer 😝

else
root
end
end
end
end
Expand Down
14 changes: 14 additions & 0 deletions test/array_serializer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,20 @@ def test_meta_and_meta_key_attr_readers
assert_equal @serializer.meta, "the meta"
assert_equal @serializer.meta_key, "the meta key"
end

def test_root_attr_reader
root_content = {root: true}
@serializer = ArraySerializer.new([@comment, @post], root_content)

assert_equal true, @serializer.root
end

def test_root_attr_reader_with_empty_array
root_content = {root: true}
@serializer = ArraySerializer.new([], root_content)

assert_equal true, @serializer.root
end
end
end
end
37 changes: 21 additions & 16 deletions test/serializers/meta_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,22 +48,27 @@ def test_meta_key_is_used
assert_equal expected, adapter.as_json
end

def test_meta_is_not_used_on_arrays
serializer = ArraySerializer.new([@blog], root: "blog", meta: {total: 10}, meta_key: "haha_meta")
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)
expected = [{
id: 1,
name: "AMS Hints",
writer: {
id: 2,
name: "Steve"
},
articles: [{
id: 3,
title: "AMS",
body: nil
}]
}]
def test_meta_is_used_on_arrays
serializer = ArraySerializer.new([@blog], meta: {total: 10}, meta_key: "haha_meta")
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer, root: 'blog')
expected = {
'blog' => [{
id: 1,
name: "AMS Hints",
writer: {
id: 2,
name: "Steve"
},
articles: [{
id: 3,
title: "AMS",
body: nil
}]
}],
'haha_meta' => {
total: 10
}
}
assert_equal expected, adapter.as_json
end

Expand Down