Support multi-column cursors

[ClaytonPassmore]

What

This change adds support for multi-column cursors.

Fixes #1226

Adds a new option to serialize cursor values as JSON for new runs. Any existing runs will continue to use the old cursor encoding mechanism, which was purely taking the cursor value and coercing it to a string via ActiveRecord.

Upon resuming a job, JSON cursors from the run model are parsed before being handed back to the enumerator.

How

This is accomplished by tracking how the cursor is encoded on the run model via a new boolean column, cursor_is_json.

For existing runs, this value will be false and for new runs, we're setting the value based on the newly added MaintenanceTasks.json_cursors config value, which defaults to false.

Why

Multi-column cursors can occur when:

• A task specifies multiple fields via #cursor_columns, or
• Iterating over an ActiveRecord collection that has a composite primary key

Since #cursor_columns is a documented feature, it seems like there's some impetus to support multi-column cursor values.

Rails added support for models with composite primary keys back in 7.1. As someone that works on a multi-tenant app, being able to iterate over models with composite primary keys is a very important feature 😄

I just added support for batching over ActiveRecord relations with composite primary keys in the job-iteration gem (see Shopify/job-iteration#650). If we can get multi-column support in the maintenance_tasks gem, then it should "just work" with models that use composite primary keys once that job iteration change lands.

Risks

This change could introduce issues for cursor values that are not serializable as JSON, or that lose data/precision after going through the encode/decode process.

It's worth noting that this change increases the risk of producing a cursor value that is too large to fit into the cursor string column on the maintenance_tasks_runs table. All databases are different, but I believe that databases like MySQL have a hard character limit on string columns. By supporting multi-column cursors, we're enabling larger cursor values to be produced. There are probably going to be edge cases where a multi-column cursor value will not fit into a 255 character column.

[adrianna-chang-shopify]

Hey @ClaytonPassmore , thanks for working on this (and getting that work shipped in job-iteration)! Agree that it would be really nice to get this working in the gem, specifically with the use case of iterating over CPK-backed Active Record relations.

A few things come to mind based on an initial pass:

• We can't assume, when an app upgrades the maintenance_tasks gem, that the AddCursorIsJsonFlagToRuns migration will run imminently. We need to ensure that the code changes would be compatible with an older version of the schema. I think the best way to handle this is to add a config that users can enable after they're confident the database changes have executed that allows us to start setting that column value.
• We should definitely add documentation around multi-column cursor support, and the fact that cursors are serialized as JSON (which also implies that cursors must be JSON-serializable. We should note this in the Custom Enumerators section as well).
• It would be awesome to add a new fixture task that iterates over a model with a CPK, and add test coverage for that alongside the tests you added to task_job_test.rb.

cc @etiennebarrie @nvasilevski if either of you have feedback here as well.

[adrianna-chang-shopify]

Should this default to false?

[ClaytonPassmore]

I was just being cautious because I believe adding a default value in certain older versions of MySQL and Postgres is a heavy operation. It seems like this shouldn't be an issue for more recent databases though. I've gone ahead and updated it to have a default and a NOT NULL constraint.

[ClaytonPassmore]

Thanks for the feedback @adrianna-chang-shopify! I threw up some changes based on your suggestions. I haven't had a chance to write a new fixture task yet but hopefully I can get to that soon.

[ClaytonPassmore]

@adrianna-chang-shopify alright, I went ahead and added another task that iterates over a model with CPK and added some test coverage like you suggested. If what I added wasn't quite what you were looking for, let me know!

I was going to write a test case that shows how the gem will fail when you try to iterate over a CPK model without JSON cursors enabled but I quickly realized that how the gem fails depends on a few factors, like the database adapter used, the column types, and the seeded records. So instead of doing that, I figured I would just leave it as "undefined behaviour".

[etiennebarrie]

Can we avoid this by looking at the first character and assuming it's JSON if it's a [? Or is there a compatibility issue with other types of cursors?

[ClaytonPassmore]

Right now in main, if the cursor is a multi-column cursor (which obviously doesn't work yet but is documented), the cursor value is an array. Since the cursor value gets serialized with to_s, it will still result in a string that starts with [, but its contents won't necessarily be JSON objects.

[ClaytonPassmore]

I guess there's also the possibility that someone could have a task that uses a single string column for their cursor and that column contains strings that start with [.

[ClaytonPassmore]

I'll take a look at these failures shortly 👍

[adrianna-chang-shopify]

My main piece of feedback right now is that it adds a good amount of complexity to be checking both the config and the existence of the column via has_attribute?(:cursor_is_json). Can we assume that if the config is on, the migration has run? We could raise when the config is set instead:

# lib/maintenance_tasks.rb

mattr_reader :json_cursors, default: false

def self.json_cursors=(value)
  msg = "Run is missing the required :cursor_is_json column. Ensure migration to add JSON cursor support is run before enabling json_cursors."
  raise msg unless Run.has_attribute?(:cursor_is_json)
  @@json_cursors = value
end

[ClaytonPassmore]

@adrianna-chang-shopify thank you for the thoughtful suggestions!

I went ahead and implemented a few of the changes you suggested. There were a couple of comments that would introduce an edge case, I responded to those with some detail. Let me know if you would like me to make those changes anyway.

The CI failures should be fixed as well -

1. Didn't realize there were system tests 😅
2. Apparently minitest-mock was split out of minitest in v6, which affected a subset of the matrix. I added it as a test dependency and things seem to be back on track.

[ClaytonPassmore]

@adrianna-chang-shopify apologies for missing that rubocop violation. Hopefully we're good for another run now.

I had to disable a rubocop rule when setting the class variable for serialize_cursors_as_json. It seems that rubocop doesn't care about mattr_attribute but as soon as you set a class variable explicitly, it gets flagged.

Edit: Oops I didn't see your reply about reading the configuration flag. I'll make that change as well.

[ClaytonPassmore]

GitHub Actions is having some issues today 😞

[adrianna-chang-shopify]

Hey @ClaytonPassmore , great work and thanks for your patience on this! A few very minor comments left, and needs a rebase, but I'll merge after that's done.

[adrianna-chang-shopify]

We can avoid the rubocop violation by making this a class instance variable instead:

    def serialize_cursors_as_json
      @serialize_cursors_as_json || false
    end

    def serialize_cursors_as_json=(value)
      msg = "Run is missing the required `cursor_is_json` column. " \
        "Ensure migration to add JSON cursor support is run before enabling serialize_cursors_as_json."

      raise(msg) if value && Run.column_names.exclude?("cursor_is_json")

      @serialize_cursors_as_json = value
    end

I don't think we should mark it as private visibility since we're expecting users to call it?

[ClaytonPassmore]

Thanks for the suggestion. I was trying to shoehorn it into working with mattr_* but this is a better approach.

I wasn't super confident with the annotations so let me know if you'd like me to update anything there.

[ClaytonPassmore]

Thanks for taking a look @adrianna-chang-shopify! Should be ready for another pass.

Wanted to mention that I merged instead of rebasing. This is the workflow I'm used to - I didn't want to force push the branch and potentially lose some of the context that earlier comments were made in.

That said - if you'd rather I rebase for a cleaner branch, I can definitely do that 👍

[adrianna-chang-shopify]

Looking good! One last thing that caught my attention -- I pushed a commit to turn this on in the dummy app so that we can run the CPK task with cursors serialized properly.

I ran into issues trying to copy the migration file over to the dummy app via bundle exec rake maintenance_tasks:install:migrations, and realized that we have a bit of a catch-22 here -- we can't install the migration if the app won't boot, the app won't boot if the column doesn't exist because of our check in serialize_cursors_as_json=:

➜  dummy git:(feature/json-cursor) ✗ rake maintenance_tasks:install:migrations
rake aborted!
Run is missing the required `cursor_is_json` column. Ensure migration to add JSON cursor support is run before enabling serialize_cursors_as_json.

I'm thinking instead we might want to validate lazily when the feature is first used? Move the column check into the Runner, and fail with a clear error there if someone is trying to use the feature without the column enabled? What do you think?

Once we sort that out, we can run the rake task to copy the migration over to the dummy app, run it, and then we should be good to go.

[ClaytonPassmore]

@adrianna-chang-shopify good catch!

I made the change you requested - I pushed as much of the logic into the RunConcern as I could to reduce coupling with Runner.

I confirmed that the migrations are now capable of being copied into the dummy app, though I see that none of the maintenance tasks migrations reside in test/dummy/db/migrate/ - only migrations for the models that exist in that app.

I didn't commit those copied migration files because it seemed intentional for them to not be there. Let me know if that was the wrong decision though 👍

[ClaytonPassmore]

I added this has_attribute? check back in to cover the case where someone:

1. Enables serialize_cursors_as_json for their application
2. Doesn't run the migration
3. Deploys their app
4. Has existing tasks running

If we didn't do this, any running tasks would suddenly blow up with a NoMethodError when we try to determine how to decode the cursor.

[adrianna-chang-shopify]

Actually, I think I've convinced myself that we don't need the extra layer of defense around checking whether the column is there or not. We can make it clear in the release notes that the feature requires running a migration to use. If a user doesn't run the migration, they'll encounter the "Unknown column 'cursor_is_json'" error which seems sufficient.

I didn't commit those copied migration files because it seemed intentional for them to not be there

Yes, sorry, I mentioned copying them over, that was a brain fart on my part 🤦‍♀️ The dummy app runs in the context of the engine, so it's already aware of those migrations (and cursor_is_json is evidently already in the schema there lol).

[ClaytonPassmore]

@adrianna-chang-shopify no worries! 😄

I made that change and added some additional text to remind people to run migrations before they turn on the feature.