Expose serialization issue with a test

[viniciusgama]

It seems ruby 2.6.6 and 2.7.1 patch versions have broken the deploy object serialization. We have our IM system listening to deploy events and notifying users and rooms of the status of their deployments. The deploy payload used to have the entire author in the commits that were deployed. While using these 2 versions the author object is now replaced by only the author_id and this broke our integration.

While doing some investigation we found out this change was the patch applied that generated both versions we have problems with and coincidentally they are related to JSON serialization.

For this PR I just wrote a simple tests so we could expose the issue. The idea is to see if anyone else have run into this problem and also look from guidance from the main maintainers.

[casperisfine]

see if anyone else have run into this problem

That's weird. We're using json 2.3.0 but haven't ran into such issue. I'll try to take a look.

[casperisfine]

Hum, clearly it's failing on CI, but locally it's fine for me:

$ ruby -v
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]
$ ruby -v
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]
$ bundle show json
/Users/byroot/.gem/ruby/2.7.1/gems/json-2.3.1
$ bin/rails test test/unit/commit_serializer_test.rb
[DEPRECATED] `Bundler.clean_env` has been deprecated in favor of `Bundler.unbundled_env`. If you instead want the environment before bundler was originally loaded, use `Bundler.original_env` (called at /Users/byroot/src/github.com/Shopify/shipit-engine/lib/shipit/command.rb:16)
/Users/byroot/.gem/ruby/2.7.1/gems/actionpack-6.0.2.2/lib/action_dispatch/middleware/stack.rb:37: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/byroot/.gem/ruby/2.7.1/gems/actionpack-6.0.2.2/lib/action_dispatch/middleware/static.rb:110: warning: The called method `initialize' is defined here
/Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/number_helper/number_converter.rb:166: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/byroot/.gem/ruby/2.7.1/gems/i18n-1.8.2/lib/i18n.rb:195: warning: The called method `translate' is defined here
/Users/byroot/.gem/ruby/2.7.1/gems/activerecord-6.0.2.2/lib/active_record/store.rb:106: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/byroot/.gem/ruby/2.7.1/gems/activerecord-6.0.2.2/lib/active_record/store.rb:109: warning: The called method `store_accessor' is defined here
Run options: --seed 23734

# Running:

/Users/byroot/.gem/ruby/2.7.1/gems/activemodel-6.0.2.2/lib/active_model/type/integer.rb:13: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/byroot/.gem/ruby/2.7.1/gems/activemodel-6.0.2.2/lib/active_model/type/value.rb:8: warning: The called method `initialize' is defined here
/Users/byroot/.gem/ruby/2.7.1/gems/activerecord-6.0.2.2/lib/active_record/relation/delegation.rb:115: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/byroot/.gem/ruby/2.7.1/gems/activerecord-6.0.2.2/lib/active_record/relation.rb:27: warning: The called method `initialize' is defined here
.

Finished in 0.151872s, 6.5845 runs/s, 39.5070 assertions/s.
1 runs, 6 assertions, 0 failures, 0 errors, 0 skips
Coverage report generated for Unit Tests to /Users/byroot/src/github.com/Shopify/shipit-engine/coverage. 2410 / 4694 LOC (51.34%) covered.

(I should work on 2.7 compat though).

🤔

[benlangfeld]

@casperisfine You ran the one that is also passing in CI, and exists to demonstrate that the issue is not in the commit serializer itself, but in the relationship between the deployment serializer and the commit serializer.

[viniciusgama]

@casperisfine the commit serializer test will work in either version. The problem is with the deploy serializer. Sorry I forgot to mention, but what I tried to show is that when the commit is serialized as part of the deploy, the author is not present. But when you serialize a commit directly, it works as expected.

Can you try running bin/rails test test/unit/deploy_serializer_test.rb

[casperisfine]

You ran the one that is also passing in CI

🤦 sorry. The other test indeed fail. I'm gonna dig down then.

[casperisfine]

Ok, so I can repro on 2.7.1 and it does pass on 2.6.5.

However I can make it fail with as_json, so I doubt about the explanation of a change in the json gem. I'd rather bet on a active_model_serializers bug (I really need to get rid of that gem...).

I'll keep digging.

[benlangfeld]

I really need to get rid of that gem...

Do you have a preferred approach for a replacement @casperisfine ?

[casperisfine]

Oh I know what it is! I had to fix that very same bug in shopify core a while ago. The Ruby's class lookup semantic change sligthy in 2.7 and it was somehow backported to 2.6.6.

I know the workaround I'll push a fix soon.

Do you have a preferred approach for a replacement

Not really, most alternative I've seen are rather crap. The only one that looks ok is https://github.com/codenoble/cache-crispies. I meant to try it sees if it's as good as it looks, but never really got around to do it.

[viniciusgama]

I was looking into https://github.com/cerebris/jsonapi-resources which has a similar enough API and wouldn't require a lot of changes on the serializers that currently exists. Any thoughts @casperisfine ?

[casperisfine]

Fixed as db85dce

[casperisfine]

Any thoughts

I'd need to audit it. The thing is there are dozens and dozens of libraries like this, and they are often opinionated in a way that doesn't fit my needs, or have absolutely terrible implementations.

[casperisfine]

Ok, so jsonapi-resources as its name indicate is only generating JSONAPI format, so with a top level "data" key, which would mean breaking the API.

[viniciusgama]

Would be nice to know what the plan is, so in case we have to make some changes we don't diverge too much from it. Btw, thanks a lot for the help @casperisfine . Really appreciated.

[casperisfine]

Would be nice to know what the plan is,

Long story short is that we're stuck on an old version of AMS because 0.10.x radically changed direction, it's no longer providing the feature shipit needs, and require to rewrite all serializers anyway. And it's not particularly well written.

Once in a while running such an old gem cause some friction but so far we managed. So the plan is to some day™ migrate to another gem, with no change in the serialization format (no API breakage for users), and if possible little change to the serializers.

That why I meant to look at cache-crispies. But really this is never urgent enough for me to prioritize. I guess I just need to do it even though there's no urgency.

For you it shouldn't change anything, unless you have private extensions that rely on ShipitSerializer, in which case you might have to rewrite some stuff, but hopefully the changes should be minimal.

BTW the 0.32.0 should be out in a few minutes.

[benlangfeld]

Would be nice to know what the plan is,

Long story short is that we're stuck on an old version of AMS because 0.10.x radically changed direction, it's no longer providing the feature shipit needs, and require to rewrite all serializers anyway. And it's not particularly well written.

Once in a while running such an old gem cause some friction but so far we managed. So the plan is to some day™ migrate to another gem, with no change in the serialization format (no API breakage for users), and if possible little change to the serializers.

That why I meant to look at cache-crispies. But really this is never urgent enough for me to prioritize. I guess I just need to do it even though there's no urgency.

For you it shouldn't change anything, unless you have private extensions that rely on ShipitSerializer, in which case you might have to rewrite some stuff, but hopefully the changes should be minimal.

BTW the 0.32.0 should be out in a few minutes.

The concern @viniciusgama expressed is that while we have the capacity to contribute things like this, we need to be aligned so as not to waste our time going in the wrong direction. If you have a route you know you want to take but just can't get to it, then we can work with that and combine availability.

[casperisfine]

is that while we have the capacity to contribute things like this, we need to be aligned so as not to waste our time going in the wrong direction

Sure. Well as a general rule I'm always happy to delegate/merge some features etc. In this case it's a matter of architecture / foundations, that's not something I generally delegate.

So really don't be concerned with this, I'll get around to do it eventually, in the meantime it's not really a big problem. It's not like we're actively spending hundreds of man hours maintaining the serializers, and porting them to another library is a matter of a few hours. The hard part is to select a library that doesn't suck, and that's on me.

[casperisfine]

So in the end I'm likely to go with panko_serializer: #1139

The API is extremely similar to AMS 0.9 which makes the migration very easy.