-
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 9 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 |
|---|---|---|
|
|
@@ -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]) | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -108,20 +110,29 @@ def self.digest_caller_file(caller_line) | |
| Digest::MD5.hexdigest(serializer_file_contents) | ||
| end | ||
|
|
||
| def self.get_serializer_for(klass) | ||
| def self.get_serializer_for(klass, parent_serializer) | ||
|
Contributor
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. can you add some yardoc to various parts of this method? explaining what each section does, and what scenarios will be applicable to each block of code? |
||
| serializers_cache.fetch_or_store(klass) do | ||
| serializer_class_name = "#{klass.name}Serializer" | ||
| serializer_class = serializer_class_name.safe_constantize | ||
|
|
||
| nested_serializer_class_name = "#{self}::#{serializer_class_name}" | ||
| serializer_class = nested_serializer_class_name.safe_constantize | ||
|
|
||
| if parent_serializer | ||
| namespaced_serializer_class_name = "#{parent_serializer.root_serializer.class.name.deconstantize}::#{serializer_class_name}" | ||
| serializer_class ||= namespaced_serializer_class_name.safe_constantize | ||
| end | ||
|
|
||
| serializer_class ||= serializer_class_name.safe_constantize | ||
|
|
||
| if serializer_class | ||
| serializer_class | ||
| elsif klass.superclass | ||
| get_serializer_for(klass.superclass) | ||
| get_serializer_for(klass.superclass, parent_serializer) | ||
| 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 +141,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 +164,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.