diff --git a/internal/rpc/server_export_import_auto.go b/internal/rpc/server_export_import_auto.go index 4a6fc1b9..87b81f63 100644 --- a/internal/rpc/server_export_import_auto.go +++ b/internal/rpc/server_export_import_auto.go @@ -59,28 +59,32 @@ func (s *Server) handleExport(req *Request) Response { issue.Dependencies = allDeps[issue.ID] } - // Populate labels for all issues - for _, issue := range issues { - labels, err := store.GetLabels(ctx, issue.ID) - if err != nil { - return Response{ - Success: false, - Error: fmt.Sprintf("failed to get labels for %s: %v", issue.ID, err), - } + // Populate labels for all issues (avoid N+1) + issueIDs := make([]string, len(issues)) + for i, issue := range issues { + issueIDs[i] = issue.ID + } + allLabels, err := store.GetLabelsForIssues(ctx, issueIDs) + if err != nil { + return Response{ + Success: false, + Error: fmt.Sprintf("failed to get labels: %v", err), } - issue.Labels = labels + } + for _, issue := range issues { + issue.Labels = allLabels[issue.ID] } - // Populate comments for all issues - for _, issue := range issues { - comments, err := store.GetIssueComments(ctx, issue.ID) - if err != nil { - return Response{ - Success: false, - Error: fmt.Sprintf("failed to get comments for %s: %v", issue.ID, err), - } + // Populate comments for all issues (avoid N+1) + allComments, err := store.GetCommentsForIssues(ctx, issueIDs) + if err != nil { + return Response{ + Success: false, + Error: fmt.Sprintf("failed to get comments: %v", err), } - issue.Comments = comments + } + for _, issue := range issues { + issue.Comments = allComments[issue.ID] } // Create temp file for atomic write @@ -387,7 +391,7 @@ func (s *Server) triggerExport(ctx context.Context, store storage.Storage, dbPat }) // CRITICAL: Populate all related data to prevent data loss - // This mirrors the logic in handleExport (lines 50-83) + // This mirrors the logic in handleExport // Populate dependencies for all issues (avoid N+1 queries) allDeps, err := store.GetAllDependencyRecords(ctx) @@ -398,22 +402,26 @@ func (s *Server) triggerExport(ctx context.Context, store storage.Storage, dbPat issue.Dependencies = allDeps[issue.ID] } - // Populate labels for all issues + // Populate labels for all issues (avoid N+1 queries) + issueIDs := make([]string, len(allIssues)) + for i, issue := range allIssues { + issueIDs[i] = issue.ID + } + allLabels, err := store.GetLabelsForIssues(ctx, issueIDs) + if err != nil { + return fmt.Errorf("failed to get labels: %w", err) + } for _, issue := range allIssues { - labels, err := store.GetLabels(ctx, issue.ID) - if err != nil { - return fmt.Errorf("failed to get labels for %s: %w", issue.ID, err) - } - issue.Labels = labels + issue.Labels = allLabels[issue.ID] } - // Populate comments for all issues + // Populate comments for all issues (avoid N+1 queries) + allComments, err := store.GetCommentsForIssues(ctx, issueIDs) + if err != nil { + return fmt.Errorf("failed to get comments: %w", err) + } for _, issue := range allIssues { - comments, err := store.GetIssueComments(ctx, issue.ID) - if err != nil { - return fmt.Errorf("failed to get comments for %s: %w", issue.ID, err) - } - issue.Comments = comments + issue.Comments = allComments[issue.ID] } // Write to JSONL file with atomic replace (temp file + rename) diff --git a/internal/storage/memory/memory.go b/internal/storage/memory/memory.go index 7926c406..37c7adec 100644 --- a/internal/storage/memory/memory.go +++ b/internal/storage/memory/memory.go @@ -912,6 +912,19 @@ func (m *MemoryStorage) GetIssueComments(ctx context.Context, issueID string) ([ return m.comments[issueID], nil } +func (m *MemoryStorage) GetCommentsForIssues(ctx context.Context, issueIDs []string) (map[string][]*types.Comment, error) { + m.mu.RLock() + defer m.mu.RUnlock() + + result := make(map[string][]*types.Comment) + for _, issueID := range issueIDs { + if comments, exists := m.comments[issueID]; exists { + result[issueID] = comments + } + } + return result, nil +} + func (m *MemoryStorage) GetStatistics(ctx context.Context) (*types.Statistics, error) { m.mu.RLock() defer m.mu.RUnlock() diff --git a/internal/storage/sqlite/batch_test.go b/internal/storage/sqlite/batch_test.go new file mode 100644 index 00000000..aab69f08 --- /dev/null +++ b/internal/storage/sqlite/batch_test.go @@ -0,0 +1,143 @@ +package sqlite + +import ( + "context" + "testing" + + "github.com/steveyegge/beads/internal/types" +) + +// TestBatchGetLabelsAndComments verifies that GetLabelsForIssues and GetCommentsForIssues +// correctly fetch data for multiple issues in a single query (avoiding N+1 pattern) +func TestBatchGetLabelsAndComments(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create test issues + issues := []*types.Issue{ + { + ID: "bd-batch1", + Title: "Issue 1", + IssueType: types.TypeTask, + Status: types.StatusOpen, + Priority: 1, + }, + { + ID: "bd-batch2", + Title: "Issue 2", + IssueType: types.TypeTask, + Status: types.StatusOpen, + Priority: 1, + }, + { + ID: "bd-batch3", + Title: "Issue 3", + IssueType: types.TypeTask, + Status: types.StatusOpen, + Priority: 1, + }, + } + + for _, issue := range issues { + if err := store.CreateIssue(ctx, issue, "test-actor"); err != nil { + t.Fatalf("Failed to create issue %s: %v", issue.ID, err) + } + } + + // Add labels to issues + if err := store.AddLabel(ctx, "bd-batch1", "bug", "test-actor"); err != nil { + t.Fatalf("Failed to add label: %v", err) + } + if err := store.AddLabel(ctx, "bd-batch1", "urgent", "test-actor"); err != nil { + t.Fatalf("Failed to add label: %v", err) + } + if err := store.AddLabel(ctx, "bd-batch2", "feature", "test-actor"); err != nil { + t.Fatalf("Failed to add label: %v", err) + } + // bd-batch3 has no labels + + // Add comments to issues + if _, err := store.AddIssueComment(ctx, "bd-batch1", "alice", "First comment"); err != nil { + t.Fatalf("Failed to add comment: %v", err) + } + if _, err := store.AddIssueComment(ctx, "bd-batch1", "bob", "Second comment"); err != nil { + t.Fatalf("Failed to add comment: %v", err) + } + if _, err := store.AddIssueComment(ctx, "bd-batch3", "charlie", "Comment on bd-batch3"); err != nil { + t.Fatalf("Failed to add comment: %v", err) + } + // bd-batch2 has no comments + + // Test batch get labels + issueIDs := []string{"bd-batch1", "bd-batch2", "bd-batch3"} + allLabels, err := store.GetLabelsForIssues(ctx, issueIDs) + if err != nil { + t.Fatalf("GetLabelsForIssues failed: %v", err) + } + + // Verify labels + if len(allLabels["bd-batch1"]) != 2 { + t.Errorf("Expected 2 labels for bd-batch1, got %d", len(allLabels["bd-batch1"])) + } + if len(allLabels["bd-batch2"]) != 1 { + t.Errorf("Expected 1 label for bd-batch2, got %d", len(allLabels["bd-batch2"])) + } + if len(allLabels["bd-batch3"]) != 0 { + t.Errorf("Expected 0 labels for bd-batch3, got %d", len(allLabels["bd-batch3"])) + } + + // Test batch get comments + allComments, err := store.GetCommentsForIssues(ctx, issueIDs) + if err != nil { + t.Fatalf("GetCommentsForIssues failed: %v", err) + } + + // Verify comments + if len(allComments["bd-batch1"]) != 2 { + t.Errorf("Expected 2 comments for bd-batch1, got %d", len(allComments["bd-batch1"])) + } + if allComments["bd-batch1"][0].Author != "alice" { + t.Errorf("Expected first comment author to be 'alice', got %s", allComments["bd-batch1"][0].Author) + } + if allComments["bd-batch1"][1].Author != "bob" { + t.Errorf("Expected second comment author to be 'bob', got %s", allComments["bd-batch1"][1].Author) + } + + if len(allComments["bd-batch2"]) != 0 { + t.Errorf("Expected 0 comments for bd-batch2, got %d", len(allComments["bd-batch2"])) + } + + if len(allComments["bd-batch3"]) != 1 { + t.Errorf("Expected 1 comment for bd-batch3, got %d", len(allComments["bd-batch3"])) + } + if len(allComments["bd-batch3"]) > 0 && allComments["bd-batch3"][0].Author != "charlie" { + t.Errorf("Expected comment author to be 'charlie', got %s", allComments["bd-batch3"][0].Author) + } +} + +// TestBatchGetEmptyIssueIDs verifies that batch methods handle empty issue ID lists +func TestBatchGetEmptyIssueIDs(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Test with empty issue ID list + labels, err := store.GetLabelsForIssues(ctx, []string{}) + if err != nil { + t.Fatalf("GetLabelsForIssues with empty list failed: %v", err) + } + if len(labels) != 0 { + t.Errorf("Expected empty map, got %d entries", len(labels)) + } + + comments, err := store.GetCommentsForIssues(ctx, []string{}) + if err != nil { + t.Fatalf("GetCommentsForIssues with empty list failed: %v", err) + } + if len(comments) != 0 { + t.Errorf("Expected empty map, got %d entries", len(comments)) + } +} diff --git a/internal/storage/sqlite/comments.go b/internal/storage/sqlite/comments.go index 411ca658..58e5e62d 100644 --- a/internal/storage/sqlite/comments.go +++ b/internal/storage/sqlite/comments.go @@ -81,3 +81,46 @@ func (s *SQLiteStorage) GetIssueComments(ctx context.Context, issueID string) ([ return comments, nil } + +// GetCommentsForIssues fetches comments for multiple issues in a single query +// Returns a map of issue_id -> []*Comment +func (s *SQLiteStorage) GetCommentsForIssues(ctx context.Context, issueIDs []string) (map[string][]*types.Comment, error) { + if len(issueIDs) == 0 { + return make(map[string][]*types.Comment), nil + } + + // Build placeholders for IN clause + placeholders := make([]interface{}, len(issueIDs)) + for i, id := range issueIDs { + placeholders[i] = id + } + + query := fmt.Sprintf(` + SELECT id, issue_id, author, text, created_at + FROM comments + WHERE issue_id IN (%s) + ORDER BY issue_id, created_at ASC + `, buildPlaceholders(len(issueIDs))) // #nosec G201 -- placeholders are generated internally + + rows, err := s.db.QueryContext(ctx, query, placeholders...) + if err != nil { + return nil, fmt.Errorf("failed to batch get comments: %w", err) + } + defer func() { _ = rows.Close() }() + + result := make(map[string][]*types.Comment) + for rows.Next() { + comment := &types.Comment{} + err := rows.Scan(&comment.ID, &comment.IssueID, &comment.Author, &comment.Text, &comment.CreatedAt) + if err != nil { + return nil, fmt.Errorf("failed to scan comment: %w", err) + } + result[comment.IssueID] = append(result[comment.IssueID], comment) + } + + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("error iterating comments: %w", err) + } + + return result, nil +} diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 7974cc8f..6983af56 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -51,6 +51,7 @@ type Storage interface { // Comments AddIssueComment(ctx context.Context, issueID, author, text string) (*types.Comment, error) GetIssueComments(ctx context.Context, issueID string) ([]*types.Comment, error) + GetCommentsForIssues(ctx context.Context, issueIDs []string) (map[string][]*types.Comment, error) // Statistics GetStatistics(ctx context.Context) (*types.Statistics, error)