Repository navigation
migrate ignores NOT NULL → nullable on an existing column, then fails integrity check #109
Description
Reproduction
With DataProviderMigrate 0.9.12-beta against an empty Supabase Postgres 15.1.0.117 database:
- Run
DataProviderMigrate migratewith the current NAPmainschema (migrations/schema.yaml). It createspublic.topup.stripe_payment_intent_idasText,isNullable: false. - Change only that column declaration to nullable (
type: Text, with noisNullable: false) and runmigrateagain without--allow-destructive.
The second invocation reports Migration completed successfully, then exits nonzero:
SCHEMA INTEGRITY CHECK FAILED
public.topup.stripe_payment_intent_id: nullability expected NULL but found NOT NULL
The table is empty. The plan contains no ALTER COLUMN ... DROP NOT NULL equivalent. This blocks NAP's additive Stripe Checkout migration: a PaymentIntent ID does not exist when the pending Checkout row is inserted and must become nullable before the code deploys.
Expected
Relaxing NOT NULL should be classified as additive and applied by the normal migrate path, then pass integrity verification. No raw SQL or destructive migration should be required. This is independent of the existing missing-FK-on-ALTER issue #79/#80, which also appears when the Stripe schema adds a tenant FK to an existing table.
Activity
MelbourneDeveloper commentedon Sep 30, 2026
NAP integration review: the existing-table nullable transition and repeat-run assertions in
PostgresUpgradeBlockersTestsare the right coverage direction. The current nullable test uses an empty synthetic table; please also cover a populated table, preserve its existing values, and prove NULL can be inserted after the additive upgrade.Our exact failing field is
public.topup.stripe_payment_intent_id. The acceptance gate is migration from NAP's deployed/main schema to PR Nimblesite/NimblesiteAgenticPlatform#501 using the CLI/YAML path without --allow-destructive, successful integrity verification, then an unchanged repeat run with no operations. Please publish a release containing the fixes; NAP remains pinned to the published 0.9.12-beta tool, so working source alone does not unblock rollout.Source review only; I have not independently run the current DataProvider patch. Additional policy comparison findings are posted on #98, and cleanup requirements on #105.
MelbourneDeveloper commentedon Sep 30, 2026
Second NAP review, 2026-09-30, of local
fix/nap-migration-blockers(7f514faplus current working changes). Source review only; no independent test run.Progress since the first review:
MigrationPlatformRegressionTestsnow drives YAML throughDataProviderMigrate.Program.Main, uses the exacttopup.stripe_payment_intent_idfield, and checks retained NOT NULL columns plus the tenant FK/cascade. This addresses the missing CLI-path coverage.Remaining acceptance gaps:
AssertNullableMigrationstill migrates an empty table. Populate it before upgrading, preserve existing values, insert NULL afterward, and show the unrelated required fields still reject NULL.sqlservermigrations, whileProgram.ExecuteMigrationonly dispatches sqlite/postgres and explicitly rejects SQL Server RLS. Those cases cannot pass against the current implementation. Resolve this without skipping tests or weakening assertions, and report actual results.The full NAP main-to-PR#501 additive upgrade is still needed, including the text-flag predicate blocker described in my new #98 comment. Publish the fixed tool and provide its version; the latest published release is still v0.9.12-beta at this review.