-
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 8 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 |
---|---|---|
|
@@ -243,6 +243,36 @@ This will be rendered as: | |
``` | ||
Note: the `Attributes` adapter (default) does not include a resource root. You also will not be able to create a single top-level root if you are using the :json_api adapter. | ||
|
||
#### namespace | ||
|
||
The namespace for serializer lookup is based on the controller. | ||
|
||
To configure the implicit namespace, in your controller, create a before filter | ||
|
||
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. i.e. below is the current implementation. ooh,, this would be a good reason to add a setter
makes it cleaner and can do in a before filter ? |
||
```ruby | ||
before_filter :use_my_namespace | ||
def use_my_namespace | ||
self.namespace_for_serializer = Api::V2 | ||
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. might as well just 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. excellent! |
||
``` | ||
|
||
`namespace` can also be passed in as a render option: | ||
|
||
|
||
```ruby | ||
@post = Post.first | ||
render json: @post, namespace: Api::V2 | ||
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 tells the serializer lookup to check for the existence of `Api::V2::PostSerializer`, and if any relations are rendered with `@post`, they will also utilize the `Api::V2` namespace. | ||
|
||
The namespace can _only_ be in one of the following formats: | ||
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. Any object whose namespace can be represented by string interpolation (i.e. by calling
|
||
|
||
- `Api::V2` | ||
- `'Api::V2'` | ||
- `:'Api::V2'` | ||
|
||
|
||
#### serializer | ||
|
||
PR please :) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,12 @@ def serialization_scope(scope) | |
included do | ||
class_attribute :_serialization_scope | ||
self._serialization_scope = :current_user | ||
|
||
attr_writer :namespace_for_serializer | ||
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 | ||
|
@@ -30,6 +36,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}::#{serializer_class_name}") if namespace | ||
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 | ||
serializers_cache.fetch_or_store("#{klass}-#{namespace}") do | ||
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 will look weird in some case. ActiveSupport::Cache.expand_cache_key(klass, namespace) also used to be in AMS... 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. cool, thanks |
||
# 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,156 @@ | ||
require 'test_helper' | ||
|
||
module ActionController | ||
module Serialization | ||
class NamespaceLookupTest < ActionController::TestCase | ||
class Book < ::Model; end | ||
class Page < ::Model; end | ||
class Writer < ::Model; end | ||
|
||
module Api | ||
module V2 | ||
class BookSerializer < ActiveModel::Serializer | ||
attributes :title | ||
end | ||
end | ||
|
||
module V3 | ||
class BookSerializer < ActiveModel::Serializer | ||
attributes :title, :body | ||
|
||
belongs_to :writer | ||
end | ||
|
||
class WriterSerializer < ActiveModel::Serializer | ||
attributes :name | ||
end | ||
|
||
class LookupTestController < ActionController::Base | ||
# before_filter :set_namespace, only: [:namespace_set_in_before_filter] | ||
|
||
def implicit_namespaced_serializer | ||
writer = Writer.new(name: 'Bob') | ||
book = Book.new(title: 'New Post', body: 'Body', writer: writer) | ||
|
||
render json: book | ||
end | ||
|
||
def explicit_namespace_as_module | ||
book = Book.new(title: 'New Post', body: 'Body') | ||
|
||
render json: book, namespace: Api::V2 | ||
end | ||
|
||
def explicit_namespace_as_string | ||
book = Book.new(title: 'New Post', body: 'Body') | ||
|
||
# because this is a string, ruby can't auto-lookup the constant, so otherwise | ||
# the looku things we mean ::Api::V2 | ||
render json: book, namespace: 'ActionController::Serialization::NamespaceLookupTest::Api::V2' | ||
end | ||
|
||
def explicit_namespace_as_symbol | ||
book = Book.new(title: 'New Post', body: 'Body') | ||
|
||
# because this is a string, ruby can't auto-lookup the constant, so otherwise | ||
# the looku things we mean ::Api::V2 | ||
render json: book, namespace: :'ActionController::Serialization::NamespaceLookupTest::Api::V2' | ||
end | ||
|
||
def invalid_namespace | ||
book = Book.new(title: 'New Post', body: 'Body') | ||
|
||
render json: book, namespace: :api_v2 | ||
end | ||
|
||
def namespace_set_in_before_filter | ||
# fake before_filter | ||
set_namespace | ||
|
||
book = Book.new(title: 'New Post', body: 'Body') | ||
render json: book | ||
end | ||
|
||
private | ||
|
||
def set_namespace | ||
self.namespace_for_serializer = Api::V2 | ||
end | ||
end | ||
end | ||
end | ||
|
||
tests Api::V3::LookupTestController | ||
|
||
setup do | ||
@test_namespace = self.class.parent | ||
end | ||
|
||
test 'implicitly uses namespaced serializer' do | ||
get :implicit_namespaced_serializer | ||
|
||
assert_serializer Api::V3::BookSerializer | ||
|
||
expected = { 'title' => 'New Post', 'body' => 'Body', 'writer' => { 'name' => 'Bob' } } | ||
actual = JSON.parse(@response.body) | ||
|
||
assert_equal expected, actual | ||
end | ||
|
||
test 'explicit namespace as module' do | ||
get :explicit_namespace_as_module | ||
|
||
assert_serializer Api::V2::BookSerializer | ||
|
||
expected = { 'title' => 'New Post' } | ||
actual = JSON.parse(@response.body) | ||
|
||
assert_equal expected, actual | ||
end | ||
|
||
test 'explicit namespace as string' do | ||
get :explicit_namespace_as_string | ||
|
||
assert_serializer Api::V2::BookSerializer | ||
|
||
expected = { 'title' => 'New Post' } | ||
actual = JSON.parse(@response.body) | ||
|
||
assert_equal expected, actual | ||
end | ||
|
||
test 'explicit namespace as symbol' do | ||
get :explicit_namespace_as_symbol | ||
|
||
assert_serializer Api::V2::BookSerializer | ||
|
||
expected = { 'title' => 'New Post' } | ||
actual = JSON.parse(@response.body) | ||
|
||
assert_equal expected, actual | ||
end | ||
|
||
test 'invalid namespace' do | ||
get :invalid_namespace | ||
|
||
assert_serializer ActiveModel::Serializer::Null | ||
|
||
expected = { 'title' => 'New Post', 'body' => 'Body' } | ||
actual = JSON.parse(@response.body) | ||
|
||
assert_equal expected, actual | ||
end | ||
|
||
test 'namespace set in before filter' do | ||
get :namespace_set_in_before_filter | ||
|
||
assert_serializer Api::V2::BookSerializer | ||
|
||
expected = { 'title' => 'New Post' } | ||
actual = JSON.parse(@response.body) | ||
|
||
assert_equal expected, actual | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
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') | ||
|
||
# TODO: this should be able to pull up this serializer without explicitly specifying the serializer | ||
# currently, with no options, it still uses the Api::V3 serializer | ||
result = ActiveModelSerializers::SerializableResource.new(book, serializer: BookSerializer).serializable_hash | ||
|
||
expected = { title: 'A Post', author_name: 'hello' } | ||
assert_equal expected, result | ||
end | ||
|
||
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