Use `recursive: true` for encoding ActiveRecord objects by shioyama · Pull Request #14 · Shopify/paquito

shioyama · 2022-08-23T06:03:34Z

When we originally created ActiveRecordCoder, msgpack had no recursive option so we created another coder (ActiveRecordPacker) to pack the record data returned from ActiveRecordCoder (and vice versa).

Now that there is a recursive option, it seems we shouldn't need to do this and just set recursive: true.

|Obviously this is not backwards compatible.~ (Never mind!) ~~For cached data we'd just need to clear the cache, for persisted data this would be problematic, but then nobody's serializing records to the database anyway so that should be fine~~

Also, as noted in a comment, previously ActiveRecordPacker contained all types necessary to pack a record, whereas now the codec uses itself, therefore if it doesn't have the required types then things go wrong. This is also not backward-compatible, and we might want to issue a warning of some kind in case those types are missing (though I'm not sure where that would go).

shioyama · 2022-08-23T06:23:48Z


-    codec = Paquito::CodecFactory.build([ActiveRecord::Base])
+    codec = Paquito::CodecFactory.build([
+      ActiveRecord::Base, Symbol, Time, DateTime, Date, BigDecimal, ActiveSupport::TimeWithZone,


This is easy to miss: previously ActiveRecordPacker contained all the necessary types to encode a record, but now since the codec is recursively applying itself it needs to have those types itself.

I think this makes sense but the failure mode here (silent failure?) seems bad.

I think in "recursive mode" .build([ActiveRecord::Base]) should append the other necessary types.

The problem is that it then allows you to use these types even outside of a record.

The problem is that it then allows you to use these types even outside of a record.

Yeah exactly.

Ok so I was wrong and you were indeed correct, the payload does not change, but the change here (requiring all types in the parent codec) is an API change if nothing else. Not sure what we should do about this one.

We could have something like "dependent types" , so if you try to call Paquito::Types.register with ActiveRecordCoder but without the other types it needs, it would raise or at least warn that it needs those types.

Alternatively we could keep the "manual nesting". It's unclear what we gain by using recursive here.

It's unclear what we gain by using recursive here.

We get rid of all of ActiveRecordPacker, that seems significant no?

It's not that much code, and it allow for better error messages. I'm not 100% against, but just saying the win isn't that clear to me.

I quite like that it leverages a feature (recursive) that suits the use case very closely. The only downside I see is that the type here has implicit dependencies, but cutting 50 lines of code seems like a win. Agree about error messages but maybe we can find another way to do that 🤔

Ok I tried adding a simple implementation of dependent types. I'm not sure if I like it. There are many cases where you don't actually need all the types, but are required to declare them anyway.

shioyama · 2022-08-23T06:26:42Z

The Ruby head failures don't seem related to this PR.

casperisfine · 2022-08-23T06:30:07Z

Obviously this is not backwards compatible.

Are you sure it's not? The format should be the same. It would be worth trying at least.

#7 was backward compatible for instance.

But if somehow we can't preserve backward compatibility, then we can't delete the old coder.

shioyama · 2022-08-23T06:31:54Z

Are you sure it's not?

Hmm, I can easily check this with a test with a hard-coded payload. Let me try that.

shioyama · 2022-08-23T06:36:33Z

Confirmed, doesn't work.

  1) Error:
PaquitoCodecFactoryActiveRecordTest#test_preserves_previous_encoding_scheme:
Paquito::UnpackError: unexpected extension type
    /Users/chrissalzberg/src/github.com/Shopify/paquito/lib/paquito/codec_factory.rb:47:in `rescue in load'
    /Users/chrissalzberg/src/github.com/Shopify/paquito/lib/paquito/codec_factory.rb:40:in `load'
    /Users/chrissalzberg/src/github.com/Shopify/paquito/test/activerecord/codec_factory_active_record_test.rb:32:in `block in <class:PaquitoCodecFactoryActiveRecordTest>'

There's a difference because in the recursive case, there is no messagepack "wrapper", whereas in the case with a separate coder there is. I forget the exact difference of bytes but it's small but important (I looked into this way back when...)

Cherry-pick that last commit onto master to confirm it works with the current coded + ActiveRecordPacker.

shioyama · 2022-08-23T06:38:11Z

Maybe I should make a separate PR with the hard-coded payload test so we can first merge that. Then we're sure what's changing here and we'll have a trace (and also avoid any backwards-incompatible changes in the future).

casperisfine · 2022-08-23T06:39:54Z

Maybe I should make a separate PR with the hard-coded payload test so we can first merge that

Yes, absolutely.

casperisfine · 2022-08-23T06:42:10Z

    assert_equal(shop, codec.load(reencoded_value))
  end

+  test "preserves previous encoding scheme" do


You also want to test dumping the value produce the same payload.

shioyama · 2022-08-23T06:59:34Z

~~FWIW the payload size in the test here drops with this change from 286 bytes to 274 bytes.~~

shioyama · 2022-08-23T07:11:51Z

Ok, seems I was wrong! I just hadn't passed all the necessary types to the codec in the new test, which I've now done. This now passes with the same payload.

I've removed ActiveRecordPacker since we shouldn't need it anymore.

casperisfine · 2022-11-24T10:05:16Z

I just realized we never merged this, I probably missed a notification and never did the last round of review :/

shioyama · 2022-11-25T03:00:02Z

+        packer: ->(value, packer) { packer.write(ActiveRecordCoder.dump(value)) },
+        unpacker: ->(unpacker) { ActiveRecordCoder.load(unpacker.read) },
+        recursive: true,
+        dependent_types: ["Symbol", "Time", "DateTime", "Date", "BigDecimal", "ActiveSupport::TimeWithZone"],


I feel like this is may be overkill.

I'd think so too.

hmcguire-shopify · 2026-03-24T16:43:14Z

@byroot @shioyama are either of y'all still interested in this? I noticed that ActiveRecordPacker is one of two (the other is #66) big places where we're calling Factory#load instead of using a pool.

I think a "simpler" solution could be to have the factory always call .pool(1) so that at least one {un,}packer is cached, but I think that would only help single threaded applications?

byroot · 2026-03-25T08:26:45Z

are either of y'all still interested in this?

I don't have short term interest in Paquito. Perhaps in the future, dunno.

but I think that would only help single threaded applications?

The MessagePack pool is designed so that if all pooled factories are in use, it fallback to use an ephemeral one. So even a pool of size 1 help if you use multiple threads. Also not all thread always use msgpack at the same time.

That being said, IIRC the pool adds overhead too, so it's not always a win on small payloads, kinda depends.

shioyama · 2026-03-30T07:44:39Z

@hmcguire-shopify I'm not working in anything related to pacquito now, probably not in the near-term future. This seemed like a sensible thing at the time, but as I recall it wasn't really a win. Happy to close if we're not going to merge it.

shioyama changed the title ~~Use recursive option for encoding ActiveRecord objects~~ Use recursive: true for encoding ActiveRecord objects Aug 23, 2022

shioyama force-pushed the shioyama/active_record_coder_recursive branch 2 times, most recently from 9ff2617 to 2a96004 Compare August 23, 2022 06:22

shioyama commented Aug 23, 2022

View reviewed changes

shioyama marked this pull request as ready for review August 23, 2022 06:26

shioyama requested a review from casperisfine August 23, 2022 06:27

shioyama mentioned this pull request Aug 23, 2022

Add ActiveRecordCoder codec test with hard-coded payload #16

Merged

casperisfine reviewed Aug 23, 2022

View reviewed changes

shioyama force-pushed the shioyama/active_record_coder_recursive branch from 662f07c to ad1e557 Compare August 23, 2022 06:47

shioyama changed the base branch from master to shioyama/add_hard_coded_activerecord_coder_codec_test August 23, 2022 06:47

shioyama force-pushed the shioyama/active_record_coder_recursive branch from ad1e557 to 59ef6d1 Compare August 23, 2022 06:48

shioyama force-pushed the shioyama/add_hard_coded_activerecord_coder_codec_test branch 3 times, most recently from 7f4618c to 31e2d14 Compare August 23, 2022 06:53

shioyama force-pushed the shioyama/active_record_coder_recursive branch 2 times, most recently from 55062d6 to c8d61a2 Compare August 23, 2022 06:56

shioyama force-pushed the shioyama/active_record_coder_recursive branch from a9427c7 to 7d173ef Compare August 23, 2022 07:11

shioyama changed the base branch from shioyama/add_hard_coded_activerecord_coder_codec_test to master August 23, 2022 07:13

shioyama added 2 commits November 25, 2022 10:45

Use recursive option for encoding ActiveRecord objects

ec030a8

Remove ActiveRecordPacker

bb7d1a2

shioyama force-pushed the shioyama/active_record_coder_recursive branch from 2632a22 to bb7d1a2 Compare November 25, 2022 01:47

shioyama commented Nov 25, 2022

View reviewed changes

Add dependent_types

9bd62a7

shioyama force-pushed the shioyama/active_record_coder_recursive branch from cab95a3 to 9bd62a7 Compare November 25, 2022 07:29

Uh oh!

Conversation

shioyama commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shioyama commented Aug 23, 2022

Uh oh!

casperisfine commented Aug 23, 2022

Uh oh!

shioyama commented Aug 23, 2022

Uh oh!

shioyama commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shioyama commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperisfine commented Aug 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shioyama commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shioyama commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperisfine commented Nov 24, 2022

Uh oh!

shioyama Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmcguire-shopify commented Mar 24, 2026

Uh oh!

byroot commented Mar 25, 2026

Uh oh!

shioyama commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shioyama commented Aug 23, 2022 •

edited

Loading

shioyama commented Aug 23, 2022 •

edited

Loading

shioyama commented Aug 23, 2022 •

edited

Loading

shioyama commented Aug 23, 2022 •

edited

Loading

shioyama commented Aug 23, 2022 •

edited

Loading

shioyama Nov 25, 2022 •

edited

Loading