From e26ed3aa9c3e7ce474638fd1a50b8549bd798da9 Mon Sep 17 00:00:00 2001 From: ARUNAVO RAY Date: Sun, 15 Mar 2026 14:10:06 +0530 Subject: [PATCH] fix: rewrite migration 0009 for SQLite compatibility and add migration validation (#230) SQLite rejects ALTER TABLE ADD COLUMN with expression defaults like DEFAULT (unixepoch()), which Drizzle-kit generated for the imported_at column. This broke upgrades from v3.12.x to v3.13.0 (#228, #229). Changes: - Rewrite migration 0009 using table-recreation pattern (CREATE, INSERT SELECT, DROP, RENAME) instead of ALTER TABLE - Add migration validation script with SQLite-specific lint rules that catch known invalid patterns before they ship - Add upgrade-path testing with seeded data and verification fixtures - Add runtime repair for users whose migration record may be stale - Add explicit migration validation step to CI workflow Fixes #228 Fixes #229 --- .github/workflows/astro-build-test.yml | 6 + drizzle/0009_nervous_tyger_tiger.sql | 171 +++++++++++++++++--- package.json | 1 + scripts/validate-migrations.ts | 212 +++++++++++++++++++++++++ src/lib/db/index.ts | 48 +++++- src/lib/db/migrations.test.ts | 26 +++ 6 files changed, 439 insertions(+), 25 deletions(-) create mode 100644 scripts/validate-migrations.ts create mode 100644 src/lib/db/migrations.test.ts diff --git a/.github/workflows/astro-build-test.yml b/.github/workflows/astro-build-test.yml index 13dd0a9..9701d1d 100644 --- a/.github/workflows/astro-build-test.yml +++ b/.github/workflows/astro-build-test.yml @@ -48,6 +48,12 @@ jobs: - name: Run tests run: bun test --coverage + + - name: Check Drizzle migrations + run: bun run db:check + + - name: Validate migrations (SQLite lint + upgrade path) + run: bun test:migrations - name: Build Astro project run: bunx --bun astro build diff --git a/drizzle/0009_nervous_tyger_tiger.sql b/drizzle/0009_nervous_tyger_tiger.sql index 98da8d7..3326905 100644 --- a/drizzle/0009_nervous_tyger_tiger.sql +++ b/drizzle/0009_nervous_tyger_tiger.sql @@ -1,24 +1,149 @@ -ALTER TABLE `repositories` ADD `imported_at` integer DEFAULT (unixepoch()) NOT NULL;--> statement-breakpoint -UPDATE `repositories` -SET `imported_at` = COALESCE( - ( - SELECT MIN(`mj`.`timestamp`) - FROM `mirror_jobs` `mj` - WHERE `mj`.`user_id` = `repositories`.`user_id` - AND `mj`.`status` = 'imported' - AND ( - (`mj`.`repository_id` IS NOT NULL AND `mj`.`repository_id` = `repositories`.`id`) - OR ( - `mj`.`repository_id` IS NULL - AND `mj`.`repository_name` IS NOT NULL - AND ( - lower(trim(`mj`.`repository_name`)) = `repositories`.`normalized_full_name` - OR lower(trim(`mj`.`repository_name`)) = lower(trim(`repositories`.`name`)) - ) - ) - ) - ), - `repositories`.`created_at`, - `imported_at` -);--> statement-breakpoint +CREATE TABLE `__new_repositories` ( + `id` text PRIMARY KEY NOT NULL, + `user_id` text NOT NULL, + `config_id` text NOT NULL, + `name` text NOT NULL, + `full_name` text NOT NULL, + `normalized_full_name` text NOT NULL, + `url` text NOT NULL, + `clone_url` text NOT NULL, + `owner` text NOT NULL, + `organization` text, + `mirrored_location` text DEFAULT '', + `is_private` integer DEFAULT false NOT NULL, + `is_fork` integer DEFAULT false NOT NULL, + `forked_from` text, + `has_issues` integer DEFAULT false NOT NULL, + `is_starred` integer DEFAULT false NOT NULL, + `is_archived` integer DEFAULT false NOT NULL, + `size` integer DEFAULT 0 NOT NULL, + `has_lfs` integer DEFAULT false NOT NULL, + `has_submodules` integer DEFAULT false NOT NULL, + `language` text, + `description` text, + `default_branch` text NOT NULL, + `visibility` text DEFAULT 'public' NOT NULL, + `status` text DEFAULT 'imported' NOT NULL, + `last_mirrored` integer, + `error_message` text, + `destination_org` text, + `metadata` text, + `imported_at` integer DEFAULT (unixepoch()) NOT NULL, + `created_at` integer DEFAULT (unixepoch()) NOT NULL, + `updated_at` integer DEFAULT (unixepoch()) NOT NULL, + FOREIGN KEY (`user_id`) REFERENCES `users`(`id`) ON UPDATE no action ON DELETE no action, + FOREIGN KEY (`config_id`) REFERENCES `configs`(`id`) ON UPDATE no action ON DELETE no action +); +--> statement-breakpoint +INSERT INTO `__new_repositories` ( + `id`, + `user_id`, + `config_id`, + `name`, + `full_name`, + `normalized_full_name`, + `url`, + `clone_url`, + `owner`, + `organization`, + `mirrored_location`, + `is_private`, + `is_fork`, + `forked_from`, + `has_issues`, + `is_starred`, + `is_archived`, + `size`, + `has_lfs`, + `has_submodules`, + `language`, + `description`, + `default_branch`, + `visibility`, + `status`, + `last_mirrored`, + `error_message`, + `destination_org`, + `metadata`, + `imported_at`, + `created_at`, + `updated_at` +) +SELECT + `repositories`.`id`, + `repositories`.`user_id`, + `repositories`.`config_id`, + `repositories`.`name`, + `repositories`.`full_name`, + `repositories`.`normalized_full_name`, + `repositories`.`url`, + `repositories`.`clone_url`, + `repositories`.`owner`, + `repositories`.`organization`, + `repositories`.`mirrored_location`, + `repositories`.`is_private`, + `repositories`.`is_fork`, + `repositories`.`forked_from`, + `repositories`.`has_issues`, + `repositories`.`is_starred`, + `repositories`.`is_archived`, + `repositories`.`size`, + `repositories`.`has_lfs`, + `repositories`.`has_submodules`, + `repositories`.`language`, + `repositories`.`description`, + `repositories`.`default_branch`, + `repositories`.`visibility`, + `repositories`.`status`, + `repositories`.`last_mirrored`, + `repositories`.`error_message`, + `repositories`.`destination_org`, + `repositories`.`metadata`, + COALESCE( + ( + SELECT MIN(`mj`.`timestamp`) + FROM `mirror_jobs` `mj` + WHERE `mj`.`user_id` = `repositories`.`user_id` + AND `mj`.`status` = 'imported' + AND ( + (`mj`.`repository_id` IS NOT NULL AND `mj`.`repository_id` = `repositories`.`id`) + OR ( + `mj`.`repository_id` IS NULL + AND `mj`.`repository_name` IS NOT NULL + AND ( + lower(trim(`mj`.`repository_name`)) = `repositories`.`normalized_full_name` + OR lower(trim(`mj`.`repository_name`)) = lower(trim(`repositories`.`name`)) + ) + ) + ) + ), + `repositories`.`created_at`, + unixepoch() + ) AS `imported_at`, + `repositories`.`created_at`, + `repositories`.`updated_at` +FROM `repositories`; +--> statement-breakpoint +DROP TABLE `repositories`; +--> statement-breakpoint +ALTER TABLE `__new_repositories` RENAME TO `repositories`; +--> statement-breakpoint +CREATE INDEX `idx_repositories_user_id` ON `repositories` (`user_id`); +--> statement-breakpoint +CREATE INDEX `idx_repositories_config_id` ON `repositories` (`config_id`); +--> statement-breakpoint +CREATE INDEX `idx_repositories_status` ON `repositories` (`status`); +--> statement-breakpoint +CREATE INDEX `idx_repositories_owner` ON `repositories` (`owner`); +--> statement-breakpoint +CREATE INDEX `idx_repositories_organization` ON `repositories` (`organization`); +--> statement-breakpoint +CREATE INDEX `idx_repositories_is_fork` ON `repositories` (`is_fork`); +--> statement-breakpoint +CREATE INDEX `idx_repositories_is_starred` ON `repositories` (`is_starred`); +--> statement-breakpoint CREATE INDEX `idx_repositories_user_imported_at` ON `repositories` (`user_id`,`imported_at`); +--> statement-breakpoint +CREATE UNIQUE INDEX `uniq_repositories_user_full_name` ON `repositories` (`user_id`,`full_name`); +--> statement-breakpoint +CREATE UNIQUE INDEX `uniq_repositories_user_normalized_full_name` ON `repositories` (`user_id`,`normalized_full_name`); diff --git a/package.json b/package.json index 24734ce..ee004fb 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "start": "bun dist/server/entry.mjs", "start:fresh": "bun run cleanup-db && bun run manage-db init && bun dist/server/entry.mjs", "test": "bun test", + "test:migrations": "bun scripts/validate-migrations.ts", "test:watch": "bun test --watch", "test:coverage": "bun test --coverage", "test:e2e": "bash tests/e2e/run-e2e.sh", diff --git a/scripts/validate-migrations.ts b/scripts/validate-migrations.ts new file mode 100644 index 0000000..8432407 --- /dev/null +++ b/scripts/validate-migrations.ts @@ -0,0 +1,212 @@ +#!/usr/bin/env bun + +import { Database } from "bun:sqlite"; +import { readFileSync } from "fs"; +import path from "path"; + +type JournalEntry = { + idx: number; + tag: string; + when: number; + breakpoints: boolean; +}; + +type Migration = { + entry: JournalEntry; + statements: string[]; +}; + +type UpgradeFixture = { + seed: (db: Database) => void; + verify: (db: Database) => void; +}; + +type TableInfoRow = { + cid: number; + name: string; + type: string; + notnull: number; + dflt_value: string | null; + pk: number; +}; + +const migrationsFolder = path.join(process.cwd(), "drizzle"); +const migrations = loadMigrations(); +const latestMigration = migrations.at(-1); + +/** + * Known SQLite limitations that Drizzle-kit's auto-generated migrations + * can violate. Each rule is checked against every SQL statement. + */ +const SQLITE_LINT_RULES: { pattern: RegExp; message: string }[] = [ + { + pattern: /ALTER\s+TABLE\s+\S+\s+ADD\s+(?:COLUMN\s+)?\S+[^;]*DEFAULT\s*\(/i, + message: + "ALTER TABLE ADD COLUMN with an expression default (e.g. DEFAULT (unixepoch())) " + + "is not allowed in SQLite. Use the table-recreation pattern instead " + + "(CREATE new table, INSERT SELECT, DROP old, RENAME).", + }, + { + pattern: /ALTER\s+TABLE\s+\S+\s+ADD\s+(?:COLUMN\s+)?\S+[^;]*DEFAULT\s+CURRENT_(TIME|DATE|TIMESTAMP)\b/i, + message: + "ALTER TABLE ADD COLUMN with DEFAULT CURRENT_TIME/CURRENT_DATE/CURRENT_TIMESTAMP " + + "is not allowed in SQLite. Use the table-recreation pattern instead.", + }, +]; + +function loadMigrations(): Migration[] { + const journalPath = path.join(migrationsFolder, "meta", "_journal.json"); + const journal = JSON.parse(readFileSync(journalPath, "utf8")) as { + entries: JournalEntry[]; + }; + + return journal.entries.map((entry) => { + const migrationPath = path.join(migrationsFolder, `${entry.tag}.sql`); + const statements = readFileSync(migrationPath, "utf8") + .split("--> statement-breakpoint") + .map((statement) => statement.trim()) + .filter(Boolean); + + return { entry, statements }; + }); +} + +function assert(condition: unknown, message: string): asserts condition { + if (!condition) { + throw new Error(message); + } +} + +function runMigration(db: Database, migration: Migration) { + db.run("BEGIN"); + + try { + for (const statement of migration.statements) { + db.run(statement); + } + + db.run("COMMIT"); + } catch (error) { + try { + db.run("ROLLBACK"); + } catch { + // Ignore rollback errors so the original failure is preserved. + } + + throw error; + } +} + +function runMigrations(db: Database, selectedMigrations: Migration[]) { + for (const migration of selectedMigrations) { + runMigration(db, migration); + } +} + +function seedPre0009Database(db: Database) { + // Seed every existing table so ALTER TABLE paths run against non-empty data. + db.run("INSERT INTO users (id, email, username, name) VALUES ('u1', 'u1@example.com', 'user1', 'User One')"); + db.run("INSERT INTO configs (id, user_id, name, github_config, gitea_config, schedule_config, cleanup_config) VALUES ('c1', 'u1', 'Default', '{}', '{}', '{}', '{}')"); + db.run("INSERT INTO accounts (id, account_id, user_id, provider_id, access_token, refresh_token, id_token, access_token_expires_at, refresh_token_expires_at, scope) VALUES ('acct1', 'acct-1', 'u1', 'github', 'access-token', 'refresh-token', 'id-token', 2000, 3000, 'repo')"); + db.run("INSERT INTO events (id, user_id, channel, payload) VALUES ('evt1', 'u1', 'sync', '{\"status\":\"queued\"}')"); + db.run("INSERT INTO mirror_jobs (id, user_id, repository_id, repository_name, status, message, timestamp) VALUES ('job1', 'u1', 'r1', 'owner/repo', 'imported', 'Imported repository', 900)"); + db.run("INSERT INTO organizations (id, user_id, config_id, name, avatar_url, public_repository_count, private_repository_count, fork_repository_count) VALUES ('org1', 'u1', 'c1', 'Example Org', 'https://example.com/org.png', 1, 0, 0)"); + db.run("INSERT INTO repositories (id, user_id, config_id, name, full_name, normalized_full_name, url, clone_url, owner, organization, default_branch, created_at, updated_at, metadata) VALUES ('r1', 'u1', 'c1', 'repo', 'owner/repo', 'owner/repo', 'https://example.com/repo', 'https://example.com/repo.git', 'owner', 'Example Org', 'main', 1000, 1100, '{\"issues\":true}')"); + db.run("INSERT INTO sessions (id, token, user_id, expires_at) VALUES ('sess1', 'session-token', 'u1', 4000)"); + db.run("INSERT INTO verification_tokens (id, token, identifier, type, expires_at) VALUES ('vt1', 'verify-token', 'u1@example.com', 'email', 5000)"); + db.run("INSERT INTO verifications (id, identifier, value, expires_at) VALUES ('ver1', 'u1@example.com', '123456', 6000)"); + db.run("INSERT INTO oauth_applications (id, client_id, client_secret, name, redirect_urls, type, user_id) VALUES ('app1', 'client-1', 'secret-1', 'Example App', '[\"https://example.com/callback\"]', 'confidential', 'u1')"); + db.run("INSERT INTO oauth_access_tokens (id, access_token, refresh_token, access_token_expires_at, refresh_token_expires_at, client_id, user_id, scopes) VALUES ('oat1', 'oauth-access-token', 'oauth-refresh-token', 7000, 8000, 'client-1', 'u1', '[\"repo\"]')"); + db.run("INSERT INTO oauth_consent (id, user_id, client_id, scopes, consent_given) VALUES ('consent1', 'u1', 'client-1', '[\"repo\"]', true)"); + db.run("INSERT INTO sso_providers (id, issuer, domain, oidc_config, user_id, provider_id) VALUES ('sso1', 'https://issuer.example.com', 'example.com', '{}', 'u1', 'provider-1')"); + db.run("INSERT INTO rate_limits (id, user_id, provider, `limit`, remaining, used, reset, retry_after, status, last_checked) VALUES ('rl1', 'u1', 'github', 5000, 4999, 1, 9000, NULL, 'ok', 8500)"); +} + +function verify0009Migration(db: Database) { + const repositoryColumns = db.query("PRAGMA table_info(repositories)").all() as TableInfoRow[]; + const importedAtColumn = repositoryColumns.find((column) => column.name === "imported_at"); + + assert(importedAtColumn, "Expected repositories.imported_at column to exist after migration"); + assert(importedAtColumn.notnull === 1, "Expected repositories.imported_at to be NOT NULL"); + assert(importedAtColumn.dflt_value === "unixepoch()", `Expected repositories.imported_at default to be unixepoch(), got ${importedAtColumn.dflt_value ?? "null"}`); + + const existingRepo = db.query("SELECT imported_at FROM repositories WHERE id = 'r1'").get() as { imported_at: number } | null; + assert(existingRepo?.imported_at === 900, `Expected existing repository imported_at to backfill from mirror_jobs timestamp 900, got ${existingRepo?.imported_at ?? "null"}`); + + db.run("INSERT INTO repositories (id, user_id, config_id, name, full_name, normalized_full_name, url, clone_url, owner, default_branch) VALUES ('r2', 'u1', 'c1', 'repo-two', 'owner/repo-two', 'owner/repo-two', 'https://example.com/repo-two', 'https://example.com/repo-two.git', 'owner', 'main')"); + const newRepo = db.query("SELECT imported_at FROM repositories WHERE id = 'r2'").get() as { imported_at: number } | null; + assert(typeof newRepo?.imported_at === "number" && newRepo.imported_at > 0, "Expected new repository insert to receive imported_at from the column default"); + + const importedAtIndex = db + .query("SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = 'repositories' AND name = 'idx_repositories_user_imported_at'") + .get() as { name: string } | null; + assert(importedAtIndex?.name === "idx_repositories_user_imported_at", "Expected repositories imported_at index to exist after migration"); +} + +const latestUpgradeFixtures: Record = { + "0009_nervous_tyger_tiger": { + seed: seedPre0009Database, + verify: verify0009Migration, + }, +}; + +function lintMigrations(selectedMigrations: Migration[]) { + const violations: string[] = []; + + for (const migration of selectedMigrations) { + for (const statement of migration.statements) { + for (const rule of SQLITE_LINT_RULES) { + if (rule.pattern.test(statement)) { + violations.push(`[${migration.entry.tag}] ${rule.message}\n Statement: ${statement.slice(0, 120)}...`); + } + } + } + } + + assert( + violations.length === 0, + `SQLite lint found ${violations.length} violation(s):\n\n${violations.join("\n\n")}`, + ); +} + +function validateMigrations() { + assert(latestMigration, "No migrations found in drizzle/meta/_journal.json"); + + // Lint all migrations for known SQLite pitfalls before running anything. + lintMigrations(migrations); + + const emptyDb = new Database(":memory:"); + try { + runMigrations(emptyDb, migrations); + } finally { + emptyDb.close(); + } + + const upgradeFixture = latestUpgradeFixtures[latestMigration.entry.tag]; + assert( + upgradeFixture, + `Missing upgrade fixture for latest migration ${latestMigration.entry.tag}. Add one in scripts/validate-migrations.ts.`, + ); + + const upgradeDb = new Database(":memory:"); + try { + runMigrations(upgradeDb, migrations.slice(0, -1)); + upgradeFixture.seed(upgradeDb); + runMigration(upgradeDb, latestMigration); + upgradeFixture.verify(upgradeDb); + } finally { + upgradeDb.close(); + } + + console.log( + `Validated ${migrations.length} migrations from scratch and upgrade path for ${latestMigration.entry.tag}.`, + ); +} + +try { + validateMigrations(); +} catch (error) { + console.error("Migration validation failed:"); + console.error(error instanceof Error ? error.stack ?? error.message : String(error)); + process.exit(1); +} diff --git a/src/lib/db/index.ts b/src/lib/db/index.ts index 634ae4a..502114e 100644 --- a/src/lib/db/index.ts +++ b/src/lib/db/index.ts @@ -35,13 +35,54 @@ if (process.env.NODE_ENV !== "test") { // Create drizzle instance with the SQLite client db = drizzle({ client: sqlite }); + /** + * Fix migration records that were marked as applied but whose DDL actually + * failed (e.g. the v3.13.0 release where ALTER TABLE with expression default + * was rejected by SQLite). Without this, Drizzle skips the migration on + * retry because it thinks it already ran. + * + * Drizzle tracks migrations by `created_at` (= journal timestamp) and only + * looks at the most recent record. If the last recorded timestamp is >= the + * failed migration's timestamp but the expected column is missing, we delete + * stale records so the migration re-runs. + */ + function repairFailedMigrations() { + try { + const migrationsTableExists = sqlite + .query("SELECT name FROM sqlite_master WHERE type='table' AND name='__drizzle_migrations'") + .get(); + + if (!migrationsTableExists) return; + + // Migration 0009 journal timestamp (from drizzle/meta/_journal.json) + const MIGRATION_0009_TIMESTAMP = 1773542995732; + + const lastMigration = sqlite + .query("SELECT id, created_at FROM __drizzle_migrations ORDER BY created_at DESC LIMIT 1") + .get() as { id: number; created_at: number } | null; + + if (!lastMigration || Number(lastMigration.created_at) < MIGRATION_0009_TIMESTAMP) return; + + // Migration 0009 is recorded as applied — verify the column actually exists + const columns = sqlite.query("PRAGMA table_info(repositories)").all() as { name: string }[]; + const hasImportedAt = columns.some((c) => c.name === "imported_at"); + + if (!hasImportedAt) { + console.log("🔧 Detected failed migration 0009 (imported_at column missing). Removing stale record so it can re-run..."); + sqlite.prepare("DELETE FROM __drizzle_migrations WHERE created_at >= ?").run(MIGRATION_0009_TIMESTAMP); + } + } catch (error) { + console.warn("⚠️ Migration repair check failed (non-fatal):", error); + } + } + /** * Run Drizzle migrations */ function runDrizzleMigrations() { try { console.log("🔄 Checking for pending migrations..."); - + // Check if migrations table exists const migrationsTableExists = sqlite .query("SELECT name FROM sqlite_master WHERE type='table' AND name='__drizzle_migrations'") @@ -51,9 +92,12 @@ if (process.env.NODE_ENV !== "test") { console.log("📦 First time setup - running initial migrations..."); } + // Fix any migrations that were recorded but actually failed (e.g. v3.13.0 bug) + repairFailedMigrations(); + // Run migrations using Drizzle migrate function migrate(db, { migrationsFolder: "./drizzle" }); - + console.log("✅ Database migrations completed successfully"); } catch (error) { console.error("❌ Error running migrations:", error); diff --git a/src/lib/db/migrations.test.ts b/src/lib/db/migrations.test.ts new file mode 100644 index 0000000..936e671 --- /dev/null +++ b/src/lib/db/migrations.test.ts @@ -0,0 +1,26 @@ +import { expect, test } from "bun:test"; + +function decodeOutput(output: ArrayBufferLike | Uint8Array | null | undefined) { + if (!output) { + return ""; + } + + return Buffer.from(output as ArrayBufferLike).toString("utf8"); +} + +test("migration validation script passes", () => { + const result = Bun.spawnSync({ + cmd: ["bun", "scripts/validate-migrations.ts"], + cwd: process.cwd(), + stdout: "pipe", + stderr: "pipe", + }); + + const stdout = decodeOutput(result.stdout); + const stderr = decodeOutput(result.stderr); + + expect( + result.exitCode, + `Migration validation script failed.\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ).toBe(0); +});