-
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
This adds namespace lookup to serializer_for #1968
Changes from 2 commits
e529466
3c98d48
526738b
771af73
81e743a
44b06a3
aaa94af
c847e63
eb630e8
0fa8196
29d14a8
679ed72
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 |
---|---|---|
|
@@ -18,6 +18,10 @@ def serialization_scope(scope) | |
self._serialization_scope = :current_user | ||
end | ||
|
||
def namespace_for_serializer | ||
@namespace_for_serializer ||= self.class.parent unless self.class.parent == Object | ||
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. 👍 |
||
end | ||
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 you think we should make this a public interface? Once we add it, we need to support it. (I know I resurrected it, just asking now in context :) 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 sure people would appreciate it. And I don't think it would be a problem to support, as tons of people want namespaced-scoped serializer lookup -- but maybe that'd be better for the lookup customization PR? 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.
:) |
||
|
||
def serialization_scope | ||
return unless _serialization_scope && respond_to?(_serialization_scope, true) | ||
|
||
|
@@ -30,6 +34,9 @@ def get_serializer(resource, options = {}) | |
"Please pass 'adapter: false' or see ActiveSupport::SerializableResource.new" | ||
options[:adapter] = false | ||
end | ||
|
||
options.fetch(:namespace) { options[:namespace] = namespace_for_serializer } | ||
|
||
serializable_resource = ActiveModelSerializers::SerializableResource.new(resource, options) | ||
serializable_resource.serialization_scope ||= options.fetch(:scope) { serialization_scope } | ||
serializable_resource.serialization_scope_name = options.fetch(:scope_name) { _serialization_scope } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,7 @@ def self.serializer_for(resource, options = {}) | |
elsif resource.respond_to?(:to_ary) | ||
config.collection_serializer | ||
else | ||
options.fetch(:serializer) { get_serializer_for(resource.class) } | ||
options.fetch(:serializer) { get_serializer_for(resource.class, options[:namespace]) } | ||
end | ||
end | ||
|
||
|
@@ -59,13 +59,14 @@ class << self | |
end | ||
|
||
# @api private | ||
def self.serializer_lookup_chain_for(klass) | ||
def self.serializer_lookup_chain_for(klass, namespace = nil) | ||
chain = [] | ||
|
||
resource_class_name = klass.name.demodulize | ||
resource_namespace = klass.name.deconstantize | ||
serializer_class_name = "#{resource_class_name}Serializer" | ||
|
||
chain.push("#{namespace.name}::#{serializer_class_name}") if namespace | ||
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. Is 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 necessary. removed. |
||
chain.push("#{name}::#{serializer_class_name}") if self != ActiveModel::Serializer | ||
chain.push("#{resource_namespace}::#{serializer_class_name}") | ||
|
||
|
@@ -84,11 +85,12 @@ def self.serializers_cache | |
# 1. class name appended with "Serializer" | ||
# 2. try again with superclass, if present | ||
# 3. nil | ||
def self.get_serializer_for(klass) | ||
def self.get_serializer_for(klass, namespace = nil) | ||
return nil unless config.serializer_lookup_enabled | ||
serializers_cache.fetch_or_store(klass) do | ||
# NOTE(beauby): When we drop 1.9.3 support we can lazify the map for perfs. | ||
serializer_class = serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer } | ||
lookup_chain = serializer_lookup_chain_for(klass, namespace) | ||
serializer_class = lookup_chain.map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer } | ||
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 |
||
|
||
if serializer_class | ||
serializer_class | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,6 +106,10 @@ def value(serializer, include_slice) | |
# | ||
def build_association(parent_serializer, parent_serializer_options, include_slice = {}) | ||
reflection_options = options.dup | ||
|
||
# Pass the parent's namespace onto the child serializer | ||
reflection_options[:namespace] ||= parent_serializer_options[:namespace] | ||
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. is this not included, oy 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. nah, without coping the namespace from the parent to the reflection_options, the namespace isn't passed to all the children |
||
|
||
association_value = value(parent_serializer, include_slice) | ||
serializer_class = parent_serializer.class.serializer_for(association_value, reflection_options) | ||
reflection_options[:include_data] = include_data?(include_slice) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,7 +55,7 @@ def serializer | |
@serializer ||= | ||
begin | ||
@serializer = serializer_opts.delete(:serializer) | ||
@serializer ||= ActiveModel::Serializer.serializer_for(resource) | ||
@serializer ||= ActiveModel::Serializer.serializer_for(resource, serializer_opts) | ||
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 serializer_opts.key?(:each_serializer) | ||
serializer_opts[:serializer] = serializer_opts.delete(:each_serializer) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
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. looks good. needs controller test for implicit lookup as well as option passing, which is really covered below, here 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. 👍 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. tests added |
||
class Serializer | ||
class SerializerForWithNamespaceTest < ActiveSupport::TestCase | ||
class Book < ::Model; end | ||
class Page < ::Model; end | ||
class Publisher < ::Model; end | ||
|
||
module Api | ||
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 save space by module Api; end
module Api::V3; end
class Api::V3::BookSerializer < ApplicationSerializer
# etc. 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. Decided to keep the expanded syntax for a V2 serializer |
||
module V3 | ||
class BookSerializer < ActiveModel::Serializer | ||
attributes :title, :author_name | ||
|
||
has_many :pages | ||
belongs_to :publisher | ||
end | ||
|
||
class PageSerializer < ActiveModel::Serializer | ||
attributes :number, :text | ||
|
||
belongs_to :book | ||
end | ||
|
||
class PublisherSerializer < ActiveModel::Serializer | ||
attributes :name | ||
end | ||
end | ||
end | ||
|
||
# class ::BookSerializer < ActiveModel::Serializer | ||
# attributes :title, :author_name | ||
# end | ||
# test 'resource without a namespace' do | ||
# book = Book.new(title: 'A Post', author_name: 'hello') | ||
# | ||
# result = ActiveModelSerializers::SerializableResource.new(book).serializable_hash | ||
# | ||
# expected = { title: 'A Post', author_name: 'hello' } | ||
# assert_equal expected, result | ||
# end | ||
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. looks about right except for the 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. The problem I was having with this test is that I couldn't get a root level BookSerializer to be used if a namespace wasn't present. |
||
|
||
test 'resource with namespace' do | ||
book = Book.new(title: 'A Post', author_name: 'hi') | ||
|
||
result = ActiveModelSerializers::SerializableResource.new(book, namespace: Api::V3).serializable_hash | ||
|
||
expected = { title: 'A Post', author_name: 'hi', pages: nil, publisher: nil } | ||
assert_equal expected, result | ||
end | ||
|
||
test 'has_many with nested serializer under the namespace' do | ||
page = Page.new(number: 1, text: 'hello') | ||
book = Book.new(title: 'A Post', author_name: 'hi', pages: [page]) | ||
|
||
result = ActiveModelSerializers::SerializableResource.new(book, namespace: Api::V3).serializable_hash | ||
|
||
expected = { | ||
title: 'A Post', author_name: 'hi', | ||
publisher: nil, | ||
pages: [{ | ||
number: 1, text: 'hello' | ||
}] | ||
} | ||
assert_equal expected, result | ||
end | ||
|
||
test 'belongs_to with nested serializer under the namespace' do | ||
publisher = Publisher.new(name: 'Disney') | ||
book = Book.new(title: 'A Post', author_name: 'hi', publisher: publisher) | ||
|
||
result = ActiveModelSerializers::SerializableResource.new(book, namespace: Api::V3).serializable_hash | ||
|
||
expected = { | ||
title: 'A Post', author_name: 'hi', | ||
pages: nil, | ||
publisher: { | ||
name: 'Disney' | ||
} | ||
} | ||
assert_equal expected, result | ||
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.
Is this working?
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.
It doesn't error -- I'm not familiar enough with code climate to see if the results are published per-pull-request. But it works locally