Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add capacity to list jobs by ID + make default (#307)
Here, in addition to the new job list sort orders added by #304, add one more for sorting by job ID, and make it the default. This is another breaking change in the job list API, and I wouldn't normally make it at this point, but our next release will contain a number of breaking changes, including to the job list API, so the time is now. My rationale for the change: * Listing and paginating by ID is an overwhelmingly common pattern in web and database APIs, and I think it being the default would be more intuitive for more people. * Ordering by ID is more ergonomic because no `JobListParams.States` invocation needs to made. It's shorter, and especially when a fairly normal use case will be to iterate across all job rows, this is a minor nicety. * Ordering by ID allows the entire job collection to be iterated regardless of job state. `JobListOrderByTime` requires a state, and some order is needed for cursors to work. * The behavior of `JobListOrderByTime` is a little odd in that it changes dynamically based on the requested list states, and there's no way to intuit what the order will be without knowing a lot about River internals and thinking very carefully about it. Furthermore, the time that'd be chosen wasn't documented anywhere, so the only way to know for sure what it would be was to read River's source code. * With the inclusion of #304, `JobListOrderByTime`'s behavior has gotten even a little more surprising because the state to be chosen to select a timestamp was the _first_ value sent to `JobListParams.States`, with any additional values sent ignored, also creating a somewhat nonsensical result (e.g. `States(running, available)` would select `attempted_at`, but would be `NULL` for any jobs in the `available` state). This behavior was not documented. I also found a bug that was a hold over from #304 as more than one sort order became available. The function `JobListCursorFromJob` took a sort order, but would produce the wrong result unless the user remembered to set the exact same sort order on their job list parameters. For example, this would do the wrong thing: res, err = client.JobList(ctx, NewJobListParams().After(JobListCursorFromJob(job4, JobListOrderByScheduledAt))) `JobListCursorFromJob` would extract `scheduled_at` from `job4`, but then list using to the default job list order. Previously that was based on time, so this result would've been wrong _unless_ the job list parameters filtered to state `available`, `retryable`, or `scheduled` so that `scheduled_at` was also used when comparing to other jobs. A caller could compensate by specifying sort order in both places, but this is pretty ugly, and there was no check to make sure that the list paramaters and cursor shared the same order: res, err = client.JobList(ctx, NewJobListParams().OrderBy(JobListOrderByScheduledAt, SortOrderAsc).After(JobListCursorFromJob(job4, JobListOrderByScheduledAt))) The corrected version of this doesn't use an order when initializing the cursor, instead using the one from the job list params, meaning that the same time field is always used between list query and what's extracted from the cursor's job row. res, err = client.JobList(ctx, NewJobListParams().OrderBy(JobListOrderByScheduledAt, SortOrderAsc).After(JobListCursorFromJob(job4))) This also makes the invocation shorter and more ergonomic to call. Along with the above, we also make a few tweaks to documentation. `JobListOrderByTime` now documents which timestamps it uses, and indicates that only the first value sent to `JobListParams.States` will be respected.
- Loading branch information
