Commit `cc98ed4532`

cc98ed4532ada8b71a72a0077f706f02c90a00a0

parent: 6a03ea8472

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-11 14:54 UTC

store: migrations opt out of foreign keys with a first-line directive

Rebuilding labels and milestones, which other tables reference, loses
the children's rows with foreign keys on. The runner switches them off
on one pinned connection for a migration whose first line says so,
re-enables them, and checks foreign_key_check afterwards.

Ref #203

docs/specs/2026-09-11-org-labels-milestones-closes-design.md +7 −4

		@@ -61,10 +61,13 @@ Migration 0052 rebuilds `labels` and `milestones` the way 0041 rebuilt
61	61	renamed parent, which would leave the children pointing at `labels_old`.
62	62	The script therefore brackets the renames with `PRAGMA legacy_alter_table
63	63	= ON` and `= OFF`, which a transaction allows; the children keep naming
64		`labels` and `milestones` and bind to the new tables. `foreign_keys` stays
65		on, so the copy is checked and the drop of the old tables cascades
66		nothing, since nothing references them. The migration test runs `PRAGMA
67		foreign_key_check` afterwards and expects no rows.
	64	`labels` and `milestones` and bind to the new tables. The migration
	65	file's first line, `-- foreign_keys: off`, has the migration runner
	66	switch foreign keys off on a pinned connection for that step, because
	67	rebuilding a parent table with children otherwise loses the children's
	68	rows. The runner checks `foreign_key_check` is empty once the step
	69	commits and foreign keys are back on; the migration test asserts it
	70	too.
