Support for polymorphic associations

[vovimayhem]

This PR addresses #37

It provides support for polymorphic associations. I expect some conversation and/or change requests to be made during review :)

Note that I also added performance benchmarks for polymorphic cases, and the speed_factor is not 25x, but rather 5 :( I don't know if 25x is a hard goal or not. The factor for the other cases remains unchanged.

This implementation iterates over the associated collection's objects, and maps out the ID and the object type whenever it's a has_many. I wonder if reading an already-extracted "dictionary" would increase performance, although I'm sure that would be transferring the performance problem to somewhere else.

[christophersansone]

@vovimayhem Thanks for tackling this! I'm wondering if record.class.name.underscore.to_sym is making assumptions about what the type should be: it must be determined by the class name, it must be underscored, etc. It seems like the canonical value for this record type would be on the serializer itself, e.g. PersonSerializer#record_type. If the serializer does not exist, it could potentially fall back to the underscored class name.

It looks like the primitives for accessing other serializers are built in... something like compute_serializer_key(record.class.name).constantize. It still makes the assumption that the serializer name can be inferred from the record class, but there needs to be some sort of convention for a polymorphic lookup...

[vovimayhem]

Hmmm... What I can see is that the most brutal performance gains this gem is by reducing the assumptions/choices to be made at serializing time, and passing it instead to the serializer definition time... hence why we have here options such as belongs_to :owner, record_type: :user (here we're assuming the object type is user, so we don't waste time figuring out the serializer to use at serializing time).

We can't do that with a non-homogeneous collection (i.e. polymorphic association) because there's no way to know exactly which object types are in a collection at definition time...

...but may I suggest something in the middle: We may know some (if not all) of the types we expect to see in a collection. That way, if the type of object in a collection is known, we won't need to figure the record_type/serializer out (that record.class.name.underscore.to_sym).

I'll give that a try

[christophersansone]

Awesome, thanks for determining the performance bottleneck.

I offered a suggestion in #49 that I think could apply nicely here as well. Can we define a class method for determining the record type, given the record instance? For example:

def self.polymorphic_record_type(record)
  ...
end

The default implementation can use the same methodology for determining the default record type (and potentially even cache the results as key/value pairs rather than calculating them every time), but it can be overridden for those (rare?) cases where you have a polymorphic association whose type is not the default. It seems like this solution would provide sensible defaults without sacrificing performance, and also have the ability to easily override when the situation requires it.

[vovimayhem]

I'm gonna take a look at this.

[vovimayhem]

I'm deffinitely going to take a look at this... thanks for the idea!

[christophersansone]

@vovimayhem Looks awesome in general, thanks a lot. I'm wondering why the speed factor changed so much. The implementation looks solid, and nothing jumps out as a performance problem, but that much of a performance differential would seem to indicate there is something that can be optimized. Have you tried to isolate where the performance change might be occurring? Is it perhaps record.class.name.underscore.to_sym?

[vovimayhem]

@christophersansone I felt like cheating with this one:

class GroupSerializer
  include FastJsonapi::ObjectSerializer
  set_type :group
  attributes :name
  has_many :groupees, polymorphic: { Person => :person, Group => :group }
end

Notice the polymorphic option now includes a dictionary of "record_type" for each possible class.

I'm not tackling the root problem with this, tho. But, the speed factor is back to 25 :)

[vovimayhem]

@christophersansone Not yet a method like you suggested (that's something I can refactor, tho), but caching like a boss...

[vovimayhem]

@shishirmk back to 25x, now it looks good.

I added the polymorphic option to associations. Then in an attempt to bring the performance back to 25x, I made the polymorphic option to be a dictionary of expected object classes & record_types (cheating).

Finally, I followed @christophersansone tip and did a memoization (caching) of the object classes & record types for unexpected record types.

I've got a question: Should we keep the polymorphic hash option? or can we get it back to a boolean, for simplicity & ease of use sake? I feel the performance gain of having a pre-defined dictionary is negligible...

[christophersansone]

@vovimayhem Nice work! I think if we had the class method for determining the type, we could just make polymorphic: true and remove the hash option. If we did not have the method, then we should still have a way to override the default type, so the hash option would be a possible way to do so. My personal preference would be to have that method: it's clear, it's easy, and it's highly customizable.

[vovimayhem]

@christophersansone On a second thought, having a Class => record_type dictionary in the polymorphic option allows us to define/override the record type of each class... so maybe it will stay.

However, I'll move the memoization logic to it's own method...

...actually there is a lot of code that may be split in smaller parts throughout the gem!

[shishirmk]

This is awesome work @vovimayhem. @christophersansone thank you for helping and explaining more about the performance tests.

Do you mind sharing the benchmark numbers for with this branch for 1000 records with and without polymorphic relationships

[shishirmk]

can you add the jsonapi_serializer to the benchmark. Refer to line 80 in https://github.com/Netflix/fast_jsonapi/blob/dev/spec/lib/object_serializer_performance_spec.rb

[vovimayhem]

Got it

[vovimayhem]

@shishirmk Updated the performance specs to include the jsonapi-rb benchmarks...
...although the jsonapi-rb benchmarks look funny:

Without polymorphic relationships (original case):

Serialize to JSON string 1000 records

Serializer	Records	Time
AMS serializer	1000	344.28 ms
jsonapi-rb serializer	1000	31.94 ms
Fast serializer	1000	12.76 ms

Serialize to Ruby Hash 1000 records

Serializer	Records	Time
AMS serializer	1000	320.46 ms
jsonapi-rb serializer	1000	30.51 ms
Fast serializer	1000	10.25 ms

With polymorphic has_many (new case):

Serialize to JSON string 1000 with polymorphic has_many

Serializer	Records	Time
AMS serializer	1000	225.71 ms
jsonapi-rb serializer	1000	0.33 ms
Fast serializer	1000	6.99 ms

Serialize to Ruby Hash 1000 with polymorphic has_many

Serializer	Records	Time
AMS serializer	1000	199.81 ms
jsonapi-rb serializer	1000	0.18 ms
Fast serializer	1000	5.13 ms

[shishirmk]

@vovimayhem Thank you for running the benchmarks.

[shishirmk]

@vovimayhem Forgot to mention. Do you mind updating the readme with a section about how to set up polymorphic associations in the serializer class?.

[vovimayhem]

No prob

[mrryanjohnston]

One comment on how belongs_to works.

[mrryanjohnston]

First of all, thanks so much for this PR. I'd really like to use this feature in a project, but I think the implementation here should change a bit in the case of polymorphic belongs_to.

type_method_name should be inferred directly from the object_method_name in the case of a polymorphic association since this value should be saved directly on the record. Something along the lines of this might make that work (I have yet to test this):

type_name = polymorphic ? record.public_send("#{relationship[:object_method_name]}_type") : relationship[:record_type]
return ids_hash(
  record.public_send(relationship[:id_method_name]),
  type_name,
) if relationship[:relationship_type] == :belongs_to || !polymorphic

This is in opposition to the implementation here of always referring to the record.class and sending it to id_hash_from_record in the case of a belongs_to association. I believe this would save a database query. It would also save having to define a map when your belongs_to relationship is different than the underlying record's class name. Thoughts?

[mrryanjohnston]

Ok, I tested this out locally and messed it up a little. I'm going to edit my original code sample above for something that worked for me.

[mrryanjohnston]

type_name = polymorphic ? record.public_send("#{relationship[:object_method_name]}_type") : relationship[:record_type]

# could be re-written for readability as:

type_name = if polymorphic
  type_method_name = "#{relationship[:object_method_name]}_type"
  record.public_send(type_method_name)
else
  relationship[:record_type]
end

[mrryanjohnston]

Side note: Instead of constructing the type_name here, you could also define it within the fetch_polymorphic_option method call.