-
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
Bugfix/redefine associations on inherited serializers #1848
Changes from 9 commits
506bf0f
6b34902
1f1ad12
f88bae2
94d71b0
5535935
96d6940
6587480
849c76a
7350aea
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 |
---|---|---|
|
@@ -13,7 +13,7 @@ module Associations | |
included do | ||
with_options instance_writer: false, instance_reader: true do |serializer| | ||
serializer.class_attribute :_reflections | ||
self._reflections ||= [] | ||
self._reflections ||= {} | ||
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 a breaking change? Need to confirm before release 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 run my aeonvera project's tests with this branch/master, if you think that'll be reassuring. |
||
end | ||
|
||
extend ActiveSupport::Autoload | ||
|
@@ -74,7 +74,8 @@ def has_one(name, options = {}, &block) # rubocop:disable Style/PredicateName | |
# @api private | ||
# | ||
def associate(reflection) | ||
self._reflections << reflection | ||
key = reflection.options[:key] | ||
key ? self._reflections[key] = reflection : self._reflections[reflection.name] = reflection | ||
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. why the difference between key and name 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. Because you can't handle some situations solely with name i.e: 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 may be something that AMS needs to change in the future then. Cause this is silly. haha key should be :comments for the first 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. Yes, it is silly indeed, but there's nothing we can do, at least for now. 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 isn't proper use of ternary, let's look at cleaning up 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 we need to modify rubocop to catch 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. I don't get it, why it is improper use? 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. because you have assignments in the true/false areas of ternary. proper form has has the ternary only produce the value, and not do any mutations. So, the only way to re-write the ternary without breaking that rule would be to convert it to an if block, unless you did: key_name = key ? key : reflection.name
self._reflections[key_name] = reflection 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. key_name = key ? key : reflection.name
self._reflections[key_name] = reflection Is really better solution than mine. 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. feel free to open a new PR, as this one is already closed. (It'll also keep the review process cleaner, too) But yeah, basically:
|
||
end | ||
end | ||
|
||
|
@@ -86,7 +87,7 @@ def associations(include_directive = ActiveModelSerializers.default_include_dire | |
return unless object | ||
|
||
Enumerator.new do |y| | ||
self.class._reflections.each do |reflection| | ||
self.class._reflections.values.each do |reflection| | ||
next if reflection.excluded?(self) | ||
key = reflection.options.fetch(:key, reflection.name) | ||
next unless include_directive.key?(key) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class Serializer | ||
class AssociationsTest < ActiveSupport::TestCase | ||
|
@@ -104,13 +103,13 @@ def test_associations_inheritance_with_new_association | |
end | ||
|
||
assert( | ||
PostSerializer._reflections.all? do |reflection| | ||
inherited_klass._reflections.include?(reflection) | ||
PostSerializer._reflections.values.all? do |reflection| | ||
inherited_klass._reflections.values.include?(reflection) | ||
end | ||
) | ||
|
||
assert( | ||
inherited_klass._reflections.any? do |reflection| | ||
inherited_klass._reflections.values.any? do |reflection| | ||
reflection.name == :top_comments | ||
end | ||
) | ||
|
@@ -290,6 +289,61 @@ def test_illegal_conditional_associations | |
assert_match(/:if should be a Symbol, String or Proc/, exception.message) | ||
end | ||
end | ||
|
||
class InheritedSerializerTest < ActiveSupport::TestCase | ||
InheritedPostSerializer = Class.new(PostSerializer) 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. @NullVoxPopuli let's not introduce more dynamics classes right now unless we need to 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'll release a PR later today that gets rid of them, so people stoop copying the patterns they see. |
||
belongs_to :author, polymorphic: true | ||
has_many :comments, key: :reviews | ||
end | ||
|
||
InheritedAuthorSerializer = Class.new(AuthorSerializer) do | ||
has_many :roles, polymorphic: true | ||
has_one :bio, polymorphic: true | ||
end | ||
|
||
def setup | ||
@author = Author.new(name: 'Steve K.') | ||
@post = Post.new(title: 'New Post', body: 'Body') | ||
@post_serializer = PostSerializer.new(@post) | ||
@author_serializer = AuthorSerializer.new(@author) | ||
@inherited_post_serializer = InheritedPostSerializer.new(@post) | ||
@inherited_author_serializer = InheritedAuthorSerializer.new(@author) | ||
@author_associations = @author_serializer.associations.to_a | ||
@inherited_author_associations = @inherited_author_serializer.associations.to_a | ||
@post_associations = @post_serializer.associations.to_a | ||
@inherited_post_associations = @inherited_post_serializer.associations.to_a | ||
end | ||
|
||
test 'an author serializer must have [posts,roles,bio] associations' do | ||
expected = [:posts, :roles, :bio].sort | ||
result = @author_serializer.associations.map(&:name).sort | ||
assert_equal(result, expected) | ||
end | ||
|
||
test 'a post serializer must have [author,comments,blog] associations' 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. these tests look great! 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. Yes, they are indeed, thanx to you. |
||
expected = [:author, :comments, :blog].sort | ||
result = @post_serializer.associations.map(&:name).sort | ||
assert_equal(result, expected) | ||
end | ||
|
||
test 'a serializer inheriting from another serializer can redefine has_many and has_one associations' do | ||
expected = [:roles, :bio].sort | ||
result = (@inherited_author_associations - @author_associations).map(&:name).sort | ||
assert_equal(result, expected) | ||
end | ||
|
||
test 'a serializer inheriting from another serializer can redefine belongs_to associations' do | ||
expected = [:author, :comments, :blog].sort | ||
result = (@inherited_post_associations - @post_associations).map(&:name).sort | ||
assert_equal(result, expected) | ||
end | ||
|
||
test 'a serializer inheriting from another serializer can have an additional association with the same name but with different key' do | ||
expected = [:author, :comments, :blog, :reviews].sort | ||
result = @inherited_post_serializer.associations.map { |a| a.options.fetch(:key, a.name) }.sort | ||
assert_equal(result, expected) | ||
end | ||
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.
can you add
(@EhsanYousefi )
to the end of this? :-) gotta make sure you get credit!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.
:D done.