-
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
root option is working (fixed #986) #987
Conversation
|
||
def setup | ||
@profile = Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' }) | ||
@profile_serializer = ProfileSerializer.new(@post, {root: 'smth'}) |
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.
These tests are testing the value of Serializer#json_key, but the issue in #986 was
render json: @current_user, root: 'resource', adapter: :json
Which uses the transformations in the controller (something like JsonAdapter.create(ProfileSerializer.new(@post, root: 'smth')).serializable_hash
). I'm not sure you just want to test ProfileSerializer.new(@post, root: 'smth').json_key
as that's not testing the result you want:
{
- "user": {
+ "resource": {
"id": 1,
"name": "Rambo"
}
}
That said, the behavior you desire was specifically removed from AMS in #958
# controller
- ADAPTER_OPTION_KEYS = [:include, :fields, :root, :adapter]
+ ADAPTER_OPTION_KEYS = [:include, :fields, :adapter]
# serializer
def initialize(object, options = {})
- @root = options[:root] || (self.class._root ? self.class.root_name : false)
+ @root = options[:root]
def json_key
- if root == true || root.nil?
- self.class.root_name
- else
- root
- end
+ self.class.root_name
end
# adapter
def root
- @options.fetch(:root) { serializer.json_key }
+ serializer.json_key.to_sym if serializer.json_key
end
def include_meta(json)
- json[meta_key] = meta if meta && root
+ json[meta_key] = meta if meta
json
end
I brought up documenting these changes in #966 but it hasn't been merged/finalized yet
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.
👍 Indeed. We could bring some of the older tests to validate it.
One thing to note is that the root option is ignored when serializing an empty array. |
The PR ported some tests from joaomdmoura@3296912. Let me know if there are other tests that need to be ported back. |
Will be very grateful for a quick resolution.