Job deletion prevention is incompatible with Que schema v4

[pjohnmeyer]

The following DB function references que_jobs.job_id:

https://github.com/hlascelles/que-scheduler/blob/5eb5d4d95f69bde0b1552e87dec2360e10ebae77/lib/que/scheduler/migrations/6/up.sql#L10-L22

The 1.0 beta has changed that column name to id, from the changelog:

• Finally, if you've built up your own tooling and customizations around Que, you may need to be aware of some DB schema changes made in the migration to schema version 4.
  • The job_id column has been renamed id and is now the primary key. This makes it easier to manage the queue using an ActiveRecord model.

I'd be happy to volunteer to PR this if you would vet my approach, which would be to create a schema version ~5~ 7 that bases the function logic on:

COMMENT ON TABLE public.que_jobs IS '4'

[hlascelles]

Good spot - you are absolutely right. :+1:

Very happy to receive PRs, yes please.

Just to confirm, is your suggested change something like: create que-scheduler migration "7" which changes the que_scheduler_prevent_job_deletion function to check if the column concerned is id or job_id and raise the right error string accordingly? That would indeed be the most accurate change.

If we want though, I am also happy for migration 7 to change the error to just not say the job id. ie change:

raise exception 'Deletion of que_scheduler job % prevented. Deleting the que_scheduler job is almost certainly a mistake.', OLD.job_id;

to:

raise exception 'Deletion of que_scheduler job prevented. Deleting the que_scheduler job is almost certainly a mistake.';

What do you think?

[pjohnmeyer]

You interpreted my suggested change correctly, and yes the que-scheduler migration number would be "7", not the "5" I typed in my initial write-up.

Your suggestion is the easier change to avoid similar problems in the future, but would slightly reduce the usefulness of the error message. I personally think it's worth it to do the extra work unless this becomes a recurring breaking point.

[hlascelles]

The full solution sounds great, a PR would be well received. :+1:

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[pjohnmeyer]

@hlascelles I thought I would get to this by now but have not had the chance. You or anybody in the community should feel free to pick it up, don't wait for me please.

[hlascelles]

No problem :+1:

Done under: https://github.com/hlascelles/que-scheduler/pull/316