Enforce cursor be serializable

[sambostock]

Context & Problem

The cursor_position is serialized by the job adapter (Resque/Sidekiq) and bypasses Active Job's fancy serialization. Both Resque and Sidekiq effectively use JSON.dump to serialize it and JSON.parse to deserialize it.

In cases where a non-JSON-compatible cursor is used, it is silently turned into a String via to_s, which can lead to weird errors when the job resumes after interruption. These are not always straightforward to debug, and don't reliably show up in tests, as Active Support adds implicit coercions to some object's == method.

time = Time.at(0).utc               # =>  1970-01-01 00:00:00 UTC  (Time)
time.to_s                           # => "1970-01-01 00:00:00 UTC" (String)
require 'json'
JSON.parse(JSON.dump(time))         # => "1970-01-01 00:00:00 UTC" (String)
time == JSON.parse(JSON.dump(time)) # => false                     (no coercion)
require 'active_support/all'
JSON.parse(JSON.dump(time))         # => "1970-01-01 00:00:00 UTC" (String)
time == JSON.parse(JSON.dump(time)) # => true                      (implicitly coerced)

Real world example

In one of our repos, we use a custom enumerator around a collection of events fetched from an API and ordered by time. We used the time the event occurred at as a cursor, which was "corrupted" by being serialized into a String. The API expected an ISO8601 timestamp (its wrapper gem converts `Time` objects automatically), but our improperly deserialized time strings were in the wrong format, so it blew up.

This went unnoticed for some time, due to the job not being interrupted, and was tricky to debug due to the reasons outlined above.

What this PR does about it

This prevents these errors by enforcing that the cursors be composed of basic Ruby objects.

This is enforced by (recursively) analyzing the each cursor yielded by the enumerator before each_iteration. If it is composed of anything other than the following classes, a descriptive CursorError is raised:

• Array
• Hash
• String
• Integer
• Float
• NilClass (nil)
• TrueClass (true)
• FalseClass (false)

Rationale

• It is better to verify each cursor and ensure we'd be able to interrupt before having started any work, than to finding out the cursor is unsafe after interruption.
• It is fastest and most reliable to simply analyze the component classes of the cursor (rather than check if serializing and deserializing yields the same object, due to implicit conversions).
• Since most cursors are simple Strings, Integers, or nil, the performance penalty of checking each cursor is negligible.

---

Original Description

The cursor ends up being serialized as a `String`, regardless of if it is a `String` or not (by calling `to_s`).

This can lead to subtle bugs often not found during testing.

For instance, if a Time is used as a cursor and the job is requeued, it instead receives the Time#to_s representation as its new input cursor.

Let's be explicit and save developers the headache of debugging that: let's enforce that the cursor be an instance of String or NilClass.

[sambostock]

Turns out both Sidekiq and Resque encode job params as JSON, so technically as long as

cursor == JSON.load(JSON.dump(cursor))

it should be a (de-)serializable cursor.

However, we need to watch out for arbitrary Ruby objects as noted in Sidekiq's docs, that validation isn't quite good enough.

[kirs]

Thanks for putting the efforts to describe the problem and how you've run into it in your app.
Code LGTM!

[sambostock]

Thanks for the feedback @GustavoCaso! I have

• gone with the simple CLASSES.any? { |klass| object.instance_of?(klass) } approach
• added a Resque cursor corruption test

I also noticed the integration tests are basically identical for Resque and Sidekiq, so I extracted a common test module in #75, to keep it independent of this change.

[rafaelfranca]

Should not Symbols be allowed?

[sambostock]

Symbol serialization is "lossy" as they are turned into string JSON keys, which means they are deserialized into Strings.

$ ruby -rjson -e 'p JSON.parse(JSON.dump(:symbol))'
"symbol"

Therefore they are disallowed.

[rafaelfranca]

Ok, that makes sense but this is a breaking change, so it should not released as a patch release.