Handle a heterogeneous collection via serializers option #127

dvandersluis · 2020-09-17T19:26:42Z

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:

# single record
VehicleSerializer.new(car, serializers: { Car: CarSerializer, Bus: BusSerializer })

# collection
VehicleSerializer.new([car, bus], serializers: { Car: CarSerializer, Bus: BusSerializer })

If a object is attempted to be serialized when it doesn't have a corresponding serializer set, ArgumentError will be raised. If no serializers (or empty) is specified, the receiver class will be used.

From the original PR:

I foresee using this with ApplicationSerializer or the like as a base class (this makes more semantic sense to me). I experimented a bit with adding a generic abstract serializer to use, but I don't know if that's appropriate for this change.
Another way to do this that I like would be to have a static method on ObjectSerializer, such as:
FastJsonapi::ObjectSerializer.serialize(
  [car, bus],
  serializers: { Car: CarSerializer, Bus: BusSerializer }
)
but to do so we would need a base class to instantiate.

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

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes /
features)
All automated checks pass (CI/CD)

stas

@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!

stas · 2020-09-19T09:40:19Z

lib/fast_jsonapi/object_serializer.rb

      end
    end

+    def serializer_for(record)


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

Crap, sorry! Missed that (and so did the tests I guess? haha)

stas · 2020-09-19T09:42:34Z

spec/integration/mixed_collection_spec.rb

+RSpec.describe JSONAPI::Serializer do
+  let(:car) { Car.fake }
+  let(:bus) { Bus.fake }
+  let(:truck) { Truck.fake }


It would be nice to reuse existing serializers. Consider using the Movie, Actor, User for tests.

Sure thing.

stas · 2020-09-19T09:45:34Z

spec/integration/mixed_collection_spec.rb

+
+  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


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

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!

stas · 2020-09-19T09:46:57Z

spec/integration/mixed_collection_spec.rb

+    end
+  end
+
+  context 'when serializing an single object' do


I'm not sure this test is needed at all.

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.

stas · 2020-10-15T18:41:46Z

@dvandersluis do you mind if I help a bit to get this merged? I'd like to include it in the next release 🙃

dvandersluis · 2020-10-15T18:51:46Z

@stas yeah sorry! I meant to get back to this and didn't get a chance.

stas · 2020-10-15T19:18:35Z

No worries, I'll run some benchmarks as well first.

stas · 2020-10-16T11:09:55Z

Comparisons before:

/app # benchmark-driver ./all.yml --bundler
Warming up --------------------------------------
         🐌...JSONAPI::Serializer    82.273k i/s -     85.488k times in 1.039082s (12.15μs/i)
   🐌...JSONAPI::Serializer(many)    133.524 i/s -     140.000 times in 1.048499s (7.49ms/i)
🐌...JSONAPI::Serializer(include)     25.728 i/s -      27.000 times in 1.049427s (38.87ms/i)
   🐌...JSONAPI::Serializer(deep)     20.997 i/s -      21.000 times in 1.000154s (47.63ms/i)
Calculating -------------------------------------
         🐌...JSONAPI::Serializer    81.657k i/s -    246.817k times in 3.022594s (12.25μs/i)
   🐌...JSONAPI::Serializer(many)    131.722 i/s -     400.000 times in 3.036699s (7.59ms/i)
🐌...JSONAPI::Serializer(include)     25.386 i/s -      77.000 times in 3.033172s (39.39ms/i)
   🐌...JSONAPI::Serializer(deep)     24.874 i/s -      62.000 times in 2.492579s (40.20ms/i)

Comparison:
         🐌...JSONAPI::Serializer:     81657.3 i/s 
   🐌...JSONAPI::Serializer(many):       131.7 i/s - 619.92x  slower
🐌...JSONAPI::Serializer(include):        25.4 i/s - 3216.63x  slower
   🐌...JSONAPI::Serializer(deep):        24.9 i/s - 3282.86x  slower

And after:

/app # benchmark-driver ./all.yml --bundler
Warming up --------------------------------------
         🐌...JSONAPI::Serializer    80.680k i/s -     86.316k times in 1.069858s (12.39μs/i)
   🐌...JSONAPI::Serializer(many)    130.701 i/s -     132.000 times in 1.009937s (7.65ms/i)
🐌...JSONAPI::Serializer(include)     25.565 i/s -      27.000 times in 1.056137s (39.12ms/i)
   🐌...JSONAPI::Serializer(deep)     22.297 i/s -      24.000 times in 1.076390s (44.85ms/i)
Calculating -------------------------------------
         🐌...JSONAPI::Serializer    81.154k i/s -    242.039k times in 2.982459s (12.32μs/i)
   🐌...JSONAPI::Serializer(many)    132.644 i/s -     392.000 times in 2.955281s (7.54ms/i)
🐌...JSONAPI::Serializer(include)     25.622 i/s -      76.000 times in 2.966254s (39.03ms/i)
   🐌...JSONAPI::Serializer(deep)     23.679 i/s -      66.000 times in 2.787241s (42.23ms/i)

Comparison:
         🐌...JSONAPI::Serializer:     81154.2 i/s 
   🐌...JSONAPI::Serializer(many):       132.6 i/s - 611.82x  slower
🐌...JSONAPI::Serializer(include):        25.6 i/s - 3167.42x  slower
   🐌...JSONAPI::Serializer(deep):        23.7 i/s - 3427.22x  slower

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

stas · 2020-10-24T11:15:31Z

@dvandersluis I'll close this in favor of #141 for now.

dvandersluis · 2021-04-22T18:00:54Z

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

stas · 2021-05-13T16:49:53Z

Sorry @dvandersluis the #141 is usable but unstable at the moment. I'm doing my best trying to find time for it! Sorry 🙈

dvandersluis · 2021-05-13T17:04:30Z

@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!

Handle a heterogeneous collection via serializers option.

ffd7526

dvandersluis force-pushed the heterogeneous-collection branch from c53ad46 to ffd7526 Compare September 17, 2020 19:29

stas reviewed Sep 19, 2020

View reviewed changes

stas closed this Oct 24, 2020

jrochkind mentioned this pull request Sep 12, 2022

Serialize collection of polymorphic objects #121

Open

Handle a heterogeneous collection via serializers option #127

Handle a heterogeneous collection via serializers option #127

Uh oh!

Conversation

dvandersluis commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the current behavior?

What is the new behavior?

Checklist

Uh oh!

stas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stas commented Oct 15, 2020

Uh oh!

dvandersluis commented Oct 15, 2020

Uh oh!

stas commented Oct 15, 2020

Uh oh!

stas commented Oct 16, 2020

Uh oh!

stas commented Oct 24, 2020

Uh oh!

dvandersluis commented Apr 22, 2021

Uh oh!

stas commented May 13, 2021

Uh oh!

dvandersluis commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dvandersluis commented Sep 17, 2020 •

edited

Loading