-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Namespaced serializer lookup for associations #1196
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
6e3be30
36cbc51
1896039
b4ebcb7
95607f6
3773fa3
34ba9f6
96834b6
2b6a390
f924a19
3b914ca
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 |
|---|---|---|
|
|
@@ -25,7 +25,8 @@ def get_serializer(resource, options = {}) | |
| "Please pass 'adapter: false' or see ActiveSupport::SerializableResource.new" | ||
| options[:adapter] = false | ||
| end | ||
| serializable_resource = ActiveModel::SerializableResource.new(resource, options) | ||
| serializable_options = options.merge(controller: self) | ||
|
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. why not mutate?
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. also why change options when not serializing
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. and do we need the whole controller? I'd rather not and looks like you just want the class name
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. Currently we just need the class name but there's virtually 0 overhead to getting the instance, so why not?
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. If you make it easy to use the controller directly, people will, and the scope will creep and and new ams will gain responsibilities it shouldn't gave and cause pain and bugs to users and maintainers Failure of Interface segregation, not keepig it narrow
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. In my opinion, the real issue lies in the fact that we miss a layer of separation between the serializer definition and the serializer instances. |
||
| serializable_resource = ActiveModel::SerializableResource.new(resource, serializable_options) | ||
| if serializable_resource.serializer? | ||
| serializable_resource.serialization_scope ||= serialization_scope | ||
| serializable_resource.serialization_scope_name = _serialization_scope | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,8 +84,10 @@ def self.serializer_for(resource, options = {}) | |
| resource.serializer_class | ||
| elsif resource.respond_to?(:to_ary) | ||
| config.array_serializer | ||
| elsif options.key?(:serializer) | ||
| options[:serializer] | ||
| else | ||
| options.fetch(:serializer) { get_serializer_for(resource.class) } | ||
| get_serializer_for(resource.class, options[:parent_serializer], options[:controller]) | ||
|
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. this is getting pretty hairy
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. Yeah, I think the whole |
||
| end | ||
| end | ||
|
|
||
|
|
@@ -108,20 +110,50 @@ def self.digest_caller_file(caller_line) | |
| Digest::MD5.hexdigest(serializer_file_contents) | ||
| end | ||
|
|
||
| def self.get_serializer_for(klass) | ||
| # Compute the lookup chain for a given serializer class, parent serializer and controller | ||
| # @param [String] serializer_class_name The class name to lookup | ||
| # @param [ActiveModel::Serializer] parent_serializer The instance of the parent serializer, if any | ||
| # @param [ActionController] controller The instance of the calling controller, if any | ||
| # | ||
| # @return [Array<String>] The lookup chain | ||
| # | ||
| def self.serializer_lookup_chain_for(serializer_class_name, parent_serializer, controller) | ||
| chain = [] | ||
|
|
||
| # Look for a serializer nested inside the current serializer first, if inside a user-defined serializer | ||
| chain.push("#{self}::#{serializer_class_name}") if self.class != ActiveModel::Serializer | ||
|
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. That kind of check makes me believe it might be time to make
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. It's been around for a while, so I'd rather deprecate it |
||
|
|
||
| # Look for a serializer inside the controller namespace | ||
| chain.push("#{controller.class.name.deconstantize}::#{serializer_class_name}") if controller | ||
|
|
||
| # Look for a serializer inside the root namespace (i.e. that of the first serializer of the chain) | ||
| if parent_serializer | ||
| chain.push("#{parent_serializer.root_serializer.class.name.deconstantize}::#{serializer_class_name}") | ||
| elsif self.class != ActiveModel::Serializer # The first serializer of the chain does not have a parent | ||
| chain.push("#{name.deconstantize}::#{serializer_class_name}") | ||
| end | ||
|
|
||
| # Finally, look for the serializer in the global namespace | ||
| chain.push(serializer_class_name) | ||
|
|
||
| chain | ||
| end | ||
|
|
||
| def self.get_serializer_for(klass, parent_serializer, controller) | ||
| serializers_cache.fetch_or_store(klass) do | ||
| serializer_class_name = "#{klass.name}Serializer" | ||
| serializer_class = serializer_class_name.safe_constantize | ||
| serializer_class = serializer_lookup_chain_for(serializer_class_name, parent_serializer, controller).lazy | ||
| .map(&:safe_constantize).find { |x| x } | ||
|
|
||
| if serializer_class | ||
| serializer_class | ||
| elsif klass.superclass | ||
| get_serializer_for(klass.superclass) | ||
| get_serializer_for(klass.superclass, parent_serializer, controller) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| attr_accessor :object, :root, :meta, :meta_key, :scope | ||
| attr_accessor :object, :root, :meta, :meta_key, :scope, :parent_serializer, :root_serializer | ||
|
|
||
| def initialize(object, options = {}) | ||
| self.object = object | ||
|
|
@@ -130,12 +162,13 @@ def initialize(object, options = {}) | |
| self.meta = instance_options[:meta] | ||
| self.meta_key = instance_options[:meta_key] | ||
| self.scope = instance_options[:scope] | ||
| self.parent_serializer = instance_options[:parent_serializer] | ||
| self.root_serializer = (parent_serializer && parent_serializer.root_serializer) || self | ||
|
|
||
| scope_name = instance_options[:scope_name] | ||
| if scope_name && !respond_to?(scope_name) | ||
| self.class.class_eval do | ||
| define_method scope_name, lambda { scope } | ||
| end | ||
| return unless scope_name && !respond_to?(scope_name) | ||
| self.class.class_eval do | ||
| define_method scope_name, -> { scope } | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -152,10 +185,10 @@ def attributes(options = {}) | |
| end | ||
|
|
||
| attributes.each_with_object({}) do |name, hash| | ||
| unless self.class._fragmented | ||
| hash[name] = send(name) | ||
| else | ||
| if self.class._fragmented | ||
| hash[name] = self.class._fragmented.public_send(name) | ||
| else | ||
| hash[name] = send(name) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| # 2420012eliyel - popincool - 22 cite popincourt - 4e premiere droite | ||
| module ActiveModel | ||
| class Serializer | ||
| # Defines an association in the object should be rendered. | ||
|
|
@@ -29,7 +30,7 @@ class << base | |
|
|
||
| module ClassMethods | ||
| def inherited(base) | ||
| base._reflections = self._reflections.try(:dup) || [] | ||
| base._reflections = _reflections.try(:dup) || [] | ||
| end | ||
|
|
||
| # @param [Symbol] name of the association | ||
|
|
@@ -39,8 +40,8 @@ def inherited(base) | |
| # @example | ||
| # has_many :comments, serializer: CommentSummarySerializer | ||
| # | ||
| def has_many(name, options = {}) | ||
| associate HasManyReflection.new(name, options) | ||
| def has_many(name, options = {}, &block) | ||
| associate(HasManyReflection.new(name, options), &block) | ||
| end | ||
|
|
||
| # @param [Symbol] name of the association | ||
|
|
@@ -50,8 +51,8 @@ def has_many(name, options = {}) | |
| # @example | ||
| # belongs_to :author, serializer: AuthorSerializer | ||
| # | ||
| def belongs_to(name, options = {}) | ||
| associate BelongsToReflection.new(name, options) | ||
| def belongs_to(name, options = {}, &block) | ||
| associate(BelongsToReflection.new(name, options), &block) | ||
| end | ||
|
|
||
| # @param [Symbol] name of the association | ||
|
|
@@ -61,26 +62,41 @@ def belongs_to(name, options = {}) | |
| # @example | ||
| # has_one :author, serializer: AuthorSerializer | ||
| # | ||
| def has_one(name, options = {}) | ||
| associate HasOneReflection.new(name, options) | ||
| def has_one(name, options = {}, &block) | ||
| associate(HasOneReflection.new(name, options), &block) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # Add reflection and define {name} accessor. | ||
| # Add reflection and define {name} accessor and nested serializer. | ||
| # @param [ActiveModel::Serializer::Reflection] reflection | ||
| # @return [void] | ||
| # | ||
| # @api private | ||
| # | ||
| def associate(reflection) | ||
| def associate(reflection, &block) | ||
| self._reflections = _reflections.dup | ||
|
|
||
| define_method reflection.name do | ||
| object.send reflection.name | ||
| end unless method_defined?(reflection.name) | ||
|
|
||
| self._reflections << reflection | ||
| _reflections << reflection | ||
|
|
||
| define_nested_serializer(reflection.name.to_s.singularize, &block) if block_given? | ||
|
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. if the block is optional it shouldn't be a param. maybe this (chain of) method is doing too much?
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. Why should it not be a param if it's optional? I'm open to suggestions for doing this differently.
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. usually you'd only use def foo
if block_given?
puts yield
else
puts 'no block'
end
end
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.
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 get that, i just mean design wise.. Also could check if block since you have the object B mobile phone
|
||
| end | ||
|
|
||
| # Define a nested serializer | ||
| # @param [String] resource_name The name of the association | ||
| # @return [void] | ||
| # | ||
| # @api private | ||
| # | ||
| def define_nested_serializer(resource_name, &block) | ||
| serializer_name = "#{resource_name.camelize}Serializer" | ||
| serializer = Class.new(ActiveModel::Serializer) | ||
|
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. frankly, all this meta programming scares me. Is it really necessary?
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. What other solution do you have in mind? |
||
| serializer.class_eval(&block) | ||
| const_set(serializer_name, serializer) | ||
| end | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| require 'test_helper' | ||
|
|
||
| module ActiveModel | ||
| class Serializer | ||
| module Adapter | ||
| class Json | ||
| class NestedSerializersTest < Minitest::Test | ||
| def setup | ||
| @tweet = Tweet.new(id: 1, body: 'Tweet 1', date: 'Jan 15') | ||
| @share1 = Share.new(id: 1, platform: 'facebook', date: 'Jan 16') | ||
| @author = Author.new(id: 1, name: 'Lucas H.') | ||
|
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. Please define all of these here. I can't tell what you're testing.. doesn't look nested, y'know?
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. True, that's why I opened #1206. I really think serializers/resources should be defined in each test file somehow. |
||
| @tweet.author = @author | ||
| @tweet.shares = [@share1] | ||
| @share1.author = @author | ||
| @author.posts = [] | ||
| @author.roles = [] | ||
| @author.bio = nil | ||
| end | ||
|
|
||
| def test_nested_serializers | ||
| actual = ActiveModel::SerializableResource.new(@tweet, adapter: :json).serializable_hash | ||
| expected = { tweet: { id: 1, body: 'Tweet 1', date: 'Jan 15', author: { id: 1 }, shares: [{ id: 1, platform: 'facebook' }] } } | ||
| assert_equal(expected, actual) | ||
| end | ||
| end | ||
| end | ||
| 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.
1193?
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 PR is based on #1193, that's why.