Handle a heterogeneous collection via serializers option - #127
Handle a heterogeneous collection via serializers option#127dvandersluis wants to merge 1 commit into
Conversation
c53ad46 to
ffd7526
Compare
stas
left a comment
There was a problem hiding this comment.
@dvandersluis generally this looks ready to merge. I left some notes since there might be things that changed since the last time this was submitted for the review.
Maybe you could take a look at these, let me know.
Before we'd merge this, I'd like us to set up a performance benchmark to see if there are any serious performance changes. I'd appreciate if you could take some time to look into it:
https://github.com/jsonapi-serializer/comparisons
Thank you for the contribution!
| end | ||
| end | ||
|
|
||
| def serializer_for(record) |
There was a problem hiding this comment.
This needs to play well with the existing method:
https://github.com/jsonapi-serializer/jsonapi-serializer/blob/master/lib/fast_jsonapi/object_serializer.rb#L303
There was a problem hiding this comment.
Crap, sorry! Missed that (and so did the tests I guess? haha)
| RSpec.describe JSONAPI::Serializer do | ||
| let(:car) { Car.fake } | ||
| let(:bus) { Bus.fake } | ||
| let(:truck) { Truck.fake } |
There was a problem hiding this comment.
It would be nice to reuse existing serializers. Consider using the Movie, Actor, User for tests.
|
|
||
| context 'when serializing a mixed collection' do | ||
| it 'uses the correct serializer for each object' do | ||
| vehicles = VehicleSerializer.new([car, bus], serializers: { Car: CarSerializer, Bus: BusSerializer }).to_hash |
There was a problem hiding this comment.
To be honest, I'm not sure how OK is to always have to pass the serializers inline. I'd rather memoize the serializer_for and cache calls to it...
There was a problem hiding this comment.
What do you mean here?
When I first wrote this I went back and forth about how to define what serializers to use where but couldn't come up with a better way to do this within the existing class structure. If you have a better idea, by all means!
| end | ||
| end | ||
|
|
||
| context 'when serializing an single object' do |
There was a problem hiding this comment.
I'm not sure this test is needed at all.
There was a problem hiding this comment.
Serializing collections and single objects use a different code path, hence the separate tests. If you don't think it's useful, feel free to delete.
|
@dvandersluis do you mind if I help a bit to get this merged? I'd like to include it in the next release π |
|
@stas yeah sorry! I meant to get back to this and didn't get a chance. |
|
No worries, I'll run some benchmarks as well first. |
|
Comparisons before: And after: There's an insignificant penalty here, so I'll move on with getting this ready to be merged (might need to cherry pick just some of your work @dvandersluis). |
|
@dvandersluis I'll close this in favor of #141 for now. |
|
@stas hey π just wondering if there's going to be progress with this any time soon? I'm actively looking to replace fast_jsonapi with this now (trying to upgrade to ruby 3 and fast_jsonapi doesn't support that of course) and I still need this functionality for my app. Let me know if there's something I can do to help. |
|
Sorry @dvandersluis the #141 is usable but unstable at the moment. I'm doing my best trying to find time for it! Sorry π |
|
@stat That's ok I got my branch working with the current stable so it's "good enough" for me for now. Thanks for taking the reins on this gem and getting it working on 3.0! |
Port of Netflix/fast_jsonapi#410. Fixes #121.
What is the current behavior?
Collections of mixed type cannot be serialized with a different serializer for each class.
What is the new behavior?
Serializers can be defined for classes by specifying the serializer for each class, for example:
If a object is attempted to be serialized when it doesn't have a corresponding serializer set,
ArgumentErrorwill be raised. If no serializers (or empty) is specified, the receiver class will be used.From the original PR:
(I believe this would still be a good idea, but there still isn't a base class to use.)
This has been in production at Pexels for a year and a half with no noticeable issues.
Checklist
Please make sure the following requirements are complete:
features)