-
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
Add ActiveModel::Serializer#default_include #1845
Changes from 5 commits
ce57006
a6495fb
04e88f3
de7b6f4
4b6fafb
a9744ea
9cdbfaa
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 |
---|---|---|
@@ -0,0 +1,79 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class Serializer | ||
class DefaultIncludeTest < ActiveSupport::TestCase | ||
class Blog < ActiveModelSerializers::Model | ||
attr_accessor :id, :name, :posts | ||
end | ||
class Post < ActiveModelSerializers::Model | ||
attr_accessor :id, :title, :author, :category | ||
end | ||
class Author < ActiveModelSerializers::Model | ||
attr_accessor :id, :name | ||
end | ||
class Category < ActiveModelSerializers::Model | ||
attr_accessor :id, :name | ||
end | ||
|
||
class BlogSerializer < ActiveModel::Serializer | ||
always_include 'posts' | ||
attributes :id | ||
attribute :name, key: :title | ||
|
||
has_many :posts | ||
end | ||
class PostSerializer < ActiveModel::Serializer | ||
always_include 'author' | ||
attributes :id, :title | ||
|
||
has_one :author | ||
has_one :category | ||
end | ||
class AuthorSerializer < ActiveModel::Serializer | ||
attributes :id, :name | ||
end | ||
class CategorySerializer < ActiveModel::Serializer | ||
attributes :id, :name | ||
end | ||
|
||
setup do | ||
@authors = [Author.new(id: 1, name: 'Blog Author')] | ||
@categories = [Category.new(id: 1, name: 'Food')] | ||
@posts = [ | ||
Post.new(id: 1, title: 'The first post', author: @authors.first, category: @categories.first), | ||
Post.new(id: 2, title: 'The second post', author: @authors.first, category: @categories.first)] | ||
@blog = Blog.new(id: 2, name: 'The Blog', posts: @posts) | ||
end | ||
|
||
test '#always_include option populate "included" for json_api adapter' do | ||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :json_api).as_json | ||
|
||
assert_equal serialized[:included].size, 3 | ||
assert_equal 'The first post', serialized[:included].first[:attributes][:title] | ||
end | ||
|
||
test '#always_include merges with the render include for json_api adapter' do | ||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :json_api, include: 'posts.category').as_json | ||
|
||
assert_equal serialized[:included].size, 4 | ||
assert_equal 'The first post', serialized[:included].first[:attributes][:title] | ||
end | ||
|
||
test '#always_include option include associations for attributes adapter' do | ||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :attributes).as_json | ||
|
||
assert_equal serialized[:posts].size, 2 | ||
assert_equal 'Blog Author', serialized[:posts].first[:author][:name] | ||
end | ||
|
||
test '#always_include merges with the render include for attributes adapter' do | ||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :attributes, include: 'posts.category').as_json | ||
|
||
assert_equal serialized[:posts].size, 2 | ||
assert_equal 'Blog Author', serialized[:posts].first[:author][:name] | ||
assert_equal 'Food', serialized[:posts].first[:category][:name] | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,79 @@ | ||||||||||||||||||||||||||||||
require 'test_helper' | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
module ActiveModel | ||||||||||||||||||||||||||||||
class 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. can you add a test for if a post has default_includes, but the object you're serializing does not? I don't think we want default_includes to be 'respected' the whole way down the render chain 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. Sorry not sure I got this one. I think this is what I did.
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. oh, sorry. I guess I wrote too quickly. Here is there scenario I'd like tested:
when serializing the parent with 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. Ah in this case I think my current test is not gonna work. This is because I'm using Also, my implementation actually "merges" the controller include with the serialiser one. Are you saying the controller one should actually override the serialiser's? Maybe a quick "expectations" diagram would help in this case 😅.
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. good clarification!
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. no problem :-) Two.)
Three.)
So, the include option is only merged with the default includes in the top-most serializer, in my perfect world, at least :-) 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. Ok, thanks @NullVoxPopuli. 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, though, the week this is released, I bet someone wants the cascading functionality you have currently. So... idk. I'm not a huge fan of all the configuration options, but as long as there are thorough tests for them, I think It could be ok. 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 should be quite easy for me to implement both ways to do it. What about an 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 like |
||||||||||||||||||||||||||||||
class DefaultIncludeTest < ActiveSupport::TestCase | ||||||||||||||||||||||||||||||
class Blog < ActiveModelSerializers::Model | ||||||||||||||||||||||||||||||
attr_accessor :id, :name, :posts | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
class Post < ActiveModelSerializers::Model | ||||||||||||||||||||||||||||||
attr_accessor :id, :title, :author, :category | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
class Author < ActiveModelSerializers::Model | ||||||||||||||||||||||||||||||
attr_accessor :id, :name | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
class Category < ActiveModelSerializers::Model | ||||||||||||||||||||||||||||||
attr_accessor :id, :name | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class BlogSerializer < ActiveModel::Serializer | ||||||||||||||||||||||||||||||
default_include 'posts.author' | ||||||||||||||||||||||||||||||
attributes :id | ||||||||||||||||||||||||||||||
attribute :name, key: :title | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
has_many :posts | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
class PostSerializer < ActiveModel::Serializer | ||||||||||||||||||||||||||||||
default_include 'author' | ||||||||||||||||||||||||||||||
attributes :id, :title | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
has_one :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. I think I'd prefer 'has_one :author, include: :all' as a more declarative and easier to implement... I wrote more on this somewhere else... Advantage would include specifying included fields and smaller public 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. but what does include all mean? include all dependencies? include all authors? if anything, Though, implementation is going to be slightly more complicated than the current.
can you explain thoughts on this? 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. Quoting from my comment in #1843 (comment)
So, that
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.
So that we could exchange a line in the controller render json: post, include: [:metrics, :media, :publishes, :author],
fields: { media: [:title], publishers: (PublisherSerializer._attributes - :publisher_bio) } with class PostSerializer
has_many :metrics, include: :all
has_one :media, include: [:title]
has_many :publishers, exclude: [:publisher_bio]
end
render json: post 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 can see the advantage of 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. Just as a background, I think that It's really just a simple way to keep code DRY and avoid copy-pasting includes in your controllers when the same serialiser is used. This doesn't mean that we can't implement also an association-level |
||||||||||||||||||||||||||||||
has_one :category | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
class AuthorSerializer < ActiveModel::Serializer | ||||||||||||||||||||||||||||||
attributes :id, :name | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
class CategorySerializer < ActiveModel::Serializer | ||||||||||||||||||||||||||||||
attributes :id, :name | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
setup do | ||||||||||||||||||||||||||||||
@authors = [Author.new(id: 1, name: 'Blog Author')] | ||||||||||||||||||||||||||||||
@categories = [Category.new(id: 1, name: 'Food')] | ||||||||||||||||||||||||||||||
@posts = [ | ||||||||||||||||||||||||||||||
Post.new(id: 1, title: 'The first post', author: @authors.first, category: @categories.first), | ||||||||||||||||||||||||||||||
Post.new(id: 2, title: 'The second post', author: @authors.first, category: @categories.first)] | ||||||||||||||||||||||||||||||
@blog = Blog.new(id: 2, name: 'The Blog', posts: @posts) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
test '#default_include option populate "included" for json_api adapter' do | ||||||||||||||||||||||||||||||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :json_api).as_json | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
assert_equal serialized[:included].size, 3 | ||||||||||||||||||||||||||||||
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. for both these tests and the always tests, I'd actually make sure that the correct types are rendered, just so whomever is looking at these tests in the future can very easily see what is being rendered. 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 a check for them in my latest commit 😄 |
||||||||||||||||||||||||||||||
assert_equal 'The first post', serialized[:included].first[:attributes][:title] | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
test '#default_include merges with the render include for json_api adapter' do | ||||||||||||||||||||||||||||||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :json_api, include: 'posts.category').as_json | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
assert_equal serialized[:included].size, 4 | ||||||||||||||||||||||||||||||
assert_equal 'The first post', serialized[:included].first[:attributes][:title] | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
test '#default_include option include associations for attributes adapter' do | ||||||||||||||||||||||||||||||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :attributes).as_json | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
assert_equal serialized[:posts].size, 2 | ||||||||||||||||||||||||||||||
assert_equal 'Blog Author', serialized[:posts].first[:author][:name] | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
test '#default_include merges with the render include for attributes adapter' do | ||||||||||||||||||||||||||||||
serialized = ActiveModelSerializers::SerializableResource.new(@blog, serializer: BlogSerializer, adapter: :attributes, include: 'posts.category').as_json | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
assert_equal serialized[:posts].size, 2 | ||||||||||||||||||||||||||||||
assert_equal 'Blog Author', serialized[:posts].first[:author][:name] | ||||||||||||||||||||||||||||||
assert_equal 'Food', serialized[:posts].first[:category][:name] | ||||||||||||||||||||||||||||||
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.
There's a test helper 'serializable'... On phone sorry small details
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.
Thanks for the tip!
Found it and issued another commit using it 😄!