diff --git a/internal/database/postgres/migrations/000057_cleanup_universal_plans.down.sql b/internal/database/postgres/migrations/000057_cleanup_universal_plans.down.sql new file mode 100644 index 000000000..b01928788 --- /dev/null +++ b/internal/database/postgres/migrations/000057_cleanup_universal_plans.down.sql @@ -0,0 +1,13 @@ +-- 000057 down: intentional no-op. +-- +-- The up migration deletes purchase_plans rows that had no plan_accounts +-- entry (universal plans). Deleted rows cannot be recovered by a SQL +-- migration because the data is gone. To restore a deleted plan, operators +-- must replay from a database backup taken before the up migration ran. +-- +-- Rolling back the migration version without restoring the data would leave +-- the schema_migrations table claiming version 56 while the deleted rows are +-- still absent -- the rollback cannot make the DB consistent with pre-57 +-- state anyway. A no-op .down.sql makes this explicit rather than hiding it +-- behind a silent empty file. +SELECT 1; -- no-op: data cannot be reconstructed from SQL alone diff --git a/internal/database/postgres/migrations/000057_cleanup_universal_plans.up.sql b/internal/database/postgres/migrations/000057_cleanup_universal_plans.up.sql new file mode 100644 index 000000000..49314de40 --- /dev/null +++ b/internal/database/postgres/migrations/000057_cleanup_universal_plans.up.sql @@ -0,0 +1,40 @@ +-- 000057: delete purchase_plans rows that have no plan_accounts entry. +-- +-- Background: before PR "fix(plans): eliminate universal plans" (closes #743), +-- the API accepted a POST /api/plans body with an empty target_accounts list +-- and wrote a purchase_plans row with no corresponding plan_accounts rows. +-- Those "universal plans" were never correctly scoped to any account; +-- migration 000057 removes them permanently. +-- +-- Cleanup semantic: DELETE. +-- The three options discussed in issue #742 are (a) delete, (b) fan-out to +-- all matching accounts, and (c) manual review per plan. A SQL migration +-- can only act uniformly across all rows; fan-out (b) would silently attach +-- every account to plans the operator may never have intended to run, and +-- manual review (c) cannot be encoded in a migration. Deletion is the only +-- safe default: the rows predate account-scoping enforcement, have no +-- explicit account assignment, and leaving them in place could trigger +-- unintended purchases once the scheduler resumes. +-- +-- Referential safety: +-- purchase_executions.plan_id FK is ON DELETE SET NULL (migration 000033). +-- purchase_history.plan_id FK is ON DELETE SET NULL (initial schema). +-- plan_accounts.plan_id FK is ON DELETE CASCADE (migration 000011). +-- Deleting from purchase_plans therefore nullifies the plan_id in any +-- linked execution/history rows and removes the (already-absent) plan_accounts +-- rows -- no orphaned child rows remain. +-- +-- Idempotency: re-running after all universal plans are already gone is a +-- no-op (zero rows match the WHERE NOT EXISTS predicate). +-- +-- Down migration note: the .down.sql is intentionally a no-op. Deleted rows +-- cannot be restored by a migration because the original data is gone. +-- Operators who need to recover a specific plan must restore from a backup +-- taken before this migration ran. + +DELETE FROM purchase_plans +WHERE NOT EXISTS ( + SELECT 1 + FROM plan_accounts pa + WHERE pa.plan_id = purchase_plans.id +); diff --git a/internal/database/postgres/migrations/000057_cleanup_universal_plans_test.go b/internal/database/postgres/migrations/000057_cleanup_universal_plans_test.go new file mode 100644 index 000000000..e54f1b9ab --- /dev/null +++ b/internal/database/postgres/migrations/000057_cleanup_universal_plans_test.go @@ -0,0 +1,160 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMigration_CleanupUniversalPlans asserts that migration 000057 deletes +// every purchase_plans row that has no plan_accounts entry (universal plan) +// while leaving correctly scoped plans intact. +// +// Fixture layout: +// +// planUniversal1, planUniversal2 -- no plan_accounts rows (should be deleted) +// planScoped -- has one plan_accounts row (must survive) +// +// The test also verifies that: +// - purchase_executions rows linked to a deleted universal plan have their +// plan_id NULLed (ON DELETE SET NULL, migration 000033). +// - purchase_history rows linked to a deleted universal plan have their +// plan_id NULLed (ON DELETE SET NULL, initial schema). +// - Re-running the migration (idempotency) is a no-op when no universal +// plans remain. +// +// The down migration is a no-op by design (deleted rows cannot be recovered +// from SQL alone), so no rollback assertion is made. +func TestMigration_CleanupUniversalPlans(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Run all migrations to head (includes 000057), then roll back one so we + // sit at 000056, seed fixtures, and re-run to apply 000057 in isolation. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, 1)) + + // Seed: two cloud accounts (needed for plan_accounts FK). + _, err = pool.Exec(ctx, ` + INSERT INTO cloud_accounts (id, name, provider, external_id) + VALUES + ('aaaaaaaa-aaaa-aaaa-aaaa-000000000001', 'acct-1', 'aws', '111111111111'), + ('aaaaaaaa-aaaa-aaaa-aaaa-000000000002', 'acct-2', 'aws', '222222222222') + `) + require.NoError(t, err) + + // Two universal plans (no plan_accounts rows). + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_plans (id, name, services) + VALUES + ('11111111-1111-1111-1111-000000000001', 'universal-plan-1', '{"aws:rds": {}}'::jsonb), + ('11111111-1111-1111-1111-000000000002', 'universal-plan-2', '{"aws:ec2": {}}'::jsonb) + `) + require.NoError(t, err) + + // One correctly scoped plan (has a plan_accounts entry). + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_plans (id, name, services) + VALUES ('22222222-2222-2222-2222-000000000001', 'scoped-plan', '{"aws:rds": {}}'::jsonb) + `) + require.NoError(t, err) + + _, err = pool.Exec(ctx, ` + INSERT INTO plan_accounts (plan_id, account_id) + VALUES ('22222222-2222-2222-2222-000000000001', 'aaaaaaaa-aaaa-aaaa-aaaa-000000000001') + `) + require.NoError(t, err) + + // Execution linked to universal-plan-1: plan_id must be NULLed after delete. + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions (id, plan_id, execution_id, status, step_number, scheduled_date) + VALUES ( + 'eeeeeeee-eeee-eeee-eeee-000000000001', + '11111111-1111-1111-1111-000000000001', + 'eeeeeeee-eeee-eeee-eeee-000000000002', + 'completed', 1, NOW() + ) + `) + require.NoError(t, err) + + // History row linked to universal-plan-1: plan_id must be NULLed after delete. + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_history + (id, account_id, purchase_id, timestamp, provider, service, region, + resource_type, term, payment, plan_id, plan_name) + VALUES ( + 'hhhhhhhh-hhhh-hhhh-hhhh-000000000001', + '111111111111', + 'ri-abc123', + NOW(), 'aws', 'rds', 'us-east-1', 'm5.large', 36, 'all-upfront', + '11111111-1111-1111-1111-000000000001', + 'universal-plan-1' + ) + `) + require.NoError(t, err) + + // Apply migration 000057. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + // Universal plans must be gone. + var universalCount int + err = pool.QueryRow(ctx, ` + SELECT COUNT(*) FROM purchase_plans + WHERE id IN ( + '11111111-1111-1111-1111-000000000001', + '11111111-1111-1111-1111-000000000002' + ) + `).Scan(&universalCount) + require.NoError(t, err) + assert.Equal(t, 0, universalCount, "both universal plans must be deleted by migration 000057") + + // Scoped plan must survive. + var scopedCount int + err = pool.QueryRow(ctx, ` + SELECT COUNT(*) FROM purchase_plans + WHERE id = '22222222-2222-2222-2222-000000000001' + `).Scan(&scopedCount) + require.NoError(t, err) + assert.Equal(t, 1, scopedCount, "scoped plan (with plan_accounts) must survive migration 000057") + + // Execution linked to deleted universal plan must have plan_id NULLed. + var execPlanID *string + err = pool.QueryRow(ctx, ` + SELECT plan_id::TEXT FROM purchase_executions + WHERE id = 'eeeeeeee-eeee-eeee-eeee-000000000001' + `).Scan(&execPlanID) + require.NoError(t, err) + assert.Nil(t, execPlanID, "execution.plan_id must be NULLed when universal plan is deleted (ON DELETE SET NULL)") + + // History row linked to deleted universal plan must have plan_id NULLed. + var histPlanID *string + err = pool.QueryRow(ctx, ` + SELECT plan_id::TEXT FROM purchase_history + WHERE id = 'hhhhhhhh-hhhh-hhhh-hhhh-000000000001' + `).Scan(&histPlanID) + require.NoError(t, err) + assert.Nil(t, histPlanID, "history.plan_id must be NULLed when universal plan is deleted (ON DELETE SET NULL)") + + // Idempotency: re-running the migration path must be a no-op. + require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, 1)) + // Re-seed only the scoped plan to simulate a clean DB at version 56. + // The universal plans are intentionally absent (already cleaned up). + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + var postIdempotentCount int + err = pool.QueryRow(ctx, `SELECT COUNT(*) FROM purchase_plans`).Scan(&postIdempotentCount) + require.NoError(t, err) + assert.Equal(t, 1, postIdempotentCount, "idempotent re-run must leave the one scoped plan intact") +}