Unexpected behavior when deploy script is missing

vectro commented 1 month ago

Steps to reproduce

Set up a Sqitch project where some changes’ deploy scripts are missing.
Run sqitch deploy

Or equivalently, run these commands:

sqitch init ...
sqitch add test1
sqitch add test2
rm deploy/test1.sql
rm deploy/test2.sql
sqitch deploy

Expected behavior

Sqitch should fail with an error the first time a plan with a missing deploy script is encountered, preferably before deploying anything.

Actual behavior

The first change with a missing deploy script succeeds, leaving a null value in changes.script_hash.
The second change with a missing deploy script fails due to the unique constraint on changes.script_hash, with the confusing error message “The deploy script is not unique”.

Other notes

Tested using Sqitch 1.4.1 with the Oracle database driver. On Oracle (and some other platforms), a unique constraint on a null column allows at most one null value on that column.

ewie commented 1 month ago

sqitch init ...
sqitch add test1
sqitch add test2
rm deploy/test1.sql
rm deploy/test2.sql
sqitch deploy

This will fail for change test1 unless you use sqitch deploy --log which does not check that deploy scripts actually exist:

https://github.com/sqitchers/sqitch/blob/5522821bdee36523bc9ab36d7c550834ca2c2748/lib/App/Sqitch/Engine.pm#L1041

But I think that Sqitch should check that the deploy and revert scripts exists when log_only is true. In normal deploy mode this check is delegated to the database client.

I came across this very same issue on Postgres today when a team mate reworked a change that was only logged after being restored from a schema dump. Problem was that the original script copies created by sqitch rework were not committed and missing from the CI run. (We regularly "squash" changes by creating a schema dump to restore that instead of incurring the overhead of deploying each change individually in our CI jobs.)

theory commented 1 month ago

Seems like maybe we could use a function to check for the presence of deploy scripts (on deploy) and revert scripts (on revert) to be called alongside check_deploy_dependencies in deploy and check_revert_dependencies in revert.

sqitchers / sqitch