diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3edc2d5f..d929265b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,6 +85,13 @@ jobs: - name: Build application run: pnpm run build + # Migrations run in a single shared transaction (TypeORM default "all" + # mode): a migration that opens its own connection (createQueryRunner / + # queryRunner.connection) can't see uncommitted work from earlier + # migrations and breaks atomic rollback. See #1211. + - name: Check migrations use only the passed queryRunner + run: pnpm run migrations:check + - name: Run migrations run: pnpm run migration:run diff --git a/docs/migrations.md b/docs/migrations.md index dc9b2eb6..9d9e31c2 100644 --- a/docs/migrations.md +++ b/docs/migrations.md @@ -62,6 +62,16 @@ export class CreateMessageTable1630000000000 implements MigrationInterface { } ``` +### Transaction behavior (important) + +TypeORM's default `migrationsTransactionMode` is **`all`**: every migration in a run executes inside a **single shared transaction**. This gives atomic rollback, but it means: + +> A migration **must use the `QueryRunner` passed to `up()` / `down()`** and must **never open its own connection** (e.g. `queryRunner.connection.createQueryRunner()` or `dataSource.createQueryRunner()`). + +A freshly created query runner is a separate pooled connection that cannot see uncommitted tables/rows created by earlier migrations in the same run (`relation "X" does not exist` on fresh databases) and escapes the shared transaction, so its changes are not rolled back if a later migration fails. This was the root cause of the `fix-invoice-number-sequence` failure resolved in [#1195](https://github.com/rinafcode/teachLink_backend/pull/1195). + +CI enforces this rule via [`scripts/validate-migrations.js`](../scripts/validate-migrations.js), which fails the build if any migration opens its own connection. + ### Current migrations | File | Description | @@ -208,15 +218,16 @@ pnpm build ## Best practices -| Practice | Why | -| ---------------------------------------------- | ----------------------------------- | -| Always implement `down()` | Enables safe rollback | -| Never modify an applied migration | Create a new migration instead | -| Test rollbacks locally | Run `up` → verify → `down` → verify | -| Use `IF EXISTS` / `IF NOT NULL` | Makes migrations idempotent | -| Backup database before staging/prod migrations | Safety net | -| Keep migrations small and focused | Easier to review and rollback | -| Use timestamp-based naming | Ensures deterministic ordering | +| Practice | Why | +| ---------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | +| Always implement `down()` | Enables safe rollback | +| Never modify an applied migration | Create a new migration instead | +| Test rollbacks locally | Run `up` → verify → `down` → verify | +| Use `IF EXISTS` / `IF NOT NULL` | Makes migrations idempotent | +| Never open your own connection in a migration | Migrations share one transaction; a separate connection can't see uncommitted work and breaks atomic rollback | +| Backup database before staging/prod migrations | Safety net | +| Keep migrations small and focused | Easier to review and rollback | +| Use timestamp-based naming | Ensures deterministic ordering | --- diff --git a/package.json b/package.json index 3c1345ca..3bfae4c3 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,8 @@ "lint:dto": "node scripts/validate-dto-decorators.js", "typecheck": "tsc --project tsconfig.build.json --noEmit", "validate:env": "node scripts/validate-env.js", - "verify": "node scripts/verify-setup.js && node scripts/validate-dto-decorators.js", + "verify": "node scripts/verify-setup.js && node scripts/validate-dto-decorators.js && node scripts/validate-migrations.js", + "migrations:check": "node scripts/validate-migrations.js", "prepare": "husky", "test": "cross-env SAFE_RM_PROTECTION_FLAG=true jest", "test:watch": "jest --watch", diff --git a/scripts/validate-migrations.js b/scripts/validate-migrations.js new file mode 100644 index 00000000..04896cc9 --- /dev/null +++ b/scripts/validate-migrations.js @@ -0,0 +1,147 @@ +/** + * CI check: migrations must not open their own connections (issue #1211). + * + * TypeORM's default `migrationsTransactionMode` is `all` — every migration in + * a run shares a single transaction. A migration that opens its own pooled + * connection via `createQueryRunner()` (or reaches the connection through + * `queryRunner.connection`) cannot see tables/rows created by earlier + * migrations in the same run, and its changes escape the shared transaction + * (they are NOT rolled back if a later migration fails). + * + * This was the root cause of the `fix-invoice-number-sequence` failure + * resolved in #1195: a fresh connection queried the `invoices` table before + * the migration that creates it had committed, so the run crashed from scratch + * on every fresh database. + * + * Rule: a migration must only use the `QueryRunner` passed to its `up()` / + * `down()` method. + * + * Usage: node scripts/validate-migrations.js [path ...] + * Exit code 0 = all pass, 1 = violations. + */ +const fs = require('fs'); +const path = require('path'); + +const ROOT = path.resolve(__dirname, '..'); +const SRC_DIR = path.join(ROOT, 'src'); + +// Directories whose `.ts` files are TypeORM migration classes. Mirrors the +// `migrations` glob in src/config/datasource.ts (`src/migrations/[0-9]*`) plus +// the migration dirs of feature modules that follow the same convention. +const MIGRATION_DIRS = [ + path.join(SRC_DIR, 'migrations'), + path.join(SRC_DIR, 'achievements', 'migrations'), + path.join(SRC_DIR, 'notifications', 'migrations'), +]; + +// Whether a file inside a migration dir is a migration class. +// - src/migrations: basename starts with a digit (TypeORM's timestamp +// convention), matching the `migrations` glob in src/config/datasource.ts. +// Non-migration helpers that live there too (services, entities) are +// excluded — runtime service code legitimately opens query runners. +// - feature-module migration dirs (achievements, notifications): every .ts +// file is a migration, regardless of naming. +const DIGIT_PREFIX = /^[0-9].*\.(ts|js)$/; +const ANY_MIGRATION = /\.(ts|js)$/; + +function isMigrationFile(absolutePath, dir) { + const regex = dir === path.join(SRC_DIR, 'migrations') ? DIGIT_PREFIX : ANY_MIGRATION; + return regex.test(path.basename(absolutePath)); +} + +function findMigrationFiles(dir) { + let results = []; + let entries; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return results; + } + for (const entry of entries) { + if (entry.isFile()) { + const fullPath = path.join(dir, entry.name); + if (isMigrationFile(fullPath, dir)) { + results.push(fullPath); + } + } + } + return results; +} + +// Patterns that open a separate pooled connection or reach outside the +// transaction the migration runner gave us. +const FORBIDDEN_PATTERNS = [ + { + name: 'createQueryRunner()', + // queryRunner.connection.createQueryRunner(), dataSource.createQueryRunner(), ... + regex: /\bcreateQueryRunner\s*\(/g, + }, + { + name: 'queryRunner.connection', + // queryRunner.connection / queryRunner.manager.connection — a direct + // escape hatch off the shared transaction. + regex: /\.connection\b/g, + }, +]; + +function violationsFor(file) { + const content = fs.readFileSync(file, 'utf-8'); + const lines = content.split('\n'); + const violations = []; + + for (const pattern of FORBIDDEN_PATTERNS) { + let match; + while ((match = pattern.regex.exec(content)) !== null) { + const lineNumber = content.slice(0, match.index).split('\n').length; + const line = lines[lineNumber - 1].trim(); + violations.push({ pattern: pattern.name, lineNumber, line }); + // Guard against zero-length matches causing an infinite loop + if (match.index === pattern.regex.lastIndex) { + pattern.regex.lastIndex += 1; + } + } + } + return violations; +} + +let exitCode = 0; + +// Allow explicit paths (useful for manual checks); default to the migration dirs. +const cliTargets = process.argv.slice(2); +const targets = cliTargets.length > 0 ? cliTargets : MIGRATION_DIRS; + +const files = []; +for (const target of targets) { + const absolute = path.isAbsolute(target) ? target : path.resolve(ROOT, target); + if (fs.existsSync(absolute) && fs.statSync(absolute).isDirectory()) { + files.push(...findMigrationFiles(absolute)); + } else if (fs.existsSync(absolute) && fs.statSync(absolute).isFile()) { + files.push(absolute); + } +} + +if (files.length === 0) { + console.error('ERROR: No migration files found!'); + process.exit(1); +} + +for (const file of files) { + const relativePath = path.relative(ROOT, file); + const violations = violationsFor(file); + for (const violation of violations) { + console.error( + `FAIL: ${relativePath}:${violation.lineNumber} — opens a separate connection via '${violation.pattern}':`, + ); + console.error(` ${violation.line}`); + console.error( + ` Migrations run in one shared transaction; use the QueryRunner passed to up()/down() instead.`, + ); + exitCode = 1; + } +} + +if (exitCode === 0) { + console.log(`PASS: All ${files.length} migration files use only the passed queryRunner`); +} + +process.exit(exitCode); diff --git a/src/migrations/README.md b/src/migrations/README.md index a0baf574..39f38a82 100644 --- a/src/migrations/README.md +++ b/src/migrations/README.md @@ -1,150 +1,110 @@ -# Advanced Database Migration System +# Database Migrations -This module provides a comprehensive database migration system with the following features: +This directory holds the TeachLink backend's **TypeORM migrations** — versioned, +ordered schema changes applied on top of the `BaselineSchema` migration. -## Features +> **Workflow guide:** [`docs/migrations.md`](../../docs/migrations.md) covers +> the full workflow — running, rolling back, drift checks, and troubleshooting. +> This README focuses on the one rule that most often breaks migrations: +> **transaction handling**. -- **Version Control**: Track all database schema changes with versioning -- **Rollback Capabilities**: Safely revert migrations with data preservation -- **Environment Management**: Synchronize migrations across different environments -- **Schema Validation**: Ensure integrity and prevent breaking changes -- **Conflict Resolution**: Handle concurrent migrations and resolve conflicts +--- -## Architecture +## How migrations run -The system consists of several core components: - -### Core Services - -1. **MigrationService**: Main orchestrator for running and tracking migrations -2. **RollbackService**: Handles migration reversals and recovery -3. **SchemaValidationService**: Ensures schema integrity before and after migrations -4. **EnvironmentSyncService**: Manages multi-environment synchronization -5. **ConflictResolutionService**: Detects and resolves migration conflicts -6. **MigrationRunnerService**: Bootstraps migration execution - -### Data Model - -The system tracks migrations in a dedicated `migrations` table with the following fields: - -- `id`: Unique identifier (UUID) -- `name`: Migration name -- `version`: Migration version -- `status`: Current status (pending, completed, failed, rolled_back) -- `appliedAt`: Timestamp when applied -- `rolledBackAt`: Timestamp when rolled back -- `createdAt`/`updatedAt`: Standard timestamps -- `errorMessage`: Error details if migration failed - -## Usage - -### Running Migrations - -Run all pending migrations: - -```bash -curl -X POST http://localhost:3000/migrations/run -``` - -### Checking Migration Status - -View all migrations and their status: - -```bash -curl GET http://localhost:3000/migrations -``` - -### Rolling Back Migrations - -Roll back the last migration: +Migrations are executed with the TypeORM CLI against `src/config/datasource.ts`: ```bash -curl -X POST http://localhost:3000/migrations/rollback +pnpm run migration:run # apply all pending migrations +pnpm run migration:revert # revert the last migration ``` -Roll back multiple migrations: +Every migration file exports a class implementing `MigrationInterface` with an +`up(queryRunner)` and a `down(queryRunner)` method. See `docs/migrations.md` +for a full example. -```bash -curl -X POST http://localhost:3000/migrations/rollback/3 -``` +--- -### Reset All Migrations +## ⚠️ Transaction rule: use the passed `queryRunner`, never open your own connection -Completely reset all migrations (development only): +TypeORM's default `migrationsTransactionMode` is **`all`**: every migration in a +single run shares **one transaction**. This gives you atomicity — if any +migration fails, the whole run rolls back — but it comes with a hard constraint: -```bash -curl -X DELETE http://localhost:3000/migrations/reset -``` +> **A migration must only use the `QueryRunner` passed to its `up()` / `down()`** +> **method. It must never open a new connection or query runner.** -### Conflict History - -Check migration conflicts: - -```bash -curl GET http://localhost:3000/migrations/conflicts -``` +### Why -## Creating New Migrations +`queryRunner.connection.createQueryRunner()` (or `dataSource.createQueryRunner()`) +opens a **separate pooled connection** that: -To create a new migration: +1. **Cannot see uncommitted changes** — tables, columns, and rows created by + earlier migrations in the same run are not yet committed, so a fresh + connection will fail with `relation "X" does not exist` when it reads them. +2. **Escapes the shared transaction** — anything done on that separate + connection is committed independently and is **not rolled back** if a later + migration fails, leaving the database in a half-migrated state. -1. Create a new file in the `samples` directory following the migration interface -2. Implement the `up` and `down` methods -3. Register the migration in your migration configuration +This is exactly the failure fixed in [#1195](https://github.com/rinafcode/teachLink_backend/pull/1195): +`fix-invoice-number-sequence` originally opened its own connection to SELECT +from `invoices` — a table created by an earlier migration in the same run — and +crashed from scratch on every fresh database. -Example migration: +### Do / Don't ```typescript -import { Injectable, Logger } from '@nestjs/common'; -import { MigrationConfig } from '../migration.service'; - -@Injectable() -export class SampleUserTableMigration implements MigrationConfig { - name = 'sample-user-table'; - version = '1.0.0'; - dependencies = []; // List any dependencies this migration has - - private readonly logger = new Logger(SampleUserTableMigration.name); - - async up(connection: any): Promise { - // Apply schema changes +// ✅ DO — use the queryRunner handed to the migration +export class DoThis implements MigrationInterface { + async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`CREATE TABLE ...`); + const rows = await queryRunner.query(`SELECT * FROM earlier_table`); // visible: same transaction } +} - async down(connection: any): Promise { - // Revert schema changes +// ❌ DON'T — opens a separate connection that can't see uncommitted work +export class DoNotDoThis implements MigrationInterface { + async up(queryRunner: QueryRunner): Promise { + const other = queryRunner.connection.createQueryRunner(); // separate pooled connection + try { + await other.query(`SELECT * FROM earlier_table`); // "relation does not exist" on fresh DBs + } finally { + await other.release(); + } } } ``` -## Configuration - -Enable automatic migration execution on startup by setting: - -```bash -AUTO_RUN_MIGRATIONS=true -``` +### Enforced in CI -## Best Practices +[`scripts/validate-migrations.js`](../../scripts/validate-migrations.js) scans +every migration file for `createQueryRunner()` and direct `.connection` access +and fails the build if found — the same footgun cannot silently come back. -1. Always test migrations in a development environment first -2. Write reversible migrations (ensure `down` undoes `up`) -3. Validate schema changes before applying -4. Handle dependencies between migrations -5. Monitor migration execution and logs -6. Create backups before running critical migrations +--- -## Error Handling +## Writing a new migration -The system provides comprehensive error handling: +1. Create a file named `<13-digit-timestamp>-.ts` in + `src/migrations/`. The timestamp (e.g. `2026-08-20` → `1795219200000`) must + be **higher than every existing migration** so it runs last. +2. Implement `up(queryRunner)` and `down(queryRunner)` using only the passed + `queryRunner`. +3. Verify locally: -- Automatic rollback on migration failure -- Detailed error logging -- Conflict detection and resolution -- Environment-specific error handling + ```bash + pnpm run migration:run + pnpm run migration:revert # proves down() works + pnpm run migration:run # re-apply to leave the DB migrated + ``` -## Security Considerations +## Rules of thumb -- Migrations should be run by authorized personnel only -- Access to migration endpoints should be restricted in production -- Review all migration scripts before execution -- Implement proper database permissions +| Rule | Why | +| --------------------------------------------------------- | -------------------------------------------- | +| Always implement `down()` | Enables safe rollback in CI and production | +| Never modify an applied migration | Create a new migration instead | +| Use the passed `queryRunner`, never `createQueryRunner()` | Migrations share one transaction (see above) | +| Prefer `IF EXISTS` / `IF NOT NULL` | Makes migrations idempotent | +| Keep migrations small and focused | Easier to review and roll back | +| Use timestamp-based naming | Ensures deterministic ordering |