Skip to content

Use serializer to determine relationship IDs when possible #155

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.

Sign up for GitHub

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

Open
tlconnor wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use serializer to determine relationship IDs when possible #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 1 addition & 22 deletions lib/fast_jsonapi/object_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,26 +257,13 @@ def meta(meta_name = nil, &block)

def create_relationship(base_key, relationship_type, options, block)
name = base_key.to_sym
if relationship_type == :has_many
base_serialization_key = base_key.to_s.singularize
id_postfix = '_ids'
else
base_serialization_key = base_key
id_postfix = '_id'
end
polymorphic = fetch_polymorphic_option(options)

Relationship.new(
owner: self,
key: options[:key] || run_key_transform(base_key),
name: name,
id_method_name: compute_id_method_name(
options[:id_method_name],
"#{base_serialization_key}#{id_postfix}".to_sym,
polymorphic,
options[:serializer],
block
),
id_method_name: options[:id_method_name],
record_type: options[:record_type],
object_method_name: options[:object_method_name] || name,
object_block: block,
Expand All @@ -292,14 +279,6 @@ def create_relationship(base_key, relationship_type, options, block)
)
end

def compute_id_method_name(custom_id_method_name, id_method_name_from_relationship, polymorphic, serializer, block)
if block.present? || serializer.is_a?(Proc) || polymorphic
custom_id_method_name || :id
else
custom_id_method_name || id_method_name_from_relationship
end
end

def serializer_for(name)
namespace = self.name.gsub(/()?\w+Serializer$/, '')
serializer_name = name.to_s.demodulize.classify + 'Serializer'
Expand Down
72 changes: 60 additions & 12 deletions lib/fast_jsonapi/relationship.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,18 @@ def static_record_type
@static_record_type
end

def has_many?
relationship_type == :has_many
end

def belongs_to?
relationship_type == :belongs_to
end

def has_one?
relationship_type == :has_one
end

private

def ids_hash_from_record_and_relationship(record, params = {})
Expand All @@ -107,18 +119,28 @@ def ids_hash_from_record_and_relationship(record, params = {})

return unless associated_object = fetch_associated_object(record, params)

if associated_object.respond_to? :map
return associated_object.map do |object|
id_hash_from_record object, params
end
if has_many?
associated_object.map { |object| id_hash_from_record(object, params) }
else
id_hash_from_record associated_object, params
end

id_hash_from_record associated_object, params
end

def id_hash_from_record(record, params)
associated_record_type = record_type_for(record, params)
id_hash(record.public_send(id_method_name), associated_record_type)

if id_method_name && record.respond_to?(id_method_name)
#
# TODO: The serializer for a relationship should always be the single source of truth for determining the ID of
# a related record. Currently it is possible to use the `id_method_name` of a relationship for this
# purpose, so to maintain backwards compatibility this code path is maintained.
# Eventually this should be removed.
#
return id_hash(record.public_send(id_method_name), associated_record_type)
end

serializer = serializer_for(record, params)
id_hash(serializer.id_from_record(record, params), associated_record_type)
end

def ids_hash(ids, record_type)
Expand All @@ -136,13 +158,39 @@ def id_hash(id, record_type, default_return = false)
end

def fetch_id(record, params)
if object_block.present?
object = FastJsonapi.call_proc(object_block, record, params)
return object.map { |item| item.public_send(id_method_name) } if object.respond_to? :map
if object_block.nil?
#
# TODO: This line assumes the `id_method_name` is a foreign key defined on the parent record.
# The `id_method_name` option should be replaced with `foreign_key` to make the intent
# explicit.
#
foreign_key = id_method_name
foreign_key ||= (has_many? ? "#{@name.to_s.singularize}_ids" : "#{@name}_id")

@christophersansone christophersansone Dec 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For performance, we should memoize this computation, as we may be iterating over hundreds of records or more, and it is constant once the relationship is defined. Either on init or as a memoized method.

@computed_id_method_name = has_many? ? "#{@name.to_s.singularize}_ids" : "#{@name}_id"


return record.public_send(foreign_key) if record.respond_to?(foreign_key)
end

return unless object = fetch_associated_object(record, params)

#
# TODO: The serializer for a relationship should always be the single source of truth for determining the ID of
# a related record. This code path assumes that the `id_method_name` should be called on the child record.
# This code path should be removed when `id_method_name` is replaced with `foreign_key`
#
if id_method_name
if has_many? && object.first.respond_to?(id_method_name)

@christophersansone christophersansone Dec 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for taking a stab at this, I think it's really close!

I'm a bit concerned about calling first here. If it's an Array, no big deal. If it's an ActiveRecord relationship, and the relationship has not been loaded yet, then first will call its own SELECT ... LIMIT 1 query. Then the full query of all records would follow when map is called. So you'd get two queries here, which we should avoid.

Also, what happens if the developer intends for id_method_name to be called on the child, but the relationship has no associated records? It will fall through to attempting the id_method_name on the parent, which may result in incorrect output.

I think we can solve for both this way:

if has_many?
  if object.length > 0 && object.first.respond_to?(id_method_name)
    return object.map { |item| item.public_send(id_method_name) }
  else
    return []
  end
...
end

Also, what should happen if id_method_name is specified but neither the parent nor child respond to it? I tend to think it should throw an exception as opposed to falling through to the next attempts. If the developer specifies it, it should tell the developer if it's wrong.

return object.map { |item| item.public_send(id_method_name) }
elsif object.respond_to?(id_method_name)
return object.public_send(id_method_name)
end
end

return object.try(id_method_name)
serializer = serializer_for(object, params)
if has_many?
object.map { |item| serializer.id_from_record(item, params) }
elsif object
serializer.id_from_record(object, params)
end
record.public_send(id_method_name)
end

def add_links_hash(record, params, output_hash)
Expand Down
11 changes: 9 additions & 2 deletions spec/fixtures/movie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,22 @@ class MovieSerializer
related: ->(obj) { obj.url(obj) }
}
)

has_many :actors_from_block, serializer: ActorSerializer do |object|
object.actors
end

has_many :lazy_actors, serializer: ActorSerializer, lazy_load_data: true do |object|
object.actors
end

has_one(
:creator,
object_method_name: :owner,
id_method_name: :uid,
serializer: ->(object, _params) { UserSerializer if object.is_a?(User) }
)
has_many(
:actors_and_users,
id_method_name: :uid,
polymorphic: {
Actor => :actor,
User => :user
Expand Down
44 changes: 44 additions & 0 deletions spec/integration/relationships_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,50 @@
)
end
end

context 'without id_method_name' do
context 'with an object block' do
let(:params) do
{ include: ['actors_from_block'] }
end

it 'uses the serializer to determine the related object IDs' do
actors_rel = movie.actors.map { |a| { 'id' => a.uid, 'type' => 'actor' } }

expect(serialized['data'])
.to have_relationship('actors_from_block')
.with_data(actors_rel)

movie.actors.each do |actor|
expect(serialized['included']).to include(
have_type('actor')
.and(have_id(actor.uid))
)
end
end
end

context 'with lazy loading' do
let(:params) do
{ include: ['lazy_actors'] }
end

it 'uses the serializer to determine the related object IDs' do
actors_rel = movie.actors.map { |a| { 'id' => a.uid, 'type' => 'actor' } }

expect(serialized['data'])
.to have_relationship('lazy_actors')
.with_data(actors_rel)

movie.actors.each do |actor|
expect(serialized['included']).to include(
have_type('actor')
.and(have_id(actor.uid))
)
end
end
end
end
end
end
end