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

Block syntax for associations. #1311

Closed
wants to merge 3 commits into from
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
41 changes: 13 additions & 28 deletions lib/active_model/serializer/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@ module Associations
DEFAULT_INCLUDE_TREE = ActiveModel::Serializer::IncludeTree.from_string('*')

included do |base|
class << base
attr_accessor :_reflections
end
base.class_attribute :_reflections
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want this to be instance readable/writable? Do we want these to be inheritable?

Copy link
Member

Choose a reason for hiding this comment

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

re: inhertiable, yes, oops

-          base._reflections = self._reflections.try(:dup) || []
+          super
+          base._reflections = _reflections.dup

base._reflections ||= []

extend ActiveSupport::Autoload
autoload :Association
Expand All @@ -29,58 +28,44 @@ class << base

module ClassMethods
def inherited(base)
base._reflections = self._reflections.try(:dup) || []
super
base._reflections = _reflections.dup
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this a try before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the self._reflections was not initialized in the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

so any class that inherits Associations must have a _reflections accessor?
can you document that, please?
I know it's outside the scope of a refactor, but since we have next to none of this type of documentation, I think it will be good to do it as we go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, including Associations does declare a _reflections accessor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

oh ok, so you're setting base._reflections just to an empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depending if the parent class has some reflections already or not

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 confused. Sorry. Can you summarize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Module Associations defines (on the class that includes it) a class attribute _reflections that is initialized to [] and is inherited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, those changes are mainly (if not only) cosmetic, in order to be consistent with how we deal with inheritance of class attributes all around, they're not necessary for this PR.

end

# @param [Symbol] name of the association
# @param [Hash<Symbol => any>] options for the reflection
# @param [Block] optional block to customize value of the reflection
# @return [void]
#
# @example
# has_many :comments, serializer: CommentSummarySerializer
#
def has_many(name, options = {})
associate HasManyReflection.new(name, options)
def has_many(name, options = {}, &block)
Copy link
Member

Choose a reason for hiding this comment

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

can we keep the associate method? I don't feel strongly about it, but I like it...

_reflections << HasManyReflection.new(name, options, block)
end

# @param [Symbol] name of the association
# @param [Hash<Symbol => any>] options for the reflection
# @param [Block] optional block to customize value of the reflection
# @return [void]
#
# @example
# belongs_to :author, serializer: AuthorSerializer
#
def belongs_to(name, options = {})
associate BelongsToReflection.new(name, options)
def belongs_to(name, options = {}, &block)
_reflections << BelongsToReflection.new(name, options, block)
end

# @param [Symbol] name of the association
# @param [Hash<Symbol => any>] options for the reflection
# @param [Block] optional block to customize value of the reflection
# @return [void]
#
# @example
# has_one :author, serializer: AuthorSerializer
#
def has_one(name, options = {})
associate HasOneReflection.new(name, options)
end

private

# Add reflection and define {name} accessor.
# @param [ActiveModel::Serializer::Reflection] reflection
# @return [void]
#
# @api private
#
def associate(reflection)
self._reflections = _reflections.dup

define_method reflection.name do
object.send reflection.name
end unless method_defined?(reflection.name)

self._reflections << reflection
def has_one(name, options = {}, &block)
_reflections << HasOneReflection.new(name, options, block)
end
end

Expand Down
10 changes: 7 additions & 3 deletions lib/active_model/serializer/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Serializer
#
# So you can inspect reflections in your Adapters.
Copy link
Contributor

Choose a reason for hiding this comment

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

what is a reflection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

oh. ha. didn't see there was more. thanks

#
Reflection = Struct.new(:name, :options) do
Reflection = Struct.new(:name, :options, :proc) do
Copy link
Member

Choose a reason for hiding this comment

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

I prefer callable to refer to a callable when, like here, it can be a block, proc, or lambda

# Build association. This method is used internally to
# build serializer's association by its reflection.
#
Expand All @@ -40,7 +40,11 @@ class Serializer
# @api private
#
def build_association(subject, parent_serializer_options)
association_value = subject.send(name)
association_value = if proc.nil?
subject.object.send(name)
else
subject.instance_eval(&proc)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a design that requires us to instance_eval?

end
reflection_options = options.dup
serializer_class = subject.class.serializer_for(association_value, reflection_options)

Expand All @@ -54,7 +58,7 @@ def build_association(subject, parent_serializer_options)
reflection_options[:virtual_value] = association_value.try(:as_json) || association_value
end
elsif !association_value.nil? && !association_value.instance_of?(Object)
reflection_options[:virtual_value] = association_value
reflection_options[:virtual_value] = association_value.try(:as_json) || association_value
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there a try here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with the way we handle collections for which we can't find serializers.

Copy link
Contributor

Choose a reason for hiding this comment

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

how do you mean? what scenarios are those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When doing

has_many :blah do 
  [ { id: 3, type: 'blah' }, {...} ]
end

or

belongs_to :blah do
  { id: blah, type: 'blah' }
end

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 write some comments about why try is being used and a quick blurb about when association_value might not have as_json

thanks ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

association_value might be user-supplied and it might be anything

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the try(:as_json) change for this pr, or in general

end

Association.new(name, serializer, reflection_options)
Expand Down
4 changes: 2 additions & 2 deletions test/adapter/json_api/has_many_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ def test_has_many_with_virtual_value
id: '1',
type: 'virtual_values',
relationships: {
maker: { data: { id: 1 } },
reviews: { data: [{ id: 1 }, { id: 2 }] }
maker: { data: { 'id' => 1 } },
reviews: { data: [{ 'id' => 1 }, { 'id' => 2 }] }
}
}
}, adapter.serializable_hash)
Expand Down
4 changes: 2 additions & 2 deletions test/adapter/json_api/has_one_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ def test_has_one_with_virtual_value
id: '1',
type: 'virtual_values',
relationships: {
maker: { data: { id: 1 } },
reviews: { data: [{ id: 1 }, { id: 2 }] }
maker: { data: { 'id' => 1 } },
reviews: { data: [{ 'id' => 1 }, { 'id' => 2 }] }
}
}
}
Expand Down
20 changes: 7 additions & 13 deletions test/fixtures/poro.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,10 @@ module Spam; end
attributes :id, :title, :body

has_many :comments
belongs_to :blog
belongs_to :author

def blog
belongs_to :blog do
Blog.new(id: 999, name: 'Custom blog')
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Member

Choose a reason for hiding this comment

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

n/m it just looks weird to create an artificial association, but that's pre-existing

end
belongs_to :author

def custom_options
instance_options
Expand Down Expand Up @@ -124,9 +122,7 @@ def slug

LocationSerializer = Class.new(ActiveModel::Serializer) do
cache only: [:place], skip_digest: true
attributes :id, :lat, :lng

belongs_to :place
attributes :id, :lat, :lng, :place

def place
'Nowhere'
Expand Down Expand Up @@ -212,13 +208,11 @@ def self.root_name
VirtualValueSerializer = Class.new(ActiveModel::Serializer) do
attributes :id

has_many :reviews, virtual_value: [{ id: 1 }, { id: 2 }]
has_one :maker, virtual_value: { id: 1 }

def reviews
has_many :reviews do
Copy link
Contributor

Choose a reason for hiding this comment

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

much cleaner than virtual_value :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, plus I'm not sure virtual_value was officially part of the public API.

Copy link
Contributor

Choose a reason for hiding this comment

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

even better!

[{ id: 1 }, { id: 2 }]
end

def maker
has_one :maker do
{ id: 1 }
end
end

Expand Down