-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
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 | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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, | ||||||||||||||||||||||||||||
|
@@ -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') | ||||||||||||||||||||||||||||
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] } | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
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. wow is this screaming for a refactor of active_model_serializers/lib/action_controller/serialization.rb Lines 37 to 49 in 35fb9de
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 | ||||||||||||||||||||||||||||
|
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.
so, you moved the
root: "blog"
form the constructor of theArraySerializer
toActiveModel::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
intoArraySerializer
, 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"