fix(store): address Codex review findings on PR #259 (BUG-734)

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.
This commit is contained in:
xarmian
2026-04-27 14:08:45 +00:00
parent 1eb7fff757
commit 288283b3af
3 changed files with 91 additions and 17 deletions
+7 -1
View File
@@ -97,7 +97,13 @@ func (s *Store) ExportWorkspace(slug string) (*models.WorkspaceExport, error) {
return nil, err 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(` linkRows, err := s.db.Query(s.q(`
SELECT id, source_id, target_id, link_type, created_by, created_at SELECT id, source_id, target_id, link_type, created_by, created_at
FROM item_links WHERE workspace_id = ? FROM item_links WHERE workspace_id = ?
+22 -16
View File
@@ -987,6 +987,13 @@ func (s *Store) CreateItemLink(workspaceID string, input models.ItemLinkCreate,
return s.getItemLink(id) 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) { func (s *Store) getItemLink(id string) (*models.ItemLink, error) {
var link models.ItemLink var link models.ItemLink
var createdAt string var createdAt string
@@ -997,15 +1004,14 @@ func (s *Store) getItemLink(id string) (*models.ItemLink, error) {
srcStatus := s.dialect.JSONExtractText("s.fields", "status") srcStatus := s.dialect.JSONExtractText("s.fields", "status")
tgtStatus := s.dialect.JSONExtractText("t.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(` 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, 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.title, t.title, s.slug, t.slug, sc.slug, tc.slug, sc.prefix, tc.prefix,
s.item_number, t.item_number, s.item_number, t.item_number,
%s, %s %s, %s
FROM item_links l FROM item_links l
JOIN items s ON s.id = l.source_id AND s.deleted_at IS NULL JOIN items s ON s.id = l.source_id
JOIN items t ON t.id = l.target_id AND t.deleted_at IS NULL JOIN items t ON t.id = l.target_id
JOIN collections sc ON sc.id = s.collection_id JOIN collections sc ON sc.id = s.collection_id
JOIN collections tc ON tc.id = t.collection_id JOIN collections tc ON tc.id = t.collection_id
WHERE l.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 nil, fmt.Errorf("commit parent link: %w", err)
} }
// Return the full link with enriched fields // Return the full link with enriched fields. Use the unfiltered readback
links, err := s.GetItemLinks(itemID) // helper so that a delete race against either endpoint between commit and
if err != nil { // readback doesn't cause the successful insert to surface as nil.
return nil, err return s.getItemLink(id)
}
for _, link := range links {
if link.ID == id {
return &link, nil
}
}
return nil, fmt.Errorf("parent link created but not found")
} }
// checkParentCycle walks the ancestor chain from parentID and returns an error // 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 // 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). // 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) { func (s *Store) GetParentMap(workspaceID string) (map[string]string, error) {
rows, err := s.db.Query(s.q(fmt.Sprintf(` rows, err := s.db.Query(s.q(fmt.Sprintf(`
SELECT source_id, target_id FROM item_links SELECT il.source_id, il.target_id FROM item_links il
WHERE workspace_id = ? AND link_type IN (%s) 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) `, childLinkTypeSQL())), workspaceID)
if err != nil { if err != nil {
return nil, fmt.Errorf("get parent map: %w", err) return nil, fmt.Errorf("get parent map: %w", err)
+62
View File
@@ -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) { func TestItemLinkDefaultType(t *testing.T) {
s := testStore(t) s := testStore(t)
ws := createTestWorkspace(t, s, "Test") ws := createTestWorkspace(t, s, "Test")