Make column sync additive by default and dropping explicit

[alexstandiford]

Why

syncColumns dropped any column present in the database but absent from the schema definition. That is only correct when the definition is authoritative for the database it points at, and an unattended caller cannot know that. A rollback, a canary, or two services sharing one database all mean the running code can be missing a column the database legitimately holds. Dropping it destroys data.

Navigator had already worked this out and guarded against it. Its container startup script runs install before serving traffic but deliberately does not run migrate, protected by a test named testStartupDoesNotRunDestructiveGenericColumnSynchronization.

The consequence was that no column could be added to a deployed table at all, so a schema change could not ship. That is filed as a Navigator bounty, but the fix belongs here.

What changed

syncColumns adds missing columns and modifies changed ones, and leaves a column it does not recognise alone.

syncColumnsAllowingDrops keeps the old behaviour for a caller that means it.

The safe behaviour is the default, not the opt-in. Dropping a column is unrecoverable; leaving one is at worst a dead column. When the two error directions are that lopsided, the cautious one should be what you get by accident.

No change to the core TableUpdateStrategy interface. Three packages implement it — mysql, sqlite, wordpress — and adding a method would have broken all three. The new capability is on the concrete class, and the interface method simply became safe.

Evidence

TableUpdateStrategy:  OK (14 tests, 29 assertions)

Five new tests, four of which failed before the change. The load-bearing one:

✔ The default does not drop a removed column
✔ Additive mode never drops a column the schema does not declare
✔ Additive mode still adds a missing column
✔ Additive mode still widens a changed column
✔ A removed column is dropped when drops are allowed

Verified against a live MySQL 8 database rather than only in mocks. I dropped a column the code needs and added one it has never heard of, then ran migrate:

Field                 Type          Null  Default
writtenByNewerCode    varchar(50)   YES   NULL      <- left alone
trustScore            int           NO    500       <- restored

Before this change, writtenByNewerCode would have been dropped.

Navigator's own suites pass unchanged against the new behaviour: 3525 unit, 269 integration.

Follow-up this unblocks

Navigator can now run migrate from its startup script, which is what makes a column addition deployable. That change belongs in Navigator and comes next, along with updating the guard test to say what it actually guards, since the reason for the prohibition no longer holds.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17a45b94f8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep additive sync from narrowing newer column types

When older code runs after a newer deployment has widened a column, passing false here prevents only DROP COLUMN; buildSyncColumnsQuery() still emits MODIFY COLUMN for every unequal type. For example, a rollback declaring VARCHAR(100) against the newer database's VARCHAR(255) narrows the column, potentially failing startup when longer values exist or truncating them under permissive settings. This defeats the rollback/canary safety promised by the new default, so narrowing or otherwise destructive modifications should also require the explicit destructive path.

Useful? React with 👍 / 👎.