-
Notifications
You must be signed in to change notification settings - Fork 46
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
Conversation
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.
fb20f1d
to
9c6a99f
Compare
ActiveModel::Serializers
to serialize cursorActiveJob::Serializers
to serialize cursor
There was a problem hiding this 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.
There was a problem hiding this 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.
Cool. I'll integrate this work into the other branch. 👍 |
8186aad
to
eb2859f
Compare
Closing in favour of #81, as these changes have been integrated into it. |
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.