dreampiler/kandev
1
0
Fork 0
Code Issues 46 Pull Requests Actions 93 Packages Projects Releases Wiki Activity

fix(github): stop SELECT * scans from breaking on TaskPR schema drift (#1988)

Browse Source
This commit is contained in:
Clément Fiere
2026-07-29 19:52:00 +02:00
committed by GitHub
parent f0b816a89f
commit 3e794ba903
2 changed files with 126 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/internal/github/store.go
+30 -6
View File
@@ -1466,11 +1466,35 @@ func (s *Store) CreateTaskPR(ctx context.Context, tp *TaskPR) error {
return err
}
// taskPRColumns is the explicit column list for every github_task_prs read.
// Using `SELECT *` here is unsafe: sqlx's StructScan errors out
// ("missing destination name X in *github.TaskPR") the moment the table has
// a column the current TaskPR struct doesn't declare a field for. That drift
// is not hypothetical — a self-update that applies a newer release's
// migration (adding a column) followed by a rollback to an older binary
// leaves exactly this mismatch permanently on disk, since SQLite migrations
// are additive and never reverted. Projecting the known columns keeps every
// read working regardless of what the table has picked up beyond them.
const taskPRColumns = `id, workspace_id, task_id, repository_id, owner, repo, pr_number, pr_url,
pr_title, head_branch, base_branch, author_login, state, review_state, checks_state,
mergeable_state, review_count, pending_review_count, required_reviews, comment_count,
unresolved_review_threads, checks_total, checks_passing, additions, deletions,
created_at, merged_at, closed_at, last_synced_at, updated_at`
// taskPRColumnsQualified is taskPRColumns with each column qualified by the
// `gtp` alias, for queries that join github_task_prs against another table.
const taskPRColumnsQualified = `gtp.id, gtp.workspace_id, gtp.task_id, gtp.repository_id, gtp.owner, gtp.repo,
gtp.pr_number, gtp.pr_url, gtp.pr_title, gtp.head_branch, gtp.base_branch, gtp.author_login,
gtp.state, gtp.review_state, gtp.checks_state, gtp.mergeable_state, gtp.review_count,
gtp.pending_review_count, gtp.required_reviews, gtp.comment_count, gtp.unresolved_review_threads,
gtp.checks_total, gtp.checks_passing, gtp.additions, gtp.deletions,
gtp.created_at, gtp.merged_at, gtp.closed_at, gtp.last_synced_at, gtp.updated_at`
// GetTaskPR returns the first PR association for a task. For multi-repo tasks
// the result is non-deterministic across repos — use ListTaskPRsByTask instead.
func (s *Store) GetTaskPR(ctx context.Context, taskID string) (*TaskPR, error) {
var tp TaskPR
err := s.ro.GetContext(ctx, &tp, `SELECT * FROM github_task_prs WHERE task_id = ? LIMIT 1`, taskID)
err := s.ro.GetContext(ctx, &tp, `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? LIMIT 1`, taskID)
if errors.Is(err, sql.ErrNoRows) {
return nil, nil
}
@@ -1487,7 +1511,7 @@ func (s *Store) GetTaskPR(ctx context.Context, taskID string) (*TaskPR, error) {
func (s *Store) GetTaskPRByRepository(ctx context.Context, taskID, repositoryID string) (*TaskPR, error) {
var tp TaskPR
err := s.ro.GetContext(ctx, &tp,
`SELECT * FROM github_task_prs WHERE task_id = ? AND repository_id = ?
`SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? AND repository_id = ?
ORDER BY updated_at DESC LIMIT 1`,
taskID, repositoryID)
if errors.Is(err, sql.ErrNoRows) {
@@ -1504,7 +1528,7 @@ func (s *Store) GetTaskPRByRepository(ctx context.Context, taskID, repositoryID
func (s *Store) GetTaskPRByRepoAndNumber(ctx context.Context, taskID, repositoryID string, prNumber int) (*TaskPR, error) {
var tp TaskPR
err := s.ro.GetContext(ctx, &tp,
`SELECT * FROM github_task_prs
`SELECT `+taskPRColumns+` FROM github_task_prs
WHERE task_id = ? AND repository_id = ? AND pr_number = ? LIMIT 1`,
taskID, repositoryID, prNumber)
if errors.Is(err, sql.ErrNoRows) {
@@ -1518,7 +1542,7 @@ func (s *Store) GetTaskPRByRepoAndNumber(ctx context.Context, taskID, repository
func (s *Store) ListTaskPRsByTask(ctx context.Context, taskID string) ([]*TaskPR, error) {
var prs []TaskPR
if err := s.ro.SelectContext(ctx, &prs,
`SELECT * FROM github_task_prs WHERE task_id = ? ORDER BY created_at ASC`, taskID); err != nil {
`SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? ORDER BY created_at ASC`, taskID); err != nil {
return nil, err
}
out := make([]*TaskPR, 0, len(prs))
@@ -1536,7 +1560,7 @@ func (s *Store) ListTaskPRsByTaskIDs(ctx context.Context, taskIDs []string) (map
return make(map[string][]*TaskPR), nil
}
query, args, err := sqlx.In(
`SELECT * FROM github_task_prs WHERE task_id IN (?) ORDER BY created_at ASC`,
`SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id IN (?) ORDER BY created_at ASC`,
taskIDs,
)
if err != nil {
@@ -1556,7 +1580,7 @@ func (s *Store) ListTaskPRsByTaskIDs(ctx context.Context, taskIDs []string) (map
func (s *Store) ListTaskPRsByWorkspaceID(ctx context.Context, workspaceID string) (map[string][]*TaskPR, error) {
var prs []TaskPR
if err := s.ro.SelectContext(ctx, &prs,
`SELECT gtp.* FROM github_task_prs gtp
`SELECT `+taskPRColumnsQualified+` FROM github_task_prs gtp
INNER JOIN tasks t ON gtp.task_id = t.id
WHERE t.workspace_id = ?
ORDER BY gtp.created_at ASC`, workspaceID); err != nil {
apps/backend/internal/github/store_taskpr_schema_drift_test.go
+96
View File
@@ -0,0 +1,96 @@
package github
import (
"context"
"testing"
"time"
)
// TestTaskPRReads_ToleratesExtraColumn locks in resilience against a
// specific production incident: the "Link GitHub pull request" dialog
// failed with "associate PR with task: missing destination name workspace_id
// in *github.TaskPR" for a task in a workspace whose github_task_prs table
// had picked up a column (via a schema migration from a newer release, e.g.
// a self-update that was later rolled back) that the running binary's
// TaskPR struct doesn't declare. sqlx's StructScan errors out on any SELECT
// * column with no matching destination field, so every TaskPR read query
// must project an explicit column list rather than `SELECT *` /
// `SELECT gtp.*` — otherwise ANY future schema drift ahead of the binary
// breaks every read path, not just the one column that happened to trigger
// the original report.
func TestTaskPRReads_ToleratesExtraColumn(t *testing.T) {
store := newTestStore(t)
ctx := context.Background()
// Simulate schema drift: a column present in the table that the
// current TaskPR struct has no field for.
if _, err := store.db.Exec(
`ALTER TABLE github_task_prs ADD COLUMN future_only_column TEXT NOT NULL DEFAULT ''`,
); err != nil {
t.Fatalf("simulate schema drift: %v", err)
}
if _, err := store.db.Exec(
`INSERT INTO tasks (id, workspace_id) VALUES ('task-drift', 'ws-drift')`,
); err != nil {
t.Fatalf("seed task: %v", err)
}
tp := &TaskPR{
WorkspaceID: "ws-drift",
TaskID: "task-drift",
Owner: "kdlbs",
Repo: "kandev",
PRNumber: 1978,
PRURL: "https://github.com/kdlbs/kandev/pull/1978",
PRTitle: "drifted schema",
State: "open",
CreatedAt: time.Now().UTC(),
}
if err := store.ReplaceTaskPR(ctx, tp); err != nil {
t.Fatalf("ReplaceTaskPR: %v", err)
}
fatalOnScanError := func(name string, err error) {
t.Helper()
if err == nil {
return
}
t.Fatalf("%s: SELECT scan broke on schema drift: %v", name, err)
}
got, err := store.GetTaskPR(ctx, "task-drift")
fatalOnScanError("GetTaskPR", err)
if got == nil || got.PRNumber != 1978 || got.WorkspaceID != "ws-drift" {
t.Fatalf("GetTaskPR: expected PR #1978 in ws-drift, got %+v", got)
}
gotByRepo, err := store.GetTaskPRByRepository(ctx, "task-drift", "")
fatalOnScanError("GetTaskPRByRepository", err)
if gotByRepo == nil || gotByRepo.PRNumber != 1978 {
t.Fatalf("GetTaskPRByRepository: expected PR #1978, got %+v", gotByRepo)
}
gotByNumber, err := store.GetTaskPRByRepoAndNumber(ctx, "task-drift", "", 1978)
fatalOnScanError("GetTaskPRByRepoAndNumber", err)
if gotByNumber == nil || gotByNumber.PRTitle != "drifted schema" {
t.Fatalf("GetTaskPRByRepoAndNumber: expected the drifted-schema PR, got %+v", gotByNumber)
}
list, err := store.ListTaskPRsByTask(ctx, "task-drift")
fatalOnScanError("ListTaskPRsByTask", err)
if len(list) != 1 || list[0].PRNumber != 1978 {
t.Fatalf("ListTaskPRsByTask: expected 1 PR (#1978), got %+v", list)
}
byIDs, err := store.ListTaskPRsByTaskIDs(ctx, []string{"task-drift"})
fatalOnScanError("ListTaskPRsByTaskIDs", err)
if len(byIDs["task-drift"]) != 1 || byIDs["task-drift"][0].PRNumber != 1978 {
t.Fatalf("ListTaskPRsByTaskIDs: expected 1 PR (#1978) for task-drift, got %+v", byIDs)
}
byWorkspace, err := store.ListTaskPRsByWorkspaceID(ctx, "ws-drift")
fatalOnScanError("ListTaskPRsByWorkspaceID", err)
if len(byWorkspace["task-drift"]) != 1 || byWorkspace["task-drift"][0].PRNumber != 1978 {
t.Fatalf("ListTaskPRsByWorkspaceID: expected 1 PR (#1978) for task-drift, got %+v", byWorkspace)
}
}