68	71
69	72	```sql
70	73	CREATE TABLE labels (

internal/store/migrations/0052_org_scope.down.sql +1 −2

		@@ -1,6 +1,6 @@
	1	-- foreign_keys: off
1	2	-- Back to per-repository rows. An org-scoped row has no repository to go
2	3	-- to; the NOT NULL on repo_id refuses the copy, which fails the migration.
3		PRAGMA foreign_keys = OFF;
4	4	PRAGMA legacy_alter_table = ON;
5	5
6	6	ALTER TABLE labels RENAME TO labels_old;
		@@ -31,4 +31,3 @@ INSERT INTO milestones (id, repo_id, title, description, due_date, state, create
31	31	DROP TABLE milestones_old;
32	32
33	33	PRAGMA legacy_alter_table = OFF;
34		PRAGMA foreign_keys = ON;

internal/store/migrations/0052_org_scope.up.sql +7 −2

		@@ -1,14 +1,15 @@
	1	-- foreign_keys: off
1	2	-- Labels and milestones scoped to a repository or to an org (#203).
2	3	-- Exactly one of repo_id and org_id is set. Uniqueness is per scope, as
3	4	-- two partial indexes; the app refuses a repo name the org already holds.
4	5	--
5	6	-- Both tables have children (issue_labels, issues.milestone_id,
6		PRAGMA foreign_keys = OFF;
	7	-- merge_requests.milestone_id). Foreign keys are off for this migration:
	8	-- rebuilding a parent table that children reference loses the children's
	9	-- rows with foreign keys on. legacy_alter_table keeps the children naming
	10	-- labels and milestones through the rename, so they bind to the new
	11	-- tables rather than to labels_old/milestones_old. foreign_key_check
	12	-- afterwards proves the ids line up.
7	13	PRAGMA legacy_alter_table = ON;
8	14
9	15	ALTER TABLE labels RENAME TO labels_old;
		@@ -45,4 +46,3 @@ CREATE UNIQUE INDEX milestones_repo_title ON milestones(repo_id, title) WHERE re
45	46	CREATE UNIQUE INDEX milestones_org_title ON milestones(org_id, title) WHERE org_id IS NOT NULL;
46	47
47	48	PRAGMA legacy_alter_table = OFF;
48		PRAGMA foreign_keys = ON;

internal/store/store.go +70 −11

		@@ -70,8 +70,16 @@ type migration struct {
70	70	name string
71	71	up string
72	72	down string
	73	// upFKOff and downFKOff are true when the up/down script's first line
	74	// is the directive "-- foreign_keys: off".
	75	upFKOff bool
	76	downFKOff bool
73	77	}
74	78
	79	// fkOffDirective, as the first line of a migration script, opts that
	80	// direction out of foreign-key enforcement for its step.
	81	const fkOffDirective = "-- foreign_keys: off"
	82
75	83	func loadMigrations() ([]migration, error) {
76	84	entries, err := fs.ReadDir(migrationFS, "migrations")
77	85	if err != nil {
		@@ -110,10 +118,15 @@ func loadMigrations() ([]migration, error) {
110	118	if err != nil {
111	119	return nil, err
112	120	}
	121	text := string(sqlBytes)
	122	firstLine, _, _ := strings.Cut(text, "\n")
	123	fkOff := strings.TrimSpace(firstLine) == fkOffDirective
113	124	if dir == "up" {
114		m.up = string(sqlBytes)
	125	m.up = text
	126	m.upFKOff = fkOff
115	127	} else {
116		m.down = string(sqlBytes)
	128	m.down = text
	129	m.downFKOff = fkOff
117	130	}
118	131	}
119	132	var ms []migration
		@@ -160,15 +173,40 @@ func (s *Store) migrateTo(target int) error {
160	173	if err != nil {
161	174	return err
162	175	}
163		step := func(sqlText string, newVersion int) error {
164		needsFKOff := strings.Contains(sqlText, "PRAGMA foreign_keys = OFF")
165		if needsFKOff {
166		if _, err := s.DB.Exec("PRAGMA foreign_keys = OFF"); err != nil {
	176	step := func(sqlText string, newVersion int, fkOff bool) error {
	177	if !fkOff {
	178	tx, err := s.DB.Begin()
	179	if err != nil {
	180	return err
	181	}
	182	defer tx.Rollback()
	183	if _, err := tx.Exec(sqlText); err != nil {
167	184	return err
168	185	}
169		defer s.DB.Exec("PRAGMA foreign_keys = ON")
	186	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
	187	return err
	188	}
	189	return tx.Commit()
	190	}
	191
	192	// A script whose first line is "-- foreign_keys: off" rebuilds a
	193	// table that other tables reference (labels, milestones): with
	194	// foreign keys on, the rebuild-by-rename loses the children's
	195	// rows. PRAGMA foreign_keys is a no-op inside a transaction, and
	196	// the pool gives no guarantee that a pragma set on one connection
	197	// is seen by the connection Begin() draws next, so the whole step
	198	// — pragma off, transaction, pragma on, foreign_key_check — runs
	199	// on a single pinned connection.
	200	ctx := context.Background()
	201	conn, err := s.DB.Conn(ctx)
	202	if err != nil {
	203	return err
	204	}
	205	defer conn.Close()
	206	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = OFF"); err != nil {
	207	return err
170	208	}
171		tx, err := s.DB.Begin()
	209	tx, err := conn.BeginTx(ctx, nil)
172	210	if err != nil {
173	211	return err
174	212	}
		@@ -179,18 +217,39 @@ func (s *Store) migrateTo(target int) error {
179	217	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
180	218	return err
181	219	}
182		return tx.Commit()
	220	if err := tx.Commit(); err != nil {
	221	return err
	222	}
	223	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
	224	return err
	225	}
	226	rows, err := conn.QueryContext(ctx, "PRAGMA foreign_key_check")
	227	if err != nil {
	228	return err
	229	}
	230	defer rows.Close()
	231	if rows.Next() {
	232	var table string
	233	var rowid sql.NullInt64
	234	var referredTable string
	235	var fkid int
	236	if err := rows.Scan(&table, &rowid, &referredTable, &fkid); err != nil {
	237	return err
	238	}
	239	return fmt.Errorf("foreign_key_check failed after migration: %s", table)
	240	}
	241	return rows.Err()
183	242	}
184	243	for cur < target {
185	244	m := ms[cur]
186		if err := step(m.up, m.version); err != nil {
	245	if err := step(m.up, m.version, m.upFKOff); err != nil {
187	246	return fmt.Errorf("migration %d up: %w", m.version, err)
188	247	}
189	248	cur = m.version
190	249	}
191	250	for cur > target {
192	251	m := ms[cur-1]
193		if err := step(m.down, m.version-1); err != nil {
	252	if err := step(m.down, m.version-1, m.downFKOff); err != nil {
194	253	return fmt.Errorf("migration %d down: %w", m.version, err)
195	254	}
196	255	cur = m.version - 1

internal/store/store_test.go +27 −7

		@@ -145,13 +145,14 @@ func TestSSHKeyLabel(t *testing.T) {
145	145	}
146	146	}
147	147
148		// Migration 0052 rebuilds labels and milestones with an org scope. The
149		// rebuild renames the old tables; since SQLite 3.26 a rename rewrites the
150		// children's foreign keys to follow it, which would bind them to the *_old
151		// tables. legacy_alter_table keeps the children naming labels and milestones,
152		// which the new tables then are. foreign_keys stays on: nothing references the
153		// *_old tables, so dropping them cascades nothing.
154		// This checks the ids, the memberships and the foreign keys all survive.
	148	// Migration 0052 rebuilds labels and milestones with an org scope. Foreign
	149	// keys are off for the migration: rebuilding a parent table with children
	150	// (issue_labels, issues.milestone_id) otherwise loses the children's rows.
	151	// legacy_alter_table keeps the children naming labels and milestones
	152	// through the rename, so they bind to the new tables rather than to
	153	// labels_old/milestones_old. foreign_key_check afterwards proves the ids
	154	// line up. This checks the ids, the memberships and the foreign keys all
	155	// survive.
155	156	func TestMigration0052KeepsMembershipsAndForeignKeys(t *testing.T) {
156	157	s := open(t)
157	158	if err := s.MigrateTo(51); err != nil {
		@@ -228,3 +229,22 @@ func TestMigration0052KeepsMembershipsAndForeignKeys(t *testing.T) {
228	229	t.Fatalf("label membership after down: %d, %v", n, err)
229	230	}
230	231	}
	232
	233	// Migrating all the way up runs 0052's "-- foreign_keys: off" step on its
	234	// own pinned connection and every other migration's script, which has no
	235	// such directive, on the pool as usual. A fresh query afterwards still
	236	// sees foreign keys on: the pinned connection re-enabled them before
	237	// returning to the pool, and no other connection was ever touched.
	238	func TestMigrationForeignKeysDirective(t *testing.T) {
	239	s := open(t)
	240	if err := s.MigrateUp(); err != nil {
	241	t.Fatal(err)
	242	}
	243	var fk int
	244	if err := s.DB.QueryRow("PRAGMA foreign_keys").Scan(&fk); err != nil {
	245	t.Fatal(err)
	246	}
	247	if fk != 1 {
	248	t.Fatalf("foreign_keys after MigrateUp: %d, want 1", fk)
	249	}
	250	}

Commit cc98ed4532

Commit `cc98ed4532`