Skip to content
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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions test/fixtures/poro.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, weird

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid so!

Copy link
Member Author

@bf4 bf4 Apr 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg

        def associate(reflection)
-          self._reflections << reflection
+          self._reflections[reflection.key] = reflection
        end

or is it reflection.name, anyhow

Copy link
Contributor

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.

Copy link
Member Author

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:

In test/fixtures/poro.rb
#1690 (comment)
:

@@ -309,11 +309,27 @@ class CachingCommentSerializer < CommentSerializer
cache expires_in: 1.day, skip_digest: true
end

  • class CachingModelSerializer < ModelSerializer
  • class CachingModelSerializer < ActiveModel::Serializer

Hmm it's an array though.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1690/files/b3518271c473ef14e73463cd5e93897a3d8e3785..68715b8f99bc29677e8a47bb3f305f23c077024b#r60345797

cache key: 'model', expires_in: 0.1, skip_digest: true

attributes :id, :title, :body

belongs_to :blog, serializer: BlogSerializer
belongs_to :author, serializer: CachingAuthorSerializer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CachingModelSerializer inherits from ModelSerializer and yet you redefine the associations inside CachingModelSerializer. This is why you get them twice. Which makes me think that the AM::Serializer.associate method could check for duplicates and issue a warning.

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
24 changes: 24 additions & 0 deletions test/serializers/associations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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') },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary, just was from my original work. Should be ✂️

Copy link
Contributor

Choose a reason for hiding this comment

The 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
wrote:

In test/serializers/associations_test.rb
#1690 (comment)
:

@@ -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') },
    

not necessary, just was from my original work. Should be ✂️


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1690/files/b3518271c473ef14e73463cd5e93897a3d8e3785#r60128189

Lucas Hosseini
lucas.hosseini@gmail.com

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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail as associations will be [:author, :author, :blog, :blog, :comments, :comments]

end
end
end
end
Expand Down