From cd3b0e30be2727b6caff68b5db33fff95d04efe6 Mon Sep 17 00:00:00 2001 From: matt wilkie Date: Mon, 15 Dec 2025 22:17:32 -0700 Subject: [PATCH] bd-ckej: fix orphan skip count mismatch on fresh import (#572) * bd-ckej: fix orphan skip count mismatch on fresh import When OrphanSkip mode is used during import and a child issue's parent doesn't exist, the issue ID was cleared to '' but then regenerated anyway in GenerateBatchIssueIDs, causing it to be created in the database. This resulted in a count mismatch: JSONL had 824 issues but only 823 were in the database (one orphan was counted but not created). Fix: Filter out orphaned issues with empty IDs before batch creation and track them in result.Skipped so the count stays accurate. * test: add TestImportOrphanSkip_CountMismatch for bd-ckej Adds comprehensive test that verifies orphaned issues are properly skipped during import when orphan_handling=OrphanSkip and parent doesn't exist. Also improves the fix to pre-filter orphaned issues before batch creation, ensuring they're not inserted then have IDs cleared (preventing count mismatches). --------- Co-authored-by: Amp --- internal/importer/importer.go | 56 +++++++++++++--- internal/importer/importer_test.go | 104 +++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 9 deletions(-) diff --git a/internal/importer/importer.go b/internal/importer/importer.go index d7804883..4bcaeb35 100644 --- a/internal/importer/importer.go +++ b/internal/importer/importer.go @@ -671,24 +671,62 @@ func upsertIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues } } +// Filter out orphaned issues if orphan_handling is set to skip (bd-ckej) +// Pre-filter before batch creation to prevent orphans from being created then ID-cleared +if opts.OrphanHandling == sqlite.OrphanSkip { + var filteredNewIssues []*types.Issue + for _, issue := range newIssues { + // Check if this is a hierarchical child whose parent doesn't exist + if strings.Contains(issue.ID, ".") { + lastDot := strings.LastIndex(issue.ID, ".") + parentID := issue.ID[:lastDot] + + // Check if parent exists in either existing DB issues or in newIssues batch + var parentExists bool + for _, dbIssue := range dbIssues { + if dbIssue.ID == parentID { + parentExists = true + break + } + } + if !parentExists { + for _, newIssue := range newIssues { + if newIssue.ID == parentID { + parentExists = true + break + } + } + } + + if !parentExists { + // Skip this orphaned issue + result.Skipped++ + continue + } + } + filteredNewIssues = append(filteredNewIssues, issue) + } + newIssues = filteredNewIssues +} + // Batch create all new issues // Sort by hierarchy depth to ensure parents are created before children if len(newIssues) > 0 { sort.Slice(newIssues, func(i, j int) bool { - depthI := strings.Count(newIssues[i].ID, ".") - depthJ := strings.Count(newIssues[j].ID, ".") + depthI := strings.Count(newIssues[i].ID, ".") + depthJ := strings.Count(newIssues[j].ID, ".") if depthI != depthJ { - return depthI < depthJ // Shallower first - } - return newIssues[i].ID < newIssues[j].ID // Stable sort + return depthI < depthJ // Shallower first + } + return newIssues[i].ID < newIssues[j].ID // Stable sort }) // Create in batches by depth level (max depth 3) for depth := 0; depth <= 3; depth++ { - var batchForDepth []*types.Issue - for _, issue := range newIssues { - if strings.Count(issue.ID, ".") == depth { - batchForDepth = append(batchForDepth, issue) + var batchForDepth []*types.Issue + for _, issue := range newIssues { + if strings.Count(issue.ID, ".") == depth { + batchForDepth = append(batchForDepth, issue) } } if len(batchForDepth) > 0 { diff --git a/internal/importer/importer_test.go b/internal/importer/importer_test.go index b639c163..b4ff00ba 100644 --- a/internal/importer/importer_test.go +++ b/internal/importer/importer_test.go @@ -1394,3 +1394,107 @@ func TestImportIssues_LegacyDeletionsConvertedToTombstones(t *testing.T) { t.Errorf("Expected DeleteReason 'duplicate of test-xyz', got %q", tombstone.DeleteReason) } } + +// TestImportOrphanSkip_CountMismatch verifies that orphaned issues are properly +// skipped during import and tracked in the result count (bd-ckej). +// +// Discovery recipe: Fresh clone >> bd init >> bd doctor --fix >> bd sync --import-only +// would show "Count mismatch: database has 823 issues, JSONL has 824" if orphaned +// child issues weren't properly filtered out (before the fix). +// +// The test imports issues where a child's parent doesn't exist in the database. +// With OrphanSkip mode, the child should be filtered out before creation. +func TestImportOrphanSkip_CountMismatch(t *testing.T) { + ctx := context.Background() + store, err := sqlite.New(ctx, ":memory:") + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Set prefix + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set prefix: %v", err) + } + + now := time.Now() + + // Prepare to import: normal issues + orphaned child + // The orphaned child has a parent (test-orphan) that doesn't exist in the database + issues := []*types.Issue{ + { + ID: "test-new1", + Title: "Normal Issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + CreatedAt: now, + UpdatedAt: now, + }, + { + ID: "test-orphan.1", // Child of non-existent parent + Title: "Orphaned Child", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + CreatedAt: now, + UpdatedAt: now, + }, + { + ID: "test-new2", + Title: "Another Normal Issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + CreatedAt: now, + UpdatedAt: now, + }, + } + + // Import with OrphanSkip mode - parent doesn't exist + result, err := ImportIssues(ctx, "", store, issues, Options{ + OrphanHandling: sqlite.OrphanSkip, + SkipPrefixValidation: true, // Allow explicit IDs during import + }) + if err != nil { + t.Fatalf("Import failed: %v", err) + } + + // Verify results: + // - 2 issues should be created (test-new1, test-new2) + // - 1 issue should be skipped (test-orphan.1 - no parent exists) + if result.Created != 2 { + t.Errorf("Expected 2 created issues, got %d", result.Created) + } + if result.Skipped != 1 { + t.Errorf("Expected 1 skipped issue (orphan), got %d", result.Skipped) + } + + // Verify the orphan is NOT in the database + allIssues, err := store.SearchIssues(ctx, "", types.IssueFilter{}) + if err != nil { + t.Fatalf("Failed to search issues: %v", err) + } + + var orphanFound bool + for _, issue := range allIssues { + if issue.ID == "test-orphan.1" { + orphanFound = true + break + } + } + if orphanFound { + t.Error("Orphaned issue test-orphan.1 should not be in database") + } + + // Verify normal issues ARE in the database + var count int + for _, issue := range allIssues { + if issue.ID == "test-new1" || issue.ID == "test-new2" { + count++ + } + } + if count != 2 { + t.Errorf("Expected 2 normal issues in database, found %d", count) + } +}