fix(sqlite): update child_counters when explicit child IDs are created (GH#728)
When creating issues with explicit hierarchical IDs (e.g., bd-test.1, bd-test.2 via --id flag or import), the child_counters table was not being updated. This caused GetNextChildID to return colliding IDs when later called with --parent. Changes: - Add ensureChildCounterUpdatedWithConn() to update counter on explicit child creation - Add ParseHierarchicalID() to extract parent and child number from IDs - Update CreateIssue to call counter update after hierarchical ID validation - Update EnsureIDs to call counter update when parent exists - Add post-insert phase in batch operations to update counters after FK constraint can be satisfied - Update tests to reflect new behavior where counter is properly initialized 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -102,6 +102,30 @@ func bulkMarkDirty(ctx context.Context, conn *sql.Conn, issues []*types.Issue) e
|
||||
return markDirtyBatch(ctx, conn, issues)
|
||||
}
|
||||
|
||||
// updateChildCountersForHierarchicalIDs updates child_counters for all hierarchical IDs in the batch.
|
||||
// This is called AFTER issues are inserted so that parents exist for the foreign key constraint.
|
||||
// (GH#728 fix)
|
||||
func updateChildCountersForHierarchicalIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue) error {
|
||||
for _, issue := range issues {
|
||||
if issue.ID == "" {
|
||||
continue // Skip issues that were filtered out (e.g., OrphanSkip)
|
||||
}
|
||||
if parentID, childNum, ok := ParseHierarchicalID(issue.ID); ok {
|
||||
// Only update if parent exists (it should after insert, but check to be safe)
|
||||
var parentCount int
|
||||
if err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&parentCount); err != nil {
|
||||
return fmt.Errorf("failed to check parent existence for %s: %w", parentID, err)
|
||||
}
|
||||
if parentCount > 0 {
|
||||
if err := ensureChildCounterUpdatedWithConn(ctx, conn, parentID, childNum); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// checkForExistingIDs verifies that:
|
||||
// 1. There are no duplicate IDs within the batch itself
|
||||
// 2. None of the issue IDs already exist in the database
|
||||
@@ -271,6 +295,12 @@ func (s *SQLiteStorage) CreateIssuesWithFullOptions(ctx context.Context, issues
|
||||
return wrapDBError("bulk insert issues", err)
|
||||
}
|
||||
|
||||
// Phase 4.5: Update child counters for hierarchical IDs (GH#728 fix)
|
||||
// This must happen AFTER insert so parents exist for the foreign key constraint
|
||||
if err := updateChildCountersForHierarchicalIDs(ctx, conn, issues); err != nil {
|
||||
return wrapDBError("update child counters", err)
|
||||
}
|
||||
|
||||
// Phase 5: Record creation events
|
||||
if err := bulkRecordEvents(ctx, conn, issues, actor); err != nil {
|
||||
return wrapDBError("record creation events", err)
|
||||
|
||||
@@ -78,11 +78,11 @@ func TestGetNextChildNumber(t *testing.T) {
|
||||
func TestGetNextChildNumber_DifferentParents(t *testing.T) {
|
||||
store, cleanup := setupTestDB(t)
|
||||
defer cleanup()
|
||||
|
||||
|
||||
ctx := context.Background()
|
||||
parent1 := "bd-af78e9a2"
|
||||
parent2 := "bd-af78e9a2.1"
|
||||
|
||||
parent2 := "bd-bf89e0b3" // Use non-hierarchical ID to avoid counter interaction
|
||||
|
||||
// Create parent issues first
|
||||
for _, id := range []string{parent1, parent2} {
|
||||
parent := &types.Issue{
|
||||
@@ -97,7 +97,7 @@ func TestGetNextChildNumber_DifferentParents(t *testing.T) {
|
||||
t.Fatalf("failed to create parent issue %s: %v", id, err)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
// Each parent should have independent counters
|
||||
child1_1, err := store.getNextChildNumber(ctx, parent1)
|
||||
if err != nil {
|
||||
@@ -106,7 +106,7 @@ func TestGetNextChildNumber_DifferentParents(t *testing.T) {
|
||||
if child1_1 != 1 {
|
||||
t.Errorf("expected parent1 child to be 1, got %d", child1_1)
|
||||
}
|
||||
|
||||
|
||||
child2_1, err := store.getNextChildNumber(ctx, parent2)
|
||||
if err != nil {
|
||||
t.Fatalf("getNextChildNumber failed: %v", err)
|
||||
@@ -114,7 +114,7 @@ func TestGetNextChildNumber_DifferentParents(t *testing.T) {
|
||||
if child2_1 != 1 {
|
||||
t.Errorf("expected parent2 child to be 1, got %d", child2_1)
|
||||
}
|
||||
|
||||
|
||||
child1_2, err := store.getNextChildNumber(ctx, parent1)
|
||||
if err != nil {
|
||||
t.Fatalf("getNextChildNumber failed: %v", err)
|
||||
@@ -186,10 +186,12 @@ func TestGetNextChildNumber_Concurrent(t *testing.T) {
|
||||
func TestGetNextChildNumber_NestedHierarchy(t *testing.T) {
|
||||
store, cleanup := setupTestDB(t)
|
||||
defer cleanup()
|
||||
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
|
||||
// Create parent issues for nested hierarchy
|
||||
// Note: When creating bd-af78e9a2.1, the counter for bd-af78e9a2 is set to 1 (GH#728 fix)
|
||||
// When creating bd-af78e9a2.1.2, the counter for bd-af78e9a2.1 is set to 2 (GH#728 fix)
|
||||
parents := []string{"bd-af78e9a2", "bd-af78e9a2.1", "bd-af78e9a2.1.2"}
|
||||
for _, id := range parents {
|
||||
parent := &types.Issue{
|
||||
@@ -204,21 +206,21 @@ func TestGetNextChildNumber_NestedHierarchy(t *testing.T) {
|
||||
t.Fatalf("failed to create parent issue %s: %v", id, err)
|
||||
}
|
||||
}
|
||||
|
||||
// Create nested hierarchy counters
|
||||
// bd-af78e9a2 → .1, .2
|
||||
// bd-af78e9a2.1 → .1.1, .1.2
|
||||
// bd-af78e9a2.1.2 → .1.2.1, .1.2.2
|
||||
|
||||
|
||||
// With GH#728 fix, counters are updated when explicit hierarchical IDs are created:
|
||||
// - Creating bd-af78e9a2.1 sets counter for bd-af78e9a2 to 1
|
||||
// - Creating bd-af78e9a2.1.2 sets counter for bd-af78e9a2.1 to 2
|
||||
// So getNextChildNumber returns the NEXT number after the existing children
|
||||
|
||||
tests := []struct {
|
||||
parent string
|
||||
expected []int
|
||||
}{
|
||||
{"bd-af78e9a2", []int{1, 2}},
|
||||
{"bd-af78e9a2.1", []int{1, 2}},
|
||||
{"bd-af78e9a2.1.2", []int{1, 2}},
|
||||
{"bd-af78e9a2", []int{2, 3}}, // counter was 1 after creating .1
|
||||
{"bd-af78e9a2.1", []int{3, 4}}, // counter was 2 after creating .1.2
|
||||
{"bd-af78e9a2.1.2", []int{1, 2}}, // no children created, starts at 1
|
||||
}
|
||||
|
||||
|
||||
for _, tt := range tests {
|
||||
for _, want := range tt.expected {
|
||||
got, err := store.getNextChildNumber(ctx, tt.parent)
|
||||
|
||||
@@ -179,6 +179,80 @@ func TestCreateIssue_HierarchicalID(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestExplicitChildIDUpdatesCounter verifies that creating issues with explicit
|
||||
// hierarchical IDs (e.g., bd-test.1, bd-test.2) updates the child counter so that
|
||||
// GetNextChildID returns the correct next ID (GH#728 fix)
|
||||
func TestExplicitChildIDUpdatesCounter(t *testing.T) {
|
||||
tmpFile := t.TempDir() + "/test.db"
|
||||
defer os.Remove(tmpFile)
|
||||
store := newTestStore(t, tmpFile)
|
||||
defer store.Close()
|
||||
ctx := context.Background()
|
||||
|
||||
// Create parent
|
||||
parent := &types.Issue{
|
||||
ID: "bd-test",
|
||||
Title: "Parent",
|
||||
Description: "Test parent",
|
||||
Status: types.StatusOpen,
|
||||
Priority: 2,
|
||||
IssueType: types.TypeEpic,
|
||||
}
|
||||
if err := store.CreateIssue(ctx, parent, "test"); err != nil {
|
||||
t.Fatalf("failed to create parent: %v", err)
|
||||
}
|
||||
|
||||
// Create explicit child .1
|
||||
child1 := &types.Issue{
|
||||
ID: "bd-test.1",
|
||||
Title: "Existing child 1",
|
||||
Description: "Created with explicit ID",
|
||||
Status: types.StatusOpen,
|
||||
Priority: 2,
|
||||
IssueType: types.TypeTask,
|
||||
}
|
||||
if err := store.CreateIssue(ctx, child1, "test"); err != nil {
|
||||
t.Fatalf("failed to create child1: %v", err)
|
||||
}
|
||||
|
||||
// Create explicit child .2
|
||||
child2 := &types.Issue{
|
||||
ID: "bd-test.2",
|
||||
Title: "Existing child 2",
|
||||
Description: "Created with explicit ID",
|
||||
Status: types.StatusOpen,
|
||||
Priority: 2,
|
||||
IssueType: types.TypeTask,
|
||||
}
|
||||
if err := store.CreateIssue(ctx, child2, "test"); err != nil {
|
||||
t.Fatalf("failed to create child2: %v", err)
|
||||
}
|
||||
|
||||
// Now use GetNextChildID - should return .3 (not .1 which would collide)
|
||||
nextID, err := store.GetNextChildID(ctx, "bd-test")
|
||||
if err != nil {
|
||||
t.Fatalf("GetNextChildID failed: %v", err)
|
||||
}
|
||||
|
||||
expected := "bd-test.3"
|
||||
if nextID != expected {
|
||||
t.Errorf("GetNextChildID returned %s, expected %s (GH#728 - counter should be updated when explicit child IDs are created)", nextID, expected)
|
||||
}
|
||||
|
||||
// Verify we can create an issue with the returned ID without collision
|
||||
child3 := &types.Issue{
|
||||
ID: nextID,
|
||||
Title: "New child via --parent",
|
||||
Description: "Created with GetNextChildID",
|
||||
Status: types.StatusOpen,
|
||||
Priority: 2,
|
||||
IssueType: types.TypeTask,
|
||||
}
|
||||
if err := store.CreateIssue(ctx, child3, "test"); err != nil {
|
||||
t.Fatalf("failed to create child3 with ID %s: %v", nextID, err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCreateIssue_HierarchicalID_ParentNotExists(t *testing.T) {
|
||||
tmpFile := t.TempDir() + "/test.db"
|
||||
defer os.Remove(tmpFile)
|
||||
|
||||
@@ -2,6 +2,7 @@ package sqlite
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"fmt"
|
||||
"strings"
|
||||
)
|
||||
@@ -65,4 +66,36 @@ func (s *SQLiteStorage) GetNextChildID(ctx context.Context, parentID string) (st
|
||||
return childID, nil
|
||||
}
|
||||
|
||||
// ensureChildCounterUpdated ensures the child_counters table has a value for parentID
|
||||
// that is at least childNum. This prevents ID collisions when children are created
|
||||
// with explicit IDs (via --id flag or import) rather than GetNextChildID.
|
||||
// (GH#728 fix)
|
||||
func (s *SQLiteStorage) ensureChildCounterUpdated(ctx context.Context, parentID string, childNum int) error {
|
||||
_, err := s.db.ExecContext(ctx, `
|
||||
INSERT INTO child_counters (parent_id, last_child)
|
||||
VALUES (?, ?)
|
||||
ON CONFLICT(parent_id) DO UPDATE SET
|
||||
last_child = MAX(last_child, excluded.last_child)
|
||||
`, parentID, childNum)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to update child counter for parent %s: %w", parentID, err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// ensureChildCounterUpdatedWithConn is like ensureChildCounterUpdated but uses a specific
|
||||
// connection for transaction consistency. (GH#728 fix)
|
||||
func ensureChildCounterUpdatedWithConn(ctx context.Context, conn *sql.Conn, parentID string, childNum int) error {
|
||||
_, err := conn.ExecContext(ctx, `
|
||||
INSERT INTO child_counters (parent_id, last_child)
|
||||
VALUES (?, ?)
|
||||
ON CONFLICT(parent_id) DO UPDATE SET
|
||||
last_child = MAX(last_child, excluded.last_child)
|
||||
`, parentID, childNum)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to update child counter for parent %s: %w", parentID, err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// generateHashID moved to ids.go (bd-0702)
|
||||
|
||||
@@ -63,6 +63,33 @@ func IsHierarchicalID(id string) (isHierarchical bool, parentID string) {
|
||||
return true, id[:lastDot]
|
||||
}
|
||||
|
||||
// ParseHierarchicalID extracts the parent ID and child number from a hierarchical ID.
|
||||
// Returns (parentID, childNum, true) for hierarchical IDs like "bd-abc.1" -> ("bd-abc", 1, true).
|
||||
// Returns ("", 0, false) for non-hierarchical IDs.
|
||||
// (GH#728 fix)
|
||||
func ParseHierarchicalID(id string) (parentID string, childNum int, ok bool) {
|
||||
lastDot := strings.LastIndex(id, ".")
|
||||
if lastDot == -1 {
|
||||
return "", 0, false
|
||||
}
|
||||
|
||||
suffix := id[lastDot+1:]
|
||||
if len(suffix) == 0 {
|
||||
return "", 0, false
|
||||
}
|
||||
|
||||
// Parse the numeric suffix
|
||||
num := 0
|
||||
for _, c := range suffix {
|
||||
if c < '0' || c > '9' {
|
||||
return "", 0, false
|
||||
}
|
||||
num = num*10 + int(c-'0')
|
||||
}
|
||||
|
||||
return id[:lastDot], num, true
|
||||
}
|
||||
|
||||
// ValidateIssueIDPrefix validates that an issue ID matches the configured prefix
|
||||
// Supports both top-level (bd-a3f8e9) and hierarchical (bd-a3f8e9.1) IDs
|
||||
func ValidateIssueIDPrefix(id, prefix string) error {
|
||||
@@ -235,6 +262,18 @@ func EnsureIDs(ctx context.Context, conn *sql.Conn, prefix string, issues []*typ
|
||||
// Default to allow for backward compatibility
|
||||
}
|
||||
}
|
||||
|
||||
// Update child_counters to prevent future ID collisions (GH#728 fix)
|
||||
// When explicit child IDs are imported, the counter must be at least the child number
|
||||
// Only update if parent exists (parentCount > 0) - for orphan modes, skip this
|
||||
// The counter will be updated when the parent is actually created/exists
|
||||
if parentCount > 0 {
|
||||
if _, childNum, ok := ParseHierarchicalID(issues[i].ID); ok {
|
||||
if err := ensureChildCounterUpdatedWithConn(ctx, conn, parentID, childNum); err != nil {
|
||||
return fmt.Errorf("failed to update child counter: %w", err)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
usedIDs[issues[i].ID] = true
|
||||
|
||||
@@ -190,6 +190,14 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
|
||||
// Parent(s) not found in JSONL history - cannot proceed
|
||||
return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID)
|
||||
}
|
||||
|
||||
// Update child_counters to prevent future ID collisions (GH#728 fix)
|
||||
// When explicit child IDs are used, the counter must be at least the child number
|
||||
if _, childNum, ok := ParseHierarchicalID(issue.ID); ok {
|
||||
if err := ensureChildCounterUpdatedWithConn(ctx, conn, parentID, childNum); err != nil {
|
||||
return fmt.Errorf("failed to update child counter: %w", err)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user