From 700e224991e58be134b7494428a2da7301446737 Mon Sep 17 00:00:00 2001 From: Hannah Cheng Date: Thu, 13 Aug 2026 23:31:16 +0800 Subject: [PATCH] feat(documents): delete documents for real DeleteDocument reported success and kept the data. ModelDocument carries gorm.DeletedAt, so the delete wrote a tombstone and left the row - including the `data` jsonb, which for inference sinks holds base64 image bytes. That was the worst of both halves. There is no Undelete RPC, no Unscoped() read anywhere, and every read path filters deleted_at IS NULL - so the retained data could never be recovered by anyone, while remaining in the database indefinitely. DataMetadata._deletedAt is populated in the DTO but unreachable by any code path, which is the same fact from the other side. We paid soft delete's cost for none of its benefit. Nothing observable to a client changes: the API already answered 200 and then 404, exactly as a hard delete does. What changes is that the answer is now true. This is what FootprintAI/grandturks#936 asked for and what #126 deliberately left open, being the one irreversible decision in the area. Both document delete paths are Unscoped: Delete a single document DeleteByCollection the cascade behind DeleteCollection(force=true), which is exactly when a caller expects payloads to be gone Collections are deliberately NOT changed. A collection row carries an id, a type and a summary rather than user payload, and hard-deleting one fires the OnDelete:SET NULL constraint on restcol-collections-schema, orphaning schema rows. That is a separate change with its own consequences. The gorm.DeletedAt fields stay on the models on purpose. Removing them would stop gorm filtering deleted_at IS NULL, and any tombstone already in a database would become visible again - resurrecting documents that callers were told were deleted. Keeping the field costs nothing now that nothing sets it. migrations/0002 erases the tombstones that already exist, since those rows are the pre-existing half of the same problem. Its down migration is deliberately a no-op with a comment saying so: nothing can restore them, and a down file that pretends otherwise is worse than one that admits it. Environments running --restcol_auto_migrate=true never apply SQL migrations, so their old tombstones persist until purged by hand. Tested against a real postgres, and confirmed the assertions fail without the change: both new checks count rows Unscoped, so a tombstone fails them where a scoped count would pass on one. Closes #136 Co-Authored-By: Claude Opus 5 (1M context) --- ...0002_purge_soft_deleted_documents.down.sql | 10 ++++ .../0002_purge_soft_deleted_documents.up.sql | 23 ++++++++ pkg/storage/documents/documents.go | 26 +++++++-- pkg/storage/documents/documents_test.go | 58 ++++++++++++++++++- 4 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 migrations/0002_purge_soft_deleted_documents.down.sql create mode 100644 migrations/0002_purge_soft_deleted_documents.up.sql 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") +}