diff --git a/migrations/0002_purge_soft_deleted_documents.down.sql b/migrations/0002_purge_soft_deleted_documents.down.sql new file mode 100644 index 0000000..b726a37 --- /dev/null +++ b/migrations/0002_purge_soft_deleted_documents.down.sql @@ -0,0 +1,10 @@ +-- 0002_purge_soft_deleted_documents.down.sql +-- +-- Deliberately empty. 0002 deletes rows; nothing here can bring them back. +-- +-- A down migration that silently does nothing is better than one that pretends: +-- restoring these documents requires a backup taken before 0002 ran, not SQL. +-- Reverting the code change (making DeleteDocument scoped again) does not +-- resurrect them either. + +SELECT 1; diff --git a/migrations/0002_purge_soft_deleted_documents.up.sql b/migrations/0002_purge_soft_deleted_documents.up.sql new file mode 100644 index 0000000..dd83710 --- /dev/null +++ b/migrations/0002_purge_soft_deleted_documents.up.sql @@ -0,0 +1,23 @@ +-- 0002_purge_soft_deleted_documents.up.sql +-- +-- DeleteDocument is now a hard delete (#136). Documents deleted before that +-- change are still in the table as tombstones: deleted_at is set, every read +-- filters them out, and nothing can bring them back — but their `data` jsonb is +-- intact, and for inference sinks that column holds base64 image bytes. +-- +-- Those rows are the reason the old behaviour failed the retention question in +-- FootprintAI/grandturks#936. Callers were told the documents were deleted. +-- This makes that true. +-- +-- IRREVERSIBLE. There is no down migration that can restore the data, only one +-- that describes what was removed. Take a backup first if the rows have any +-- value to you — by definition of the old API they should not, since nothing +-- could read them. + +DELETE FROM "restcol-documents" WHERE deleted_at IS NOT NULL; + +-- Collections are deliberately NOT purged here. CollectionCURD.Delete is still +-- a soft delete: a collection row carries an id, a type and a summary, not user +-- payload, and hard-deleting one would fire the OnDelete:SET NULL constraint on +-- restcol-collections-schema and leave orphaned schema rows behind. That is a +-- separate change with its own consequences; see #136. diff --git a/pkg/storage/documents/documents.go b/pkg/storage/documents/documents.go index eb5c762..e8c02c9 100644 --- a/pkg/storage/documents/documents.go +++ b/pkg/storage/documents/documents.go @@ -159,14 +159,25 @@ func (c *DocumentCURD) Query(ctx context.Context, return records, nil } +// Delete removes a document permanently. The row and its data are gone; there +// is no recovery. +// +// Unscoped is deliberate (#136). ModelDocument carries gorm.DeletedAt, so +// without it this writes a tombstone and keeps the row — including the `data` +// jsonb, which for inference sinks holds base64 image bytes. That gave the +// worst of both: the caller could not recover the document (there is no +// Undelete RPC, and every read filters deleted_at IS NULL) while the bytes +// stayed in the database indefinitely. A delete that reports success and +// retains the data is not a delete. func (c *DocumentCURD) Delete(ctx context.Context, tableName string, pid appmodelprojects.ProjectID, cid appmodelcollections.CollectionID, did appmodeldocuments.DocumentID) error { - err := c.With(ctx, tableName).Where("id = ? AND model_collection_id = ? AND model_project_id = ?", - did.String(), cid.String(), pid.String()).Delete(&appmodeldocuments.ModelDocument{}).Error + err := c.With(ctx, tableName).Unscoped(). + Where("id = ? AND model_collection_id = ? AND model_project_id = ?", + did.String(), cid.String(), pid.String()). + Delete(&appmodeldocuments.ModelDocument{}).Error return storage.WrapStorageError(err) } -// CountByCollection returns the number of active (non-soft-deleted) documents -// in the given collection. +// CountByCollection returns the number of documents in the given collection. func (c *DocumentCURD) CountByCollection(ctx context.Context, tableName string, pid appmodelprojects.ProjectID, cid appmodelcollections.CollectionID) (int64, error) { var count int64 err := c.With(ctx, tableName). @@ -179,9 +190,12 @@ func (c *DocumentCURD) CountByCollection(ctx context.Context, tableName string, return count, nil } -// DeleteByCollection soft-deletes every document in the given collection. +// DeleteByCollection permanently removes every document in the given +// collection. Same reasoning as Delete: this is what DeleteCollection(force) +// cascades through, so leaving it scoped would mean the cascade retained every +// payload it claimed to remove. func (c *DocumentCURD) DeleteByCollection(ctx context.Context, tableName string, pid appmodelprojects.ProjectID, cid appmodelcollections.CollectionID) error { - err := c.With(ctx, tableName). + err := c.With(ctx, tableName).Unscoped(). Where("model_project_id = ? AND model_collection_id = ?", pid.String(), cid.String()). Delete(&appmodeldocuments.ModelDocument{}).Error return storage.WrapStorageError(err) diff --git a/pkg/storage/documents/documents_test.go b/pkg/storage/documents/documents_test.go index b01e56c..0ec3ec5 100644 --- a/pkg/storage/documents/documents_test.go +++ b/pkg/storage/documents/documents_test.go @@ -216,10 +216,21 @@ func TestDocumentCURD_Delete(t *testing.T) { err = docCURD.Delete(context.Background(), "", regularProject.ID, collection.ID, testDoc.ID) assert.NoError(t, err) - // Verify document is soft deleted (should return empty record) + // Gone from the normal read path... deletedDoc, err := docCURD.Get(context.Background(), "", regularProject.ID, collection.ID, testDoc.ID) assert.NoError(t, err) // Get doesn't error, just returns empty record - assert.Empty(t, deletedDoc.ID.String()) // ID should be empty for soft-deleted record + assert.Empty(t, deletedDoc.ID.String()) // ID is empty when the row is not there + + // ...and gone from the table, not merely hidden by gorm's deleted_at + // filter. This is the assertion that separates a hard delete from a soft + // one; without Unscoped here the query below would still find the row and + // the test would pass on a tombstone (#136). + var remaining int64 + assert.NoError(t, postgrescli.GormDB().Unscoped(). + Model(&appmodeldocuments.ModelDocument{}). + Where("id = ?", testDoc.ID.String()). + Count(&remaining).Error) + assert.Zero(t, remaining, "the row must be deleted, not tombstoned") // Test delete non-existent document (should not error in GORM) nonExistentID := appmodeldocuments.NewDocumentID() @@ -283,3 +294,46 @@ func TestDocumentCURD_Delete_WithWrongScope(t *testing.T) { assert.NoError(t, err) assert.Equal(t, testDoc.ID, foundDoc.ID) } + +// TestDocumentCURD_DeleteByCollection_IsPermanent covers the cascade path that +// DeleteCollection(force=true) uses. It is the one most likely to be missed: +// deleting a whole collection is exactly when a caller expects the payloads to +// be gone, and a scoped delete there would retain every one of them. +func TestDocumentCURD_DeleteByCollection_IsPermanent(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)) + assert.NoError(t, err) + + regularProject, _, err := storageprojects.TestProjectSuite(postgrescli) + assert.Nil(t, err) + + collection, err := storagecollectionstestutils.TestCollectionSuite(postgrescli, regularProject) + assert.NoError(t, err) + + docCURD := NewDocumentCURD(postgrescli) + assert.Nil(t, docCURD.AutoMigrate()) + + for i := 0; i < 3; i++ { + assert.NoError(t, docCURD.Write(ctx, "", &appmodeldocuments.ModelDocument{ + ID: appmodeldocuments.NewDocumentID(), + Data: appmodeldocuments.NewModelDocumentData(map[string]interface{}{"n": i}), + ModelCollectionID: collection.ID, + ModelProjectID: regularProject.ID, + })) + } + + assert.NoError(t, docCURD.DeleteByCollection(ctx, "", regularProject.ID, collection.ID)) + + // Unscoped: counts tombstones as well as live rows, so this fails if the + // cascade only marked them deleted. + var remaining int64 + assert.NoError(t, postgrescli.GormDB().Unscoped(). + Model(&appmodeldocuments.ModelDocument{}). + Where("model_project_id = ? AND model_collection_id = ?", + regularProject.ID.String(), collection.ID.String()). + Count(&remaining).Error) + assert.Zero(t, remaining, "the cascade must remove the rows, not tombstone them") +}