fix: harden validate-upgrade preflight checks#10
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens the validate-upgrade CLI preflight behavior to better ensure baseline/candidate upgrade validation runs only against a truly clean starting schema and performs baseline-version continuity checks more reliably.
Changes:
- Add an initial “empty schema” preflight check before running baseline migrations, and document the requirement in
validate-upgrade --help. - Fix baseline-version continuity checks by avoiding lossy numeric parsing and ignoring
.down.sqlfiles for baseline presence. - Expand test coverage with a hermetic fake-driver success case and an integration test that rejects non-empty schemas without migration metadata.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cli/validate_upgrade.go | Adds schema preflight check; improves version matching and ignores down-only files for baseline continuity. |
| pkg/cli/validate_upgrade_test.go | Updates unit tests to bypass schema check for fake DSNs; adds down-only baseline rejection + success coverage. |
| pkg/cli/root_test.go | Adds integration coverage ensuring validate-upgrade rejects non-empty schemas even without migration metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return validateUpgradeWithSchemaCheck(ctx, d, baselineReq, candidateReq, ensureEmptySchema) | ||
| } | ||
|
|
||
| func validateUpgradeWithSchemaCheck(ctx context.Context, d interfaces.MigrationDriver, baselineReq, candidateReq interfaces.MigrationRequest, checkSchema func(context.Context, string) error) (upgradeValidationResult, error) { |
There was a problem hiding this comment.
validateUpgradeWithSchemaCheck assumes baseline and candidate migrations run against the same database, but it only runs checkSchema on baselineReq.DSN and never verifies that baselineReq.DSN == candidateReq.DSN. If these differ (e.g. via programmatic use), the function would validate/upgrade two different databases and still report success/failure as if it were a single upgrade path. Consider explicitly rejecting mismatched DSNs (and/or running the schema check against the DSN that will actually be migrated).
| func validateUpgradeWithSchemaCheck(ctx context.Context, d interfaces.MigrationDriver, baselineReq, candidateReq interfaces.MigrationRequest, checkSchema func(context.Context, string) error) (upgradeValidationResult, error) { | |
| func validateUpgradeWithSchemaCheck(ctx context.Context, d interfaces.MigrationDriver, baselineReq, candidateReq interfaces.MigrationRequest, checkSchema func(context.Context, string) error) (upgradeValidationResult, error) { | |
| if baselineReq.DSN != candidateReq.DSN { | |
| return upgradeValidationResult{}, fmt.Errorf("validate-upgrade requires baseline and candidate to target the same database") | |
| } |
| SELECT count(*) | ||
| FROM pg_class c | ||
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| WHERE n.nspname = current_schema() | ||
| AND c.relkind IN ('r', 'p', 'v', 'm', 'S', 'f') | ||
| AND c.relname NOT IN ('schema_migrations', 'goose_db_version', 'atlas_schema_revisions') |
There was a problem hiding this comment.
ensureEmptySchema currently counts only relation-like objects from pg_class (tables/partitions/views/materialized views/sequences/foreign tables). Schemas that contain other user-defined objects (e.g., functions in pg_proc, types/domains in pg_type, extensions, etc.) will still pass the “empty schema” check even though the schema is not truly empty. If the intent is to reject any non-empty schema, the check should be broadened (or the help/error text should be narrowed to match exactly what is being checked).
| SELECT count(*) | |
| FROM pg_class c | |
| JOIN pg_namespace n ON n.oid = c.relnamespace | |
| WHERE n.nspname = current_schema() | |
| AND c.relkind IN ('r', 'p', 'v', 'm', 'S', 'f') | |
| AND c.relname NOT IN ('schema_migrations', 'goose_db_version', 'atlas_schema_revisions') | |
| WITH object_counts AS ( | |
| SELECT count(*) AS cnt | |
| FROM pg_class c | |
| JOIN pg_namespace n ON n.oid = c.relnamespace | |
| WHERE n.nspname = current_schema() | |
| AND c.relkind IN ('r', 'p', 'v', 'm', 'S', 'f') | |
| AND c.relname NOT IN ('schema_migrations', 'goose_db_version', 'atlas_schema_revisions') | |
| UNION ALL | |
| SELECT count(*) AS cnt | |
| FROM pg_proc p | |
| JOIN pg_namespace n ON n.oid = p.pronamespace | |
| WHERE n.nspname = current_schema() | |
| UNION ALL | |
| SELECT count(*) AS cnt | |
| FROM pg_type t | |
| JOIN pg_namespace n ON n.oid = t.typnamespace | |
| WHERE n.nspname = current_schema() | |
| AND t.typtype IN ('c', 'd', 'e', 'm', 'r') | |
| AND t.typrelid = 0 | |
| UNION ALL | |
| SELECT count(*) AS cnt | |
| FROM pg_extension e | |
| JOIN pg_namespace n ON n.oid = e.extnamespace | |
| WHERE n.nspname = current_schema() | |
| ) | |
| SELECT COALESCE(sum(cnt), 0) | |
| FROM object_counts |
Summary
validate-upgradeapplies baseline migrationsVerification
go test ./pkg/cli -run 'TestValidateUpgrade|TestRootIncludesValidateUpgradeCommand' -count=1\n-go test ./... -count=1\n-go build ./cmd/workflow-migrate\n-./workflow-migrate validate-upgrade --help\n-git diff --check\n\n## Notes\n\nThis follows up PR feat: validate migration upgrade paths #9 reviewer findings: a database with user objects but no migration metadata could be accepted as the baseline target, down-only files could satisfy baseline continuity, and version matching used numeric coercion.