diff --git a/pkg/storage/documents/purge_migration_test.go b/pkg/storage/documents/purge_migration_test.go new file mode 100644 index 0000000..18f8b88 --- /dev/null +++ b/pkg/storage/documents/purge_migration_test.go @@ -0,0 +1,172 @@ +package storagedocuments + +import ( + "context" + "testing" + + "github.com/sdinsure/agent/pkg/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/footprintai/restcol/migrations" + appmodeldocuments "github.com/footprintai/restcol/pkg/models/documents" + storagecollections "github.com/footprintai/restcol/pkg/storage/collections" + storageprojects "github.com/footprintai/restcol/pkg/storage/projects" + storagetestutils "github.com/footprintai/restcol/pkg/storage/testutil" +) + +// TestMigration0002PurgesTombstonesAndKeepsLiveRows proves what +// 0002_purge_soft_deleted_documents claims, which until now nothing did. +// +// # WHY THIS WAS MISSING, AND WHY IT MATTERED +// +// migrations/embed_test.go says so in its own header: those tests are "about +// COVERAGE, not about the SQL" - they assert every file is reachable through +// the embed, not that any of them does anything. So `0002` shipped, was pinned +// by grandturks (#1004/#1014), and was applied to a deployed cluster, without a +// single assertion that it removes a tombstone. +// +// The gap surfaced when FootprintAI/grandturks#977's criterion 4 - "after +// #970's migration, count(*) WHERE deleted_at IS NOT NULL returns 0" - was run +// against ubm01. It returned 0. It had also returned 0 BEFORE the migration ran, +// because that database had never held a tombstone: `DELETE 0`. The criterion +// passed while demonstrating nothing, which is the exact failure sprint #827 +// closed on, one level down. +// +// This test is the missing half. It cannot be satisfied vacuously: it CREATES a +// tombstone, asserts it is there, then asserts the migration removed it. +// +// # THE TOMBSTONE IS MADE THE WAY THE OLD CODE MADE IT +// +// DocumentCURD.Delete is Unscoped now (#136/#137), so it hard-deletes and +// cannot produce the rows this migration exists to purge. A scoped gorm delete +// is what the pre-#137 code did, and it is what wrote every tombstone in every +// deployed database. Reproducing it here rather than INSERTing a hand-made row +// keeps the fixture honest: gorm sets deleted_at exactly as it did in +// production, and if that behaviour ever changes this test changes with it. +// +// # THE SECOND ASSERTION IS THE ONE THAT PROTECTS DATA +// +// "Tombstones are gone" is half the contract. "Live rows are untouched" is the +// half that makes the migration safe to run, and a `DELETE FROM ... WHERE +// deleted_at IS NOT NULL` with a broken predicate would still satisfy the +// first. Both are asserted, and the keeper is read back through the normal path +// rather than counted, so a row that survived as a tombstone would not pass. +func TestMigration0002PurgesTombstonesAndKeepsLiveRows(t *testing.T) { + if testing.Short() { + t.Skip("requires a local postgres; skipping under -short") + } + + ctx := context.Background() + postgrescli, err := storagetestutils.NewTestPostgresCli(logger.NewLogger(false)) + require.NoError(t, err) + + regularProject, _, err := storageprojects.TestProjectSuite(postgrescli) + require.NoError(t, err) + + docCURD := NewDocumentCURD(postgrescli) + require.NoError(t, docCURD.AutoMigrate()) + + collectionsCURD := storagecollections.NewCollectionCURD(postgrescli) + require.NoError(t, collectionsCURD.AutoMigrate()) + + collection, err := storagecollections.TestCollectionSuite(postgrescli, regularProject) + require.NoError(t, err) + require.NoError(t, collectionsCURD.Write(ctx, "", collection)) + + newDoc := func(payload map[string]interface{}) *appmodeldocuments.ModelDocument { + return &appmodeldocuments.ModelDocument{ + ID: appmodeldocuments.NewDocumentID(), + Data: appmodeldocuments.NewModelDocumentData(payload), + ModelCollectionID: collection.ID, + ModelProjectID: regularProject.ID, + } + } + + keeper := newDoc(map[string]interface{}{"keep": "this row must survive"}) + require.NoError(t, docCURD.Write(ctx, "", keeper)) + + // The payload names what is actually at stake: for taskWorker-sinked + // documents the `data` jsonb holds base64 image bytes, which is why #936 + // raised retention over these rows rather than tidiness. + doomed := newDoc(map[string]interface{}{"imageBytes": "iVBORw0KGgoAAAANSUhEUg=="}) + require.NoError(t, docCURD.Write(ctx, "", doomed)) + + // A SCOPED delete - what the code did before #137 - leaves the row with + // deleted_at set and its data intact. + require.NoError(t, postgrescli.GormDB(). + Where("id = ?", doomed.ID.String()). + Delete(&appmodeldocuments.ModelDocument{}).Error) + + countTombstones := func() int64 { + var n int64 + require.NoError(t, postgrescli.GormDB().Unscoped(). + Model(&appmodeldocuments.ModelDocument{}). + Where("deleted_at IS NOT NULL"). + Count(&n).Error) + return n + } + + // Guard the fixture itself. If this is 0 the migration below would "pass" + // against nothing, which is precisely the vacuous result that prompted the + // test - so it is a hard failure, not an assumption. + require.NotZero(t, countTombstones(), + "fixture did not produce a tombstone; the rest of this test would be vacuous") + + // The row is unreachable through the API and still on disk - the state + // #936 objected to. + // + // The jsonb path is data->'json_value'->>'imageBytes', not + // data->>'imageBytes': ModelDocumentData marshals the caller's map under a + // `json_value` key (documents.go:86). Reading the wrong path returns NULL, + // which is how this assertion first failed - and it would have been easy to + // "fix" by weakening it to a NOT NULL check on the whole column, which + // would no longer show that the PAYLOAD survives. + var doomedData string + require.NoError(t, postgrescli.GormDB().Unscoped(). + Model(&appmodeldocuments.ModelDocument{}). + Where("id = ?", doomed.ID.String()). + Select("data->'json_value'->>'imageBytes'").Scan(&doomedData).Error) + assert.Equal(t, "iVBORw0KGgoAAAANSUhEUg==", doomedData, + "the tombstoned row should still hold its payload before the purge") + + // Apply the migration from the EMBEDDED copy, not from disk - that is what + // a consumer pinning this module actually runs (#1004). + up, err := migrations.EmbeddedMigrations.ReadFile("0002_purge_soft_deleted_documents.up.sql") + require.NoError(t, err, "the migration must be reachable through the embed") + require.NoError(t, postgrescli.GormDB().Exec(string(up)).Error) + + assert.Zero(t, countTombstones(), + "0002 left a soft-deleted row behind - the migration does not do what it says") + + // And the live row is still readable through the normal path. Counting + // would not be enough: a keeper that survived AS A TOMBSTONE would still be + // counted, and would still be broken. + found, err := docCURD.Get(ctx, "", regularProject.ID, collection.ID, keeper.ID) + require.NoError(t, err) + assert.Equal(t, keeper.ID, found.ID, + "0002 removed a LIVE document - its predicate is wrong and it is unsafe to run") +} + +// TestMigration0002IsIdempotent - #970's runbook tells operators "re-running is +// harmless", and that claim is load-bearing: the deployed sites are applied by +// hand, so a second run is likely rather than hypothetical. +func TestMigration0002IsIdempotent(t *testing.T) { + if testing.Short() { + t.Skip("requires a local postgres; skipping under -short") + } + + postgrescli, err := storagetestutils.NewTestPostgresCli(logger.NewLogger(false)) + require.NoError(t, err) + + docCURD := NewDocumentCURD(postgrescli) + require.NoError(t, docCURD.AutoMigrate()) + + up, err := migrations.EmbeddedMigrations.ReadFile("0002_purge_soft_deleted_documents.up.sql") + require.NoError(t, err) + + for i := 0; i < 3; i++ { + require.NoErrorf(t, postgrescli.GormDB().Exec(string(up)).Error, + "run %d of 0002 failed; the runbook promises re-running is harmless", i+1) + } +}