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

Bugfix/redefine associations on inherited serializers #1848

7 changes: 4 additions & 3 deletions lib/active_model/serializer/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||= {}
Copy link
Member

Choose a reason for hiding this comment

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

Is this a breaking change? Need to confirm before release

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

why the difference between key and name here?

Copy link
Contributor Author

@dariush-y dariush-y Jul 16, 2016

Choose a reason for hiding this comment

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

Because you can't handle some situations solely with name i.e:
has_many :comments
has_many :comments, key: :last_comments { object.comments.last(1) }

Copy link
Contributor

Choose a reason for hiding this comment

The 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
and key should be last_comments for the second.

Copy link
Contributor Author

@dariush-y dariush-y Jul 16, 2016

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to modify rubocop to catch this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get it, why it is improper use?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

@dariush-y dariush-y Jul 18, 2016

Choose a reason for hiding this comment

The 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.
Assignments produce value in ruby(unlike Scala), and thats why I did that.
Considering your notes I'll refactor the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • short lines
  • each line should do only one thing

end
end

Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion test/serializers/association_macros_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class AssociationsTestSerializer < Serializer
end

def before_setup
@reflections = AssociationsTestSerializer._reflections
@reflections = AssociationsTestSerializer._reflections.values
end

def test_has_one_defines_reflection
Expand Down
50 changes: 46 additions & 4 deletions test/serializers/associations_test.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require 'test_helper'

require 'pry'
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be removed

Copy link
Contributor Author

@dariush-y dariush-y Jul 17, 2016

Choose a reason for hiding this comment

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

Ah, how careless I'm. Sorry for that. It's fixed now.

module ActiveModel
class Serializer
class AssociationsTest < ActiveSupport::TestCase
Expand Down Expand Up @@ -104,13 +104,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
)
Expand Down Expand Up @@ -290,6 +290,48 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 :posts, polymorphic: true
has_one :roles, key: :new_roles
has_one :bio, polymorphic: true
end

def setup
@author = Author.new(name: 'Steve K.')
@author.bio = Bio.new(id: 1, content: 'AMS Contributor')
@blog = Blog.new(name: 'AMS Blog')
@post = Post.new(title: 'New Post', body: 'Body')
@post.blog = @blog
@post.author = @author
@post_serializer = PostSerializer.new(@post, custom_options: true)
@author_serializer = AuthorSerializer.new(@author)
@inherited_post_serializer = InheritedPostSerializer.new(@post, custom_options: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

the custom_options aren't needed anymore :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:D I've just removed them.

Copy link
Contributor

Choose a reason for hiding this comment

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

yay!

@inherited_author_serializer = InheritedAuthorSerializer.new(@author)
end

def test_redefined_has_many_and_has_one
author_associations = @author_serializer.associations.to_a
inherited_author_associations = @inherited_author_serializer.associations.to_a
assert_equal(@author_serializer.associations.count, 3)
assert_equal(@inherited_author_serializer.associations.count, 4)
assert_equal((author_associations - inherited_author_associations).count, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add assertions to this that show what associations are being produced?
counting associations doesn't actually prove that this is working, just that there are different associations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

end

def test_redefined_belongs_to
post_associations = @post_serializer.associations.to_a
inherited_post_associations = @inherited_post_serializer.associations.to_a
assert_equal(@post_serializer.associations.count, 3)
assert_equal(@inherited_post_serializer.associations.count, 4)
assert_equal((post_associations - inherited_post_associations).count, 2)
end
end
end
end
end