-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Move handling of root option from serializer to adapter.
#1234
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 |
|---|---|---|
|
|
@@ -171,7 +171,7 @@ def test_render_array_using_custom_root | |
| with_adapter :json do | ||
| get :render_array_using_custom_root | ||
| end | ||
| expected = { custom_roots: [{ name: 'Name 1', description: 'Description 1' }] } | ||
| expected = { custom_root: [{ name: 'Name 1', description: 'Description 1' }] } | ||
|
Member
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 raises a huge red flag for me. Why is this changing?
Contributor
Author
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 believe it should not have been there in the first place. Modifying explicitly supplied options is such an antipattern. I could move this change to an other PR though. |
||
| assert_equal 'application/json', @response.content_type | ||
| assert_equal expected.to_json, @response.body | ||
| end | ||
|
|
@@ -181,7 +181,7 @@ def test_render_array_that_is_empty_using_custom_root | |
| get :render_array_that_is_empty_using_custom_root | ||
| end | ||
|
|
||
| expected = { custom_roots: [] } | ||
| expected = { custom_root: [] } | ||
| assert_equal 'application/json', @response.content_type | ||
| assert_equal expected.to_json, @response.body | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| require 'test_helper' | ||
|
|
||
| module ActiveModel | ||
| class Serializer | ||
| module Adapter | ||
| class Json | ||
| class ArrayTest < Minitest::Test | ||
| def setup | ||
| @comment = Comment.new | ||
| @post = Post.new | ||
| @resource = build_named_collection @comment, @post | ||
| end | ||
|
|
||
| def build_named_collection(*resource) | ||
| resource.define_singleton_method(:name) { 'MeResource' } | ||
| resource | ||
| end | ||
|
|
||
| def test_root_default | ||
| serializer = ArraySerializer.new([@comment, @post]) | ||
| adapter = Json.new(serializer) | ||
| assert_equal(:comments, adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root | ||
| serializer = ArraySerializer.new([@comment, @post]) | ||
| adapter = Json.new(serializer, root: 'custom_root') | ||
| assert_equal(:custom_root, adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root_with_no_serializers | ||
| serializer = ArraySerializer.new([]) | ||
| adapter = Json.new(serializer, root: 'custom_root') | ||
| assert_equal(:custom_root, adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root_with_resource_with_name_and_serializers | ||
| serializer = ArraySerializer.new(@resource) | ||
| adapter = Json.new(serializer) | ||
| assert_equal(:comments, adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root_with_resource_with_name_and_no_serializers | ||
| serializer = ArraySerializer.new(build_named_collection) | ||
| adapter = Json.new(serializer) | ||
| assert_equal(:me_resources, adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root_with_resource_with_nil_name_and_no_serializers | ||
| resource = [] | ||
| resource.define_singleton_method(:name) { nil } | ||
| serializer = ArraySerializer.new(resource) | ||
| adapter = Json.new(serializer) | ||
| assert_equal(:'', adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root_with_resource_without_name_and_no_serializers | ||
| serializer = ArraySerializer.new([]) | ||
| adapter = Json.new(serializer) | ||
| assert_equal(:'', adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root_with_explicit_root | ||
| serializer = ArraySerializer.new(@resource) | ||
| adapter = Json.new(serializer, root: 'custom_root') | ||
| assert_equal(:custom_root, adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_root_with_explicit_root_and_no_serializers | ||
| serializer = ArraySerializer.new(build_named_collection) | ||
| adapter = Json.new(serializer, root: 'custom_root') | ||
| assert_equal(:custom_root, adapter.send(:root)) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| require 'test_helper' | ||
|
|
||
| module ActiveModel | ||
| class Serializer | ||
| module Adapter | ||
| class Json | ||
| class RootTest < Minitest::Test | ||
| def setup | ||
| @virtual_value = VirtualValue.new(id: 1) | ||
| end | ||
|
|
||
| def test_overwrite_root | ||
| serializer = VirtualValueSerializer.new(@virtual_value) | ||
| adapter = Json.new(serializer, root: 'smth') | ||
| assert_equal(:smth, adapter.send(:root)) | ||
| end | ||
|
|
||
| def test_underscore_in_root | ||
| serializer = VirtualValueSerializer.new(@virtual_value) | ||
| adapter = Json.new(serializer) | ||
| assert_equal(:virtual_value, adapter.send(:root)) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,11 @@ def test_includes_post_id | |
| end | ||
|
|
||
| def test_includes_linked_post | ||
| @adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:post]) | ||
| actual = ActiveModel::SerializableResource.new( | ||
|
Member
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. use the
Contributor
Author
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. Is the helper globally available? I think it should be. If so, I'll update these tests. |
||
| @comment, | ||
| adapter: :json_api, | ||
| include: [:post]) | ||
| .serializable_hash | ||
| expected = [{ | ||
| id: '42', | ||
| type: 'posts', | ||
|
|
@@ -52,11 +56,16 @@ def test_includes_linked_post | |
| author: { data: { type: 'authors', id: '1' } } | ||
| } | ||
| }] | ||
| assert_equal expected, @adapter.serializable_hash[:included] | ||
| assert_equal expected, actual[:included] | ||
| end | ||
|
|
||
| def test_limiting_linked_post_fields | ||
| @adapter = ActiveModel::Serializer::Adapter::JsonApi.new(@serializer, include: [:post], fields: { post: [:title] }) | ||
| actual = ActiveModel::SerializableResource.new( | ||
| @comment, | ||
| adapter: :json_api, | ||
| include: [:post], | ||
| fields: { post: [:title] }) | ||
| .serializable_hash | ||
| expected = [{ | ||
| id: '42', | ||
| type: 'posts', | ||
|
|
@@ -69,7 +78,7 @@ def test_limiting_linked_post_fields | |
| author: { data: { type: 'authors', id: '1' } } | ||
| } | ||
| }] | ||
| assert_equal expected, @adapter.serializable_hash[:included] | ||
| assert_equal expected, actual[:included] | ||
| end | ||
|
|
||
| def test_include_nil_author | ||
|
|
||
This file was deleted.
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.
vs.
and
?
Shouldn't this just be using the Serializer
typeor am I tired?I'm not convinced
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.
Yes. Highly possible that the serializer's
typeis the right thing to use here. Main point of this PR was that an option that is specific to theJsonadapter should not clutter theSerializerandAdapter::Baseclasses with lots of overcomplicated code.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.
Note that
typeis nil unless explicitly specified though.