Skip to content

Commit aa663c4

Browse files
committed
fix(storage): move BatchCreateOptions to storage package for backend-agnostic batch creation
Fixes the leaky abstraction where molecules and importer packages directly used sqlite.BatchCreateOptions and type-asserted to *sqlite.SQLiteStorage. Changes: - Add storage.BatchCreateOptions and storage.OrphanHandling as backend-agnostic types - Add CreateIssuesWithFullOptions to Storage interface - Implement for SQLite, Dolt, and Memory backends - Update molecules.go to use Storage interface method instead of sqlite type assertion - Update importer.go to use storage types instead of sqlite-specific interface - Update cook.go to use storage interface method - Add deprecated aliases in sqlite package for backward compatibility This enables batch create optimizations for all backends including Dolt. Closes: bd-nrcp Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Executed-By: beads/jasper Rig: beads Role: polecat
1 parent e07ac18 commit aa663c4

12 files changed

Lines changed: 159 additions & 72 deletions

File tree

cmd/bd/cook.go

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"github.com/spf13/cobra"
1212
"github.com/steveyegge/beads/internal/formula"
1313
"github.com/steveyegge/beads/internal/storage"
14-
"github.com/steveyegge/beads/internal/storage/sqlite"
1514
"github.com/steveyegge/beads/internal/types"
1615
"github.com/steveyegge/beads/internal/ui"
1716
)
@@ -783,12 +782,6 @@ func cookFormula(ctx context.Context, s storage.Storage, f *formula.Formula, pro
783782
return nil, fmt.Errorf("no database connection")
784783
}
785784

786-
// Check for SQLite store (needed for batch create with skip prefix)
787-
sqliteStore, ok := s.(*sqlite.SQLiteStorage)
788-
if !ok {
789-
return nil, fmt.Errorf("cook requires SQLite storage")
790-
}
791-
792785
// Map step ID -> created issue ID
793786
idMapping := make(map[string]string)
794787

@@ -838,10 +831,11 @@ func cookFormula(ctx context.Context, s storage.Storage, f *formula.Formula, pro
838831
}
839832

