Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate using ActiveJob::Serializers to serialize cursor #252

Closed

Conversation

sambostock
Copy link
Contributor

This investigates an alternate approach to #81, as suggested by @bdewater.

If this approach makes sense, #81 will be updated and this PR will be closed.
If not, then it will also be closed.

sambostock and others added 5 commits July 4, 2022 12:00
Version 2.0 of the gem will forbid cursors that are not (de)serializable using
built in types. This is because adapters serialize to JSON, and we must ensure
this is possible in a lossless fashion.

Ahead of this change, we introduce a deprecation warning, as well as a
configuration which makes it possible to enable the strict behavior.
This configuration can be set globally, and/or at the job level.

For instance, it is possible to enable it globally, but disable it for a jobs
that have yet to be migrated, or alternatively to enable it for jobs that are
already compliant, but leave it disabled globally to allow for non-compliant
jobs.

In theory, any non-compliant jobs are "already broken", however it is possible
that the job works around the "bug" in (de)serialization. One known scenario is
where jobs complete rapidly enough to never actually require cursor
serialization, and as such never actually encounter the problem of
un(de)serializable cursors in production environments. However, this could
technically occur at any time.
Co-authored-by: Sander Verdonschot <[email protected]>
...rather than restrict cursors to simple types.
@sambostock sambostock force-pushed the active-job-serializer-cursor branch from fb20f1d to 9c6a99f Compare July 12, 2022 21:55
@sambostock sambostock requested review from Mangara and bdewater July 15, 2022 14:08
@sambostock sambostock mentioned this pull request Jul 15, 2022
2 tasks
@sambostock sambostock changed the title Investigate using ActiveModel::Serializers to serialize cursor Investigate using ActiveJob::Serializers to serialize cursor Jul 15, 2022
Copy link
Contributor

@bdewater bdewater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for investigating this further. I like the outcome, it plays well with existing Active Job semantics and the only awkward thing (Arguments.serialize([cursor]).first) is not that bad & tucked away.

Copy link
Contributor

@Mangara Mangara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach as well! Accepting whatever Active Job can serialize makes the cursor consistent with the parameters, and it will include things like Active Record objects, which are serialized as Global IDs.

@sambostock
Copy link
Contributor Author

Cool. I'll integrate this work into the other branch. 👍

@sambostock sambostock force-pushed the deprecate-unsafe-cursors branch from 8186aad to eb2859f Compare July 19, 2022 06:04
@sambostock
Copy link
Contributor Author

Closing in favour of #81, as these changes have been integrated into it.

@sambostock sambostock closed this Jul 19, 2022
@sambostock sambostock deleted the active-job-serializer-cursor branch July 19, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants