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

Fix #876 - include meta when using json adapter with custom root #936

Merged
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: 1 addition & 1 deletion lib/active_model/serializer/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def meta_key
end

def root
serializer.json_key
@options.fetch(:root) { serializer.json_key }
end

def include_meta(json)
Expand Down
7 changes: 4 additions & 3 deletions lib/active_model/serializer/adapter/json.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@ def serializable_hash(options = {})
@result = @core.merge @hash
end

if root = options.fetch(:root, serializer.json_key)
if root
@result = { root => @result }
else
@result
end
@result
end
end

Expand All @@ -49,4 +50,4 @@ def fragment_cache(cached_hash, non_cached_hash)
end
end
end
end
end
39 changes: 39 additions & 0 deletions test/action_controller/serialization_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ def render_using_custom_root
render json: @profile, root: "custom_root"
end

def render_using_custom_root_and_meta
@profile = Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' })
render json: @profile, root: "custom_root", meta: { total: 10 }
end

def render_using_default_adapter_root
with_adapter ActiveModel::Serializer::Adapter::JsonApi do
# JSON-API adapter sets root by default
Expand All @@ -28,6 +33,14 @@ def render_using_custom_root_in_adapter_with_a_default
render json: @profile, root: "profile", adapter: :json_api
end

def render_array_using_custom_root_and_meta
array = [
Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' }),
Profile.new({ name: 'Name 2', description: 'Description 2', comments: 'Comments 2' })
]
render json: array, root: "custom_root", meta: { total: 10 }
end

def render_array_using_implicit_serializer
array = [
Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' }),
Expand Down Expand Up @@ -161,6 +174,13 @@ def test_render_using_custom_root
assert_equal '{"custom_root":{"name":"Name 1","description":"Description 1"}}', @response.body
end

def test_render_using_custom_root_and_meta
get :render_using_custom_root_and_meta

assert_equal 'application/json', @response.content_type
assert_equal '{"custom_root":{"name":"Name 1","description":"Description 1"},"meta":{"total":10}}', @response.body
end

def test_render_using_default_root
get :render_using_default_adapter_root

Expand Down Expand Up @@ -197,6 +217,25 @@ def test_render_using_custom_root_in_adapter_with_a_default
assert_equal expected.to_json, @response.body
end

def test_render_array_using_custom_root_and_meta
get :render_array_using_custom_root_and_meta
assert_equal 'application/json', @response.content_type

expected = { custom_root: [
{
name: 'Name 1',
description: 'Description 1',
},
{
name: 'Name 2',
description: 'Description 2',
}],
meta: { total: 10 }
}

assert_equal expected.to_json, @response.body
end

def test_render_array_using_implicit_serializer
get :render_array_using_implicit_serializer
assert_equal 'application/json', @response.content_type
Expand Down
35 changes: 31 additions & 4 deletions test/serializers/meta_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ 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")
def test_meta_is_not_present_on_arrays_without_root
serializer = ArraySerializer.new([@blog], meta: {total: 10})
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)
expected = [{
id: 1,
Expand All @@ -67,11 +67,38 @@ def test_meta_is_not_used_on_arrays
assert_equal expected, adapter.as_json
end

def test_meta_is_present_on_arrays_with_root
serializer = ArraySerializer.new([@blog], meta: {total: 10}, meta_key: "haha_meta")
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer, root: 'blog')
Copy link
Member

Choose a reason for hiding this comment

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

so, you moved the root: "blog" form the constructor of the ArraySerializer to ActiveModel::Serializer::Adapter::Json because the way AMS is used in the controller, root is sent only to the adapter, never to the serializer?

Since it's basically wrong to pass root into ArraySerializer, maybe, not in this PR, we should make that raise an exception or issue a warning "Hey, serializer doesn't get root, the adapter does"

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

private

def load_adapter(options)
serializer = AlternateBlogSerializer.new(@blog, options)
ActiveModel::Serializer::Adapter::Json.new(serializer)
adapter_opts, serializer_opts =
options.partition { |k, _| ActionController::Serialization::ADAPTER_OPTION_KEYS.include? k }.map { |h| Hash[h] }

Copy link
Member

Choose a reason for hiding this comment

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

wow is this screaming for a refactor of

define_method renderer_method do |resource, options|
@_adapter_opts, @_serializer_opts =
options.partition { |k, _| ADAPTER_OPTION_KEYS.include? k }.map { |h| Hash[h] }
if use_adapter? && (serializer = get_serializer(resource))
@_serializer_opts[:scope] ||= serialization_scope
@_serializer_opts[:scope_name] = _serialization_scope
# omg hax
object = serializer.new(resource, @_serializer_opts)
adapter = ActiveModel::Serializer::Adapter.create(object, @_adapter_opts)
super(adapter, options)
or what? (not in this PR, just saying)

I remember reading through all these tests and code while trying to figure out how to test my serializers. I ended up doing something like in my comment in 878 which was a lot more work that you'd think. Also, too bad minitest doesn't have shared examples, though I think ActiveModel::Lint might be a fine pattern.

serializer = AlternateBlogSerializer.new(@blog, serializer_opts)
ActiveModel::Serializer::Adapter::Json.new(serializer, adapter_opts)
end
end
end
Expand Down