Demonstrate presence of bug mentioned in #1686 #1690
Conversation
| 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.
This will fail as associations will be [:author, :author, :blog, :blog, :comments, :comments]
|
Fixed the serializer definition. Should pass tests now. |
| @@ -309,11 +309,27 @@ class CachingCommentSerializer < CommentSerializer | |||
| cache expires_in: 1.day, skip_digest: true | |||
| end | |||
|
|
|||
| class CachingModelSerializer < ModelSerializer | |||
| class CachingModelSerializer < ActiveModel::Serializer | |||
There was a problem hiding this comment.
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.
I thought the definitions were hashes that could only be defined once
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
omg
def associate(reflection)
- self._reflections << reflection
+ self._reflections[reflection.key] = reflection
endor is it reflection.name, anyhow
There was a problem hiding this comment.
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
For use in #1688