From a7f98c01fca0e20ca9c7ac039fcb06c4e0960754 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 6 Oct 2026 01:04:29 -0400 Subject: [PATCH] fix(store): search, tags, children and progress exclude soft-deleted collections (BUG-3428, phase 1) (#1824) * test(store): search, tags, children and progress must exclude soft-deleted collections (BUG-3428, red) * fix(store): global search, tags, children and progress exclude soft-deleted collections; the open-children guard keeps them (BUG-3428) * test(store): bare-number search excludes soft-deleted collections (BUG-3428) * test(store): has_children agrees with the children list; tighter progress and facet assertions (BUG-3428, codex r1, red) * fix(store): has_children leaves out children in soft-deleted collections (BUG-3428, codex r1) --- .../bug3428_deleted_collection_reads_test.go | 217 ++++++++++++++++++ internal/store/done_field_test.go | 23 +- internal/store/items.go | 30 ++- internal/store/search.go | 12 + 4 files changed, 261 insertions(+), 21 deletions(-) create mode 100644 internal/store/bug3428_deleted_collection_reads_test.go diff --git a/internal/store/bug3428_deleted_collection_reads_test.go b/internal/store/bug3428_deleted_collection_reads_test.go new file mode 100644 index 000000000..fd7ee5ed2 --- /dev/null +++ b/internal/store/bug3428_deleted_collection_reads_test.go @@ -0,0 +1,217 @@ +package store + +import ( + "strconv" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3428 phase 1: the reads BUG-3425 left out. Global search, workspace tags, +// and child items / progress still included the items of a soft-deleted +// collection. Each test keeps a live item beside the doomed one, so a read +// that returns nothing cannot pass. Reuses bug3425World (same package). + +func searchIDs(t *testing.T, s *Store, ws *models.Workspace, q string) (map[string]bool, *SearchResponse) { + t.Helper() + resp, err := s.Search(SearchParams{Query: q, Workspace: ws.Slug, Unrestricted: true}) + if err != nil { + t.Fatalf("Search(%q): %v", q, err) + } + ids := map[string]bool{} + for _, r := range resp.Results { + ids[r.Item.ID] = true + } + return ids, resp +} + +func TestSearch_ExcludesSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + + ids, _ := searchIDs(t, w.s, w.ws, "Zephyr") + if !ids[w.doomed.ID] || !ids[w.live.ID] { + t.Fatalf("control: text search should find both items before the delete") + } + if ref, _ := searchIDs(t, w.s, w.ws, refOf(w.doomed)); !ref[w.doomed.ID] { + t.Fatalf("control: a ref search should find the doomed item before the delete") + } + num := strconv.Itoa(*w.doomed.ItemNumber) + if byNum, _ := searchIDs(t, w.s, w.ws, num); !byNum[w.doomed.ID] { + t.Fatalf("control: a bare-number search should find the doomed item before the delete") + } + + w.deleteDoomed(t) + ids, resp := searchIDs(t, w.s, w.ws, "Zephyr") + if ids[w.doomed.ID] { + t.Errorf("text search still finds an item from a soft-deleted collection") + } + if !ids[w.live.ID] { + t.Errorf("text search lost the live item") + } + if resp.Total != len(resp.Results) { + t.Errorf("total %d disagrees with %d results", resp.Total, len(resp.Results)) + } + // The facet query swallows its own errors into empty maps, so the live + // collection's count is asserted too: an empty map cannot pass. + if resp.Facets == nil || resp.Facets.Collections[w.liveColl.Slug] != 1 { + t.Errorf("facets lost the live collection: %+v", resp.Facets) + } else if resp.Facets.Collections[w.doomColl.Slug] != 0 { + t.Errorf("facets still count the soft-deleted collection: %v", resp.Facets.Collections) + } + if ref, _ := searchIDs(t, w.s, w.ws, refOf(w.doomed)); ref[w.doomed.ID] { + t.Errorf("a ref search still finds an item from a soft-deleted collection") + } + if byNum, _ := searchIDs(t, w.s, w.ws, num); byNum[w.doomed.ID] { + t.Errorf("a bare-number search still finds an item from a soft-deleted collection") + } + if byNum, _ := searchIDs(t, w.s, w.ws, strconv.Itoa(*w.live.ItemNumber)); !byNum[w.live.ID] { + t.Errorf("a bare-number search lost the live item") + } +} + +func TestListWorkspaceTags_ExcludesSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + tag := func(it *models.Item, tags string) { + t.Helper() + if _, err := w.s.UpdateItem(it.ID, models.ItemUpdate{Tags: &tags}); err != nil { + t.Fatalf("tag %s: %v", it.Title, err) + } + } + tag(w.live, `["shared"]`) + tag(w.doomed, `["shared","doomed-only"]`) + counts := func() map[string]int { + t.Helper() + tc, err := w.s.ListWorkspaceTags(w.ws.ID, nil, nil) + if err != nil { + t.Fatalf("ListWorkspaceTags: %v", err) + } + m := map[string]int{} + for _, c := range tc { + m[c.Tag] = c.Count + } + return m + } + if c := counts(); c["shared"] != 2 || c["doomed-only"] != 1 { + t.Fatalf("control: want shared=2 doomed-only=1 before the delete, got %v", c) + } + w.deleteDoomed(t) + c := counts() + if c["shared"] != 1 { + t.Errorf("shared counts the soft-deleted collection's item: %d, want 1", c["shared"]) + } + if _, ok := c["doomed-only"]; ok { + t.Errorf("a tag only a soft-deleted collection's item carries still lists: %v", c) + } +} + +func TestChildrenAndProgress_ExcludeSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + s := w.s + parentColl := createTestCollection(t, s, w.ws.ID, "Plans") + parent := createTestItem(t, s, w.ws.ID, parentColl.ID, "Parent", "") + for _, child := range []*models.Item{w.live, w.doomed} { + if _, err := s.CreateItemLink(w.ws.ID, models.ItemLinkCreate{TargetID: parent.ID, LinkType: "parent"}, child.ID); err != nil { + t.Fatalf("link %s: %v", child.Title, err) + } + } + check := func(when string, wantDoomed bool, wantTotal int) { + t.Helper() + kids, err := s.GetChildItems(parent.ID) + if err != nil { + t.Fatalf("%s GetChildItems: %v", when, err) + } + if !hasItem(kids, w.live.ID) || hasItem(kids, w.doomed.ID) != wantDoomed { + t.Errorf("%s GetChildItems: live=%v doomed=%v, want doomed=%v", when, hasItem(kids, w.live.ID), hasItem(kids, w.doomed.ID), wantDoomed) + } + batch, err := s.GetChildItemsForParents([]string{parent.ID}) + if err != nil { + t.Fatalf("%s GetChildItemsForParents: %v", when, err) + } + if !hasItem(batch[parent.ID], w.live.ID) || hasItem(batch[parent.ID], w.doomed.ID) != wantDoomed { + t.Errorf("%s GetChildItemsForParents disagrees: doomed=%v, want %v", when, hasItem(batch[parent.ID], w.doomed.ID), wantDoomed) + } + total, _, err := s.GetItemProgress(parent.ID) + if err != nil { + t.Fatalf("%s GetItemProgress: %v", when, err) + } + if total != wantTotal { + t.Errorf("%s GetItemProgress total = %d, want %d", when, total, wantTotal) + } + all, err := s.GetAllItemProgress(w.ws.ID, parentColl.Slug, false) + if err != nil { + t.Fatalf("%s GetAllItemProgress: %v", when, err) + } + found := false + for _, p := range all { + if p.ItemID == parent.ID { + found = true + if p.Total != wantTotal { + t.Errorf("%s GetAllItemProgress total = %d, want %d", when, p.Total, wantTotal) + } + } + } + if !found { + t.Errorf("%s GetAllItemProgress has no row for the parent", when) + } + } + check("before the delete (control)", true, 2) + w.deleteDoomed(t) + check("after the delete", false, 1) +} + +// The open-children guard reads children through GetChildItemsTx and is NOT +// narrowed by BUG-3428: whether an open child in a soft-deleted collection +// should still block closing its parent is a write-path rule, left for the +// lead's ruling. This pins the current behaviour so changing it is a decision. +func TestGetChildItemsTx_StillIncludesSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + s := w.s + parentColl := createTestCollection(t, s, w.ws.ID, "Plans") + parent := createTestItem(t, s, w.ws.ID, parentColl.ID, "Parent", "") + if _, err := s.CreateItemLink(w.ws.ID, models.ItemLinkCreate{TargetID: parent.ID, LinkType: "parent"}, w.doomed.ID); err != nil { + t.Fatalf("link: %v", err) + } + w.deleteDoomed(t) + tx, err := s.DB().Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + kids, err := s.GetChildItemsTx(tx, parent.ID) + if err != nil { + t.Fatalf("GetChildItemsTx: %v", err) + } + if !hasItem(kids, w.doomed.ID) { + t.Errorf("the guard's child read no longer sees a soft-deleted collection's child; that changes what blocks closing a parent") + } +} + +// Codex r1: has_children must agree with the children list. A child whose only +// child lives in a soft-deleted collection has none to list, so it must not +// claim to have any. +func TestPopulateHasChildren_ExcludesSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + s := w.s + middle := w.live + if _, err := s.CreateItemLink(w.ws.ID, models.ItemLinkCreate{TargetID: middle.ID, LinkType: "parent"}, w.doomed.ID); err != nil { + t.Fatalf("link: %v", err) + } + flag := func() bool { + t.Helper() + items := []models.Item{*middle} + s.PopulateHasChildren(items) + return items[0].HasChildren + } + if !flag() { + t.Fatalf("control: has_children should be true before the delete") + } + w.deleteDoomed(t) + if flag() { + t.Errorf("has_children still true when the only child is in a soft-deleted collection") + } +} diff --git a/internal/store/done_field_test.go b/internal/store/done_field_test.go index 546533ee9..d820a9a72 100644 --- a/internal/store/done_field_test.go +++ b/internal/store/done_field_test.go @@ -165,10 +165,11 @@ func TestGetItemProgress_DefaultsToStatusWithoutSettings(t *testing.T) { } // TestGetItemProgress_HonorsSoftDeletedChildCollections verifies a -// regression Codex flagged: soft-deleted child collections must still -// contribute done filters, otherwise their items can never satisfy the -// per-collection `collection_id = ? AND ...` clause and would always be -// counted as active. +// regression Codex flagged: a child whose collection was soft-deleted must +// never be counted as ACTIVE because its collection's done rule went +// missing. Since BUG-3428 such a child is left out of progress entirely, as +// every other item read leaves it out, which closes that miscount at the +// source: it is neither open nor done. func TestGetItemProgress_HonorsSoftDeletedChildCollections(t *testing.T) { t.Parallel() s := testStore(t) @@ -211,18 +212,14 @@ func TestGetItemProgress_HonorsSoftDeletedChildCollections(t *testing.T) { t.Fatalf("soft delete tasks: %v", err) } - // Expect the same done count — soft-deleted collections still - // contribute their done rules. - _, doneAfter, err := s.GetItemProgress(plan.ID) + // Both children were in the deleted collection: out of BOTH counts, so + // the open one is not counted as active work and the plan reads 0/0. + totalAfter, doneAfter, err := s.GetItemProgress(plan.ID) if err != nil { t.Fatalf("GetItemProgress after soft-delete: %v", err) } - if doneAfter != 1 { - t.Errorf( - "expected done count to remain 1 after collection soft-delete, got %d — "+ - "soft-deleted collections must still contribute done filters", - doneAfter, - ) + if totalAfter != 0 || doneAfter != 0 { + t.Errorf("after the collection's soft-delete want 0/0 (its children leave progress, BUG-3428), got %d/%d", doneAfter, totalAfter) } } diff --git a/internal/store/items.go b/internal/store/items.go index a72a48847..8aacfa3d0 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -1437,7 +1437,8 @@ func (s *Store) ListWorkspaceTags(workspaceID string, collectionIDs, itemIDs []s query := ` SELECT ` + valueExpr + ` AS tag, COUNT(DISTINCT i.id) AS cnt FROM items i, ` + fromExpr + ` - WHERE i.workspace_id = ? AND i.deleted_at IS NULL` + WHERE i.workspace_id = ? AND i.deleted_at IS NULL + AND EXISTS (SELECT 1 FROM collections c WHERE c.id = i.collection_id AND c.deleted_at IS NULL)` args := []interface{}{workspaceID} if len(collectionIDs) > 0 && len(itemIDs) > 0 { @@ -4610,6 +4611,7 @@ func (s *Store) GetItemProgress(parentItemID string) (total int, done int, err e SELECT COUNT(CASE WHEN NOT %s THEN 1 END), COUNT(CASE WHEN %s AND NOT %s THEN 1 END) FROM items i + JOIN collections c ON c.id = i.collection_id AND c.deleted_at IS NULL JOIN item_links il ON il.source_id = i.id AND il.link_type IN (%s) AND il.target_id = ? WHERE i.deleted_at IS NULL `, abandonedExpr, doneExpr, abandonedExpr, childLinkTypeSQL())), args...).Scan(&total, &done) @@ -4909,6 +4911,7 @@ func (s *Store) GetAllItemProgress(workspaceID, collectionSlug string, includeAr LEFT JOIN item_links il ON il.link_type IN (%s) AND il.target_id = p.id LEFT JOIN items t ON t.id = il.source_id AND t.deleted_at IS NULL + AND EXISTS (SELECT 1 FROM collections tc WHERE tc.id = t.collection_id AND tc.deleted_at IS NULL) WHERE p.workspace_id = ? AND pc.slug = ? %s @@ -4935,8 +4938,11 @@ func (s *Store) GetAllItemProgress(workspaceID, collectionSlug string, includeAr // GetChildItems returns all non-deleted child items linked to the given parent // via item_links. Returns children from any collection. +// +// A child in a soft-deleted collection is left out (BUG-3428), as every other +// item read leaves it out. func (s *Store) GetChildItems(parentItemID string) ([]models.Item, error) { - return s.getChildItems(s.db, parentItemID) + return s.getChildItems(s.db, parentItemID, false) } // GetChildItemsTx is the in-transaction variant of GetChildItems. The @@ -4962,10 +4968,13 @@ func (s *Store) GetChildItems(parentItemID string) ([]models.Item, error) { // `SELECT DISTINCT … FOR UPDATE`. The advisory-lock pattern sidesteps // that constraint while still giving us a serialized snapshot. func (s *Store) GetChildItemsTx(tx *sql.Tx, parentItemID string) ([]models.Item, error) { + // The open-children guard is this function's caller, and it still counts + // a child in a soft-deleted collection as open work (BUG-3428 leaves that + // write rule to a ruling), so this read keeps those children. if tx == nil { - return s.GetChildItems(parentItemID) + return s.getChildItems(s.db, parentItemID, true) } - return s.getChildItems(tx, parentItemID) + return s.getChildItems(tx, parentItemID, true) } // acquireParentChildrenLocksForUpdate is the in-tx helper UpdateItem @@ -5198,7 +5207,11 @@ type childQueryer interface { Query(query string, args ...any) (*sql.Rows, error) } -func (s *Store) getChildItems(q childQueryer, parentItemID string) ([]models.Item, error) { +func (s *Store) getChildItems(q childQueryer, parentItemID string, includeDeletedCollections bool) ([]models.Item, error) { + collFilter := " AND c.deleted_at IS NULL" + if includeDeletedCollections { + collFilter = "" + } rows, err := q.Query(s.q(fmt.Sprintf(` SELECT DISTINCT i.id, i.workspace_id, i.collection_id, i.title, i.slug, i.content, `+contentStateSQL+`, i.fields, i.tags, i.pinned, i.sort_order, i.parent_id, i.assigned_user_id, i.agent_role_id, i.role_sort_order, @@ -5212,9 +5225,9 @@ func (s *Store) getChildItems(q childQueryer, parentItemID string) ([]models.Ite JOIN item_links il ON il.source_id = i.id AND il.link_type IN (%s) AND il.target_id = ? LEFT JOIN users au ON au.id = i.assigned_user_id LEFT JOIN agent_roles ar ON ar.id = i.agent_role_id - WHERE i.deleted_at IS NULL + WHERE i.deleted_at IS NULL%s ORDER BY i.sort_order ASC, i.created_at ASC - `, childLinkTypeSQL())), parentItemID) + `, childLinkTypeSQL(), collFilter)), parentItemID) if err != nil { return nil, fmt.Errorf("get child items: %w", err) } @@ -5263,7 +5276,7 @@ func (s *Store) GetChildItemsForParents(parentIDs []string) (map[string][]models JOIN item_links il ON il.source_id = i.id AND il.link_type IN (%s) AND il.target_id IN (%s) LEFT JOIN users au ON au.id = i.assigned_user_id LEFT JOIN agent_roles ar ON ar.id = i.agent_role_id - WHERE i.deleted_at IS NULL + WHERE i.deleted_at IS NULL AND c.deleted_at IS NULL ORDER BY i.sort_order ASC, i.created_at ASC `, childLinkTypeSQL(), strings.Join(placeholders, ","))), args...) if err != nil { @@ -5371,6 +5384,7 @@ func (s *Store) PopulateHasChildren(items []models.Item) { query := fmt.Sprintf(` SELECT DISTINCT il.target_id FROM item_links il JOIN items child ON child.id = il.source_id AND child.deleted_at IS NULL + JOIN collections cc ON cc.id = child.collection_id AND cc.deleted_at IS NULL WHERE il.link_type IN (%s) AND il.target_id IN (%s) `, childLinkTypeSQL(), strings.Join(placeholders, ",")) diff --git a/internal/store/search.go b/internal/store/search.go index 151f581b4..741d60150 100644 --- a/internal/store/search.go +++ b/internal/store/search.go @@ -147,6 +147,7 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) { } refQuery, refArgs = appendSearchPermissionFilter(refQuery, refArgs, params) + refQuery = appendLiveCollectionFilter(refQuery) // Apply content filters to ref lookup too refQuery, refArgs = appendSearchCollectionFilter(refQuery, refArgs, params) @@ -240,6 +241,7 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) { } numQuery, numArgs = appendSearchPermissionFilter(numQuery, numArgs, params) + numQuery = appendLiveCollectionFilter(numQuery) // Apply content filters to numeric lookup too numQuery, numArgs = appendSearchCollectionFilter(numQuery, numArgs, params) @@ -406,6 +408,7 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) { } query, args = appendSearchPermissionFilter(query, args, params) + query = appendLiveCollectionFilter(query) // Collection filter — the resolved per-workspace set, or a literal slug. query, args = appendSearchCollectionFilter(query, args, params) @@ -618,6 +621,7 @@ func (s *Store) appendSearchFilters(query string, args []interface{}, params Sea } query, args = appendSearchPermissionFilter(query, args, params) + query = appendLiveCollectionFilter(query) query, args = appendSearchCollectionFilter(query, args, params) @@ -790,6 +794,14 @@ func appendSearchPermissionFilter(query string, args []interface{}, params Searc return query, args } +// appendLiveCollectionFilter leaves out the items of a soft-deleted collection +// (BUG-3428), for every caller alike. Every search query joins collections as +// c and calls it beside appendSearchPermissionFilter, so the results, the +// total and the facets agree. +func appendLiveCollectionFilter(query string) string { + return query + ` AND c.deleted_at IS NULL` +} + func appendSearchCollectionFilter(query string, args []interface{}, params SearchParams) (string, []interface{}) { if params.CollectionFilterIDs != nil { if len(params.CollectionFilterIDs) == 0 {