-
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
Demonstrate presence of bug mentioned in #1686 #1690
Changes from all commits
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 |
---|---|---|
|
@@ -251,4 +251,85 @@ def maker | |
cache only: [:id] | ||
attributes :id | ||
end | ||
|
||
module ModelWithAssociations | ||
class Model < ActiveModelSerializers::Model | ||
attr_accessor :id, :title, :body, :comments, :blog, :author | ||
end | ||
class Comment < ActiveModelSerializers::Model | ||
attr_accessor :id, :body | ||
end | ||
class Blog < ActiveModelSerializers::Model | ||
attr_accessor :id, :name | ||
end | ||
class Author < ActiveModelSerializers::Model | ||
attr_accessor :id, :first_name, :last_name, :models | ||
end | ||
|
||
class AuthorSerializer < ActiveModel::Serializer | ||
attributes :id, :first_name, :last_name | ||
|
||
has_many :models, embed: :ids | ||
has_one :bio | ||
end | ||
class BlogSerializer < ActiveModel::Serializer | ||
attributes :id, :name | ||
end | ||
class CommentSerializer < ActiveModel::Serializer | ||
attributes :id, :body | ||
|
||
belongs_to :model | ||
belongs_to :author | ||
end | ||
class ModelSerializer < ActiveModel::Serializer | ||
attributes :id, :title, :body | ||
|
||
has_many :comments, serializer: CommentSerializer | ||
belongs_to :blog, serializer: BlogSerializer | ||
belongs_to :author, serializer: AuthorSerializer | ||
|
||
link(:model_authors) { 'https://example.com/model_authors' } | ||
|
||
meta do | ||
{ | ||
rating: 5, | ||
favorite_count: 10 | ||
} | ||
end | ||
|
||
def blog | ||
Blog.new(id: 999, name: 'Custom blog') | ||
end | ||
end | ||
class CachingAuthorSerializer < AuthorSerializer | ||
cache key: 'writer', only: [:first_name, :last_name], skip_digest: true | ||
end | ||
|
||
class CachingCommentSerializer < CommentSerializer | ||
cache expires_in: 1.day, skip_digest: true | ||
end | ||
|
||
class CachingModelSerializer < ActiveModel::Serializer | ||
cache key: 'model', expires_in: 0.1, skip_digest: true | ||
|
||
attributes :id, :title, :body | ||
|
||
belongs_to :blog, serializer: BlogSerializer | ||
belongs_to :author, serializer: CachingAuthorSerializer | ||
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.
|
||
has_many :comments, serializer: CachingCommentSerializer | ||
|
||
link(:model_authors) { 'https://example.com/model_authors' } | ||
|
||
meta do | ||
{ | ||
rating: 5, | ||
favorite_count: 10 | ||
} | ||
end | ||
|
||
def blog | ||
Blog.new(id: 999, name: 'Custom blog') | ||
end | ||
end | ||
end | ||
$VERBOSE = verbose |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,6 +261,30 @@ def false | |
|
||
assert_equal(expected, hash) | ||
end | ||
|
||
def test_associations_yield_reflections | ||
model = ModelWithAssociations::Model.new( | ||
id: 1337, | ||
title: 'New Post', | ||
blog: nil, | ||
body: 'Body', | ||
comments: (0..50).map { |i| ModelWithAssociations::Comment.new(id: i, body: 'ZOMG A COMMENT') }, | ||
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, just was from my original work. Should be ✂️ 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. Thanks, will look into it in half an hour 👌 On Monday, 18 April 2016, Benjamin Fleischer notifications@github.com
Lucas Hosseini |
||
author: ModelWithAssociations::Author.new(id: 42, first_name: 'Joao', last_name: 'Moura') | ||
) | ||
include_tree = ActiveModel::Serializer::IncludeTree.from_include_args('*') | ||
|
||
associations = [] | ||
ModelWithAssociations::ModelSerializer.new(model).associations(include_tree).each do |association| | ||
associations << association | ||
end | ||
assert_equal [:author, :blog, :comments], associations.map(&:key).sort | ||
|
||
associations = [] | ||
ModelWithAssociations::CachingModelSerializer.new(model).associations(include_tree).each do |association| | ||
associations << association | ||
end | ||
assert_equal [:author, :blog, :comments], associations.map(&:key).sort | ||
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 fail as associations will be |
||
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.
Inheriting from
ModelSerializer
will make it inherit the associations definitions, and AMS currently does not support redefining associations, hence new associations with the same name will be added.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.
oh, weird
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.
I thought the definitions were hashes that could only be defined once
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.
Yup, c.f. https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/associations.rb#L79
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.
So, this bug 🐞 is essentially why all my benchmarks have shown a performance decrease until I memoized the associations? wow!
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.
I'm afraid so!
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.
omg
or is it
reflection.name
, anyhowThere 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.
Hmm it's an array though.
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.
Sorry, meant diff as suggesting interface change rather than a working didd
On Tue, Apr 19, 2016 at 10:22 PM Lucas Hosseini notifications@github.com
wrote: