| new file mode 100644 |
| index 0000000..4ba5349 |
| --- /dev/null |
| +++ b/internal/memoria/delete_test.go |
| @@ -0,0 +1,359 @@ |
| +// A deleted document must not survive as a citation. |
| +// |
| +// These tests live in package memoria for the same reason lag_test.go |
| +// does: reaching the state they are about means breaking the in-memory |
| +// index under a running app, and the only way to do that from outside |
| +// would be to grow a production method whose only caller is a test. |
| +package memoria |
| + |
| +import ( |
| + "context" |
| + "net/http" |
| + "net/http/httptest" |
| + "strconv" |
| + "strings" |
| + "testing" |
| + |
| + "amadan.net/oficina/memoria/internal/memoria/index" |
| +) |
| + |
| +// storeOneDocument puts a document in and proves it is findable, so every |
| +// test below starts from a document that is genuinely there. |
| +func storeOneDocument(t *testing.T, a *app, term string) int64 { |
| + t.Helper() |
| + ctx := context.Background() |
| + id, err := a.store.Ingest(ctx, "upload", "regional-notes.txt", "regional notes", |
| + "The "+term+" terms for the departing regional lead were agreed in March.") |
| + if err != nil { |
| + t.Fatalf("Ingest: %v", err) |
| + } |
| + hits, err := a.store.Search(ctx, term, 10) |
| + if err != nil { |
| + t.Fatalf("Search: %v", err) |
| + } |
| + if len(hits) != 1 { |
| + t.Fatalf("%d hits for the freshly stored document, want 1. Nothing below is about a "+ |
| + "stale citation if the document was never findable in the first place", len(hits)) |
| + } |
| + return id |
| +} |
| + |
| +// postDelete presses the delete button the way the page does. |
| +func postDelete(t *testing.T, h http.Handler, id int64) *httptest.ResponseRecorder { |
| + t.Helper() |
| + req := httptest.NewRequest(http.MethodPost, "/documents/"+strconv.FormatInt(id, 10)+"/delete", nil) |
| + req.Header.Set("Origin", lagOrigin) |
| + rec := httptest.NewRecorder() |
| + h.ServeHTTP(rec, req) |
| + return rec |
| +} |
| + |
| +// getWithFlash follows a redirect carrying whatever cookie came back, so |
| +// the sentence a scriptless browser is shown is read off the page it is |
| +// shown on rather than out of a header. |
| +func getWithFlash(t *testing.T, h http.Handler, path string, from *httptest.ResponseRecorder) string { |
| + t.Helper() |
| + req := httptest.NewRequest(http.MethodGet, path, nil) |
| + for _, c := range from.Result().Cookies() { |
| + req.AddCookie(c) |
| + } |
| + rec := httptest.NewRecorder() |
| + h.ServeHTTP(rec, req) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET %s answered %d, want 200", path, rec.Code) |
| + } |
| + return rec.Body.String() |
| +} |
| + |
| +// A search never cites a document the database does not have. |
| +// |
| +// The failure this replaces: Store.Delete removed the rows in a |
| +// transaction and then removed the index entries, and a failure of that |
| +// second step was returned as a plain error and swallowed by the handler. |
| +// The document was gone and its passages stayed in the index, so Search |
| +// returned a hit for it, the handler resolved the title out of the |
| +// database and found nothing, and the reader got an empty-titled result |
| +// linking to a page that 404s — a citation to text that is not there, |
| +// which is the failure §4.1 exists to prevent. |
| +// |
| +// The divergence is produced by running the real Delete with the store's |
| +// index swapped for a scratch one, so the removal does not reach the |
| +// index Search reads. That is exactly the state a failed RemoveDocument |
| +// leaves — rows gone, entries still there — and it is reached this way |
| +// rather than by deleting rows in the test because Delete is the code |
| +// that produces it. There is no way to make the index's DELETE fail while |
| +// its SELECT still works: closing it breaks both. |
| +// |
| +// This test stays honest after the ordering fix in Delete, and that is |
| +// the point of it. Removing the index entries first stops Delete creating |
| +// this state; it does not stop it existing — an ingest racing a delete on |
| +// the same document can still re-index it between the two steps, and a |
| +// partial rebuild can leave it at boot. Whatever the index says, the |
| +// database decides whether a passage may be cited. |
| +// |
| +// MUTATION-TESTED 2026-09-04, each reverted, each observed red: |
| +// |
| +// (a) the `if !livePassage[h.ChunkID] { continue }` guard deleted from |
| +// Store.Search — the code as it stood before this review → "the |
| +// search cited 1 document(s) that are not in the database ... its |
| +// title came back empty ("")", and the page assertion under it |
| +// (b) `livePassage` filled from the HITS instead of from the chunk rows |
| +// the database returned, which is how someone shortens this later |
| +// → the same two failures. The guard is only a guard because it |
| +// asks the database. |
| +// |
| +// A third was tried and came back GREEN, which is why the code no longer |
| +// has the thing it mutated: an earlier version of the guard also asked |
| +// whether the DOCUMENT was still in the database, and filling that map |
| +// from the hits changed nothing. A chunk row cannot outlive its document |
| +// row — one transaction, and the schema cascades — so the document half |
| +// could not fail for anything the passage half does not already catch, |
| +// and a guard nobody can see fail is the kind this file exists to stop. |
| +func TestASearchNeverCitesADocumentThatIsNotInTheDatabase(t *testing.T) { |
| + h, a := appWithAWorkingIndex(t) |
| + ctx := context.Background() |
| + const term = "severance" |
| + id := storeOneDocument(t, a, term) |
| + |
| + // Delete for real, with an index that does not hear about it. |
| + scratch, err := index.Open() |
| + if err != nil { |
| + t.Fatalf("index.Open: %v", err) |
| + } |
| + t.Cleanup(func() { scratch.Close() }) |
| + live := a.store.ix |
| + a.store.ix = scratch |
| + if err := a.store.Delete(ctx, id); err != nil { |
| + t.Fatalf("Delete: %v", err) |
| + } |
| + a.store.ix = live |
| + |
| + // Two vacuity guards, because either failing would make every |
| + // assertion below pass for the wrong reason. |
| + docs, err := a.store.Summaries(ctx) |
| + if err != nil { |
| + t.Fatalf("listing documents: %v", err) |
| + } |
| + if len(docs) != 0 { |
| + t.Fatalf("%d documents are still in the database, want 0; the delete did not happen, so "+ |
| + "this is not a test about a stale index", len(docs)) |
| + } |
| + if n, err := a.store.Count(); err != nil { |
| + t.Fatalf("counting passages: %v", err) |
| + } else if n == 0 { |
| + t.Fatalf("the index holds no passages, so it is not stale and a search finding nothing " + |
| + "here would prove nothing at all") |
| + } |
| + |
| + hits, err := a.store.Search(ctx, term, 10) |
| + if err != nil { |
| + t.Fatalf("Search: %v", err) |
| + } |
| + if len(hits) != 0 { |
| + t.Errorf("the search cited %d document(s) that are not in the database: %v. The row is "+ |
| + "gone, so its title came back empty (%q) and the link goes to a page that 404s — "+ |
| + "a citation to text that is not there.", len(hits), hits, hits[0].Title) |
| + } |
| + |
| + // And the same thing where a reader would see it. A result rendered |
| + // from a hit like that is a blank link on the page. |
| + body := getWithFlash(t, h, "/?q="+term, httptest.NewRecorder()) |
| + if strings.Contains(body, `href="/documents/`+strconv.FormatInt(id, 10)+`#p`) { |
| + t.Errorf("the results page links to document %d, which is deleted:\n%s", id, body) |
| + } |
| +} |
| + |
| +// A search never cites a passage the document no longer contains. |
| +// |
| +// This is the same bug in the other function, and it is the one the |
| +// document check above does not catch. Ingest replaces a document's |
| +// chunks inside its transaction and drops the old index entries AFTER the |
| +// commit; when that drop fails it returns ErrIndexLag, and the document |
| +// is still there, so the stale entries pass every check that asks only |
| +// whether the document exists. What the reader gets is a snippet of text |
| +// the author deleted in the last revision, under a live title, linking to |
| +// an ordinal that now holds something else. |
| +// |
| +// It also makes the sentence the app says about ErrIndexLag wrong. |
| +// "Stored but will not be found by a search until this instance restarts" |
| +// is said about a document whose previous revision is still answering |
| +// searches. |
| +// |
| +// Built the same way as the test above, and with the same limitation: the |
| +// old entries are left behind by re-ingesting with the store's index |
| +// swapped for a scratch one, because there is no way to make the index's |
| +// DELETE fail while its SELECT still works. |
| +// |
| +// MUTATION-TESTED 2026-09-04, each reverted, each observed red: |
| +// |
| +// (a) `!livePassage[h.ChunkID]` weakened to the check this guard was |
| +// first written as — whether the DOCUMENT is still among the rows |
| +// the database returned — → "the search cited 1 passage(s) that are |
| +// no longer in the document ... quoted under the document's live |
| +// title". That is the mutation that matters here: the document IS |
| +// still there, so a check that looks at documents sees nothing |
| +// wrong, and the test above passes throughout. |
| +// (b) `livePassage` filled from the hits rather than from the chunk |
| +// rows the database returned → the same failure |
| +// (c) the guard deleted outright → the same failure, plus the deleted- |
| +// document test above |
| +func TestASearchNeverCitesAPassageTheDocumentNoLongerHas(t *testing.T) { |
| + _, a := appWithAWorkingIndex(t) |
| + ctx := context.Background() |
| + const gone = "reorganisation" |
| + id := storeOneDocument(t, a, gone) |
| + |
| + // Re-ingest under the same key with the word taken out, with an index |
| + // that does not hear about the old passages. |
| + scratch, err := index.Open() |
| + if err != nil { |
| + t.Fatalf("index.Open: %v", err) |
| + } |
| + t.Cleanup(func() { scratch.Close() }) |
| + live := a.store.ix |
| + a.store.ix = scratch |
| + again, err := a.store.Ingest(ctx, "upload", "regional-notes.txt", "regional notes", |
| + "The terms for the departing regional lead were agreed in March.") |
| + if err != nil { |
| + t.Fatalf("re-Ingest: %v", err) |
| + } |
| + a.store.ix = live |
| + if again != id { |
| + t.Fatalf("the re-ingest made document %d instead of updating %d, so this is two documents "+ |
| + "and not a revision at all", again, id) |
| + } |
| + |
| + // Vacuity: the document is still there, the word is genuinely out of |
| + // it, and the index still holds the passage that had it. |
| + body := passageText(t, a, id) |
| + if strings.Contains(body, gone) { |
| + t.Fatalf("the new revision still contains %q, so there is no stale passage here", gone) |
| + } |
| + if n, err := a.store.Count(); err != nil { |
| + t.Fatalf("counting passages: %v", err) |
| + } else if n == 0 { |
| + t.Fatalf("the index holds no passages, so nothing stale survived and a search finding " + |
| + "nothing here would prove nothing") |
| + } |
| + |
| + hits, err := a.store.Search(ctx, gone, 10) |
| + if err != nil { |
| + t.Fatalf("Search: %v", err) |
| + } |
| + if len(hits) != 0 { |
| + t.Errorf("the search cited %d passage(s) that are no longer in the document: %v. The "+ |
| + "snippet is text the last revision removed, and it is quoted under the document's "+ |
| + "live title — a citation to text that is not there.", len(hits), hits) |
| + } |
| +} |
| + |
| +// passageText joins a document's stored passages, so a test can say what |
| +// the document now contains rather than what it was given. |
| +func passageText(t *testing.T, a *app, id int64) string { |
| + t.Helper() |
| + ps, err := a.store.Passages(context.Background(), id) |
| + if err != nil { |
| + t.Fatalf("Passages: %v", err) |
| + } |
| + var b strings.Builder |
| + for _, p := range ps { |
| + b.WriteString(p.Text) |
| + b.WriteString("\n") |
| + } |
| + return b.String() |
| +} |
| + |
| +// A delete the index refused is not a delete. |
| +// |
| +// Delete removes the index entries BEFORE the transaction, so a failure |
| +// there leaves the document whole and findable rather than deleted and |
| +// findable. The old order left the second of those: the row gone, the |
| +// passages still in the index, and — since deleteDocument swallowed the |
| +// error and redirected — nobody told. The passage someone deleted may be |
| +// exactly the passage that must not be findable, so the delete that |
| +// cannot be completed is refused outright. |
| +// |
| +// MUTATION-TESTED 2026-09-04, each reverted, each observed red: |
| +// |
| +// (a) Store.Delete's index removal moved back after the transaction — |
| +// the code as it stood before this review → "the document was |
| +// deleted from the database even though the index still holds its |
| +// passages" |
| +// (b) `flash.Set` deleted from deleteDocument's error branch → "the |
| +// page says nothing about the delete that did not happen" |
| +// |
| +// One more was tried and came back GREEN, recorded because a mutation |
| +// nobody can see fail is worth knowing about: deleting the early `return` |
| +// from that branch so it falls through to the redirect below. It is |
| +// behaviour-preserving — the flash is already set and one redirect still |
| +// goes out — so it is a style edit, not a defect, and no test should be |
| +// contorted to catch it. |
| +func TestADeleteTheIndexRefusesLeavesTheDocumentAlone(t *testing.T) { |
| + h, a := appWithAWorkingIndex(t) |
| + ctx := context.Background() |
| + id := storeOneDocument(t, a, "redundancy") |
| + |
| + // From here the index refuses everything, removals included. |
| + if err := a.store.ix.Close(); err != nil { |
| + t.Fatalf("closing the index: %v", err) |
| + } |
| + |
| + rec := postDelete(t, h, id) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("delete answered %d, want 303:\n%s", rec.Code, rec.Body.String()) |
| + } |
| + |
| + docs, err := a.store.Summaries(ctx) |
| + if err != nil { |
| + t.Fatalf("listing documents: %v", err) |
| + } |
| + if len(docs) != 1 { |
| + t.Errorf("%d documents are in the database, want 1: the document was deleted from the "+ |
| + "database even though the index still holds its passages, which leaves a passage "+ |
| + "that can be found and cited for a document that no longer exists.", len(docs)) |
| + } |
| + |
| + c := newDropZone() |
| + body := getWithFlash(t, h, "/", rec) |
| + if !strings.Contains(body, c.DeleteFailed) { |
| + t.Errorf("the page says nothing about the delete that did not happen:\n want %q in the "+ |
| + "page after the redirect", c.DeleteFailed) |
| + } |
| +} |
| + |
| +// The ordinary delete still deletes, said here because the test above |
| +// only proves the refusal. A guard that made every delete fail would pass |
| +// it. |
| +// |
| +// MUTATION-TESTED 2026-09-04, reverted: the transaction removed from |
| +// Store.Delete, leaving only the index removal → "1 documents are in the |
| +// database after a delete that reported no error". |
| +func TestAnOrdinaryDeleteStillDeletes(t *testing.T) { |
| + _, a := appWithAWorkingIndex(t) |
| + ctx := context.Background() |
| + const term = "restructure" |
| + id := storeOneDocument(t, a, term) |
| + |
| + if err := a.store.Delete(ctx, id); err != nil { |
| + t.Fatalf("Delete: %v", err) |
| + } |
| + docs, err := a.store.Summaries(ctx) |
| + if err != nil { |
| + t.Fatalf("listing documents: %v", err) |
| + } |
| + if len(docs) != 0 { |
| + t.Errorf("%d documents are in the database after a delete that reported no error", len(docs)) |
| + } |
| + if n, err := a.store.Count(); err != nil { |
| + t.Fatalf("counting passages: %v", err) |
| + } else if n != 0 { |
| + t.Errorf("%d passages are still indexed after a delete that reported no error", n) |
| + } |
| + hits, err := a.store.Search(ctx, term, 10) |
| + if err != nil { |
| + t.Fatalf("Search: %v", err) |
| + } |
| + if len(hits) != 0 { |
| + t.Errorf("a deleted document is still searchable: %v", hits) |
| + } |
| +} |
amadan