-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Transform keys referenced in values in serialized documents #1645
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
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
[Back to Guides](../README.md) | ||
|
||
# Key Transforms | ||
|
||
Key Transforms modify the casing of keys and keys referenced in values in | ||
serialized responses. | ||
|
||
Provided key transforms: | ||
|
||
| Option | Result | | ||
|----|----| | ||
| `:camel` | ExampleKey | | ||
| `:camel_lower` | exampleKey | | ||
| `:dash` | example-key | | ||
| `:unaltered` | the original, unaltered key | | ||
| `:underscore` | example_key | | ||
| `nil` | use the adapter default | | ||
|
||
Key translation precedence is as follows: | ||
|
||
##### Adapter option | ||
|
||
`key_transform` is provided as an option via render. | ||
|
||
```render json: posts, each_serializer: PostSerializer, key_transform: :camel_lower``` | ||
|
||
##### Configuration option | ||
|
||
`key_transform` is set in `ActiveModelSerializers.config.key_transform`. | ||
|
||
```ActiveModelSerializers.config.key_transform = :camel_lower``` | ||
|
||
##### Adapter default | ||
|
||
Each adapter has a default transform configured: | ||
|
||
| Adapter | Default Key Transform | | ||
|----|----| | ||
| `Json` | `:unaltered` | | ||
| `JsonApi` | `:dash` | |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ class Json < Base | |
def serializable_hash(options = nil) | ||
options ||= {} | ||
serialized_hash = { root => Attributes.new(serializer, instance_options).serializable_hash(options) } | ||
transform_key_casing!(serialized_hash, options[:serialization_context]) | ||
self.class.transform_key_casing!(serialized_hash, options) | ||
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. how come this is a class method on the adapter? 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. (philosophical/stategic/curious) 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. To facilitate things like https://github.com/rails-api/active_model_serializers/pull/1645/files#diff-65abd6a4fd7fd55fe487ff138e0841dbR8. A class method here seemed like the best option at the time. I figured if there's a better way we could discuss refactoring and handle that in another PR instead of increasing the scope of this one. |
||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,8 +37,8 @@ def initialize(serializer, options = {}) | |
@fieldset = options[:fieldset] || ActiveModel::Serializer::Fieldset.new(options.delete(:fields)) | ||
end | ||
|
||
def default_key_transform | ||
:dashed | ||
def self.default_key_transform | ||
:dash | ||
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. oy |
||
end | ||
|
||
# {http://jsonapi.org/format/#crud Requests are transactional, i.e. success or failure} | ||
|
@@ -48,9 +48,9 @@ def serializable_hash(options = nil) | |
document = if serializer.success? | ||
success_document(options) | ||
else | ||
failure_document | ||
failure_document(options) | ||
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. do the options passed to here also trigger key transforming? 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. In the sense that if |
||
end | ||
transform_key_casing!(document, options[:serialization_context]) | ||
self.class.transform_key_casing!(document, options) | ||
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. why was the serialization_context passed before? and now just options? 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 think #1643 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. nice 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. When I introduced this I thought it made more sense to pass just the |
||
end | ||
|
||
# {http://jsonapi.org/format/#document-top-level Primary data} | ||
|
@@ -71,7 +71,7 @@ def serializable_hash(options = nil) | |
def success_document(options) | ||
is_collection = serializer.respond_to?(:each) | ||
serializers = is_collection ? serializer : [serializer] | ||
primary_data, included = resource_objects_for(serializers) | ||
primary_data, included = resource_objects_for(serializers, options) | ||
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. seems strange that 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. Yeah, I'm not a fan. |
||
|
||
hash = {} | ||
# toplevel_data | ||
|
@@ -148,7 +148,7 @@ def success_document(options) | |
# }.reject! {|_,v| v.nil? } | ||
# prs: | ||
# https://github.com/rails-api/active_model_serializers/pull/1004 | ||
def failure_document | ||
def failure_document(options) | ||
hash = {} | ||
# PR Please :) | ||
# Jsonapi.add!(hash) | ||
|
@@ -163,10 +163,10 @@ def failure_document | |
# ] | ||
if serializer.respond_to?(:each) | ||
hash[:errors] = serializer.flat_map do |error_serializer| | ||
Error.resource_errors(error_serializer) | ||
Error.resource_errors(error_serializer, options) | ||
end | ||
else | ||
hash[:errors] = Error.resource_errors(serializer) | ||
hash[:errors] = Error.resource_errors(serializer, options) | ||
end | ||
hash | ||
end | ||
|
@@ -224,21 +224,21 @@ def fragment_cache(cached_hash, non_cached_hash) | |
# [x] url helpers https://github.com/rails-api/active_model_serializers/issues/1269 | ||
# meta | ||
# [x] https://github.com/rails-api/active_model_serializers/pull/1340 | ||
def resource_objects_for(serializers) | ||
def resource_objects_for(serializers, options) | ||
@primary = [] | ||
@included = [] | ||
@resource_identifiers = Set.new | ||
serializers.each { |serializer| process_resource(serializer, true) } | ||
serializers.each { |serializer| process_relationships(serializer, @include_tree) } | ||
serializers.each { |serializer| process_resource(serializer, true, options) } | ||
serializers.each { |serializer| process_relationships(serializer, @include_tree, options) } | ||
|
||
[@primary, @included] | ||
end | ||
|
||
def process_resource(serializer, primary) | ||
resource_identifier = ResourceIdentifier.new(serializer).as_json | ||
def process_resource(serializer, primary, options) | ||
resource_identifier = ResourceIdentifier.new(serializer, options).as_json | ||
return false unless @resource_identifiers.add?(resource_identifier) | ||
|
||
resource_object = resource_object_for(serializer) | ||
resource_object = resource_object_for(serializer, options) | ||
if primary | ||
@primary << resource_object | ||
else | ||
|
@@ -248,21 +248,21 @@ def process_resource(serializer, primary) | |
true | ||
end | ||
|
||
def process_relationships(serializer, include_tree) | ||
def process_relationships(serializer, include_tree, options) | ||
serializer.associations(include_tree).each do |association| | ||
process_relationship(association.serializer, include_tree[association.key]) | ||
process_relationship(association.serializer, include_tree[association.key], options) | ||
end | ||
end | ||
|
||
def process_relationship(serializer, include_tree) | ||
def process_relationship(serializer, include_tree, options) | ||
if serializer.respond_to?(:each) | ||
serializer.each { |s| process_relationship(s, include_tree) } | ||
serializer.each { |s| process_relationship(s, include_tree, options) } | ||
return | ||
end | ||
return unless serializer && serializer.object | ||
return unless process_resource(serializer, false) | ||
return unless process_resource(serializer, false, options) | ||
|
||
process_relationships(serializer, include_tree) | ||
process_relationships(serializer, include_tree, options) | ||
end | ||
|
||
# {http://jsonapi.org/format/#document-resource-object-attributes Document Resource Object Attributes} | ||
|
@@ -286,9 +286,9 @@ def attributes_for(serializer, fields) | |
end | ||
|
||
# {http://jsonapi.org/format/#document-resource-objects Document Resource Objects} | ||
def resource_object_for(serializer) | ||
def resource_object_for(serializer, options) | ||
resource_object = cache_check(serializer) do | ||
resource_object = ResourceIdentifier.new(serializer).as_json | ||
resource_object = ResourceIdentifier.new(serializer, options).as_json | ||
|
||
requested_fields = fieldset && fieldset.fields_for(resource_object[:type]) | ||
attributes = attributes_for(serializer, requested_fields) | ||
|
@@ -297,7 +297,7 @@ def resource_object_for(serializer) | |
end | ||
|
||
requested_associations = fieldset.fields_for(resource_object[:type]) || '*' | ||
relationships = relationships_for(serializer, requested_associations) | ||
relationships = relationships_for(serializer, requested_associations, options) | ||
resource_object[:relationships] = relationships if relationships.any? | ||
|
||
links = links_for(serializer) | ||
|
@@ -425,15 +425,16 @@ def resource_object_for(serializer) | |
# id: 'required-id', | ||
# meta: meta | ||
# }.reject! {|_,v| v.nil? } | ||
def relationships_for(serializer, requested_associations) | ||
def relationships_for(serializer, requested_associations, options) | ||
include_tree = ActiveModel::Serializer::IncludeTree.from_include_args(requested_associations) | ||
serializer.associations(include_tree).each_with_object({}) do |association, hash| | ||
hash[association.key] = Relationship.new( | ||
serializer, | ||
association.serializer, | ||
association.options, | ||
association.links, | ||
association.meta | ||
options, | ||
options: association.options, | ||
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. thanks for using named parameters. I love named parameters on anything with 2 or more parameters. |
||
links: association.links, | ||
meta: association.meta | ||
).as_json | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,14 +182,14 @@ def parse_relationship(assoc_name, assoc_data, options) | |
prefix_key = field_key(assoc_name, options).to_s.singularize | ||
hash = | ||
if assoc_data.is_a?(Array) | ||
{ "#{prefix_key}_ids".to_sym => assoc_data.map { |ri| ri[:id] } } | ||
{ "#{prefix_key}_ids".to_sym => assoc_data.map { |ri| ri['id'] } } | ||
else | ||
{ "#{prefix_key}_id".to_sym => assoc_data && assoc_data.is_a?(Hash) ? assoc_data[:id] : nil } | ||
{ "#{prefix_key}_id".to_sym => assoc_data ? assoc_data['id'] : nil } | ||
end | ||
|
||
polymorphic = (options[:polymorphic] || []).include?(assoc_name.to_sym) | ||
if polymorphic | ||
hash["#{prefix_key}_type".to_sym] = assoc_data.present? ? assoc_data[:type] : nil | ||
hash["#{prefix_key}_type".to_sym] = assoc_data.present? ? assoc_data['type'] : nil | ||
end | ||
|
||
hash | ||
|
@@ -198,7 +198,7 @@ def parse_relationship(assoc_name, assoc_data, options) | |
# @api private | ||
def parse_relationships(relationships, options) | ||
transform_keys(relationships, options) | ||
.map { |(k, v)| parse_relationship(k, v[:data], options) } | ||
.map { |(k, v)| parse_relationship(k, v['data'], options) } | ||
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 assume this is a bug fix in master? 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. Not sure if it's a bug. KeyTransform in master always returned symbols so a recent PR changes this code to work with that restriction. This PR makes Transform more flexible so that set of changes isn't necessary anymore. 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. 👍 |
||
.reduce({}, :merge) | ||
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.
This message will remain until the next rc5 or 0.10.0 whicher comes first
?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.
Mehhhh, the
post_install_message
is not a good way to issue a deprecation I think. I mean, there is even a gem for ignoring post install messages that is pretty popular.... https://github.com/tpope/gem-shut-the-fuck-upWhy not just issue a real deprecation warning?
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.
@ianks as our deprecation champion for this change, how would you recommend we do this? it's a changed default, not method, so we can't reroute.. right? or you think the change you be reverted for now?
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.
Champion, eh?! I guess I have a vested interest now 😆
The only time that this issue really affects people is when the options is nil, where it falls back to the default for the adapter. I would recommend that instead of accepting
nil
silently, we issue a deprecation notice when the option is not set.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.
Basically, issue a deprecation in the
default_key_transform
method.