fix(sqlite): prevent FK constraint failure on batch create (GH#956)
Add checkForExistingIDs check to transaction-based batch creation (sqliteTxStorage.CreateIssues) before calling insertIssues. This prevents INSERT OR IGNORE from silently skipping duplicate IDs, which would cause FK constraint failures when recording events for issues that weren't actually inserted. Also fixes unrelated test bug: renamed parseCommaSeparated to parseCommaSeparatedList in validators_test.go. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -6,6 +6,7 @@ import (
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/steveyegge/beads/internal/storage"
|
||||
"github.com/steveyegge/beads/internal/types"
|
||||
)
|
||||
|
||||
@@ -596,3 +597,92 @@ func TestDefensiveClosedAtFix(t *testing.T) {
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// TestGH956_BatchCreateExistingID tests that batch creation properly rejects
|
||||
// issues with IDs that already exist in the database, preventing FK constraint
|
||||
// failures when recording events for issues that were silently skipped by
|
||||
// INSERT OR IGNORE.
|
||||
func TestGH956_BatchCreateExistingID(t *testing.T) {
|
||||
s, cleanup := setupTestDB(t)
|
||||
defer cleanup()
|
||||
ctx := context.Background()
|
||||
|
||||
// Create an existing issue first
|
||||
existingIssue := &types.Issue{
|
||||
Title: "Existing Issue",
|
||||
Priority: 1,
|
||||
IssueType: types.TypeTask,
|
||||
Status: types.StatusOpen,
|
||||
}
|
||||
err := s.CreateIssue(ctx, existingIssue, "test")
|
||||
if err != nil {
|
||||
t.Fatalf("failed to create existing issue: %v", err)
|
||||
}
|
||||
|
||||
t.Run("rejects batch with duplicate existing ID", func(t *testing.T) {
|
||||
// Try to create a batch where one issue has an ID that already exists
|
||||
issues := []*types.Issue{
|
||||
{
|
||||
Title: "New Issue 1",
|
||||
Priority: 1,
|
||||
IssueType: types.TypeTask,
|
||||
Status: types.StatusOpen,
|
||||
},
|
||||
{
|
||||
ID: existingIssue.ID, // This ID already exists!
|
||||
Title: "Duplicate Issue",
|
||||
Priority: 1,
|
||||
IssueType: types.TypeTask,
|
||||
Status: types.StatusOpen,
|
||||
},
|
||||
}
|
||||
|
||||
err := s.CreateIssues(ctx, issues, "test")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for duplicate ID, got nil")
|
||||
}
|
||||
|
||||
// The error should indicate the ID already exists, not be a FK constraint error
|
||||
errStr := err.Error()
|
||||
if !strings.Contains(errStr, "already exists") {
|
||||
t.Errorf("expected error to contain 'already exists', got: %s", errStr)
|
||||
}
|
||||
if strings.Contains(errStr, "FOREIGN KEY") {
|
||||
t.Errorf("should not be a FK constraint error, got: %s", errStr)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("transaction batch with duplicate existing ID", func(t *testing.T) {
|
||||
// Test the transaction-based batch creation path (sqliteTxStorage.CreateIssues)
|
||||
err := s.RunInTransaction(ctx, func(tx storage.Transaction) error {
|
||||
issues := []*types.Issue{
|
||||
{
|
||||
Title: "Tx New Issue",
|
||||
Priority: 1,
|
||||
IssueType: types.TypeTask,
|
||||
Status: types.StatusOpen,
|
||||
},
|
||||
{
|
||||
ID: existingIssue.ID, // This ID already exists!
|
||||
Title: "Tx Duplicate Issue",
|
||||
Priority: 1,
|
||||
IssueType: types.TypeTask,
|
||||
Status: types.StatusOpen,
|
||||
},
|
||||
}
|
||||
return tx.CreateIssues(ctx, issues, "test")
|
||||
})
|
||||
if err == nil {
|
||||
t.Fatal("expected error for duplicate ID in transaction, got nil")
|
||||
}
|
||||
|
||||
// The error should indicate the ID already exists, not be a FK constraint error
|
||||
errStr := err.Error()
|
||||
if !strings.Contains(errStr, "already exists") {
|
||||
t.Errorf("expected error to contain 'already exists', got: %s", errStr)
|
||||
}
|
||||
if strings.Contains(errStr, "FOREIGN KEY") {
|
||||
t.Errorf("should not be a FK constraint error, got: %s", errStr)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -296,6 +296,13 @@ func (t *sqliteTxStorage) CreateIssues(ctx context.Context, issues []*types.Issu
|
||||
seenIDs[issue.ID] = true
|
||||
}
|
||||
|
||||
// GH#956: Check for conflicts with existing IDs in database before inserting.
|
||||
// This prevents INSERT OR IGNORE from silently skipping duplicates, which would
|
||||
// cause FK constraint failures when recording events for non-inserted issues.
|
||||
if err := checkForExistingIDs(ctx, t.conn, issues); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Insert all issues
|
||||
if err := insertIssues(ctx, t.conn, issues); err != nil {
|
||||
return fmt.Errorf("failed to insert issues: %w", err)
|
||||
|
||||
Reference in New Issue
Block a user