From ad7b21f3a2876811cc70beac28637f5a55ac6749 Mon Sep 17 00:00:00 2001 From: Jay Brown Date: Wed, 8 Apr 2026 10:54:32 +0000 Subject: [PATCH] Merged in bugfix/delete-client-cascade (pull request #220) fix cascading delete issue * test passing --- internal/client/delete.go | 2 + internal/client/delete_test.go | 66 +++++++++++++++++++ ..._document_outcomes_delete_actions.down.sql | 15 +++++ ...ch_document_outcomes_delete_actions.up.sql | 17 +++++ internal/document/delete.go | 3 + internal/document/delete_test.go | 58 ++++++++++++++++ internal/folder/delete.go | 2 + internal/folder/delete_test.go | 66 +++++++++++++++++++ 8 files changed, 229 insertions(+) create mode 100644 internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.down.sql create mode 100644 internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.up.sql diff --git a/internal/client/delete.go b/internal/client/delete.go index a52d35d8..a03cd34b 100644 --- a/internal/client/delete.go +++ b/internal/client/delete.go @@ -96,6 +96,8 @@ type clientDependencyDelete struct { // deleteClientDependencies removes collector data, batch uploads, and sync records // for a client within an existing transaction. Must be called after documents and // folders are already deleted. Executes deletes in FK dependency order. +// When batch_uploads are deleted, batch_document_outcomes are CASCADE-deleted by the +// database FK constraint. func deleteClientDependencies(ctx context.Context, q *repository.Queries, clientID string) error { deletes := []clientDependencyDelete{ {"collector min clean versions", q.DeleteCollectorMinCleanVersions}, diff --git a/internal/client/delete_test.go b/internal/client/delete_test.go index ae696268..b1e322d4 100644 --- a/internal/client/delete_test.go +++ b/internal/client/delete_test.go @@ -104,6 +104,72 @@ func TestHardDeleteClient(t *testing.T) { require.Error(t, err) }) + t.Run("deletes client with batch outcomes", func(t *testing.T) { + clientID := "test-client-batch-del" + test.CreateTestClient(t, cfg, clientID, "Test Client Batch Del") + + // Get root folder + rootFolder, err := q.GetFolderByPath(ctx, &repository.GetFolderByPathParams{ + Clientid: clientID, + Path: "/", + }) + require.NoError(t, err) + + // Create batch upload for client + batchID, err := q.CreateBatchUpload(ctx, &repository.CreateBatchUploadParams{ + ClientID: clientID, + OriginalFilename: "test.zip", + TotalDocuments: 1, + }) + require.NoError(t, err) + + // Create a document linked to the batch + filename := "resolved.pdf" + docID := createDocWithEntryForClient(t, cfg, clientID, &rootFolder.ID, "batch-client-doc") + + // Update the document to have the batch_id by creating via params + // (the helper doesn't set batch_id, so create a second doc with batch_id) + docID2, err := q.CreateDocument(ctx, &repository.CreateDocumentParams{ + Clientid: clientID, + Hash: "batch-client-doc2", + Filename: &filename, + Folderid: &rootFolder.ID, + BatchID: &batchID, + }) + require.NoError(t, err) + _ = docID + + // Insert resolved outcome (with document_id set) + err = q.InsertBatchDocumentOutcome(ctx, &repository.InsertBatchDocumentOutcomeParams{ + BatchID: batchID, + Filename: "resolved.pdf", + Column3: repository.BatchOutcomeStatusInitComplete, + DocumentID: &docID2, + }) + require.NoError(t, err) + + // Insert unresolved outcome (document_id NULL) + err = q.InsertBatchDocumentOutcome(ctx, &repository.InsertBatchDocumentOutcomeParams{ + BatchID: batchID, + Filename: "unresolved.pdf", + Column3: repository.BatchOutcomeStatusSubmitted, + }) + require.NoError(t, err) + + // Hard delete the client -- should fail before FK fix + _, err = svc.HardDelete(ctx, clientID) + require.NoError(t, err) + + // Verify client is gone + _, err = q.GetClient(ctx, clientID) + require.Error(t, err) + + // Verify batch_document_outcomes are gone (cascade from batch_uploads deletion) + rows, err := q.ListBatchDocumentOutcomes(ctx, batchID) + require.NoError(t, err) + assert.Empty(t, rows) + }) + t.Run("returns NotFoundError for non-existent client", func(t *testing.T) { _, err := svc.HardDelete(ctx, "non-existent-client") require.Error(t, err) diff --git a/internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.down.sql b/internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.down.sql new file mode 100644 index 00000000..af183867 --- /dev/null +++ b/internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.down.sql @@ -0,0 +1,15 @@ +-- Revert to original FK constraints (NO ACTION). + +ALTER TABLE batch_document_outcomes + DROP CONSTRAINT fk_batch_doc_outcomes_document_id; + +ALTER TABLE batch_document_outcomes + DROP CONSTRAINT fk_batch_doc_outcomes_batch_id; + +ALTER TABLE batch_document_outcomes + ADD CONSTRAINT batch_document_outcomes_document_id_fkey + FOREIGN KEY (document_id) REFERENCES documents(id); + +ALTER TABLE batch_document_outcomes + ADD CONSTRAINT batch_document_outcomes_batch_id_fkey + FOREIGN KEY (batch_id) REFERENCES batch_uploads(id); diff --git a/internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.up.sql b/internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.up.sql new file mode 100644 index 00000000..3da1f50b --- /dev/null +++ b/internal/database/migrations/00000000000126_batch_document_outcomes_delete_actions.up.sql @@ -0,0 +1,17 @@ +-- Fix batch_document_outcomes FK constraints to support cascading deletes. +-- document_id: SET NULL preserves batch outcome history when a document is hard-deleted. +-- batch_id: CASCADE removes outcome rows when their owning batch_upload is deleted. + +ALTER TABLE batch_document_outcomes + DROP CONSTRAINT batch_document_outcomes_document_id_fkey; + +ALTER TABLE batch_document_outcomes + DROP CONSTRAINT batch_document_outcomes_batch_id_fkey; + +ALTER TABLE batch_document_outcomes + ADD CONSTRAINT fk_batch_doc_outcomes_document_id + FOREIGN KEY (document_id) REFERENCES documents(id) ON DELETE SET NULL; + +ALTER TABLE batch_document_outcomes + ADD CONSTRAINT fk_batch_doc_outcomes_batch_id + FOREIGN KEY (batch_id) REFERENCES batch_uploads(id) ON DELETE CASCADE; diff --git a/internal/document/delete.go b/internal/document/delete.go index 8e05a4b8..069fcc9a 100644 --- a/internal/document/delete.go +++ b/internal/document/delete.go @@ -17,6 +17,9 @@ import ( // - document entries (S3 references) // - the document row itself (documentLabels auto-cascade via ON DELETE CASCADE) // +// batch_document_outcomes.document_id is SET NULL by the database FK constraint, +// preserving batch outcome history when a document is removed. +// // S3 source files are intentionally NOT deleted. Their paths are collected before // the transaction, logged at INFO level, and returned for optional verbose responses. // diff --git a/internal/document/delete_test.go b/internal/document/delete_test.go index f88e6d4c..94fde97c 100644 --- a/internal/document/delete_test.go +++ b/internal/document/delete_test.go @@ -144,6 +144,64 @@ func TestHardDeleteDocument(t *testing.T) { assert.True(t, errors.As(err, ¬FoundErr)) }) + t.Run("deletes document that has batch outcome rows", func(t *testing.T) { + q := cfg.GetDBQueries() + + // Use a unique client to avoid collisions with other subtests + batchClientID := "test-doc-batch-del" + test.CreateTestClient(t, cfg, batchClientID, "Test Doc Batch Del") + + rootFolder, err := q.GetFolderByPath(ctx, &repository.GetFolderByPathParams{ + Clientid: batchClientID, + Path: "/", + }) + require.NoError(t, err) + + // Create a batch upload for the client + batchID, err := q.CreateBatchUpload(ctx, &repository.CreateBatchUploadParams{ + ClientID: batchClientID, + OriginalFilename: "test.zip", + TotalDocuments: 1, + }) + require.NoError(t, err) + + // Create a document linked to the batch + filename := "test.pdf" + docID, err := q.CreateDocument(ctx, &repository.CreateDocumentParams{ + Clientid: batchClientID, + Hash: "batch-doc-del-test", + Filename: &filename, + Folderid: &rootFolder.ID, + BatchID: &batchID, + }) + require.NoError(t, err) + + // Insert a submitted outcome referencing this document + err = q.InsertBatchDocumentOutcome(ctx, &repository.InsertBatchDocumentOutcomeParams{ + BatchID: batchID, + Filename: "test.pdf", + Column3: repository.BatchOutcomeStatusSubmitted, + DocumentID: &docID, + }) + require.NoError(t, err) + + // Hard delete the document -- should fail before FK fix + s3Paths, err := svc.HardDelete(ctx, docID) + require.NoError(t, err) + + // Verify document is gone + _ = s3Paths + _, err = q.GetDocumentSummary(ctx, docID) + require.Error(t, err) + + // Verify the batch outcome row still exists but document_id is NULL + // (history preserved via SET NULL on FK) + rows, err := q.ListBatchDocumentOutcomes(ctx, batchID) + require.NoError(t, err) + require.Len(t, rows, 1) + assert.Nil(t, rows[0].DocumentID) + }) + t.Run("returns empty S3 paths when document has no entries", func(t *testing.T) { q := cfg.GetDBQueries() diff --git a/internal/folder/delete.go b/internal/folder/delete.go index b21e97b4..ef9db9a8 100644 --- a/internal/folder/delete.go +++ b/internal/folder/delete.go @@ -21,6 +21,8 @@ import ( // When includeDocuments is true, all documents in the folder tree are cascade-deleted // (using the same document cascade as document.HardDelete), then documentUploads and // folder rows are removed. The entire operation executes in a single database transaction. +// batch_document_outcomes are handled by database FK actions: document_id is SET NULL +// when a document is deleted, and rows CASCADE-delete when their batch_upload is removed. // // S3 source files are intentionally NOT deleted. Their paths are collected, logged at // INFO level, and returned for optional verbose responses. diff --git a/internal/folder/delete_test.go b/internal/folder/delete_test.go index dba550b7..8704b446 100644 --- a/internal/folder/delete_test.go +++ b/internal/folder/delete_test.go @@ -137,6 +137,72 @@ func TestHardDeleteFolder(t *testing.T) { require.Error(t, err) }) + t.Run("deletes folder with batch-backed documents and outcomes", func(t *testing.T) { + // Use a unique client to avoid collisions + batchClientID := "test-folder-batch-del" + test.CreateTestClient(t, cfg, batchClientID, "Test Folder Batch Del") + + batchRootFolder, err := q.GetFolderByPath(ctx, &repository.GetFolderByPathParams{ + Clientid: batchClientID, + Path: "/", + }) + require.NoError(t, err) + + batchSvc := folder.New(cfg) + + // Create a subfolder under root + subFolder, err := batchSvc.CreateFolder(ctx, "/batch-del-sub", &batchRootFolder.ID, batchClientID, "testuser") + require.NoError(t, err) + + // Create a batch upload for the client + batchID, err := q.CreateBatchUpload(ctx, &repository.CreateBatchUploadParams{ + ClientID: batchClientID, + OriginalFilename: "test.zip", + TotalDocuments: 1, + }) + require.NoError(t, err) + + // Create a document in the subfolder with batch_id + filename := "batch-folder-doc.pdf" + docID, err := q.CreateDocument(ctx, &repository.CreateDocumentParams{ + Clientid: batchClientID, + Hash: "batch-folder-doc-hash", + Filename: &filename, + Folderid: &subFolder.ID, + BatchID: &batchID, + }) + require.NoError(t, err) + + // Add a document entry (S3 ref) + err = q.AddDocumentEntry(ctx, &repository.AddDocumentEntryParams{ + Documentid: docID, + Bucket: "test-bucket", + Key: "test-key/batch-folder-doc-hash", + }) + require.NoError(t, err) + + // Insert a resolved outcome with document_id set + err = q.InsertBatchDocumentOutcome(ctx, &repository.InsertBatchDocumentOutcomeParams{ + BatchID: batchID, + Filename: "batch-folder-doc.pdf", + Column3: repository.BatchOutcomeStatusInitComplete, + DocumentID: &docID, + }) + require.NoError(t, err) + + // Hard delete the folder with includeDocuments=true -- should fail before FK fix + _, err = batchSvc.HardDelete(ctx, subFolder.ID, true) + require.NoError(t, err) + + // Verify folder is gone + _, err = q.GetFolderByID(ctx, subFolder.ID) + require.Error(t, err) + + // Verify document is gone + _, err = q.GetDocumentSummary(ctx, docID) + require.Error(t, err) + }) + t.Run("counts documents in nested folder tree correctly", func(t *testing.T) { parent, err := svc.CreateFolder(ctx, "/del-count", &rootFolder.ID, clientID, "testuser") require.NoError(t, err)