fix(orphan): handle prefixes with dots in orphan detection (GH#508)
The orphan detection was incorrectly flagging issues with dots in their
prefix (e.g., "my.project-abc123") as orphans because it was looking for
any dot in the ID, treating everything before the first dot as the
parent ID.
The fix:
- Add IsHierarchicalID() helper that correctly detects hierarchical IDs
by checking if the ID ends with .{digits} (e.g., "bd-abc.1")
- Update SQL query in orphan detection migration to use GLOB patterns
that only match IDs ending with numeric suffixes
- Update all Go code that checks for hierarchical IDs to use the new
helper function
Test cases added:
- Unit tests for IsHierarchicalID covering normal, dotted prefix, and
edge cases
- Integration test verifying dotted prefixes do not trigger false
positives
Fixes: #508
This commit is contained in:
@@ -73,6 +73,38 @@ func isValidHex(s string) bool {
|
|||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// IsHierarchicalID checks if an issue ID is hierarchical (has a parent).
|
||||||
|
// Hierarchical IDs have the format {parentID}.{N} where N is a numeric child suffix.
|
||||||
|
// Returns true and the parent ID if hierarchical, false and empty string otherwise.
|
||||||
|
//
|
||||||
|
// This correctly handles prefixes that contain dots (e.g., "my.project-abc123"
|
||||||
|
// is NOT hierarchical, but "my.project-abc123.1" IS hierarchical with parent
|
||||||
|
// "my.project-abc123").
|
||||||
|
//
|
||||||
|
// The key insight is that hierarchical IDs always end with .{digits} where
|
||||||
|
// the digits represent the child number (1, 2, 3, etc.).
|
||||||
|
func IsHierarchicalID(id string) (isHierarchical bool, parentID string) {
|
||||||
|
lastDot := strings.LastIndex(id, ".")
|
||||||
|
if lastDot == -1 {
|
||||||
|
return false, ""
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check if the suffix after the last dot is purely numeric
|
||||||
|
suffix := id[lastDot+1:]
|
||||||
|
if len(suffix) == 0 {
|
||||||
|
return false, ""
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, c := range suffix {
|
||||||
|
if c < '0' || c > '9' {
|
||||||
|
return false, ""
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// It's hierarchical - parent is everything before the last dot
|
||||||
|
return true, id[:lastDot]
|
||||||
|
}
|
||||||
|
|
||||||
// ValidateIssueIDPrefix validates that an issue ID matches the configured prefix
|
// ValidateIssueIDPrefix validates that an issue ID matches the configured prefix
|
||||||
// Supports both top-level (bd-a3f8e9) and hierarchical (bd-a3f8e9.1) IDs
|
// Supports both top-level (bd-a3f8e9) and hierarchical (bd-a3f8e9.1) IDs
|
||||||
func ValidateIssueIDPrefix(id, prefix string) error {
|
func ValidateIssueIDPrefix(id, prefix string) error {
|
||||||
@@ -203,11 +235,8 @@ func EnsureIDs(ctx context.Context, conn *sql.Conn, prefix string, issues []*typ
|
|||||||
}
|
}
|
||||||
|
|
||||||
// For hierarchical IDs (bd-a3f8e9.1), ensure parent exists
|
// For hierarchical IDs (bd-a3f8e9.1), ensure parent exists
|
||||||
if strings.Contains(issues[i].ID, ".") {
|
// Use IsHierarchicalID to correctly handle prefixes with dots (GH#508)
|
||||||
// Extract parent ID (everything before the last dot)
|
if isHierarchical, parentID := IsHierarchicalID(issues[i].ID); isHierarchical {
|
||||||
lastDot := strings.LastIndex(issues[i].ID, ".")
|
|
||||||
parentID := issues[i].ID[:lastDot]
|
|
||||||
|
|
||||||
var parentCount int
|
var parentCount int
|
||||||
err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&parentCount)
|
err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&parentCount)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
134
internal/storage/sqlite/ids_test.go
Normal file
134
internal/storage/sqlite/ids_test.go
Normal file
@@ -0,0 +1,134 @@
|
|||||||
|
package sqlite
|
||||||
|
|
||||||
|
import "testing"
|
||||||
|
|
||||||
|
// TestIsHierarchicalID tests the IsHierarchicalID function which detects
|
||||||
|
// if an issue ID is hierarchical (has a parent) based on the .N suffix pattern.
|
||||||
|
// This test covers the fix for GH#508 where prefixes with dots were incorrectly
|
||||||
|
// flagged as hierarchical.
|
||||||
|
func TestIsHierarchicalID(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
id string
|
||||||
|
wantHierarchical bool
|
||||||
|
wantParentID string
|
||||||
|
}{
|
||||||
|
// Standard hierarchical IDs
|
||||||
|
{
|
||||||
|
name: "simple child .1",
|
||||||
|
id: "bd-abc123.1",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "bd-abc123",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "child .2",
|
||||||
|
id: "bd-xyz789.2",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "bd-xyz789",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "multi-digit child .10",
|
||||||
|
id: "bd-test.10",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "bd-test",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "large child number .999",
|
||||||
|
id: "bd-issue.999",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "bd-issue",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "nested hierarchical",
|
||||||
|
id: "bd-parent.1.2",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "bd-parent.1",
|
||||||
|
},
|
||||||
|
|
||||||
|
// Non-hierarchical IDs (no suffix or non-numeric suffix)
|
||||||
|
{
|
||||||
|
name: "simple top-level",
|
||||||
|
id: "bd-abc123",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "no dot at all",
|
||||||
|
id: "test-issue",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
|
||||||
|
// GH#508: Prefixes with dots should NOT be detected as hierarchical
|
||||||
|
{
|
||||||
|
name: "prefix with dot - my.project",
|
||||||
|
id: "my.project-abc123",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "prefix with multiple dots",
|
||||||
|
id: "com.example.app-issue1",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "prefix with dot AND hierarchical child",
|
||||||
|
id: "my.project-abc123.1",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "my.project-abc123",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "complex prefix with hierarchical",
|
||||||
|
id: "com.example.app-xyz.5",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "com.example.app-xyz",
|
||||||
|
},
|
||||||
|
|
||||||
|
// Edge cases
|
||||||
|
{
|
||||||
|
name: "dot but non-numeric suffix",
|
||||||
|
id: "bd-abc.def",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "mixed suffix (starts with digit)",
|
||||||
|
id: "bd-test.1abc",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "trailing dot only",
|
||||||
|
id: "bd-test.",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "empty after dot",
|
||||||
|
id: "bd-test.",
|
||||||
|
wantHierarchical: false,
|
||||||
|
wantParentID: "",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "child 0",
|
||||||
|
id: "bd-parent.0",
|
||||||
|
wantHierarchical: true,
|
||||||
|
wantParentID: "bd-parent",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
gotHierarchical, gotParentID := IsHierarchicalID(tt.id)
|
||||||
|
if gotHierarchical != tt.wantHierarchical {
|
||||||
|
t.Errorf("IsHierarchicalID(%q) hierarchical = %v, want %v",
|
||||||
|
tt.id, gotHierarchical, tt.wantHierarchical)
|
||||||
|
}
|
||||||
|
if gotParentID != tt.wantParentID {
|
||||||
|
t.Errorf("IsHierarchicalID(%q) parentID = %q, want %q",
|
||||||
|
tt.id, gotParentID, tt.wantParentID)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -7,23 +7,36 @@ import (
|
|||||||
)
|
)
|
||||||
|
|
||||||
// MigrateOrphanDetection detects orphaned child issues and logs them for user action
|
// MigrateOrphanDetection detects orphaned child issues and logs them for user action
|
||||||
// Orphaned children are issues with hierarchical IDs (e.g., "parent.child") where the
|
// Orphaned children are issues with hierarchical IDs (e.g., "parent.1") where the
|
||||||
// parent issue no longer exists in the database.
|
// parent issue no longer exists in the database.
|
||||||
//
|
//
|
||||||
|
// Hierarchical IDs have the format {parentID}.{N} where N is a numeric child suffix.
|
||||||
|
// This correctly handles prefixes that contain dots (e.g., "my.project-abc123" is NOT
|
||||||
|
// hierarchical, but "my.project-abc123.1" IS hierarchical). See GH#508.
|
||||||
|
//
|
||||||
// This migration does NOT automatically delete or convert orphans - it only logs them
|
// This migration does NOT automatically delete or convert orphans - it only logs them
|
||||||
// so the user can decide whether to:
|
// so the user can decide whether to:
|
||||||
// - Delete the orphans if they're no longer needed
|
// - Delete the orphans if they're no longer needed
|
||||||
// - Convert them to top-level issues by renaming them
|
// - Convert them to top-level issues by renaming them
|
||||||
// - Restore the missing parent issues
|
// - Restore the missing parent issues
|
||||||
func MigrateOrphanDetection(db *sql.DB) error {
|
func MigrateOrphanDetection(db *sql.DB) error {
|
||||||
// Query for orphaned children using the pattern from the issue description:
|
// Query for orphaned children:
|
||||||
// SELECT id FROM issues WHERE id LIKE '%.%'
|
// - Must end with .N where N is 1-4 digits (covers child numbers 0-9999)
|
||||||
// AND substr(id, 1, instr(id || '.', '.') - 1) NOT IN (SELECT id FROM issues)
|
// - Parent (everything before the last .N) must not exist in issues table
|
||||||
|
// - Uses GLOB patterns to ensure suffix is purely numeric
|
||||||
|
// - rtrim removes trailing digits, then trailing dot, to get parent ID
|
||||||
|
//
|
||||||
|
// GH#508: The old query used instr() to find the first dot, which incorrectly
|
||||||
|
// flagged IDs with dots in the prefix (e.g., "my.project-abc") as orphans.
|
||||||
|
// The fix uses GLOB patterns to only match IDs ending with .{digits}.
|
||||||
rows, err := db.Query(`
|
rows, err := db.Query(`
|
||||||
SELECT id
|
SELECT id
|
||||||
FROM issues
|
FROM issues
|
||||||
WHERE id LIKE '%.%'
|
WHERE
|
||||||
AND substr(id, 1, instr(id || '.', '.') - 1) NOT IN (SELECT id FROM issues)
|
-- Must end with .N where N is 1-4 digits (child number suffix)
|
||||||
|
(id GLOB '*.[0-9]' OR id GLOB '*.[0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9][0-9]')
|
||||||
|
-- Parent (remove trailing digits then dot) must not exist
|
||||||
|
AND rtrim(rtrim(id, '0123456789'), '.') NOT IN (SELECT id FROM issues)
|
||||||
ORDER BY id
|
ORDER BY id
|
||||||
`)
|
`)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@@ -620,4 +620,83 @@ func TestMigrateOrphanDetection(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
// GH#508: Verify that prefixes with dots don't trigger false positives
|
||||||
|
t.Run("prefix with dots is not flagged as orphan", func(t *testing.T) {
|
||||||
|
store, cleanup := setupTestDB(t)
|
||||||
|
defer cleanup()
|
||||||
|
db := store.db
|
||||||
|
|
||||||
|
// Override the prefix for this test
|
||||||
|
_, err := db.Exec(`UPDATE config SET value = 'my.project' WHERE key = 'issue_prefix'`)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("failed to update prefix: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Insert issues with dotted prefix directly (bypassing prefix validation)
|
||||||
|
testCases := []struct {
|
||||||
|
id string
|
||||||
|
expectOrphan bool
|
||||||
|
}{
|
||||||
|
// These should NOT be flagged as orphans (dots in prefix)
|
||||||
|
{"my.project-abc123", false},
|
||||||
|
{"my.project-xyz789", false},
|
||||||
|
{"com.example.app-issue1", false},
|
||||||
|
|
||||||
|
// This SHOULD be flagged as orphan (hierarchical, parent doesn't exist)
|
||||||
|
{"my.project-missing.1", true},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tc := range testCases {
|
||||||
|
_, err := db.Exec(`
|
||||||
|
INSERT INTO issues (id, title, status, priority, issue_type, created_at, updated_at)
|
||||||
|
VALUES (?, ?, ?, ?, ?, datetime('now'), datetime('now'))
|
||||||
|
`, tc.id, "Test Issue", "open", 1, "task")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("failed to insert %s: %v", tc.id, err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Query for orphans using the same logic as the migration
|
||||||
|
rows, err := db.Query(`
|
||||||
|
SELECT id
|
||||||
|
FROM issues
|
||||||
|
WHERE
|
||||||
|
(id GLOB '*.[0-9]' OR id GLOB '*.[0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9][0-9]')
|
||||||
|
AND rtrim(rtrim(id, '0123456789'), '.') NOT IN (SELECT id FROM issues)
|
||||||
|
ORDER BY id
|
||||||
|
`)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("query failed: %v", err)
|
||||||
|
}
|
||||||
|
defer rows.Close()
|
||||||
|
|
||||||
|
var orphans []string
|
||||||
|
for rows.Next() {
|
||||||
|
var id string
|
||||||
|
if err := rows.Scan(&id); err != nil {
|
||||||
|
t.Fatalf("scan failed: %v", err)
|
||||||
|
}
|
||||||
|
orphans = append(orphans, id)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify only the expected orphan is detected
|
||||||
|
if len(orphans) != 1 {
|
||||||
|
t.Errorf("expected 1 orphan, got %d: %v", len(orphans), orphans)
|
||||||
|
}
|
||||||
|
if len(orphans) == 1 && orphans[0] != "my.project-missing.1" {
|
||||||
|
t.Errorf("expected orphan 'my.project-missing.1', got %q", orphans[0])
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify non-hierarchical dotted IDs are NOT flagged
|
||||||
|
for _, tc := range testCases {
|
||||||
|
if !tc.expectOrphan {
|
||||||
|
for _, orphan := range orphans {
|
||||||
|
if orphan == tc.id {
|
||||||
|
t.Errorf("false positive: %s was incorrectly flagged as orphan", tc.id)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -122,7 +122,8 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
|
|||||||
}
|
}
|
||||||
|
|
||||||
// For hierarchical IDs (bd-a3f8e9.1), ensure parent exists
|
// For hierarchical IDs (bd-a3f8e9.1), ensure parent exists
|
||||||
if strings.Contains(issue.ID, ".") {
|
// Use IsHierarchicalID to correctly handle prefixes with dots (GH#508)
|
||||||
|
if isHierarchical, parentID := IsHierarchicalID(issue.ID); isHierarchical {
|
||||||
// Try to resurrect entire parent chain if any parents are missing
|
// Try to resurrect entire parent chain if any parents are missing
|
||||||
// Use the conn-based version to participate in the same transaction
|
// Use the conn-based version to participate in the same transaction
|
||||||
resurrected, err := s.tryResurrectParentChainWithConn(ctx, conn, issue.ID)
|
resurrected, err := s.tryResurrectParentChainWithConn(ctx, conn, issue.ID)
|
||||||
@@ -131,8 +132,6 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
|
|||||||
}
|
}
|
||||||
if !resurrected {
|
if !resurrected {
|
||||||
// Parent(s) not found in JSONL history - cannot proceed
|
// Parent(s) not found in JSONL history - cannot proceed
|
||||||
lastDot := strings.LastIndex(issue.ID, ".")
|
|
||||||
parentID := issue.ID[:lastDot]
|
|
||||||
return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID)
|
return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -137,7 +137,8 @@ func (t *sqliteTxStorage) CreateIssue(ctx context.Context, issue *types.Issue, a
|
|||||||
}
|
}
|
||||||
|
|
||||||
// For hierarchical IDs (bd-a3f8e9.1), ensure parent exists
|
// For hierarchical IDs (bd-a3f8e9.1), ensure parent exists
|
||||||
if strings.Contains(issue.ID, ".") {
|
// Use IsHierarchicalID to correctly handle prefixes with dots (GH#508)
|
||||||
|
if isHierarchical, parentID := IsHierarchicalID(issue.ID); isHierarchical {
|
||||||
// Try to resurrect entire parent chain if any parents are missing
|
// Try to resurrect entire parent chain if any parents are missing
|
||||||
resurrected, err := t.parent.tryResurrectParentChainWithConn(ctx, t.conn, issue.ID)
|
resurrected, err := t.parent.tryResurrectParentChainWithConn(ctx, t.conn, issue.ID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -145,8 +146,6 @@ func (t *sqliteTxStorage) CreateIssue(ctx context.Context, issue *types.Issue, a
|
|||||||
}
|
}
|
||||||
if !resurrected {
|
if !resurrected {
|
||||||
// Parent(s) not found in JSONL history - cannot proceed
|
// Parent(s) not found in JSONL history - cannot proceed
|
||||||
lastDot := strings.LastIndex(issue.ID, ".")
|
|
||||||
parentID := issue.ID[:lastDot]
|
|
||||||
return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID)
|
return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user