-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[WIP] Move root option handling to adapter #1782
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 |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| module ActiveModel | ||
| class Serializer | ||
| # @see #serializable_hash for more details on these valid keys. | ||
| SERIALIZABLE_HASH_VALID_KEYS = [:only, :except, :methods, :include, :root].freeze | ||
| SERIALIZABLE_HASH_VALID_KEYS = [:only, :except, :methods, :include].freeze | ||
| extend ActiveSupport::Autoload | ||
| autoload :Adapter | ||
| autoload :Null | ||
|
|
@@ -114,15 +114,14 @@ def self.serialization_adapter_instance | |
| @serialization_adapter_instance ||= ActiveModelSerializers::Adapter::Attributes | ||
| end | ||
|
|
||
| attr_accessor :object, :root, :scope | ||
| attr_accessor :object, :scope | ||
|
|
||
| # `scope_name` is set as :current_user by default in the controller. | ||
| # If the instance does not have a method named `scope_name`, it | ||
| # defines the method so that it calls the +scope+. | ||
| def initialize(object, options = {}) | ||
| self.object = object | ||
| self.instance_options = options | ||
| self.root = instance_options[:root] | ||
| self.scope = instance_options[:scope] | ||
|
|
||
| scope_name = instance_options[:scope_name] | ||
|
|
@@ -178,8 +177,7 @@ def serializable_hash(adapter_options = nil, options = {}, adapter_instance = se | |
| # TODO: When moving attributes adapter logic here, @see #serializable_hash | ||
| # So that the below is true: | ||
| # @param options [nil, Hash] The same valid options passed to `as_json` | ||
| # (:root, :only, :except, :methods, and :include). | ||
| # The default for `root` is nil. | ||
| # (:only, :except, :methods, and :include). | ||
| # The default value for include_root is false. You can change it to true if the given | ||
| # JSON string includes a single root node. | ||
| def as_json(adapter_opts = nil) | ||
|
|
@@ -188,7 +186,7 @@ def as_json(adapter_opts = nil) | |
|
|
||
| # Used by adapter as resource root. | ||
| def json_key | ||
| root || _type || object.class.model_name.to_s.underscore | ||
| _type || object.class.model_name.to_s.underscore | ||
|
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. This is now done here.
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. as https://github.com/rails-api/active_model_serializers/pull/1782/files#r66382756 I'm fine removing |
||
| end | ||
|
|
||
| def read_attribute_for_serialization(attr) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,12 +5,11 @@ class CollectionSerializer | |
| include Enumerable | ||
| delegate :each, to: :@serializers | ||
|
|
||
| attr_reader :object, :root | ||
| attr_reader :object | ||
|
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. I'm trying to think if this needs a deprecation. |
||
|
|
||
| def initialize(resources, options = {}) | ||
| @object = resources | ||
| @options = options | ||
| @root = options[:root] | ||
| @serializers = serializers_from_resources | ||
| end | ||
|
|
||
|
|
@@ -37,7 +36,6 @@ def serializable_hash(adapter_options, options, adapter_instance) | |
| # Disabling cop since it's good to highlight the complexity of this method by | ||
| # including all the logic right here. | ||
| def json_key | ||
|
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. I'd love for this method to be simpler :( |
||
| return root if root | ||
| # 1. get from options[:serializer] for empty resource collection | ||
| key = object.empty? && | ||
| (explicit_serializer_class = options[:serializer]) && | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,14 @@ def serializable_hash(options = nil) | |
| self.class.transform_key_casing!(serialized_hash, instance_options) | ||
| end | ||
|
|
||
| def root | ||
| if instance_options.key?(:root) | ||
| instance_options[:root] | ||
| elsif serializer.json_key | ||
| serializer.json_key.to_sym | ||
|
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. Not sure why |
||
| end | ||
| end | ||
|
|
||
| def meta | ||
| instance_options.fetch(:meta, nil) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
|
|
||
| module ActiveModelSerializers | ||
| class SerializableResource | ||
| ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :meta, :meta_key, :links, :serialization_context, :key_transform]) | ||
| ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :meta, | ||
| :meta_key, :links, :root, | ||
| :serialization_context, :key_transform]) | ||
|
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. Added
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. As opposed to https://github.com/rails-api/active_model_serializers/pull/1782/files#r66382756 👍 here since it's the adapter's job to build the JSON response. the serializer just provides 'attributes' and 'associations' and hints as to what it's root should be, when needed |
||
| include ActiveModelSerializers::Logging | ||
|
|
||
| delegate :serializable_hash, :as_json, :to_json, to: :adapter | ||
|
|
||
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.
root still belongs here, though technically it is an option for
as_json. This isn't about partitioning and need not be changed. In fact, it's an option in the ActiveModel::Serializers::JSON interfaceThere 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.
You're right, will revert.