From 288283b3afd87876fe9553c0d67eec4dd7074de2 Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 27 Apr 2026 14:08:45 +0000 Subject: [PATCH] fix(store): address Codex review findings on PR #259 (BUG-734) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from Codex's review of the soft-delete filter on item-link queries: 1. MEDIUM — GetParentMap now JOINs items on both sides and filters on deleted_at IS NULL. handlers_dashboard.go uses this map directly to detect orphaned tasks (items not present in the map are flagged), so without the filter a task whose parent had been soft-deleted would silently fail to appear as orphaned. 2. LOW — Revert the deleted_at filter on getItemLink (lowercase, private). Its only caller is the post-insert readback in CreateItemLink, which means filtering buys nothing user-facing and introduces a delete-race window where a successful INSERT returns nil. SetParentLink's readback was switched from GetItemLinks to getItemLink for the same reason. User-facing surfaces still go through GetItemLinks (plural) and GetParentForItem, both of which retain the filter. 3. LOW — Add an explicit comment in export.go documenting that item_links are exported in full (including links to soft-deleted items), and why that intentionally diverges from the user-facing query behavior. Tests: TestGetParentMap_ExcludesSoftDeletedEndpoints exercises the dashboard regression path on both source-side and target-side soft-delete, plus the restore round-trip. --- internal/store/export.go | 8 ++++- internal/store/items.go | 38 ++++++++++++---------- internal/store/items_test.go | 62 ++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 17 deletions(-) diff --git a/internal/store/export.go b/internal/store/export.go index 46b9d77f..7058cfe9 100644 --- a/internal/store/export.go +++ b/internal/store/export.go @@ -97,7 +97,13 @@ func (s *Store) ExportWorkspace(slug string) (*models.WorkspaceExport, error) { return nil, err } - // Item links + // Item links — exported in full, including links whose source or target item + // is soft-deleted. This is intentional and differs from user-facing reads + // (GetItemLinks/GetParentForItem/GetParentMap, which all filter on + // items.deleted_at IS NULL — see BUG-734). Backups need to round-trip the + // raw graph so that re-importing into a workspace where the deleted items + // are restored preserves the original relationships. The import path + // already silently skips links whose endpoints are missing entirely. linkRows, err := s.db.Query(s.q(` SELECT id, source_id, target_id, link_type, created_by, created_at FROM item_links WHERE workspace_id = ? diff --git a/internal/store/items.go b/internal/store/items.go index 754f39e6..fe17b3de 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -987,6 +987,13 @@ func (s *Store) CreateItemLink(workspaceID string, input models.ItemLinkCreate, return s.getItemLink(id) } +// getItemLink is the unfiltered post-insert readback used by CreateItemLink to +// hydrate the freshly-inserted row with collection/source/target metadata. It +// intentionally does NOT filter on items.deleted_at IS NULL: the only caller +// is the immediate readback after INSERT, and a delete race against either +// endpoint would otherwise cause the just-successful insert to return nil +// (Codex review on PR #259). User-facing surfaces all read links via +// GetItemLinks (plural) or GetParentForItem, both of which DO filter. func (s *Store) getItemLink(id string) (*models.ItemLink, error) { var link models.ItemLink var createdAt string @@ -997,15 +1004,14 @@ func (s *Store) getItemLink(id string) (*models.ItemLink, error) { srcStatus := s.dialect.JSONExtractText("s.fields", "status") tgtStatus := s.dialect.JSONExtractText("t.fields", "status") - // Filter out links pointing to or from soft-deleted items — see BUG-734. err := s.db.QueryRow(s.q(fmt.Sprintf(` SELECT l.id, l.workspace_id, l.source_id, l.target_id, l.link_type, l.created_by, l.created_at, s.title, t.title, s.slug, t.slug, sc.slug, tc.slug, sc.prefix, tc.prefix, s.item_number, t.item_number, %s, %s FROM item_links l - JOIN items s ON s.id = l.source_id AND s.deleted_at IS NULL - JOIN items t ON t.id = l.target_id AND t.deleted_at IS NULL + JOIN items s ON s.id = l.source_id + JOIN items t ON t.id = l.target_id JOIN collections sc ON sc.id = s.collection_id JOIN collections tc ON tc.id = t.collection_id WHERE l.id = ? @@ -1172,17 +1178,10 @@ func (s *Store) SetParentLink(workspaceID, itemID, parentID, createdBy string) ( return nil, fmt.Errorf("commit parent link: %w", err) } - // Return the full link with enriched fields - links, err := s.GetItemLinks(itemID) - if err != nil { - return nil, err - } - for _, link := range links { - if link.ID == id { - return &link, nil - } - } - return nil, fmt.Errorf("parent link created but not found") + // Return the full link with enriched fields. Use the unfiltered readback + // helper so that a delete race against either endpoint between commit and + // readback doesn't cause the successful insert to surface as nil. + return s.getItemLink(id) } // checkParentCycle walks the ancestor chain from parentID and returns an error @@ -1281,10 +1280,17 @@ func (s *Store) GetParentForItem(itemID string) (*models.ItemLink, error) { // GetParentMap returns a map of item ID -> parent item ID for all parent links // in a workspace. Used for efficient batch lookups (e.g., dashboard, list enrichment). +// +// Links whose source or target item is soft-deleted are excluded so that +// dashboard orphan-detection (handlers_dashboard.go) and similar enrichment +// passes don't treat a task whose parent has been archived as still parented. +// See BUG-734. func (s *Store) GetParentMap(workspaceID string) (map[string]string, error) { rows, err := s.db.Query(s.q(fmt.Sprintf(` - SELECT source_id, target_id FROM item_links - WHERE workspace_id = ? AND link_type IN (%s) + SELECT il.source_id, il.target_id FROM item_links il + JOIN items s ON s.id = il.source_id AND s.deleted_at IS NULL + JOIN items t ON t.id = il.target_id AND t.deleted_at IS NULL + WHERE il.workspace_id = ? AND il.link_type IN (%s) `, childLinkTypeSQL())), workspaceID) if err != nil { return nil, fmt.Errorf("get parent map: %w", err) diff --git a/internal/store/items_test.go b/internal/store/items_test.go index 5bffd56d..5da3fef8 100644 --- a/internal/store/items_test.go +++ b/internal/store/items_test.go @@ -1070,6 +1070,68 @@ func TestGetParentForItem_HidesSoftDeletedParent(t *testing.T) { } } +// TestGetParentMap_ExcludesSoftDeletedEndpoints covers the dashboard +// orphan-detection path: a task whose parent has been soft-deleted should +// NOT appear in GetParentMap, so handlers_dashboard.go correctly flags the +// task as orphaned. See BUG-734 / Codex review on PR #259. +func TestGetParentMap_ExcludesSoftDeletedEndpoints(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent", "") + child := createTestItem(t, s, ws.ID, col.ID, "Child", "") + if _, err := s.SetParentLink(ws.ID, child.ID, parent.ID, "user"); err != nil { + t.Fatalf("SetParentLink: %v", err) + } + + // Sanity: child→parent mapping present. + m, err := s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap: %v", err) + } + if m[child.ID] != parent.ID { + t.Fatalf("expected parent map %s→%s, got %s→%s", child.ID, parent.ID, child.ID, m[child.ID]) + } + + // Soft-delete the parent. The child must now look "parentless" so the + // dashboard orphan detector flags it. + if err := s.DeleteItem(parent.ID); err != nil { + t.Fatalf("DeleteItem: %v", err) + } + m, err = s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap after parent delete: %v", err) + } + if _, hasEntry := m[child.ID]; hasEntry { + t.Errorf("expected child to drop from parent map after parent soft-deleted (orphan-detection regression)") + } + + // Restoring the parent should bring the mapping back. + if _, err := s.RestoreItem(parent.ID); err != nil { + t.Fatalf("RestoreItem: %v", err) + } + m, err = s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap after parent restore: %v", err) + } + if m[child.ID] != parent.ID { + t.Errorf("expected parent map to be restored to %s→%s, got %s→%s", child.ID, parent.ID, child.ID, m[child.ID]) + } + + // Soft-deleting the child side should also drop the entry. + if err := s.DeleteItem(child.ID); err != nil { + t.Fatalf("DeleteItem child: %v", err) + } + m, err = s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap after child delete: %v", err) + } + if _, hasEntry := m[child.ID]; hasEntry { + t.Errorf("expected child to drop from parent map after the child itself was soft-deleted") + } +} + func TestItemLinkDefaultType(t *testing.T) { s := testStore(t) ws := createTestWorkspace(t, s, "Test")