840833
// Create all issues using batch with skip prefix validation
841-
opts := sqlite.BatchCreateOptions{
834+
opts := storage.BatchCreateOptions{
842835
SkipPrefixValidation: true, // Molecules use mol-* prefix
836+
OrphanHandling: storage.OrphanAllow,
843837
}
844-
if err := sqliteStore.CreateIssuesWithFullOptions(ctx, issues, actor, opts); err != nil {
838+
if err := s.CreateIssuesWithFullOptions(ctx, issues, actor, opts); err != nil {
845839
return nil, fmt.Errorf("failed to create issues: %w", err)
846840
}
847841

cmd/bd/deletion_tracking_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"testing"
99

1010
"github.com/steveyegge/beads/internal/config"
11+
"github.com/steveyegge/beads/internal/storage"
1112
"github.com/steveyegge/beads/internal/storage/sqlite"
1213
"github.com/steveyegge/beads/internal/types"
1314
)
@@ -908,7 +909,7 @@ func TestMultiRepoFlushPrefixFiltering(t *testing.T) {
908909

909910
// Use batch create with SkipPrefixValidation to simulate multi-repo hydration
910911
// (in real multi-repo mode, issues from other repos are imported with prefix validation skipped)
911-
if err := store.CreateIssuesWithFullOptions(ctx, []*types.Issue{primaryIssue, additionalIssue}, "test", sqlite.BatchCreateOptions{SkipPrefixValidation: true}); err != nil {
912+
if err := store.CreateIssuesWithFullOptions(ctx, []*types.Issue{primaryIssue, additionalIssue}, "test", storage.BatchCreateOptions{SkipPrefixValidation: true, OrphanHandling: storage.OrphanAllow}); err != nil {
912913
t.Fatalf("Failed to batch create issues: %v", err)
913914
}
914915

internal/importer/importer.go

Lines changed: 15 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -15,24 +15,21 @@ import (
1515
"github.com/steveyegge/beads/internal/linear"
1616
"github.com/steveyegge/beads/internal/routing"
1717
"github.com/steveyegge/beads/internal/storage"
18-
"github.com/steveyegge/beads/internal/storage/sqlite"
1918
"github.com/steveyegge/beads/internal/types"
2019
"github.com/steveyegge/beads/internal/utils"
2120
)
2221

23-
// OrphanHandling defines how to handle hierarchical child issues whose parents are missing.
24-
// This mirrors the string values historically used by the SQLite backend config.
25-
type OrphanHandling string
22+
// OrphanHandling is an alias for storage.OrphanHandling for backward compatibility.
23+
// Deprecated: Use storage.OrphanHandling directly.
24+
type OrphanHandling = storage.OrphanHandling
2625

26+
// Orphan handling constants - aliases for storage package constants.
27+
// Deprecated: Use storage.OrphanStrict, etc. directly.
2728
const (
28-
// OrphanStrict fails import on missing parent (safest)
29-
OrphanStrict OrphanHandling = "strict"
30-
// OrphanResurrect auto-resurrects missing parents from JSONL history
31-
OrphanResurrect OrphanHandling = "resurrect"
32-
// OrphanSkip skips orphaned issues with warning
33-
OrphanSkip OrphanHandling = "skip"
34-
// OrphanAllow imports orphans without validation (default, works around bugs)
35-
OrphanAllow OrphanHandling = "allow"
29+
OrphanStrict = storage.OrphanStrict
30+
OrphanResurrect = storage.OrphanResurrect
31+
OrphanSkip = storage.OrphanSkip
32+
OrphanAllow = storage.OrphanAllow
3633
)
3734

3835
// Options contains import configuration
@@ -908,25 +905,13 @@ func upsertIssues(ctx context.Context, store storage.Storage, issues []*types.Is
908905
}
909906
}
910907
if len(batchForDepth) > 0 {
911-
// Prefer a backend-specific import/batch API when available, so we can honor
912-
// options like SkipPrefixValidation (multi-repo mode) without requiring the
913-
// entire importer to be SQLite-specific.
914-
type importBatchCreator interface {
915-
CreateIssuesWithFullOptions(ctx context.Context, issues []*types.Issue, actor string, opts sqlite.BatchCreateOptions) error
908+
// Use the backend-agnostic batch creation API with full options.
909+
batchOpts := storage.BatchCreateOptions{
910+
OrphanHandling: opts.OrphanHandling,
911+
SkipPrefixValidation: opts.SkipPrefixValidation,
916912
}
917-
if bc, ok := store.(importBatchCreator); ok {
918-
batchOpts := sqlite.BatchCreateOptions{
919-
OrphanHandling: sqlite.OrphanHandling(opts.OrphanHandling),
920-
SkipPrefixValidation: opts.SkipPrefixValidation,
921-
}
922-
if err := bc.CreateIssuesWithFullOptions(ctx, batchForDepth, "import", batchOpts); err != nil {
923-
return fmt.Errorf("error creating depth-%d issues: %w", depth, err)
924-
}
925-
} else {
926-
// Generic fallback. OrphanSkip and OrphanStrict are enforced above.
927-
if err := store.CreateIssues(ctx, batchForDepth, "import"); err != nil {
928-
return fmt.Errorf("error creating depth-%d issues: %w", depth, err)
929-
}
913+
if err := store.CreateIssuesWithFullOptions(ctx, batchForDepth, "import", batchOpts); err != nil {
914+
return fmt.Errorf("error creating depth-%d issues: %w", depth, err)
930915
}
931916
result.Created += len(batchForDepth)
932917
}

internal/molecules/molecules.go

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import (
3131

3232
"github.com/steveyegge/beads/internal/debug"
3333
"github.com/steveyegge/beads/internal/storage"
34-
"github.com/steveyegge/beads/internal/storage/sqlite"
3534
"github.com/steveyegge/beads/internal/types"
3635
)
3736

@@ -149,27 +148,14 @@ func (l *Loader) loadMolecules(ctx context.Context, molecules []*types.Issue) (i
149148

150149
// Use batch creation with prefix validation skipped.
151150
// Molecules have their own ID namespace (mol-*) independent of project prefix.
152-
if sqliteStore, ok := l.store.(*sqlite.SQLiteStorage); ok {
153-
opts := sqlite.BatchCreateOptions{
154-
SkipPrefixValidation: true, // Molecules use their own prefix
155-
}
156-
if err := sqliteStore.CreateIssuesWithFullOptions(ctx, newMolecules, "molecules-loader", opts); err != nil {
157-
return 0, fmt.Errorf("batch create molecules: %w", err)
158-
}
159-
return len(newMolecules), nil
151+
opts := storage.BatchCreateOptions{
152+
SkipPrefixValidation: true, // Molecules use their own prefix
153+
OrphanHandling: storage.OrphanAllow,
160154
}
161-
162-
// Fallback for non-SQLite stores (e.g., memory storage in tests)
163-
loaded := 0
164-
for _, mol := range newMolecules {
165-
if err := l.store.CreateIssue(ctx, mol, "molecules-loader"); err != nil {
166-
debug.Logf("failed to load molecule %s: %v", mol.ID, err)
167-
continue
168-
}
169-
loaded++
155+
if err := l.store.CreateIssuesWithFullOptions(ctx, newMolecules, "molecules-loader", opts); err != nil {
156+
return 0, fmt.Errorf("batch create molecules: %w", err)
170157
}
171-
172-
return loaded, nil
158+
return len(newMolecules), nil
173159
}
174160

175161
// loadMoleculesFromFile loads molecules from a JSONL file.

internal/molecules/molecules_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"path/filepath"
77
"testing"
88

9+
"github.com/steveyegge/beads/internal/storage"
910
"github.com/steveyegge/beads/internal/storage/sqlite"
1011
"github.com/steveyegge/beads/internal/types"
1112
)
@@ -168,7 +169,7 @@ func TestLoader_SkipExistingMolecules(t *testing.T) {
168169
Status: types.StatusOpen,
169170
IsTemplate: true,
170171
}
171-
opts := sqlite.BatchCreateOptions{SkipPrefixValidation: true}
172+
opts := storage.BatchCreateOptions{SkipPrefixValidation: true, OrphanHandling: storage.OrphanAllow}
172173
if err := store.CreateIssuesWithFullOptions(ctx, []*types.Issue{existingMol}, "test", opts); err != nil {
173174
t.Fatalf("Failed to create existing molecule: %v", err)
174175
}

internal/storage/batch.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Package storage defines the interface for issue storage backends.
2+
package storage
3+
4+
// OrphanHandling specifies how to handle issues with missing parent references.
5+
type OrphanHandling string
6+
7+
const (
8+
// OrphanStrict fails import on missing parent (safest)
9+
OrphanStrict OrphanHandling = "strict"
10+
// OrphanResurrect auto-resurrects missing parents from JSONL history
11+
OrphanResurrect OrphanHandling = "resurrect"
12+
// OrphanSkip skips orphaned issues with warning
13+
OrphanSkip OrphanHandling = "skip"
14+
// OrphanAllow imports orphans without validation (default, works around bugs)
15+
OrphanAllow OrphanHandling = "allow"
16+
)
17+
18+
// BatchCreateOptions contains options for batch issue creation.
19+
// This is a backend-agnostic type that can be used by any storage implementation.
20+
type BatchCreateOptions struct {
21+
// OrphanHandling specifies how to handle issues with missing parent references
22+
OrphanHandling OrphanHandling
23+
// SkipPrefixValidation skips prefix validation for existing IDs (used during import)
24+
SkipPrefixValidation bool
25+
}

internal/storage/dolt/issues.go

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"time"
1010

1111
"github.com/steveyegge/beads/internal/idgen"
12+
"github.com/steveyegge/beads/internal/storage"
1213
"github.com/steveyegge/beads/internal/types"
1314
)
1415

@@ -116,6 +117,16 @@ func (s *DoltStore) CreateIssue(ctx context.Context, issue *types.Issue, actor s
116117

117118
// CreateIssues creates multiple issues in a single transaction
118119
func (s *DoltStore) CreateIssues(ctx context.Context, issues []*types.Issue, actor string) error {
120+
return s.CreateIssuesWithFullOptions(ctx, issues, actor, storage.BatchCreateOptions{
121+
OrphanHandling: storage.OrphanAllow,
122+
SkipPrefixValidation: false,
123+
})
124+
}
125+
126+
// CreateIssuesWithFullOptions creates multiple issues with full options control.
127+
// This is the backend-agnostic batch creation method that supports orphan handling
128+
// and prefix validation options.
129+
func (s *DoltStore) CreateIssuesWithFullOptions(ctx context.Context, issues []*types.Issue, actor string, opts storage.BatchCreateOptions) error {
119130
if len(issues) == 0 {
120131
return nil
121132
}
@@ -136,6 +147,15 @@ func (s *DoltStore) CreateIssues(ctx context.Context, issues []*types.Issue, act
136147
}
137148
defer func() { _ = tx.Rollback() }()
138149

150+
// Get prefix from config for validation
151+
var configPrefix string
152+
err = tx.QueryRowContext(ctx, "SELECT value FROM config WHERE `key` = ?", "issue_prefix").Scan(&configPrefix)
153+
if err == sql.ErrNoRows || configPrefix == "" {
154+
return fmt.Errorf("database not initialized: issue_prefix config is missing (run 'bd init --prefix <prefix>' first)")
155+
} else if err != nil {
156+
return fmt.Errorf("failed to get config: %w", err)
157+
}
158+
139159
for _, issue := range issues {
140160
now := time.Now().UTC()
141161
if issue.CreatedAt.IsZero() {
@@ -174,6 +194,35 @@ func (s *DoltStore) CreateIssues(ctx context.Context, issues []*types.Issue, act
174194
issue.ContentHash = issue.ComputeContentHash()
175195
}
176196

197+
// Validate prefix if not skipped (for imports with different prefixes)
198+
if !opts.SkipPrefixValidation && issue.ID != "" {
199+
if err := validateIssueIDPrefix(issue.ID, configPrefix); err != nil {
200+
return fmt.Errorf("prefix validation failed for %s: %w", issue.ID, err)
201+
}
202+
}
203+
204+
// Handle orphan checking for hierarchical IDs
205+
if issue.ID != "" {
206+
if parentID, _, ok := parseHierarchicalID(issue.ID); ok {
207+
var parentCount int
208+
err := tx.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&parentCount)
209+
if err != nil {
210+
return fmt.Errorf("failed to check parent existence: %w", err)
211+
}
212+
if parentCount == 0 {
213+
switch opts.OrphanHandling {
214+
case storage.OrphanStrict:
215+
return fmt.Errorf("parent issue %s does not exist (strict mode)", parentID)
216+
case storage.OrphanSkip:
217+
// Skip this issue
218+
continue
219+
case storage.OrphanResurrect, storage.OrphanAllow:
220+
// Allow orphan - continue with insert
221+
}
222+
}
223+
}
224+
}
225+
177226
if err := insertIssue(ctx, tx, issue); err != nil {
178227
return fmt.Errorf("failed to insert issue %s: %w", issue.ID, err)
179228
}
@@ -188,6 +237,35 @@ func (s *DoltStore) CreateIssues(ctx context.Context, issues []*types.Issue, act
188237
return tx.Commit()
189238
}
190239

240+
// validateIssueIDPrefix validates that the issue ID has the correct prefix
241+
func validateIssueIDPrefix(id, prefix string) error {
242+
if !strings.HasPrefix(id, prefix+"-") {
243+
return fmt.Errorf("issue ID %s does not match configured prefix %s", id, prefix)
244+
}
245+
return nil
246+
}
247+
248+
// parseHierarchicalID checks if an ID is hierarchical (e.g., "bd-abc.1") and returns the parent ID and child number
249+
func parseHierarchicalID(id string) (parentID string, childNum int, ok bool) {
250+
// Find the last dot that separates parent from child number
251+
lastDot := strings.LastIndex(id, ".")
252+
if lastDot == -1 {
253+
return "", 0, false
254+
}
255+
256+
parentID = id[:lastDot]
257+
suffix := id[lastDot+1:]
258+
259+
// Parse child number
260+
var num int
261+
_, err := fmt.Sscanf(suffix, "%d", &num)
262+
if err != nil {
263+
return "", 0, false
264+
}
265+
266+
return parentID, num, true
267+
}
268+
191269
// GetIssue retrieves an issue by ID
192270
func (s *DoltStore) GetIssue(ctx context.Context, id string) (*types.Issue, error) {
193271
s.mu.RLock()

internal/storage/memory/memory.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,15 @@ func (m *MemoryStorage) CreateIssues(ctx context.Context, issues []*types.Issue,
322322
return nil
323323
}
324324

325+
// CreateIssuesWithFullOptions creates multiple issues with full options control.
326+
// For MemoryStorage, this delegates to CreateIssues as the options (orphan handling,
327+
// prefix validation) are primarily relevant for persistent storage backends.
328+
func (m *MemoryStorage) CreateIssuesWithFullOptions(ctx context.Context, issues []*types.Issue, actor string, opts storage.BatchCreateOptions) error {
329+
// MemoryStorage doesn't enforce prefix validation or orphan handling
330+
// as it's primarily used for testing and no-db mode
331+
return m.CreateIssues(ctx, issues, actor)
332+
}
333+
325334
// GetIssue retrieves an issue by ID
326335
func (m *MemoryStorage) GetIssue(ctx context.Context, id string) (*types.Issue, error) {
327336
m.mu.RLock()

internal/storage/sqlite/batch_ops.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"strings"
88
"time"
99

10+
"github.com/steveyegge/beads/internal/storage"
1011
"github.com/steveyegge/beads/internal/types"
1112
)
1213

@@ -231,11 +232,9 @@ func (s *SQLiteStorage) CreateIssues(ctx context.Context, issues []*types.Issue,
231232
return s.CreateIssuesWithOptions(ctx, issues, actor, OrphanResurrect)
232233
}
233234

234-
// BatchCreateOptions contains options for batch issue creation
235-
type BatchCreateOptions struct {
236-
OrphanHandling OrphanHandling // How to handle missing parent issues
237-
SkipPrefixValidation bool // Skip prefix validation for existing IDs (used during import)
238-
}
235+
// BatchCreateOptions is an alias for storage.BatchCreateOptions for backward compatibility.
236+
// Deprecated: Use storage.BatchCreateOptions directly.
237+
type BatchCreateOptions = storage.BatchCreateOptions
239238

240239
// CreateIssuesWithOptions creates multiple issues with configurable orphan handling
241240
func (s *SQLiteStorage) CreateIssuesWithOptions(ctx context.Context, issues []*types.Issue, actor string, orphanHandling OrphanHandling) error {

internal/storage/sqlite/config.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"strings"
77

88
"github.com/steveyegge/beads/internal/config"
9+
"github.com/steveyegge/beads/internal/storage"
910
)
1011

1112
// SetConfig sets a configuration value
@@ -74,14 +75,17 @@ func (s *SQLiteStorage) DeleteConfig(ctx context.Context, key string) error {
7475
return wrapDBError("delete config", err)
7576
}
7677

77-
// OrphanHandling defines how to handle orphan issues during import
78-
type OrphanHandling string
78+
// OrphanHandling is an alias for storage.OrphanHandling for backward compatibility.
79+
// Deprecated: Use storage.OrphanHandling directly.
80+
type OrphanHandling = storage.OrphanHandling
7981

82+
// Orphan handling constants - aliases for storage package constants.
83+
// Deprecated: Use storage.OrphanStrict, etc. directly.
8084
const (
81-
OrphanStrict OrphanHandling = "strict" // Reject imports with orphans
82-
OrphanResurrect OrphanHandling = "resurrect" // Auto-resurrect parents from JSONL
83-
OrphanSkip OrphanHandling = "skip" // Skip orphans silently
84-
OrphanAllow OrphanHandling = "allow" // Allow orphans (default)
85+
OrphanStrict = storage.OrphanStrict // Reject imports with orphans
86+
OrphanResurrect = storage.OrphanResurrect // Auto-resurrect parents from JSONL
87+
OrphanSkip = storage.OrphanSkip // Skip orphans silently
88+
OrphanAllow = storage.OrphanAllow // Allow orphans (default)
8589
)
8690

8791
// GetOrphanHandling gets the import.orphan_handling config value

0 commit comments

Comments
 (0)