diff --git a/src/services/extensions/v2/README.md b/src/services/extensions/v2/README.md index c5f9e2a..8cc1f4f 100644 --- a/src/services/extensions/v2/README.md +++ b/src/services/extensions/v2/README.md @@ -104,15 +104,22 @@ guaranteeing a published row is never half-written. `extension_revisions` (renamed from `extension_submissions` in migration 0021) holds proposed content, always attached to a real extension row and cascading with it. -Migration 0021 rebuilds `extensions`, `extension_revisions` and `developers` -in one step, because SQLite cannot relax `NOT NULL`, add a `CHECK`, or add a -foreign key in place. It also renames `extensions.author_id` to `developer_id` -(nothing public depended on the old name — v1's response field is `author` -either way) and replaces `developers.created_at`/`updated_at`'s placeholder -1970 default with `CURRENT_TIMESTAMP`, which is what every writer already -uses. Existing 1970 values are left alone: they are the only record those rows -have, and a timestamp invented at migration time would look real without being -so. +Migration 0021 rebuilds `extensions` and replaces `extension_submissions` with +`extension_revisions`, because SQLite cannot relax `NOT NULL`, add a `CHECK`, or +add a foreign key in place. It also renames `extensions.author_id` to +`developer_id` — nothing public depended on the old name, since v1's response +field is `author` either way. + +**Ordering in 0021 is load-bearing.** It never drops a table that still has +children, so `extension_submissions` is copied aside and dropped before +`extensions` is rebuilt. Foreign keys cannot be relaxed to avoid this: +`PRAGMA foreign_keys` is a no-op inside a transaction and wrangler wraps each +migration file in one, while `PRAGMA defer_foreign_keys` does not help either — +dropping a parent increments SQLite's deferred-violation counter per child row +and nothing decrements it, so the commit fails even when the data is sound. The +same constraint is why `developers` is not rebuilt: three tables reference it. +`migrations.test.ts` applies the chain under those conditions so this cannot +regress. Apply migrations **only from this repository**, from `db/migrations`, with `npm run db:migrate:extensions-v2:local` / `:remote`. The Extensions site has no D1 migration source. diff --git a/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql b/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql index d47d736..301d2f9 100644 --- a/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql +++ b/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql @@ -7,17 +7,26 @@ -- the first approval), and extension_submissions becomes extension_revisions: -- one proposed content version, always attached to a real extension row. -- --- The tables are rebuilt rather than ALTERed because SQLite cannot relax NOT --- NULL, add a CHECK, or add a foreign key in place. Two renames ride along, --- since the rebuild is already paid for: extensions.author_id becomes --- developer_id, and developers' created_at/updated_at lose the placeholder 1970 --- default that migration 0002 was forced to use and no writer ever produced. +-- extensions is rebuilt rather than ALTERed because SQLite cannot relax NOT +-- NULL, add a CHECK, or add a foreign key in place. author_id becomes +-- developer_id on the way through, since the rebuild is already paid for. -- -- Hand-written, not drizzle-kit-generated: the generated diff cannot infer the -- table rename or the backfills below non-interactively, so only the snapshot -- in meta/0021_snapshot.json comes from drizzle-kit. The end state is verified -- against schema.ts by test/services/extensions/v2/migrations.test.ts. -PRAGMA foreign_keys=OFF;--> statement-breakpoint +-- +-- The ordering below is load-bearing: nothing here drops a table that still +-- has children, which is why extension_submissions is copied aside and dropped +-- before extensions is rebuilt. Neither pragma can buy you out of this. +-- foreign_keys=OFF is a no-op inside a transaction, and wrangler wraps each +-- migration file in one - an earlier version of this file opened with it, +-- passed locally where statements run outside a transaction, and failed the +-- first remote apply. defer_foreign_keys is not a substitute either: DROP +-- TABLE on a parent increments SQLite's deferred-violation counter once per +-- child row and nothing ever decrements it, so COMMIT fails even when +-- foreign_key_check is clean. It is also why developers is not rebuilt here - +-- three tables reference it. See migrations.test.ts's applyAllAsD1(). -- idx_extensions_id_nocase, created further down, is the constraint that stops -- a new lowercase id colliding with an adopted mixed-case one. A catalogue @@ -48,6 +57,22 @@ GROUP BY LOWER(id) HAVING COUNT(*) > 1;--> statement-breakpoint DROP TABLE _extension_id_case_conflicts;--> statement-breakpoint +-- A dangling developer reference would be caught by the real foreign key on +-- the rebuilt table, but as a bare "FOREIGN KEY constraint failed" from the +-- middle of the copy. Check it here, before anything is copied, so the failure +-- names what is wrong and points at the rows. +CREATE TABLE _unresolved_references ( + kind TEXT NOT NULL, + row_id TEXT NOT NULL, + CONSTRAINT extension_references_must_resolve CHECK (1 = 0) +);--> statement-breakpoint + +INSERT INTO _unresolved_references (kind, row_id) +SELECT 'extension.developer_id', e.id FROM extensions e +WHERE NOT EXISTS (SELECT 1 FROM developers d WHERE d.id = e.author_id);--> statement-breakpoint + +DROP TABLE _unresolved_references;--> statement-breakpoint + -- A submission naming a developer that does not exist cannot become an -- extension row: developer_id is NOT NULL with a foreign key. Such a -- submission is already unapprovable today - the pre-0021 approve() only ever @@ -104,59 +129,12 @@ WHERE LOWER(COALESCE(extension_id, json_extract(payload, '$.extension.id'))) DROP TABLE _reserved_submission_targets;--> statement-breakpoint --- developers first, while every table that references it is still the old one: --- the drop-and-rename re-parses every schema, and doing it with a referrer --- pointing at a dropped table is the case that errors. -CREATE TABLE `__new_developers` ( - `id` text PRIMARY KEY NOT NULL, - `type` text NOT NULL, - `name` text NOT NULL, - `url` text, - `owner_user_id` text, - `approved_at` text, - `created_at` text DEFAULT CURRENT_TIMESTAMP NOT NULL, - `updated_at` text DEFAULT CURRENT_TIMESTAMP NOT NULL, - `avatar_url` text, - `contact_email` text, - `ownership_epoch` integer DEFAULT 1 NOT NULL, - `content_revision` integer DEFAULT 1 NOT NULL, - `approved_revision` integer, - `approved_by` text, - `github_org_verified` integer, - `github_verification_note` text, - `github_verified_at` text, - `github_url_verified` integer, - `url_check_cooldown_until` text, - FOREIGN KEY (`owner_user_id`) REFERENCES `users`(`id`) ON UPDATE no action ON DELETE no action, - CONSTRAINT "developers_ownership_epoch_check" CHECK("__new_developers"."ownership_epoch" >= 1), - CONSTRAINT "developers_content_revision_check" CHECK("__new_developers"."content_revision" >= 1), - CONSTRAINT "developers_github_org_verified_check" CHECK("__new_developers"."github_org_verified" IN (0, 1)), - CONSTRAINT "developers_github_url_verified_check" CHECK("__new_developers"."github_url_verified" = 1) -);--> statement-breakpoint - --- Existing 1970 values stay. They are wrong, but they are the only record --- those rows have, and a timestamp invented here would look real without --- being so. -INSERT INTO `__new_developers` ( - id, type, name, url, owner_user_id, approved_at, created_at, updated_at, - avatar_url, contact_email, ownership_epoch, content_revision, - approved_revision, approved_by, github_org_verified, - github_verification_note, github_verified_at, github_url_verified, - url_check_cooldown_until -) -SELECT - id, type, name, url, owner_user_id, approved_at, created_at, updated_at, - avatar_url, contact_email, ownership_epoch, content_revision, - approved_revision, approved_by, github_org_verified, - github_verification_note, github_verified_at, github_url_verified, - url_check_cooldown_until -FROM `developers`;--> statement-breakpoint - -DROP TABLE `developers`;--> statement-breakpoint -ALTER TABLE `__new_developers` RENAME TO `developers`;--> statement-breakpoint +-- extension_submissions is the only table referencing extensions, so it goes +-- first. AS SELECT rather than a declared table: it carries no constraints +-- across, so the holding table survives the rebuild it spans. +CREATE TABLE `_submissions_backup` AS SELECT * FROM `extension_submissions`;--> statement-breakpoint -CREATE UNIQUE INDEX `idx_developers_owner_unique` ON `developers` (`owner_user_id`);--> statement-breakpoint -CREATE INDEX `idx_developers_approved` ON `developers` (`approved_at`);--> statement-breakpoint +DROP TABLE `extension_submissions`;--> statement-breakpoint CREATE TABLE `__new_extensions` ( `id` text PRIMARY KEY NOT NULL, @@ -216,7 +194,7 @@ SELECT target.target_id, ( SELECT s.developer_id - FROM extension_submissions s + FROM _submissions_backup s WHERE LOWER(COALESCE(s.extension_id, json_extract(s.payload, '$.extension.id'))) = target.target_id ORDER BY s.created_at DESC, s.id DESC LIMIT 1 @@ -227,7 +205,7 @@ SELECT FROM ( SELECT DISTINCT LOWER(COALESCE(extension_id, json_extract(payload, '$.extension.id'))) AS target_id - FROM extension_submissions + FROM _submissions_backup ) AS target WHERE target.target_id IS NOT NULL AND NOT EXISTS ( @@ -288,7 +266,7 @@ SELECT s.created_at, s.reviewed_at, s.ownership_epoch -FROM extension_submissions s +FROM _submissions_backup s JOIN extensions e ON LOWER(e.id) = LOWER(COALESCE(s.extension_id, json_extract(s.payload, '$.extension.id'))) -- A payload without an extension object cannot become a revision. This has @@ -327,28 +305,4 @@ CREATE INDEX `idx_extension_revisions_extension_page` ON `extension_revisions` ( CREATE INDEX `idx_extension_revisions_submitter_page` ON `extension_revisions` (`submitted_by`,"created_at" desc,"id" desc);--> statement-breakpoint CREATE INDEX `idx_extension_revisions_queue_page` ON `extension_revisions` (`status`,`created_at`,`id`);--> statement-breakpoint -DROP TABLE `extension_submissions`;--> statement-breakpoint - --- The rebuilds above run with foreign_keys=OFF, which means SQLite does not --- re-validate the copied rows against the new declarations - a pre-existing --- extension pointing at a developer that no longer exists would be carried --- through silently, and every read would then have to defend against it --- forever. Fail the deploy instead, and let the reads assume the join always --- matches. Same CHECK-on-a-scratch-table trick as migration 0020, for the --- same reason: SQLite has no RAISE() outside a trigger. -CREATE TABLE _unresolved_references ( - kind TEXT NOT NULL, - row_id TEXT NOT NULL, - CONSTRAINT extension_references_must_resolve CHECK (1 = 0) -);--> statement-breakpoint - -INSERT INTO _unresolved_references (kind, row_id) -SELECT 'extension.developer_id', e.id FROM extensions e -WHERE NOT EXISTS (SELECT 1 FROM developers d WHERE d.id = e.developer_id) -UNION ALL -SELECT 'revision.extension_id', r.id FROM extension_revisions r -WHERE NOT EXISTS (SELECT 1 FROM extensions e WHERE e.id = r.extension_id);--> statement-breakpoint - -DROP TABLE _unresolved_references;--> statement-breakpoint - -PRAGMA foreign_keys=ON; +DROP TABLE `_submissions_backup`;--> statement-breakpoint diff --git a/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json b/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json index 809b481..cf8531d 100644 --- a/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json +++ b/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json @@ -394,7 +394,7 @@ "primaryKey": false, "notNull": true, "autoincrement": false, - "default": "CURRENT_TIMESTAMP" + "default": "'1970-01-01T00:00:00.000Z'" }, "updated_at": { "name": "updated_at", @@ -402,7 +402,7 @@ "primaryKey": false, "notNull": true, "autoincrement": false, - "default": "CURRENT_TIMESTAMP" + "default": "'1970-01-01T00:00:00.000Z'" }, "avatar_url": { "name": "avatar_url", diff --git a/src/services/extensions/v2/db/schema.ts b/src/services/extensions/v2/db/schema.ts index 8a7aff0..27dd1f1 100644 --- a/src/services/extensions/v2/db/schema.ts +++ b/src/services/extensions/v2/db/schema.ts @@ -125,16 +125,18 @@ export const developers = sqliteTable( url: text("url"), ownerUserId: text("owner_user_id").references(() => users.id), approvedAt: text("approved_at"), - // Migration 0002 could only give these a constant default (SQLite rejects - // non-constant ALTER TABLE ADD COLUMN defaults), so they carried a - // placeholder 1970 epoch that no write ever produced. 0021 rebuilds the - // table and replaces it with the value every writer already uses. - createdAt: text("created_at") - .notNull() - .default(sql`CURRENT_TIMESTAMP`), - updatedAt: text("updated_at") - .notNull() - .default(sql`CURRENT_TIMESTAMP`), + // Placeholder default from migration 0002 (SQLite rejects non-constant + // ALTER TABLE ADD COLUMN defaults). Every write sets this explicitly (see + // db/developer-profiles.ts) - the literal default is never actually read, + // but it is part of the real column definition, so it is kept here for + // baseline-diff fidelity against the existing database. + // + // Replacing it needs a table rebuild, which this table cannot have: three + // tables reference it and a parent cannot be dropped with foreign keys + // enforced (see migration 0021's header). Rebuilding all three children + // too is a lot of risk for a default nothing reads. + createdAt: text("created_at").notNull().default("1970-01-01T00:00:00.000Z"), + updatedAt: text("updated_at").notNull().default("1970-01-01T00:00:00.000Z"), avatarUrl: text("avatar_url"), contactEmail: text("contact_email"), ownershipEpoch: integer("ownership_epoch").notNull().default(1), diff --git a/test/services/extensions/v2/migrations.test.ts b/test/services/extensions/v2/migrations.test.ts index 6089982..8884302 100644 --- a/test/services/extensions/v2/migrations.test.ts +++ b/test/services/extensions/v2/migrations.test.ts @@ -76,6 +76,31 @@ function seedSubmissionFixture(db: DatabaseSync): void { ); } +// Applies the chain the way D1 does, which is not how the other tests here run +// it: foreign keys enforced, and 0021 inside the transaction wrangler wraps +// each migration file in. That combination is what a local apply cannot see, +// and it is what let a broken 0021 reach production. +function applyAllAsD1( + db: DatabaseSync, + seed?: (db: DatabaseSync) => void +): void { + db.exec("PRAGMA foreign_keys = ON;"); + for (const name of migrationNames.filter( + (candidate) => !candidate.startsWith("0021") + )) { + db.exec(migration(name)); + } + seed?.(db); + db.exec("BEGIN"); + try { + db.exec(migration("0021_restructure_extensions_revisions.sql")); + db.exec("COMMIT"); + } catch (error) { + db.exec("ROLLBACK"); + throw error; + } +} + describe("Extensions D1 migrations", () => { it("upgrades the split-owned schema without losing users or domain references", () => { const db = new DatabaseSync(":memory:"); @@ -226,25 +251,6 @@ describe("Extensions D1 migrations", () => { .get("legacy-history") ).toEqual({ changed_by: "legacy-user" }); expect(db.prepare("PRAGMA foreign_key_check").all()).toEqual([]); - - // 0021 rebuilds developers only to replace the placeholder 1970 default - // migration 0002 was forced to use. Rows keep whatever they had - a - // wrong-but-real timestamp beats one invented here - while a new insert - // that omits the column now gets the value every writer already uses. - expect( - db - .prepare("SELECT created_at FROM developers WHERE id = ?") - .get("legacy-developer") - ).toEqual({ created_at: "1970-01-01T00:00:00.000Z" }); - - db.prepare( - "INSERT INTO developers (id, type, name, owner_user_id) VALUES (?,?,?,?)" - ).run("post-migration", "user", "After", null); - const fresh = db - .prepare("SELECT created_at, updated_at FROM developers WHERE id = ?") - .get("post-migration") as { created_at: string; updated_at: string }; - expect(fresh.created_at).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/); - expect(fresh.updated_at).toBe(fresh.created_at); } finally { db.close(); } @@ -300,6 +306,60 @@ describe("Extensions D1 migrations", () => { } }); + // Regression guard for the failed remote apply of 0021: it worked locally, + // where PRAGMA foreign_keys=OFF is honoured, and failed on D1, where it is + // not. Everything else in this file runs statements outside a transaction + // with foreign keys off, which cannot see the difference. + it("applies on D1's terms: foreign keys on, one transaction", () => { + const db = new DatabaseSync(":memory:"); + + try { + applyAllAsD1(db, (seeded) => { + seedSubmissionFixture(seeded); + // extension_id must be set, not null. It is what makes extensions a + // parent with a child row, and so the only thing that makes dropping + // it a foreign key violation - without it this test passes on the very + // ordering it exists to reject. + seeded + .prepare( + `INSERT INTO extension_submissions + (id, extension_id, developer_id, submitted_by, status, payload, target_key) + VALUES (?,?,?,?,?,?,?)` + ) + .run( + "edit-of-live", + "live-ext", + "acme", + "submitter", + "pending", + '{"developer":{"id":"acme"},"extension":{"id":"live-ext","name":"E"}}', + "live-ext" + ); + }); + + // Committed, so the deferred counter reached zero and the data is sound. + expect(db.prepare("PRAGMA foreign_key_check").all()).toEqual([]); + expect( + db + .prepare("SELECT extension_id FROM extension_revisions WHERE id = ?") + .get("edit-of-live") + ).toEqual({ extension_id: "live-ext" }); + expect(db.prepare("SELECT COUNT(*) AS n FROM extensions").get()).toEqual({ + n: 1 + }); + // The holding table used to carry submissions across the rebuild is gone. + expect( + db + .prepare( + "SELECT COUNT(*) AS n FROM sqlite_master WHERE name LIKE '\\_%' ESCAPE '\\'" + ) + .get() + ).toEqual({ n: 0 }); + } finally { + db.close(); + } + }); + // The pre-0021 flow could leave a submission naming a developer that does // not exist. Such a row is already unapprovable - the old approve() only // ever UPDATEd a developer - but it must not vanish without saying so. @@ -510,38 +570,38 @@ describe("Extensions D1 migrations", () => { const db = new DatabaseSync(":memory:"); try { - for (const name of migrationNames.filter( - (candidate) => !candidate.startsWith("0021") - )) { - db.exec(migration(name)); - } - - // Enforcement off, which is exactly how such a row could have come to - // exist before the constraint was there to stop it. - db.exec("PRAGMA foreign_keys = OFF;"); - db.prepare( - `INSERT INTO extensions ( - id, type, author_id, name, description, releases, website, license, - icon_url, readme, source, version, download_url - ) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)` - ).run( - "dangling", - "mod", - "developer-that-never-existed", - "Dangling", - "d", - "[]", - "https://example.com", - '{"name":"MIT"}', - null, - "# d", - '{"type":"github","repo":"example/d"}', - "1.0.0", - "https://example.com/d.zip" - ); - + // Run on D1's terms: the pre-flight check has to fire before the copy, + // because the rebuilt table's real foreign key would otherwise reject + // the row first with a bare, unattributed error. expect(() => - db.exec(migration("0021_restructure_extensions_revisions.sql")) + applyAllAsD1(db, (seeded) => { + // Enforcement off only while seeding, which is how such a row could + // have come to exist before the constraint was there to stop it. + seeded.exec("PRAGMA foreign_keys = OFF;"); + seeded + .prepare( + `INSERT INTO extensions ( + id, type, author_id, name, description, releases, website, + license, icon_url, readme, source, version, download_url + ) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)` + ) + .run( + "dangling", + "mod", + "developer-that-never-existed", + "Dangling", + "d", + "[]", + "https://example.com", + '{"name":"MIT"}', + null, + "# d", + '{"type":"github","repo":"example/d"}', + "1.0.0", + "https://example.com/d.zip" + ); + seeded.exec("PRAGMA foreign_keys = ON;"); + }) ).toThrow(/CHECK constraint failed: extension_references_must_resolve/); } finally { db.close();