From 18723ae943ad9d3291a3f9a7f70a3c1aac5c9919 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 7 Sep 2026 17:52:55 -0400 Subject: [PATCH 1/6] refactor(db): replace the kysely Migrator with Flyway Aurora DSQL has no advisory locks and allows one DDL statement per transaction. Kysely's PostgresAdapter takes a pg_advisory_xact_lock on every run and declares supportsTransactionalDdl, so Migrator wraps the whole run in one transaction. Neither is configurable through Migrator options, so the migrator has to go before the engine can. Flyway is swapped in against RDS first, on its own, so a failure here is unambiguous. It covers both engines from one migration corpus, and it replaces machinery we were maintaining: per-migration checksums instead of the bespoke sha256 schema-comment fingerprint, and `flyway validate` alongside the git-based immutability guard. - migrations are renamed to Flyway's V__ form. Pure renames, no content change - flyway.sh is the single place Flyway is configured. It resolves the connection exactly as src/config.ts does and runs the pinned image when no flyway is on PATH, so docker stays the only prerequisite - production is adopted with baselineOnMigrate at the last kysely-era version rather than by replaying the baseline - testkit stamps flyway_schema_history instead of kysely_migration; checksums are left NULL, which Flyway treats as "not mine to validate" - the schema-comment fingerprint stays, but Flyway does not write it, so `db fingerprint` does and the Makefile and CI call it before generating types - the immutability guard is now keyed by version rather than by path, which is what production's schema history actually pins - dsql-lint runs advisory-only on newly added migrations. The corpus still contains the plpgsql that a later migration dropped, so linting all of it could only ever fail Verified against PostgreSQL 16: a from-scratch `flyway migrate` produces a schema byte-identical to applying the same files directly (pg_dump diff), the run is idempotent, `flyway validate` passes, a pre-Flyway schema is adopted without re-running anything, an empty target is refused, and the generated types are unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/lambda-deploy.yml | 25 ++- .github/workflows/lambda-tests.yml | 111 +++++++++--- .github/workflows/regenerate-db-types.yaml | 4 + apps/backend/AGENTS.md | 4 +- apps/backend/Makefile | 7 +- apps/backend/db/Dockerfile | 19 +- apps/backend/db/README.md | 69 +++++--- apps/backend/db/flyway.sh | 107 +++++++++++ ..._schema.sql => V0000__baseline_schema.sql} | 0 ...V20260812011405__rename_project_roles.sql} | 0 ...12012951__add_expenditure_admin_notes.sql} | 0 ...812022651__add_access_pattern_indexes.sql} | 0 ...V20260823054531__add_followup_indexes.sql} | 0 ...20260823055243__add_analytics_rollups.sql} | 0 ...260825023851__drop_project_admin_role.sql} | 0 ...06215733__move_rollups_to_application.sql} | 0 apps/backend/db/package-lock.json | 3 +- apps/backend/db/package.json | 8 +- apps/backend/db/src/cli.ts | 166 +++--------------- apps/backend/db/src/config.ts | 4 + apps/backend/db/src/generate-types.ts | 9 +- apps/backend/db/src/migrator.ts | 52 ------ apps/backend/db/src/provider.ts | 52 ------ apps/backend/db/testkit.ts | 74 +++++--- apps/backend/docker-compose.yml | 2 +- 25 files changed, 368 insertions(+), 348 deletions(-) create mode 100755 apps/backend/db/flyway.sh rename apps/backend/db/migrations/{0000_baseline_schema.sql => V0000__baseline_schema.sql} (100%) rename apps/backend/db/migrations/{20260812011405_rename_project_roles.sql => V20260812011405__rename_project_roles.sql} (100%) rename apps/backend/db/migrations/{20260812012951_add_expenditure_admin_notes.sql => V20260812012951__add_expenditure_admin_notes.sql} (100%) rename apps/backend/db/migrations/{20260812022651_add_access_pattern_indexes.sql => V20260812022651__add_access_pattern_indexes.sql} (100%) rename apps/backend/db/migrations/{20260823054531_add_followup_indexes.sql => V20260823054531__add_followup_indexes.sql} (100%) rename apps/backend/db/migrations/{20260823055243_add_analytics_rollups.sql => V20260823055243__add_analytics_rollups.sql} (100%) rename apps/backend/db/migrations/{20260825023851_drop_project_admin_role.sql => V20260825023851__drop_project_admin_role.sql} (100%) rename apps/backend/db/migrations/{20260906215733_move_rollups_to_application.sql => V20260906215733__move_rollups_to_application.sql} (100%) delete mode 100644 apps/backend/db/src/migrator.ts delete mode 100644 apps/backend/db/src/provider.ts diff --git a/.github/workflows/lambda-deploy.yml b/.github/workflows/lambda-deploy.yml index 38bf5406..6f501aa7 100644 --- a/.github/workflows/lambda-deploy.yml +++ b/.github/workflows/lambda-deploy.yml @@ -222,21 +222,30 @@ jobs: id: status run: | set -o pipefail - npm run migrate:status --prefix apps/backend/db 2>&1 | tee status.log - pending=$(grep -oE '[0-9]+ pending' status.log | tail -1 | cut -d' ' -f1) + npm run --silent migrate:info --prefix apps/backend/db -- -outputType=json > info.json + jq -r '.migrations[] | "\(.state)\t\(.version)\t\(.description)"' info.json | tee status.log # Unknown (unexpected output) falls through as "1" so we take the safe # path and snapshot before touching anything. + pending=$(jq '[.migrations[] | select(.state == "Pending")] | length' info.json || echo 1) echo "pending=${pending:-1}" >> "$GITHUB_OUTPUT" echo "$pending migration(s) pending" - # 0000_baseline_schema is idempotent, but "idempotent" is one typo away from - # DROP SCHEMA. It is adopted once by hand against production (see - # apps/backend/db/README.md); if it still shows as pending here, we are - # pointed at a database we did not expect -- refuse rather than guess. + # V0000__baseline_schema is idempotent, but "idempotent" is one typo away + # from DROP SCHEMA, and production is never built by running it -- Flyway + # baselines over the schema that is already there. + # + # An empty target therefore means DB_HOST is not the database we think it + # is. A pending baseline on a database Flyway is already tracking means the + # history and the schema have drifted apart. Refuse either way rather than + # guess. See apps/backend/db/README.md. - name: Refuse to execute the baseline against production run: | - if grep -E '0000_baseline_schema' status.log | grep -q 'PENDING'; then - echo "::error::0000_baseline_schema is PENDING on the target database. It was never adopted (or DB_HOST is wrong). Refusing to run -- see apps/backend/db/README.md." + if jq -e '.allSchemasEmpty' info.json > /dev/null; then + echo "::error::schema \"branch\" is empty on the target database. Refusing to build production from scratch -- check DB_HOST." + exit 1 + fi + if jq -e '.schemaVersion != null and any(.migrations[]; .version == "0000" and .state == "Pending")' info.json > /dev/null; then + echo "::error::V0000__baseline_schema is PENDING on a database Flyway already tracks. The schema history and the schema have drifted -- refusing to run." exit 1 fi diff --git a/.github/workflows/lambda-tests.yml b/.github/workflows/lambda-tests.yml index 4b15f590..36f17f43 100644 --- a/.github/workflows/lambda-tests.yml +++ b/.github/workflows/lambda-tests.yml @@ -44,13 +44,13 @@ jobs: uses: actions/setup-node@v4 with: node-version: '20' - # Build the schema the same way production does -- by applying - # db/migrations -- then load the dev seed rows the tests expect. The tests - # also call ensureSchema()/resetData() from db/testkit.ts themselves, so - # this doubles as a per-PR smoke test that migrations apply to a virgin DB. - - name: Apply migrations and seed + # Applies db/migrations to a virgin database and loads the dev seed rows + # the tests expect. Through testkit rather than Flyway: the tests build the + # schema that way too (ensureSchema/resetData), and the `checks` job below + # runs the real Flyway migration once instead of six times over. + - name: Build the schema and seed working-directory: apps/backend/db - run: npm ci --no-audit --no-fund && npm run migrate && npm run seed + run: npm ci --no-audit --no-fund && npm run reset env: DATABASE_URL: postgres://branch_dev:password@localhost:5432/branch_db - name: Build shared packages @@ -120,6 +120,8 @@ jobs: esac echo "sha=$base" >> "$GITHUB_OUTPUT" + # Keyed by Flyway version rather than by path: what production's schema + # history pins is the version, its checksum and its script name. - name: Applied migrations are immutable if: steps.base.outputs.sha != '' env: @@ -127,17 +129,44 @@ jobs: run: | set -euo pipefail MB=$(git merge-base "$BASE" HEAD) - # Anything but A(dded) under migrations/ is a violation: M modified, - # D deleted, R renamed, C copied, T typechange. --find-renames is - # explicit so a rename reports R (not D+A) regardless of the runner's - # diff.renames default. - violations=$(git diff --name-status --find-renames "$MB" HEAD -- "$DIR" | grep -vE '^A[[:space:]]' || true) - if [ -n "$violations" ]; then - echo "::error::Migrations already on main can never be modified, renamed or deleted -- someone has already run them. Add a NEW migration that fixes the old one." - printf '%s\n' "$violations" - exit 1 + TAB=$(printf '\t') + + # versionblobfilename, sorted by version so `join` can pair + # the two revisions up. + list() { + git ls-tree -r "$1" -- "$DIR" | while read -r _ _ sha path; do + name=$(basename "$path") + printf '%s\t%s\t%s\n' \ + "$(printf '%s' "$name" | sed -E 's/^V?0*([0-9]+)_+.*/\1/')" \ + "$sha" "$name" + done | sort + } + list "$MB" > "$RUNNER_TEMP/was.tsv" + list HEAD > "$RUNNER_TEMP/now.tsv" + + fail=0 + gone=$(join -t"$TAB" -v1 -j1 "$RUNNER_TEMP/was.tsv" "$RUNNER_TEMP/now.tsv" | cut -f3) + if [ -n "$gone" ]; then + echo "::error::deleted, but already run on production -- add a NEW migration that undoes it: $(echo $gone)" + fail=1 fi + # The rename check applies only once a name is in Flyway form: + # adopting Flyway renamed every file exactly once. + join -t"$TAB" -j1 "$RUNNER_TEMP/was.tsv" "$RUNNER_TEMP/now.tsv" > "$RUNNER_TEMP/both.tsv" + awk -F'\t' -v dir="$DIR" ' + $2 != $4 { + printf "::error file=%s/%s::contents changed. Flyway checksums every applied migration, so this fails the next deploy. Add a NEW migration that fixes the old one.\n", dir, $5 + bad = 1 + } + $3 ~ /^V/ && $3 != $5 { + printf "::error file=%s/%s::renamed from %s. Production'"'"'s schema history records the old name, and Flyway rejects the mismatch.\n", dir, $5, $3 + bad = 1 + } + END { exit bad } + ' "$RUNNER_TEMP/both.tsv" || fail=1 + exit $fail + - name: New migrations must be named correctly if: steps.base.outputs.sha != '' env: @@ -150,12 +179,12 @@ jobs: last=$(git ls-tree -r --name-only "$MB" -- "$DIR" | xargs -r -n1 basename | sort | tail -1 || true) fail=0 for f in $added; do - if ! echo "$f" | grep -qE '^(0000_baseline_schema|[0-9]{14}_[a-z0-9_]+)\.sql$'; then - echo "::error file=$DIR/$f::name must be YYYYMMDDHHMMSS_lower_snake_case.sql -- use 'make new-migration NAME=...' so it is generated for you" + if ! echo "$f" | grep -qE '^V(0000|[0-9]{14})__[a-z0-9_]+\.sql$'; then + echo "::error file=$DIR/$f::name must be VYYYYMMDDHHMMSS__lower_snake_case.sql -- use 'make new-migration NAME=...' so it is generated for you" fail=1 fi - # Out-of-order merges are allowed at runtime (the migrator sets - # allowUnorderedMigrations), so this is advice, not a failure. + # Out-of-order merges are allowed at runtime (flyway.sh sets + # outOfOrder), so this is advice, not a failure. if [ -n "${last:-}" ] && [ "$f" \< "$last" ]; then echo "::warning file=$DIR/$f::sorts before $last, which is already on main, so it will be applied out of order. Harmless, but renaming it to a current timestamp keeps the history readable." fi @@ -189,7 +218,7 @@ jobs: fail=1 fi if printf '%s\n' "$sql" | grep -inE 'CONCURRENTLY'; then - echo "::error file=$f::CONCURRENTLY cannot run inside a transaction, and the migrator wraps the run in one. This database is tiny -- use a plain CREATE INDEX." + echo "::error file=$f::CONCURRENTLY cannot run inside a transaction, and Flyway wraps each migration in one. This database is tiny -- use a plain CREATE INDEX." fail=1 fi if printf '%s\n' "$sql" | grep -inE '^[[:space:]]*INSERT[[:space:]]+INTO[[:space:]]+(branch\.)?users\b'; then @@ -198,6 +227,31 @@ jobs: done exit $fail + # Advisory until the Aurora DSQL cutover: the corpus still contains the + # plpgsql the rollup migration later dropped, so linting all of it can + # only ever fail. New migrations are what we can keep clean. + # + # Only added files, and with the placeholder substituted -- dsql-lint + # parses SQL, and `${async}` is not SQL. + - name: Aurora DSQL compatibility (advisory) + if: steps.base.outputs.sha != '' + continue-on-error: true + env: + BASE: ${{ steps.base.outputs.sha }} + run: | + set -euo pipefail + MB=$(git merge-base "$BASE" HEAD) + files=$(git diff --diff-filter=A --name-only "$MB" HEAD -- "$DIR" || true) + [ -n "$files" ] || { echo "no new migrations"; exit 0; } + npm install --prefix "$RUNNER_TEMP/dsql-lint" --no-audit --no-fund @aws/dsql-lint + lint=$RUNNER_TEMP/dsql-lint/node_modules/@aws/dsql-lint/bin/dsql-lint + for f in $files; do + [ -f "$f" ] || continue + echo "--- $f" + sed 's/${async}/ASYNC/g' "$f" > "$RUNNER_TEMP/$(basename "$f")" + node "$lint" "$RUNNER_TEMP/$(basename "$f")" || true + done + - name: Build and test shared packages uses: ./.github/actions/build-shared-packages with: @@ -209,20 +263,27 @@ jobs: - name: Apply every migration from scratch run: npm run migrate --prefix apps/backend/db - - name: Ledger must be clean afterwards + - name: Schema history must be clean afterwards run: | set -o pipefail - npm run migrate:status --prefix apps/backend/db | tee status.log - if grep -q 'PENDING' status.log; then - echo "::error::migrations still pending after a successful migrate run" + npm run --silent migrate:info --prefix apps/backend/db -- -outputType=json > info.json + pending=$(jq '[.migrations[] | select(.state == "Pending")] | length' info.json) + if [ "$pending" != '0' ]; then + echo "::error::$pending migration(s) still pending after a successful migrate run" + jq -r '.migrations[] | select(.state == "Pending") | .filepath' info.json exit 1 fi - name: Re-running migrations must be a no-op run: npm run migrate --prefix apps/backend/db + - name: Applied migrations must still match their checksums + run: npm run migrate:validate --prefix apps/backend/db + - name: Seeds must apply on top of the migrated schema - run: npm run seed --prefix apps/backend/db + run: | + npm run fingerprint --prefix apps/backend/db + npm run seed --prefix apps/backend/db # Drift check: the committed types must be what these migrations produce. - name: Committed types must match the migrations diff --git a/.github/workflows/regenerate-db-types.yaml b/.github/workflows/regenerate-db-types.yaml index 22e7b94f..f25d244f 100644 --- a/.github/workflows/regenerate-db-types.yaml +++ b/.github/workflows/regenerate-db-types.yaml @@ -139,6 +139,10 @@ jobs: echo "Applying db/migrations" npm run migrate --prefix apps/backend/db + # Flyway records what it applied, not which files it applied; the + # type generator refuses a schema the current migrations did not build. + npm run fingerprint --prefix apps/backend/db + echo "Verifying schema application" psql -h localhost -U postgres -d testdb -c "\dt branch.*" env: diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index a8959e09..a189cc7c 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -47,8 +47,8 @@ http://localhost:3000//health - Schema built from `db/migrations/*.sql` (Postgres schema `branch`); dev/test rows in `db/seed.sql`. Tables: `users`, `projects`, `project_memberships` (roles: Director/Student), `donors`, `project_donations`, `expenditures` (status: approved/pending/denied/needs_more_info), `reports` (report_type: technical/narrative), plus the trigger-maintained rollups `expenditure_rollup` and `project_rollup`. - **Analytics reads go through the rollups, never the base tables.** `expenditure_rollup` (grain: project × month × category × status) and `project_rollup` (per project: member/donation/report counts and `total_donated`) are maintained by `AFTER INSERT OR UPDATE OR DELETE` row triggers, so they are exact rather than eventually consistent — there is nothing to refresh and no scheduler. Spend lives **only** in `expenditure_rollup`; do not add a `total_spent` column to `project_rollup`. Two rules when touching the triggers: increments upsert, decrements are a plain `UPDATE` (a project cascade-delete reaches `expenditures` and the rollups in an undefined order, so a zero-row `UPDATE` must be a safe no-op), and emptied buckets are left at zero rather than deleted. `TRUNCATE` fires no row triggers, so each base table also has an `AFTER TRUNCATE ... FOR EACH STATEMENT` trigger — a new base table needs both kinds. Time-relative figures (the active-project count, "spend on active projects") cannot be rolled up and still read `projects` live. - Kysely connects via `db.ts` in each lambda (`Kysely` + `pg.Pool`, env `DB_HOST/DB_PORT/DB_USER/DB_PASSWORD/DB_NAME`). Query with the `branch.` schema prefix: `db.selectFrom('branch.users')`. -- **Changing the schema:** `make new-migration NAME=add_thing` → write SQL → `make migrate` (applies it and regenerates `shared/types/db-types.d.ts`). `make show-migrations` shows applied vs pending; the ledger is `branch.kysely_migration`. Forward-only — never edit a merged migration, and never hand-edit `db-types.d.ts`. Migrations apply to prod automatically on merge **before** the lambda deploy, so a single PR may only contain additive changes; see `db/README.md` for expand/contract. -- The migration runner is kysely's `Migrator` over plain `.sql` files (`db/src/`). Root `package.json` still carries NestJS/TypeORM **dependencies** from the original scaffold, but nothing uses them; its dead `migration:*` scripts have been removed. +- **Changing the schema:** `make new-migration NAME=add_thing` → write SQL → `make migrate` (applies it and regenerates `shared/types/db-types.d.ts`). `make show-migrations` shows applied vs pending; the ledger is `branch.flyway_schema_history`. Forward-only — never edit a merged migration, and never hand-edit `db-types.d.ts`. Migrations apply to prod automatically on merge **before** the lambda deploy, so a single PR may only contain additive changes; see `db/README.md` for expand/contract. +- The migration runner is Flyway over plain `.sql` files, configured once in `db/flyway.sh`; `db/src/` holds seeding, the schema rebuild and type generation. Root `package.json` still carries NestJS/TypeORM **dependencies** from the original scaffold, but nothing uses them; its dead `migration:*` scripts have been removed. ## Shared packages diff --git a/apps/backend/Makefile b/apps/backend/Makefile index 60095c09..8134e5de 100644 --- a/apps/backend/Makefile +++ b/apps/backend/Makefile @@ -108,6 +108,7 @@ grant-admin: migrate: @docker compose --env-file $(ENV_FILE) build migrator @docker compose --env-file $(ENV_FILE) run --rm migrator npm run migrate + @docker compose --env-file $(ENV_FILE) run --rm migrator npm run fingerprint @docker compose --env-file $(ENV_FILE) run --rm migrator npm run seed -- --if-empty @$(MAKE) types @$(MAKE) show-migrations @@ -116,10 +117,10 @@ migrate: # List every migration and whether it has been applied. Read-only. # -# The source of truth is the branch.kysely_migration table -- one row per applied -# migration -- diffed against the .sql files in db/migrations. +# The source of truth is the branch.flyway_schema_history table -- one row per +# applied migration -- diffed against the .sql files in db/migrations. show-migrations: - @docker compose --env-file $(ENV_FILE) run --rm migrator npm run migrate:status + @docker compose --env-file $(ENV_FILE) run --rm migrator npm run migrate:info # Create an empty timestamped migration file, then say what to do next. # usage: make new-migration NAME=add_expenditure_project_id_index diff --git a/apps/backend/db/Dockerfile b/apps/backend/db/Dockerfile index 30b3f55f..765d3c38 100644 --- a/apps/backend/db/Dockerfile +++ b/apps/backend/db/Dockerfile @@ -1,18 +1,25 @@ # One-shot migration runner, built from the repo root like the lambdas. # -# A Dockerfile rather than bind-mounting the repo into node:20-alpine and running -# `npm ci` on every `up`: that would clobber the host's apps/backend/db/node_modules -# with alpine-built artifacts and cost ~30s per start. This way docker caches the -# install layer. -FROM node:20-alpine +# Based on the Flyway image rather than bolting Flyway onto node:20-alpine: it +# already carries a JDK, the PostgreSQL JDBC driver, and /flyway/drivers, which +# is where the Aurora DSQL JDBC connector goes when we cut over. +# +# node is still here because flyway only migrates: seeding, the schema rebuild +# and type generation stay in src/cli.ts. +FROM flyway/flyway:13.5.0-alpine + +RUN apk add --no-cache nodejs npm WORKDIR /db COPY apps/backend/db/package.json apps/backend/db/package-lock.json ./ RUN npm ci --no-audit --no-fund -COPY apps/backend/db/tsconfig.json apps/backend/db/testkit.ts apps/backend/db/seed.sql ./ +COPY apps/backend/db/tsconfig.json apps/backend/db/testkit.ts apps/backend/db/seed.sql apps/backend/db/flyway.sh ./ COPY apps/backend/db/src ./src COPY apps/backend/db/migrations ./migrations +# The base image runs /flyway/flyway as its entrypoint, which would prepend +# `flyway` to every compose `command:`. +ENTRYPOINT [] CMD ["npm", "run", "migrate"] diff --git a/apps/backend/db/README.md b/apps/backend/db/README.md index 01303f0a..8ca7436a 100644 --- a/apps/backend/db/README.md +++ b/apps/backend/db/README.md @@ -1,8 +1,8 @@ # Database migrations -Schema changes are plain `.sql` files in `migrations/`, applied by [kysely's -`Migrator`](https://kysely.dev/docs/migrations). They are applied to **production -automatically when your PR merges**. +Schema changes are plain `.sql` files in `migrations/`, applied by +[Flyway](https://documentation.red-gate.com/flyway). They are applied to +**production automatically when your PR merges**. ## Changing the schema @@ -10,7 +10,7 @@ automatically when your PR merges**. cd apps/backend make up # if the stack isn't already running -make new-migration NAME=add_expenditure_notes # creates migrations/_add_expenditure_notes.sql +make new-migration NAME=add_expenditure_notes # creates migrations/V__add_expenditure_notes.sql # ...write your SQL... make migrate # applies it, reseeds if empty, regenerates types, prints status ``` @@ -59,31 +59,42 @@ and CI will reject the change. ## How it works -- `migrations/*.sql` — applied in filename order. `make new-migration` generates a - UTC `YYYYMMDDHHMMSS_` prefix so concurrent PRs can't collide. +- `migrations/V__.sql` — applied in version order. + `make new-migration` generates a UTC `VYYYYMMDDHHMMSS__` prefix so concurrent PRs + can't collide. The double underscore is Flyway's separator and is not optional. - `seed.sql` — dev/test data only, **never applied to production**. - `testkit.ts` — `ensureSchema()` / `resetData()` used by the lambda tests. -- `src/` — the runner CLI, the type generator, and the shared post-processing that - keeps local and CI type output byte-identical. +- `flyway.sh` — the only place Flyway is configured. Uses the flyway on `PATH`, + otherwise the pinned image, so docker is the only prerequisite. +- `src/` — seeding and schema-rebuild commands, the type generator, and the shared + post-processing that keeps local and CI type output byte-identical. -**What's applied is tracked in the database**, in `branch.kysely_migration` — one row -per applied migration (`name`, `timestamp`). "Pending" is just the `.sql` files on -disk minus the rows in that table. It's an ordinary table, so in any environment: +**What's applied is tracked in the database**, in `branch.flyway_schema_history` — +one row per applied migration, including a checksum of the file. "Pending" is the +`.sql` files on disk minus the rows in that table. It's an ordinary table, so in any +environment: ```sql -select * from branch.kysely_migration order by name; +select version, description, checksum, success from branch.flyway_schema_history + order by installed_rank; ``` -All pending migrations run inside a **single transaction** with +The checksum is why a merged migration can never be edited: Flyway compares the file +against the recorded checksum on the next run and fails the deploy. + +Each migration file runs inside its **own transaction** with `search_path = branch, public`, so table names can be unqualified and a failure -part-way through rolls the whole run back. That's also why `CREATE INDEX +part-way through a file rolls that file back. That's also why `CREATE INDEX CONCURRENTLY` and `VACUUM` don't work here — they can't run in a transaction. This database is tiny; a plain `CREATE INDEX` is fine. -Out-of-order merges are allowed (`allowUnorderedMigrations`): if your migration -merges after someone whose timestamp is later, it simply applies late. The -alternative — the default — is a production deploy that fails with `corrupted -migrations` and can only be unblocked by hand-editing `kysely_migration` in RDS. +Out-of-order merges are allowed (`outOfOrder`): if your migration merges after +someone whose timestamp is later, it simply applies late. The alternative — the +default — is a production deploy that fails and can only be unblocked by +hand-editing `flyway_schema_history` in RDS. + +`${async}` in a migration is a Flyway placeholder. It expands to nothing on +PostgreSQL and to `ASYNC` on Aurora DSQL, which has no synchronous `CREATE INDEX`. ## In CI @@ -108,9 +119,9 @@ manual restore-into-a-new-instance procedure, not a button: ## One-time: adopting an existing database -`0000_baseline_schema.sql` is the schema as it existed before migrations, and is the -only migration allowed to use `IF NOT EXISTS` — that's what lets it be applied to a -database that already has these tables. +`V0000__baseline_schema.sql` is the schema as it existed before migrations, and is +the only migration allowed to use `IF NOT EXISTS` — that's what lets it be applied to +a database that already has these tables. `IF NOT EXISTS` skips the **entire** `CREATE TABLE` when the table exists, so it cannot detect a column or constraint that differs. Before running the migrator @@ -119,16 +130,18 @@ against a pre-existing database, diff it: ```bash # use a pg_dump matching the server's major version pg_dump --schema-only --schema=branch --no-owner --no-privileges --no-comments \ - -T 'branch.kysely_migration*' -d "$URL" | grep -v '^--' + -T 'branch.flyway_schema_history' -d "$URL" | grep -v '^--' ``` Run that against a local database with only the baseline applied, and against the target; `diff -u` the two. Once it's empty, run `npm run migrate` against the target -with a human watching — the baseline no-ops and records itself in the ledger. +with a human watching. -If the schemas genuinely diverge, use `npm run db -- stamp 0000_baseline_schema` to -record it without executing, then write a follow-up migration reconciling the -difference. +Flyway adopts it rather than replaying it: `baselineOnMigrate` writes a single +baseline row at the version pinned in `flyway.sh` when it finds a non-empty schema +with no history table, and everything at or below that version is then considered +applied. If the schemas genuinely diverge, write a follow-up migration reconciling +the difference — there is no way to record one file as applied on its own. -The `migrate` job refuses to run if `0000_baseline_schema` is still pending, on the -assumption that it means `DB_HOST` is pointing somewhere unexpected. +The `migrate` job refuses to run against an **empty** target, on the assumption that +it means `DB_HOST` is pointing somewhere unexpected. diff --git a/apps/backend/db/flyway.sh b/apps/backend/db/flyway.sh new file mode 100755 index 00000000..d6016f7f --- /dev/null +++ b/apps/backend/db/flyway.sh @@ -0,0 +1,107 @@ +#!/bin/sh +# Runs the Flyway CLI against the database src/config.ts would connect to, with +# the settings this repo needs. The Makefile, CI and the migrator container all +# go through here, so the configuration exists once. +# +# ./flyway.sh migrate +# ./flyway.sh info +# ./flyway.sh validate +# +# Uses the flyway on PATH (the migrator image has one) and otherwise the pinned +# image, so a laptop and a CI runner need docker and nothing else. +set -eu + +FLYWAY_IMAGE=flyway/flyway:13.5.0-alpine +DB_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) + +# Adopting the pre-Flyway database: everything at or below this version is +# already applied on production, so Flyway baselines instead of re-running it. +# It is the last migration that shipped under kysely's Migrator and never moves; +# a new migration sorts above it and applies normally. +BASELINE_VERSION=20260906215733 + +# Same precedence as src/config.ts: DATABASE_URL wins, otherwise the discrete +# DB_* vars with the docker-compose defaults. Percent-encoded credentials in +# DATABASE_URL are not decoded -- pass those through DB_USER/DB_PASSWORD. +if [ -n "${DATABASE_URL:-}" ]; then + rest=${DATABASE_URL#*://} + case "$rest" in + *@*) creds=${rest%%@*} ; hostpath=${rest#*@} ;; + *) creds='' ; hostpath=$rest ;; + esac + db_user=${creds%%:*} + case "$creds" in *:*) db_password=${creds#*:} ;; *) db_password='' ;; esac + hostport=${hostpath%%/*} + db_name=${hostpath#*/} + db_name=${db_name%%\?*} + db_host=${hostport%%:*} + case "$hostport" in *:*) db_port=${hostport#*:} ;; *) db_port=5432 ;; esac +else + db_user=${DB_USER:-branch_dev} + db_password=${DB_PASSWORD:-password} + db_host=${DB_HOST:-localhost} + db_port=${DB_PORT:-5432} + db_name=${DB_NAME:-branch_db} +fi + +# TLS, mirroring src/config.ts: a CA bundle means full verification (CI reaches +# RDS over the public internet), DB_SSL alone means encrypted-but-unverified. +ca_path=${DB_SSL_CA:-} +if [ -n "$ca_path" ]; then + ssl='?sslmode=verify-full&sslrootcert=' +elif [ "${DB_SSL:-}" = 'true' ]; then + ssl='?sslmode=require' +else + ssl='' +fi + +# -outOfOrder: two contributors' migrations sometimes merge out of timestamp +# order (Alice authors first, Bob merges first). Without it the next deploy +# fails and can only be unblocked by hand-editing the history table. +# -cleanDisabled: `flyway clean` drops the schema. Nothing here ever wants it, +# and it is one typo away from production. +# -placeholders.async: empty on PostgreSQL, ASYNC on Aurora DSQL, which has no +# synchronous CREATE INDEX. Keeps one migration corpus valid on both engines. +add_flags() { + set -- \ + "-locations=filesystem:$1" \ + -schemas=branch \ + -defaultSchema=branch \ + -createSchemas=true \ + -outOfOrder=true \ + -baselineOnMigrate=true \ + "-baselineVersion=$BASELINE_VERSION" \ + -validateMigrationNaming=true \ + -cleanDisabled=true \ + "-placeholders.async=${FLYWAY_PLACEHOLDER_ASYNC:-}" + printf '%s\n' "$@" +} + +if command -v flyway >/dev/null 2>&1; then + [ -z "$ca_path" ] || ssl="${ssl}${ca_path}" + # shellcheck disable=SC2046 # deliberate word splitting: one flag per line + set -- flyway $(add_flags "$DB_DIR/migrations") "$@" +else + ca_mount='' + if [ -n "$ca_path" ]; then + ssl="${ssl}/rds-ca.pem" + ca_mount="--volume=$ca_path:/rds-ca.pem:ro" + fi + # --network=host so localhost:5432 means the same thing inside the container + # as outside it -- that is where compose and the CI postgres service listen. + # shellcheck disable=SC2046 + set -- docker run --rm --network=host \ + --volume="$DB_DIR/migrations:/db/migrations:ro" \ + --env=FLYWAY_URL --env=FLYWAY_USER --env=FLYWAY_PASSWORD \ + ${ca_mount:+"$ca_mount"} "$FLYWAY_IMAGE" \ + $(add_flags /db/migrations) "$@" +fi + +# Credentials go through the environment, never argv: `docker run` arguments are +# visible to every process on the host, and Flyway echoes its own command line. +FLYWAY_URL="jdbc:postgresql://${db_host}:${db_port}/${db_name}${ssl}" +FLYWAY_USER=$db_user +FLYWAY_PASSWORD=$db_password +export FLYWAY_URL FLYWAY_USER FLYWAY_PASSWORD + +exec "$@" diff --git a/apps/backend/db/migrations/0000_baseline_schema.sql b/apps/backend/db/migrations/V0000__baseline_schema.sql similarity index 100% rename from apps/backend/db/migrations/0000_baseline_schema.sql rename to apps/backend/db/migrations/V0000__baseline_schema.sql diff --git a/apps/backend/db/migrations/20260812011405_rename_project_roles.sql b/apps/backend/db/migrations/V20260812011405__rename_project_roles.sql similarity index 100% rename from apps/backend/db/migrations/20260812011405_rename_project_roles.sql rename to apps/backend/db/migrations/V20260812011405__rename_project_roles.sql diff --git a/apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql b/apps/backend/db/migrations/V20260812012951__add_expenditure_admin_notes.sql similarity index 100% rename from apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql rename to apps/backend/db/migrations/V20260812012951__add_expenditure_admin_notes.sql diff --git a/apps/backend/db/migrations/20260812022651_add_access_pattern_indexes.sql b/apps/backend/db/migrations/V20260812022651__add_access_pattern_indexes.sql similarity index 100% rename from apps/backend/db/migrations/20260812022651_add_access_pattern_indexes.sql rename to apps/backend/db/migrations/V20260812022651__add_access_pattern_indexes.sql diff --git a/apps/backend/db/migrations/20260823054531_add_followup_indexes.sql b/apps/backend/db/migrations/V20260823054531__add_followup_indexes.sql similarity index 100% rename from apps/backend/db/migrations/20260823054531_add_followup_indexes.sql rename to apps/backend/db/migrations/V20260823054531__add_followup_indexes.sql diff --git a/apps/backend/db/migrations/20260823055243_add_analytics_rollups.sql b/apps/backend/db/migrations/V20260823055243__add_analytics_rollups.sql similarity index 100% rename from apps/backend/db/migrations/20260823055243_add_analytics_rollups.sql rename to apps/backend/db/migrations/V20260823055243__add_analytics_rollups.sql diff --git a/apps/backend/db/migrations/20260825023851_drop_project_admin_role.sql b/apps/backend/db/migrations/V20260825023851__drop_project_admin_role.sql similarity index 100% rename from apps/backend/db/migrations/20260825023851_drop_project_admin_role.sql rename to apps/backend/db/migrations/V20260825023851__drop_project_admin_role.sql diff --git a/apps/backend/db/migrations/20260906215733_move_rollups_to_application.sql b/apps/backend/db/migrations/V20260906215733__move_rollups_to_application.sql similarity index 100% rename from apps/backend/db/migrations/20260906215733_move_rollups_to_application.sql rename to apps/backend/db/migrations/V20260906215733__move_rollups_to_application.sql diff --git a/apps/backend/db/package-lock.json b/apps/backend/db/package-lock.json index 649f6761..835bcfac 100644 --- a/apps/backend/db/package-lock.json +++ b/apps/backend/db/package-lock.json @@ -9,12 +9,12 @@ "version": "1.0.0", "license": "ISC", "dependencies": { - "kysely": "^0.28.8", "pg": "^8.16.3" }, "devDependencies": { "@types/node": "^20.11.30", "@types/pg": "^8.15.6", + "kysely": "^0.28.8", "kysely-codegen": "^0.19.0", "ts-node": "^10.9.2", "typescript": "^5.9.3" @@ -663,6 +663,7 @@ "version": "0.28.8", "resolved": "https://registry.npmjs.org/kysely/-/kysely-0.28.8.tgz", "integrity": "sha512-QUOgl5ZrS9IRuhq5FvOKFSsD/3+IA6MLE81/bOOTRA/YQpKDza2sFdN5g6JCB9BOpqMJDGefLCQ9F12hRS13TA==", + "dev": true, "license": "MIT", "engines": { "node": ">=20.0.0" diff --git a/apps/backend/db/package.json b/apps/backend/db/package.json index 98472569..d5efeb91 100644 --- a/apps/backend/db/package.json +++ b/apps/backend/db/package.json @@ -5,21 +5,23 @@ "description": "Database migrations, seed data and type generation for BRANCH.", "scripts": { "db": "ts-node --transpile-only src/cli.ts", - "migrate": "npm run db -- up", - "migrate:status": "npm run db -- status", + "migrate": "./flyway.sh migrate", + "migrate:info": "./flyway.sh info", + "migrate:validate": "./flyway.sh validate", "migrate:new": "npm run db -- new", "seed": "npm run db -- seed", "reset": "npm run db -- reset", + "fingerprint": "npm run db -- fingerprint", "types": "ts-node --transpile-only src/generate-types.ts" }, "license": "ISC", "dependencies": { - "kysely": "^0.28.8", "pg": "^8.16.3" }, "devDependencies": { "@types/node": "^20.11.30", "@types/pg": "^8.15.6", + "kysely": "^0.28.8", "kysely-codegen": "^0.19.0", "ts-node": "^10.9.2", "typescript": "^5.9.3" diff --git a/apps/backend/db/src/cli.ts b/apps/backend/db/src/cli.ts index 1db07d8b..231847e6 100644 --- a/apps/backend/db/src/cli.ts +++ b/apps/backend/db/src/cli.ts @@ -1,25 +1,22 @@ /** - * db CLI -- the single entrypoint for everything schema related. + * db CLI -- the schema-adjacent commands that are not migrations themselves. * - * npm run migrate apply every pending migration - * npm run migrate:status list applied/pending - * npm run migrate:new -- x scaffold migrations/_x.sql + * npm run migrate apply every pending migration (flyway.sh migrate) + * npm run migrate:info list applied/pending (flyway.sh info) + * npm run migrate:new -- x scaffold migrations/V__x.sql * npm run seed truncate + re-apply seed.sql * npm run seed -- --if-empty seed only an empty database * npm run reset rebuild the schema from migrations, then seed - * npm run db -- stamp NAME record a migration as applied WITHOUT running it + * npm run fingerprint record which migrations built the live schema * - * The Makefile in apps/backend wraps these; CI calls them directly. + * Migrations are Flyway's job -- see flyway.sh. The Makefile in apps/backend + * wraps all of this; CI calls it directly. */ import fs from 'node:fs'; import path from 'node:path'; -import { sql } from 'kysely'; import type { PoolClient } from 'pg'; -import { SCHEMA, createPool, describeTarget } from './config'; -import { createDb, createMigrator } from './migrator'; -import { MIGRATIONS_DIR } from './provider'; +import { MIGRATIONS_DIR, createPool, describeTarget } from './config'; import { - migrationsFingerprint, rebuildSchema, resetData, seedIfEmpty, @@ -28,9 +25,9 @@ import { const TEMPLATE = `-- __NAME__ -- --- Every pending migration runs inside a SINGLE transaction, with --- search_path = branch, public -- so table names can be unqualified, and --- CREATE INDEX CONCURRENTLY / VACUUM will not work here. +-- Flyway runs this file inside a SINGLE transaction, with search_path = +-- branch, public -- so table names can be unqualified, and CREATE INDEX +-- CONCURRENTLY / VACUUM will not work here. -- -- This migration is applied to PRODUCTION automatically when the PR merges, -- BEFORE the new lambda code is deployed. It must be safe for the code that is @@ -38,68 +35,12 @@ const TEMPLATE = `-- __NAME__ -- expand/contract rules that destructive changes need. -- -- Forward-only: there is no rollback. Fix a mistake with a new migration, and --- never edit a migration that has been merged -- someone has already run it. --- Do not use IF NOT EXISTS: you want a failure, not silent drift. +-- never edit a migration that has been merged -- Flyway checksums it, and +-- someone has already run it. Do not use IF NOT EXISTS: you want a failure, not +-- silent drift. `; -async function up(): Promise { - const db = createDb(); - try { - console.log(`migrating ${describeTarget()}`); - // migrateToLatest NEVER throws -- it returns the error. Not checking it is - // the classic way to ship a green deploy that migrated nothing. - const { error, results } = await createMigrator(db).migrateToLatest(); - - for (const result of results ?? []) { - console.log(`${result.status.padEnd(11)} ${result.migrationName}`); - } - - if (error) { - console.error('\nmigration failed, nothing was applied:'); - console.error(error); - process.exitCode = 1; - return; - } - - if (!results?.length) console.log('database is already up to date'); - - // Lets db/testkit.ts skip its rebuild when the live schema already matches - // the migration files on disk. Value is a hex digest, safe to inline. - await sql - .raw( - `comment on schema ${SCHEMA} is 'migrations:${migrationsFingerprint()}'`, - ) - .execute(db); - } finally { - await db.destroy(); - } -} - -async function status(): Promise { - const db = createDb(); - try { - const migrations = await createMigrator(db).getMigrations(); - if (!migrations.length) { - console.log(`no *.sql files in ${MIGRATIONS_DIR}`); - return; - } - - console.log(`${describeTarget()}\n`); - for (const migration of migrations) { - const applied = migration.executedAt?.toISOString() ?? 'PENDING'; - console.log(`${applied.padEnd(26)} ${migration.name}`); - } - - const pending = migrations.filter( - (migration) => !migration.executedAt, - ).length; - console.log(`\n${migrations.length - pending} applied, ${pending} pending`); - } finally { - await db.destroy(); - } -} - function newMigration(name?: string): void { if (!name || !/^[a-z0-9]+(_[a-z0-9]+)*$/.test(name)) { console.error( @@ -111,67 +52,18 @@ function newMigration(name?: string): void { } // UTC YYYYMMDDHHMMSS, generated so nobody hand-types one: collisions between - // concurrent PRs are then effectively impossible. + // concurrent PRs are then effectively impossible. `V__` + // is Flyway's naming scheme -- the double underscore is the separator, and it + // cannot be omitted. const stamp = new Date().toISOString().replace(/[-:T]/g, '').slice(0, 14); - const file = path.join(MIGRATIONS_DIR, `${stamp}_${name}.sql`); + const file = path.join(MIGRATIONS_DIR, `V${stamp}__${name}.sql`); - fs.writeFileSync(file, TEMPLATE.replace('__NAME__', `${stamp}_${name}`), { + fs.writeFileSync(file, TEMPLATE.replace('__NAME__', `V${stamp}__${name}`), { flag: 'wx', }); console.log(`created ${path.relative(process.cwd(), file)}`); } -/** - * Record a migration as applied WITHOUT running it. Only for adopting a database - * whose schema was built by hand -- verify with pg_dump first. The table - * definitions below are copied from kysely's Migrator so a later migrate run - * finds exactly what it expects. - */ -async function stamp(name?: string): Promise { - if (!name) { - console.error('usage: npm run db -- stamp 0000_baseline_schema'); - process.exitCode = 1; - return; - } - - const db = createDb(); - try { - await sql.raw(`create schema if not exists ${SCHEMA}`).execute(db); - await sql - .raw( - `create table if not exists ${SCHEMA}.kysely_migration ( - name varchar(255) not null primary key, - "timestamp" varchar(255) not null)`, - ) - .execute(db); - await sql - .raw( - `create table if not exists ${SCHEMA}.kysely_migration_lock ( - id varchar(255) not null primary key, - is_locked integer not null default 0)`, - ) - .execute(db); - await sql - .raw( - `insert into ${SCHEMA}.kysely_migration_lock (id, is_locked) - values ('migration_lock', 0) on conflict (id) do nothing`, - ) - .execute(db); - await sql - .raw( - `insert into ${SCHEMA}.kysely_migration (name, "timestamp") - values ('${name}', '${new Date().toISOString()}')`, - ) - .execute(db); - - console.log( - `stamped ${name} as applied (not executed) on ${describeTarget()}`, - ); - } finally { - await db.destroy(); - } -} - async function withClient( fn: (client: PoolClient) => Promise, ): Promise { @@ -189,14 +81,8 @@ async function main(): Promise { const [command, ...args] = process.argv.slice(2); switch (command) { - case 'up': - return up(); - case 'status': - return status(); case 'new': return newMigration(args[0]); - case 'stamp': - return stamp(args[0]); case 'seed': return withClient(async (client) => { if (args.includes('--if-empty')) { @@ -214,13 +100,19 @@ async function main(): Promise { return withClient(async (client) => { await rebuildSchema(client); await resetData(client); - await stampFingerprint(client); console.log('schema rebuilt from migrations and reseeded'); }); + // Flyway records what it applied, not which files were on disk when it ran. + // `make types` refuses to generate from a schema the current migrations did + // not build, so a real migrate run has to leave the same marker + // rebuildSchema() does. + case 'fingerprint': + return withClient(async (client) => { + await stampFingerprint(client); + console.log(`fingerprinted ${describeTarget()}`); + }); default: - console.error( - 'usage: db ', - ); + console.error('usage: db '); process.exitCode = 1; } } diff --git a/apps/backend/db/src/config.ts b/apps/backend/db/src/config.ts index f856de51..a2b95271 100644 --- a/apps/backend/db/src/config.ts +++ b/apps/backend/db/src/config.ts @@ -1,9 +1,13 @@ import fs from 'node:fs'; +import path from 'node:path'; import { Pool } from 'pg'; /** Every table lives in this schema; the generated DB types key off it. */ export const SCHEMA = 'branch'; +/** Flyway is pointed at the same directory by flyway.sh. */ +export const MIGRATIONS_DIR = path.join(__dirname, '..', 'migrations'); + /** * The one place that knows how to reach the database. * diff --git a/apps/backend/db/src/generate-types.ts b/apps/backend/db/src/generate-types.ts index c41dc5a6..63862410 100644 --- a/apps/backend/db/src/generate-types.ts +++ b/apps/backend/db/src/generate-types.ts @@ -12,7 +12,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { createPool, databaseUrl } from './config'; -import { assertSchemaIsCurrent } from '../testkit'; +import { HISTORY_TABLE, assertSchemaIsCurrent } from '../testkit'; import { postprocess } from './postprocess-types'; /** @@ -47,14 +47,17 @@ async function main(): Promise { databaseUrl(), '--dialect', 'postgres', - // Ignore anything created outside schema "branch". kysely already excludes - // kysely_migration / kysely_migration_lock by name. + // Ignore anything created outside schema "branch". // // Deliberately NOT --default-schema: that would strip the `branch.` prefix // from the generated DB keys and break every db.selectFrom('branch.users') // in all six lambdas. '--include-pattern', 'branch.*', + // kysely-codegen skips kysely_migration% by name and knows nothing about + // Flyway, so without this the history table becomes a shared DB type. + '--exclude-pattern', + `branch.${HISTORY_TABLE}`, '--out-file', tmp, ], diff --git a/apps/backend/db/src/migrator.ts b/apps/backend/db/src/migrator.ts deleted file mode 100644 index b7d6393f..00000000 --- a/apps/backend/db/src/migrator.ts +++ /dev/null @@ -1,52 +0,0 @@ -import { CompiledQuery, Kysely, Migrator, PostgresDialect } from 'kysely'; -import { SCHEMA, createPool } from './config'; -import { SqlFileMigrationProvider } from './provider'; - -export function createDb(): Kysely { - return new Kysely({ - dialect: new PostgresDialect({ - pool: createPool(), - // Awaited by PostgresDriver before the connection is handed out, so every - // migration runs with search_path=branch and may use unqualified table - // names. Doing this with a `SET` inside a migration file instead would - // leak the setting onto a pooled connection. - onCreateConnection: async (connection) => { - await connection.executeQuery( - CompiledQuery.raw(`set search_path to ${SCHEMA}, public`), - ); - }, - }), - }); -} - -export function createMigrator(db: Kysely): Migrator { - return new Migrator({ - db, - provider: new SqlFileMigrationProvider(), - - // Bookkeeping lives beside the tables it tracks: branch.kysely_migration - // (one row per applied migration) and branch.kysely_migration_lock. - // - // Keep the DEFAULT table names. kysely-codegen's introspector skips tables - // named kysely_migration / kysely_migration_lock in every schema, so they - // stay out of shared/types/db-types.d.ts for free. Renaming them to - // something like schema_migrations would leak the interfaces into the - // shared types package. - migrationTableSchema: SCHEMA, - - // Two contributors' migrations will sometimes merge out of timestamp order - // (Alice authors first, Bob merges first). With the default `false` that - // makes the next production deploy fail with "corrupted migrations", - // fixable only by renaming the file and hand-editing kysely_migration in - // RDS. With `true` the late migration is simply applied late. Deletions and - // renames of already-applied migrations are still rejected either way, which - // is the guard that actually matters. - allowUnorderedMigrations: true, - - // Byte-order comparison, matching the .sort() the provider uses to order - // files. The default is localeCompare, whose handling of `_` can disagree - // with byte order and raise a spurious ordering error when two migrations - // share a timestamp. - nameComparator: (a, b) => (a < b ? -1 : a > b ? 1 : 0), - }); -} diff --git a/apps/backend/db/src/provider.ts b/apps/backend/db/src/provider.ts deleted file mode 100644 index 375ce792..00000000 --- a/apps/backend/db/src/provider.ts +++ /dev/null @@ -1,52 +0,0 @@ -import fs from 'node:fs'; -import path from 'node:path'; -import { sql } from 'kysely'; -import type { Kysely, Migration, MigrationProvider } from 'kysely'; - -export const MIGRATIONS_DIR = path.join(__dirname, '..', 'migrations'); - -/** Byte-order sort, matching what kysely uses internally to order migrations. */ -export function migrationFilenames(dir: string = MIGRATIONS_DIR): string[] { - return fs - .readdirSync(dir) - .filter((file) => file.endsWith('.sql')) - .sort(); -} - -/** - * Serves every `*.sql` file in db/migrations to kysely's Migrator. The migration - * name is the filename without `.sql`, and kysely orders by that name, so the - * numeric prefix defines the order. - * - * Forward-only by design: kysely's migrateDown silently does nothing for a - * migration with no `down` (it leaves the tracking row in place), so we never - * define one and never expose a rollback command. Undo by writing a new - * migration. - */ -export class SqlFileMigrationProvider implements MigrationProvider { - constructor(private readonly dir: string = MIGRATIONS_DIR) {} - - async getMigrations(): Promise> { - const migrations: Record = {}; - - for (const file of migrationFilenames(this.dir)) { - const name = file.slice(0, -'.sql'.length); - if (name.length > 255) { - throw new Error( - `migration name is longer than kysely_migration.name (varchar(255)): ${file}`, - ); - } - - const contents = fs.readFileSync(path.join(this.dir, file), 'utf8'); - migrations[name] = { - // node-postgres sends parameterless queries over the simple query - // protocol, so one file may hold many `;`-separated statements. - up: async (db: Kysely) => { - await sql.raw(contents).execute(db); - }, - }; - } - - return migrations; - } -} diff --git a/apps/backend/db/testkit.ts b/apps/backend/db/testkit.ts index 9546f960..bca72558 100644 --- a/apps/backend/db/testkit.ts +++ b/apps/backend/db/testkit.ts @@ -26,9 +26,9 @@ export interface Queryable { } /** - * Byte-order sort, matching the sort kysely's Migrator uses to order migrations. - * If these two ever disagree, tests and production apply migrations in different - * orders. + * Byte-order sort. Flyway orders by parsed version rather than by filename, but + * `V__` sorts identically either way. If that ever stops + * being true, tests and production apply migrations in different orders. */ function migrationFiles(): string[] { return fs @@ -62,41 +62,61 @@ export function migrationsFingerprint(): string { return crypto.createHash('sha256').update(allMigrationSql()).digest('hex'); } +export const HISTORY_TABLE = 'flyway_schema_history'; + /** - * Writes kysely's migration ledger as if every migration file had just been - * applied by the Migrator: branch.kysely_migration with one row per file, plus - * branch.kysely_migration_lock. The DDL is copied from kysely's Migrator so a - * later `npm run migrate` finds exactly what it expects. + * Writes Flyway's history as if every migration file had just been applied. The + * DDL is copied from what Flyway itself creates on PostgreSQL, so a later + * `npm run migrate` finds exactly what it expects. * * Without this, rebuildSchema() would leave a schema whose tables exist but - * whose ledger is empty -- and since the dev stack and the tests share one local - * database, the next `make migrate` would try to re-apply every migration and - * fail on "already exists". + * whose history is empty -- and since the dev stack and the tests share one + * local database, the next `make migrate` would baseline at the adoption + * version and re-apply every migration written since, failing on "already + * exists". + * + * checksum is left NULL on purpose. Flyway only compares checksums it recorded + * itself, and nothing validates a test database, so computing Flyway's CRC32 + * here would be a second implementation to keep in step for no benefit. */ export async function stampLedger(client: Queryable): Promise { await client.query( - `CREATE TABLE IF NOT EXISTS ${SCHEMA}.kysely_migration ( - name varchar(255) NOT NULL PRIMARY KEY, - "timestamp" varchar(255) NOT NULL)`, + `CREATE TABLE IF NOT EXISTS ${SCHEMA}.${HISTORY_TABLE} ( + installed_rank integer NOT NULL PRIMARY KEY, + version varchar(50), + description varchar(200) NOT NULL, + type varchar(20) NOT NULL, + script varchar(1000) NOT NULL, + checksum integer, + installed_by varchar(100) NOT NULL, + installed_on timestamp NOT NULL DEFAULT now(), + execution_time integer NOT NULL, + success boolean NOT NULL)`, ); await client.query( - `CREATE TABLE IF NOT EXISTS ${SCHEMA}.kysely_migration_lock ( - id varchar(255) NOT NULL PRIMARY KEY, - is_locked integer NOT NULL DEFAULT 0)`, - ); - await client.query( - `INSERT INTO ${SCHEMA}.kysely_migration_lock (id, is_locked) - VALUES ('migration_lock', 0) ON CONFLICT (id) DO NOTHING`, + `CREATE INDEX IF NOT EXISTS ${HISTORY_TABLE}_s_idx + ON ${SCHEMA}.${HISTORY_TABLE} (success)`, ); - const now = new Date().toISOString(); + // V__.sql -- Flyway stores the description with the + // underscores turned back into spaces. const rows = migrationFiles() - .map((file) => `('${file.slice(0, -'.sql'.length)}', '${now}')`) + .map((file, index) => { + const [version, description] = file + .slice(1, -'.sql'.length) + .split('__', 2); + return ( + `(${index + 1}, '${version}', '${description.replace(/_/g, ' ')}',` + + ` 'SQL', '${file}', NULL, current_user, 0, TRUE)` + ); + }) .join(', '); if (rows) { await client.query( - `INSERT INTO ${SCHEMA}.kysely_migration (name, "timestamp") - VALUES ${rows} ON CONFLICT (name) DO NOTHING`, + `INSERT INTO ${SCHEMA}.${HISTORY_TABLE} + (installed_rank, version, description, type, script, checksum, + installed_by, execution_time, success) + VALUES ${rows} ON CONFLICT (installed_rank) DO NOTHING`, ); } } @@ -114,8 +134,8 @@ export async function rebuildSchema(client: Queryable): Promise { await client.query(`SET search_path TO ${SCHEMA}, public`); try { // A parameterless multi-statement query goes over the simple query protocol - // and runs as one implicit transaction -- the same all-or-nothing semantics - // kysely's Migrator gives us in production. + // and runs as one implicit transaction. Flyway commits per file rather than + // per run, so this is stricter than production, not looser. await client.query(allMigrationSql()); } finally { await client.query('RESET search_path'); @@ -168,7 +188,7 @@ async function truncateAll(client: Queryable): Promise { const { rows } = await client.query( `SELECT quote_ident(schemaname) || '.' || quote_ident(tablename) AS t FROM pg_tables - WHERE schemaname = '${SCHEMA}' AND tablename NOT LIKE 'kysely_migration%' + WHERE schemaname = '${SCHEMA}' AND tablename <> '${HISTORY_TABLE}' ORDER BY tablename`, ); const tables = (rows ?? []).map((row) => row.t as string); diff --git a/apps/backend/docker-compose.yml b/apps/backend/docker-compose.yml index c2d83a59..e55699dc 100644 --- a/apps/backend/docker-compose.yml +++ b/apps/backend/docker-compose.yml @@ -43,7 +43,7 @@ services: DB_USER: ${DB_USER:-branch_dev} DB_PASSWORD: ${DB_PASSWORD:-password} DB_NAME: ${DB_NAME:-branch_db} - command: sh -c "npm run migrate && npm run seed -- --if-empty" + command: sh -c "npm run migrate && npm run fingerprint && npm run seed -- --if-empty" depends_on: postgres: condition: service_healthy From e33f0b09b69a2dcd35ffcc4d5f5395b0ef4f5775 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 7 Sep 2026 17:59:42 -0400 Subject: [PATCH 2/6] chore: trim comments Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/lambda-deploy.yml | 10 +------ .github/workflows/lambda-tests.yml | 20 +++---------- .github/workflows/regenerate-db-types.yaml | 3 +- apps/backend/db/Dockerfile | 10 +------ apps/backend/db/flyway.sh | 34 +++------------------- apps/backend/db/src/cli.ts | 8 +---- apps/backend/db/src/generate-types.ts | 3 +- apps/backend/db/testkit.ts | 7 +---- 8 files changed, 14 insertions(+), 81 deletions(-) diff --git a/.github/workflows/lambda-deploy.yml b/.github/workflows/lambda-deploy.yml index 6f501aa7..2dbee8f4 100644 --- a/.github/workflows/lambda-deploy.yml +++ b/.github/workflows/lambda-deploy.yml @@ -162,7 +162,7 @@ jobs: # reviewers: a pending approval here would strand a merged PR with its schema # applied and its lambda code undeployed. environment: production-db - # kysely takes a pg advisory lock, but serialize anyway so two merges in a row + # Flyway locks the history table, but serialize anyway so two merges in a row # queue instead of racing -- and never cancel a half-run migration. concurrency: group: db-migrate-prod @@ -230,14 +230,6 @@ jobs: echo "pending=${pending:-1}" >> "$GITHUB_OUTPUT" echo "$pending migration(s) pending" - # V0000__baseline_schema is idempotent, but "idempotent" is one typo away - # from DROP SCHEMA, and production is never built by running it -- Flyway - # baselines over the schema that is already there. - # - # An empty target therefore means DB_HOST is not the database we think it - # is. A pending baseline on a database Flyway is already tracking means the - # history and the schema have drifted apart. Refuse either way rather than - # guess. See apps/backend/db/README.md. - name: Refuse to execute the baseline against production run: | if jq -e '.allSchemasEmpty' info.json > /dev/null; then diff --git a/.github/workflows/lambda-tests.yml b/.github/workflows/lambda-tests.yml index 36f17f43..a079c768 100644 --- a/.github/workflows/lambda-tests.yml +++ b/.github/workflows/lambda-tests.yml @@ -44,10 +44,6 @@ jobs: uses: actions/setup-node@v4 with: node-version: '20' - # Applies db/migrations to a virgin database and loads the dev seed rows - # the tests expect. Through testkit rather than Flyway: the tests build the - # schema that way too (ensureSchema/resetData), and the `checks` job below - # runs the real Flyway migration once instead of six times over. - name: Build the schema and seed working-directory: apps/backend/db run: npm ci --no-audit --no-fund && npm run reset @@ -120,8 +116,7 @@ jobs: esac echo "sha=$base" >> "$GITHUB_OUTPUT" - # Keyed by Flyway version rather than by path: what production's schema - # history pins is the version, its checksum and its script name. + # Keyed by version, not path: that is what Flyway's history pins. - name: Applied migrations are immutable if: steps.base.outputs.sha != '' env: @@ -131,8 +126,7 @@ jobs: MB=$(git merge-base "$BASE" HEAD) TAB=$(printf '\t') - # versionblobfilename, sorted by version so `join` can pair - # the two revisions up. + # versionblobname, sorted because `join` requires sorted input. list() { git ls-tree -r "$1" -- "$DIR" | while read -r _ _ sha path; do name=$(basename "$path") @@ -151,8 +145,7 @@ jobs: fail=1 fi - # The rename check applies only once a name is in Flyway form: - # adopting Flyway renamed every file exactly once. + # Rename check only for V-names: adopting Flyway renamed every file once. join -t"$TAB" -j1 "$RUNNER_TEMP/was.tsv" "$RUNNER_TEMP/now.tsv" > "$RUNNER_TEMP/both.tsv" awk -F'\t' -v dir="$DIR" ' $2 != $4 { @@ -227,12 +220,7 @@ jobs: done exit $fail - # Advisory until the Aurora DSQL cutover: the corpus still contains the - # plpgsql the rollup migration later dropped, so linting all of it can - # only ever fail. New migrations are what we can keep clean. - # - # Only added files, and with the placeholder substituted -- dsql-lint - # parses SQL, and `${async}` is not SQL. + # Advisory: the existing corpus still holds plpgsql the DSQL cutover drops. - name: Aurora DSQL compatibility (advisory) if: steps.base.outputs.sha != '' continue-on-error: true diff --git a/.github/workflows/regenerate-db-types.yaml b/.github/workflows/regenerate-db-types.yaml index f25d244f..d1585d47 100644 --- a/.github/workflows/regenerate-db-types.yaml +++ b/.github/workflows/regenerate-db-types.yaml @@ -139,8 +139,7 @@ jobs: echo "Applying db/migrations" npm run migrate --prefix apps/backend/db - # Flyway records what it applied, not which files it applied; the - # type generator refuses a schema the current migrations did not build. + # Type generation refuses a schema without this marker. npm run fingerprint --prefix apps/backend/db echo "Verifying schema application" diff --git a/apps/backend/db/Dockerfile b/apps/backend/db/Dockerfile index 765d3c38..8a513a3d 100644 --- a/apps/backend/db/Dockerfile +++ b/apps/backend/db/Dockerfile @@ -1,11 +1,4 @@ # One-shot migration runner, built from the repo root like the lambdas. -# -# Based on the Flyway image rather than bolting Flyway onto node:20-alpine: it -# already carries a JDK, the PostgreSQL JDBC driver, and /flyway/drivers, which -# is where the Aurora DSQL JDBC connector goes when we cut over. -# -# node is still here because flyway only migrates: seeding, the schema rebuild -# and type generation stay in src/cli.ts. FROM flyway/flyway:13.5.0-alpine RUN apk add --no-cache nodejs npm @@ -19,7 +12,6 @@ COPY apps/backend/db/tsconfig.json apps/backend/db/testkit.ts apps/backend/db/se COPY apps/backend/db/src ./src COPY apps/backend/db/migrations ./migrations -# The base image runs /flyway/flyway as its entrypoint, which would prepend -# `flyway` to every compose `command:`. +# The base image entrypoint would prepend `flyway` to every compose `command:`. ENTRYPOINT [] CMD ["npm", "run", "migrate"] diff --git a/apps/backend/db/flyway.sh b/apps/backend/db/flyway.sh index d6016f7f..12a39817 100755 --- a/apps/backend/db/flyway.sh +++ b/apps/backend/db/flyway.sh @@ -1,28 +1,13 @@ #!/bin/sh -# Runs the Flyway CLI against the database src/config.ts would connect to, with -# the settings this repo needs. The Makefile, CI and the migrator container all -# go through here, so the configuration exists once. -# -# ./flyway.sh migrate -# ./flyway.sh info -# ./flyway.sh validate -# -# Uses the flyway on PATH (the migrator image has one) and otherwise the pinned -# image, so a laptop and a CI runner need docker and nothing else. set -eu FLYWAY_IMAGE=flyway/flyway:13.5.0-alpine DB_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) -# Adopting the pre-Flyway database: everything at or below this version is -# already applied on production, so Flyway baselines instead of re-running it. -# It is the last migration that shipped under kysely's Migrator and never moves; -# a new migration sorts above it and applies normally. +# Last migration applied under kysely; already on production, so Flyway baselines here. BASELINE_VERSION=20260906215733 -# Same precedence as src/config.ts: DATABASE_URL wins, otherwise the discrete -# DB_* vars with the docker-compose defaults. Percent-encoded credentials in -# DATABASE_URL are not decoded -- pass those through DB_USER/DB_PASSWORD. +# Percent-encoded credentials in DATABASE_URL are not decoded -- use DB_USER/DB_PASSWORD. if [ -n "${DATABASE_URL:-}" ]; then rest=${DATABASE_URL#*://} case "$rest" in @@ -44,8 +29,6 @@ else db_name=${DB_NAME:-branch_db} fi -# TLS, mirroring src/config.ts: a CA bundle means full verification (CI reaches -# RDS over the public internet), DB_SSL alone means encrypted-but-unverified. ca_path=${DB_SSL_CA:-} if [ -n "$ca_path" ]; then ssl='?sslmode=verify-full&sslrootcert=' @@ -55,13 +38,7 @@ else ssl='' fi -# -outOfOrder: two contributors' migrations sometimes merge out of timestamp -# order (Alice authors first, Bob merges first). Without it the next deploy -# fails and can only be unblocked by hand-editing the history table. -# -cleanDisabled: `flyway clean` drops the schema. Nothing here ever wants it, -# and it is one typo away from production. -# -placeholders.async: empty on PostgreSQL, ASYNC on Aurora DSQL, which has no -# synchronous CREATE INDEX. Keeps one migration corpus valid on both engines. +# outOfOrder: PRs merge out of timestamp order. async: Aurora DSQL has no synchronous CREATE INDEX. add_flags() { set -- \ "-locations=filesystem:$1" \ @@ -87,8 +64,6 @@ else ssl="${ssl}/rds-ca.pem" ca_mount="--volume=$ca_path:/rds-ca.pem:ro" fi - # --network=host so localhost:5432 means the same thing inside the container - # as outside it -- that is where compose and the CI postgres service listen. # shellcheck disable=SC2046 set -- docker run --rm --network=host \ --volume="$DB_DIR/migrations:/db/migrations:ro" \ @@ -97,8 +72,7 @@ else $(add_flags /db/migrations) "$@" fi -# Credentials go through the environment, never argv: `docker run` arguments are -# visible to every process on the host, and Flyway echoes its own command line. +# Credentials via environment, never argv: `docker run` args are world-visible. FLYWAY_URL="jdbc:postgresql://${db_host}:${db_port}/${db_name}${ssl}" FLYWAY_USER=$db_user FLYWAY_PASSWORD=$db_password diff --git a/apps/backend/db/src/cli.ts b/apps/backend/db/src/cli.ts index 231847e6..1fb3bd9b 100644 --- a/apps/backend/db/src/cli.ts +++ b/apps/backend/db/src/cli.ts @@ -52,9 +52,7 @@ function newMigration(name?: string): void { } // UTC YYYYMMDDHHMMSS, generated so nobody hand-types one: collisions between - // concurrent PRs are then effectively impossible. `V__` - // is Flyway's naming scheme -- the double underscore is the separator, and it - // cannot be omitted. + // concurrent PRs are then effectively impossible. const stamp = new Date().toISOString().replace(/[-:T]/g, '').slice(0, 14); const file = path.join(MIGRATIONS_DIR, `V${stamp}__${name}.sql`); @@ -102,10 +100,6 @@ async function main(): Promise { await resetData(client); console.log('schema rebuilt from migrations and reseeded'); }); - // Flyway records what it applied, not which files were on disk when it ran. - // `make types` refuses to generate from a schema the current migrations did - // not build, so a real migrate run has to leave the same marker - // rebuildSchema() does. case 'fingerprint': return withClient(async (client) => { await stampFingerprint(client); diff --git a/apps/backend/db/src/generate-types.ts b/apps/backend/db/src/generate-types.ts index 63862410..6b5becf6 100644 --- a/apps/backend/db/src/generate-types.ts +++ b/apps/backend/db/src/generate-types.ts @@ -54,8 +54,7 @@ async function main(): Promise { // in all six lambdas. '--include-pattern', 'branch.*', - // kysely-codegen skips kysely_migration% by name and knows nothing about - // Flyway, so without this the history table becomes a shared DB type. + // kysely-codegen only skips kysely_migration% by name, not Flyway's table. '--exclude-pattern', `branch.${HISTORY_TABLE}`, '--out-file', diff --git a/apps/backend/db/testkit.ts b/apps/backend/db/testkit.ts index bca72558..295a7108 100644 --- a/apps/backend/db/testkit.ts +++ b/apps/backend/db/testkit.ts @@ -74,10 +74,6 @@ export const HISTORY_TABLE = 'flyway_schema_history'; * local database, the next `make migrate` would baseline at the adoption * version and re-apply every migration written since, failing on "already * exists". - * - * checksum is left NULL on purpose. Flyway only compares checksums it recorded - * itself, and nothing validates a test database, so computing Flyway's CRC32 - * here would be a second implementation to keep in step for no benefit. */ export async function stampLedger(client: Queryable): Promise { await client.query( @@ -98,8 +94,7 @@ export async function stampLedger(client: Queryable): Promise { ON ${SCHEMA}.${HISTORY_TABLE} (success)`, ); - // V__.sql -- Flyway stores the description with the - // underscores turned back into spaces. + // Flyway stores the description with the underscores turned back into spaces. const rows = migrationFiles() .map((file, index) => { const [version, description] = file From 9648c4cd55426bb486526cc61f361c924d04bf40 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 7 Sep 2026 22:50:30 -0400 Subject: [PATCH 3/6] refactor(db): rename the migration that landed with #400 #400 gained V20260907213524__project_rollup_bump_reports_hit before it merged, so it arrived in the pre-Flyway naming form. Rename it and move the adoption baseline up to it -- production has already applied it, so Flyway must baseline over it rather than re-run it. Co-Authored-By: Claude Opus 5 (1M context) --- apps/backend/db/flyway.sh | 2 +- ...sql => V20260907213524__project_rollup_bump_reports_hit.sql} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename apps/backend/db/migrations/{20260907213524_project_rollup_bump_reports_hit.sql => V20260907213524__project_rollup_bump_reports_hit.sql} (100%) diff --git a/apps/backend/db/flyway.sh b/apps/backend/db/flyway.sh index 12a39817..6693ad00 100755 --- a/apps/backend/db/flyway.sh +++ b/apps/backend/db/flyway.sh @@ -5,7 +5,7 @@ FLYWAY_IMAGE=flyway/flyway:13.5.0-alpine DB_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) # Last migration applied under kysely; already on production, so Flyway baselines here. -BASELINE_VERSION=20260906215733 +BASELINE_VERSION=20260907213524 # Percent-encoded credentials in DATABASE_URL are not decoded -- use DB_USER/DB_PASSWORD. if [ -n "${DATABASE_URL:-}" ]; then diff --git a/apps/backend/db/migrations/20260907213524_project_rollup_bump_reports_hit.sql b/apps/backend/db/migrations/V20260907213524__project_rollup_bump_reports_hit.sql similarity index 100% rename from apps/backend/db/migrations/20260907213524_project_rollup_bump_reports_hit.sql rename to apps/backend/db/migrations/V20260907213524__project_rollup_bump_reports_hit.sql From 9cff4e2d9a0c13b8de757fc47296b19b7da2f8e3 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 7 Sep 2026 23:18:45 -0400 Subject: [PATCH 4/6] fix(db): close the baseline floor and the silent-adoption holes Review of #401 found the adoption baseline is not just a one-time marker but a permanent floor, and that the deploy guard traded away a protection the old one had. Both fail silently, which is the worst shape for this. - BASELINE_VERSION is a floor, not a marker. Flyway reports any migration at or below it "Below Baseline" and never runs it: nothing pending, validate passes, migrate exits 0, column absent. CI builds from an empty schema where baselining never fires, so a PR scaffolded earlier the same day merges green and production quietly misses the change. `checks` now rejects a new migration whose version is not strictly above BASELINE_VERSION. It skips versions already on main so a rename is never mistaken for a new migration. - The out-of-order step called that case "Harmless". True above the baseline, where outOfOrder applies the migration late; false at or below it, where it is never applied at all. Reworded, and the hard failure above covers the rest. Same correction in db/README.md. - baselineOnMigrate is now off unless asked for. It only ever fires against a populated schema with no history table, which on production means a database we do not recognise -- a restored snapshot or a clone -- and adopting one silently is how you migrate the wrong thing. The deploy workflow gains an adopt_baseline input for the one-time cutover; compose sets it for local dev, where a pre-Flyway database should just be adopted. - The deploy guards used `jq -e` inside `if`. Malformed output makes jq exit non-zero, which reads as "the dangerous condition is absent" -- the opposite of the documented safe fallthrough. The preflight now validates the JSON once and fails on it, and the guards compare plain strings. - testkit applies the migration files directly, so nothing substituted Flyway's ${async} placeholder: the first migration written per the README would break `npm run reset`, `make db-reset` and every lambda's ensureSchema() with a postgres syntax error, while CI stayed green because CI goes through Flyway. testkit substitutes the placeholders itself now, and throws on an unknown one rather than silently deleting it. - The docker fallback leaned on `--network=host`, which Docker Desktop ignores unless it is switched on, and on `localhost`, which can resolve to ::1 inside the container before the host's IPv4. Added --add-host=host.docker.internal and a rewrite of loopback hostnames. Verified on PostgreSQL 16: the below-baseline skip reproduces and the new check catches it while leaving an above-baseline migration alone; adoption is refused by default and works with the opt-in; a fresh empty database still migrates in full; the guard behaves correctly across healthy/empty/untracked/adopting/ drifted/malformed; ${async} now applies through both testkit and Flyway and an unknown placeholder throws; schema still byte-identical and types unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/lambda-deploy.yml | 42 ++++++++++++++++++++++++----- .github/workflows/lambda-tests.yml | 41 +++++++++++++++++++++++++--- apps/backend/db/README.md | 33 +++++++++++++++++------ apps/backend/db/flyway.sh | 17 +++++++++++- apps/backend/db/testkit.ts | 26 +++++++++++++++--- apps/backend/docker-compose.yml | 3 +++ 6 files changed, 140 insertions(+), 22 deletions(-) diff --git a/.github/workflows/lambda-deploy.yml b/.github/workflows/lambda-deploy.yml index 2dbee8f4..86cdf389 100644 --- a/.github/workflows/lambda-deploy.yml +++ b/.github/workflows/lambda-deploy.yml @@ -15,6 +15,10 @@ on: description: 'Apply pending DB migrations without redeploying lambdas' type: boolean default: false + adopt_baseline: + description: 'One-time: let Flyway adopt a populated database that has no schema history' + type: boolean + default: false jobs: detect-changes: @@ -172,6 +176,10 @@ jobs: contents: read actions: read pull-requests: read + env: + # Never on for a push. Adopting an unrecognised database is a deliberate, + # human-triggered act -- see the guard below. + FLYWAY_BASELINE_ON_MIGRATE: ${{ inputs.adopt_baseline == true }} steps: - uses: actions/checkout@v4 - name: Setup Node.js @@ -221,22 +229,42 @@ jobs: - name: Pending migrations (read-only preflight) id: status run: | - set -o pipefail + set -euo pipefail npm run --silent migrate:info --prefix apps/backend/db -- -outputType=json > info.json + # Every check below reads this file. Unparseable output is a failure, not + # a reason to fall through -- `jq -e` on malformed JSON exits non-zero, + # which inside `if` would read as "the dangerous condition is absent". + jq -e 'type == "object" and has("migrations") and has("allSchemasEmpty")' \ + info.json > /dev/null \ + || { echo "::error::could not parse the output of flyway info"; exit 1; } + jq -r '.migrations[] | "\(.state)\t\(.version)\t\(.description)"' info.json | tee status.log - # Unknown (unexpected output) falls through as "1" so we take the safe - # path and snapshot before touching anything. - pending=$(jq '[.migrations[] | select(.state == "Pending")] | length' info.json || echo 1) - echo "pending=${pending:-1}" >> "$GITHUB_OUTPUT" + pending=$(jq '[.migrations[] | select(.state == "Pending")] | length' info.json) + echo "pending=$pending" >> "$GITHUB_OUTPUT" echo "$pending migration(s) pending" + # Production is never built by running the baseline, and it is never adopted + # by accident: baselineOnMigrate is off unless this run explicitly asks for + # it. An empty schema means DB_HOST is not the database we think it is; an + # unrecognised populated one is a restored snapshot, a clone or a hand-built + # schema. Refuse both. See apps/backend/db/README.md for the one-time + # adoption procedure. - name: Refuse to execute the baseline against production + env: + ADOPT: ${{ inputs.adopt_baseline }} run: | - if jq -e '.allSchemasEmpty' info.json > /dev/null; then + set -euo pipefail + if [ "$(jq -r '.allSchemasEmpty' info.json)" = 'true' ]; then echo "::error::schema \"branch\" is empty on the target database. Refusing to build production from scratch -- check DB_HOST." exit 1 fi - if jq -e '.schemaVersion != null and any(.migrations[]; .version == "0000" and .state == "Pending")' info.json > /dev/null; then + tracked=$(jq -r '.schemaVersion' info.json) + if [ "$tracked" = 'null' ] && [ "$ADOPT" != 'true' ]; then + echo "::error::the target database has a populated schema \"branch\" but no Flyway history. Refusing to adopt it -- re-run this workflow manually with adopt_baseline if that is genuinely production." + exit 1 + fi + if [ "$tracked" != 'null' ] \ + && jq -e 'any(.migrations[]; .version == "0000" and .state == "Pending")' info.json > /dev/null; then echo "::error::V0000__baseline_schema is PENDING on a database Flyway already tracks. The schema history and the schema have drifted -- refusing to run." exit 1 fi diff --git a/.github/workflows/lambda-tests.yml b/.github/workflows/lambda-tests.yml index a079c768..5fe15a1f 100644 --- a/.github/workflows/lambda-tests.yml +++ b/.github/workflows/lambda-tests.yml @@ -176,10 +176,45 @@ jobs: echo "::error file=$DIR/$f::name must be VYYYYMMDDHHMMSS__lower_snake_case.sql -- use 'make new-migration NAME=...' so it is generated for you" fail=1 fi - # Out-of-order merges are allowed at runtime (flyway.sh sets - # outOfOrder), so this is advice, not a failure. + # Allowed at runtime (flyway.sh sets outOfOrder) for anything above + # the baseline; at or below it the next step fails the build. if [ -n "${last:-}" ] && [ "$f" \< "$last" ]; then - echo "::warning file=$DIR/$f::sorts before $last, which is already on main, so it will be applied out of order. Harmless, but renaming it to a current timestamp keeps the history readable." + echo "::warning file=$DIR/$f::sorts before $last, which is already on main, so it will be applied out of order. Renaming it to a current UTC timestamp keeps the history readable." + fi + done + exit $fail + + # baselineOnMigrate pins production at BASELINE_VERSION, and Flyway reports + # anything at or below it "Below Baseline" -- skipped, with pending 0, a + # passing validate and an exit 0. CI builds from an empty schema, where + # baselining never fires, so nothing else can catch this. + - name: New migrations must sort above the adoption baseline + if: steps.base.outputs.sha != '' + env: + BASE: ${{ steps.base.outputs.sha }} + run: | + set -euo pipefail + baseline=$(sed -n 's/^BASELINE_VERSION=//p' apps/backend/db/flyway.sh) + case "$baseline" in + '' | *[!0-9]*) + echo "::error::could not read a numeric BASELINE_VERSION from apps/backend/db/flyway.sh" + exit 1 ;; + esac + MB=$(git merge-base "$BASE" HEAD) + version_of() { basename "$1" | sed -E 's/^V0*([0-9]+)_+.*/\1/'; } + + # Versions already on main, so a rename reported as an addition is not + # mistaken for a new migration. + git ls-tree -r --name-only "$MB" -- "$DIR" | while read -r p; do version_of "$p"; done \ + | sort -u > "$RUNNER_TEMP/known.txt" + + fail=0 + for f in $(git diff --diff-filter=A --name-only "$MB" HEAD -- "$DIR"); do + v=$(version_of "$f") + grep -qxF "$v" "$RUNNER_TEMP/known.txt" && continue + if [ "$v" -le "$baseline" ]; then + echo "::error file=$f::version $v is at or below the adoption baseline $baseline, so production would report it \"Below Baseline\" and never run it -- while CI stays green. Rename it with a current UTC timestamp." + fail=1 fi done exit $fail diff --git a/apps/backend/db/README.md b/apps/backend/db/README.md index 8ca7436a..6f355738 100644 --- a/apps/backend/db/README.md +++ b/apps/backend/db/README.md @@ -93,8 +93,19 @@ someone whose timestamp is later, it simply applies late. The alternative — th default — is a production deploy that fails and can only be unblocked by hand-editing `flyway_schema_history` in RDS. +**`outOfOrder` does not extend below the baseline.** `BASELINE_VERSION` in +`flyway.sh` is the version production was adopted at, and Flyway reports anything at +or below it as `Below Baseline` — skipped, permanently, with nothing pending, a +passing `validate` and a green deploy. CI builds from an empty schema, where +baselining never happens, so CI cannot see it either. The `checks` job therefore +fails any new migration whose version is not strictly above `BASELINE_VERSION`. Let +`make new-migration` generate the timestamp and this never comes up. + `${async}` in a migration is a Flyway placeholder. It expands to nothing on PostgreSQL and to `ASYNC` on Aurora DSQL, which has no synchronous `CREATE INDEX`. +Placeholders are Flyway's, so `testkit.ts` substitutes them itself when it applies +the files directly — add any new one to `PLACEHOLDERS` there as well as to +`flyway.sh`, or the tests fail on a postgres syntax error. ## In CI @@ -134,14 +145,20 @@ pg_dump --schema-only --schema=branch --no-owner --no-privileges --no-comments \ ``` Run that against a local database with only the baseline applied, and against the -target; `diff -u` the two. Once it's empty, run `npm run migrate` against the target -with a human watching. - -Flyway adopts it rather than replaying it: `baselineOnMigrate` writes a single -baseline row at the version pinned in `flyway.sh` when it finds a non-empty schema -with no history table, and everything at or below that version is then considered -applied. If the schemas genuinely diverge, write a follow-up migration reconciling -the difference — there is no way to record one file as applied on its own. +target; `diff -u` the two. + +Once it's empty, adopt the database: Flyway writes a single baseline row at +`BASELINE_VERSION` from `flyway.sh` instead of replaying the files, and everything at +or below that version counts as applied. If the schemas genuinely diverge, write a +follow-up migration reconciling the difference — there is no way to record one file +as applied on its own. + +Adoption is **off by default and never happens on a push**: a populated schema with +no history table is far more often a restored snapshot or a clone than the real +production database, and adopting one silently is how you end up migrating the wrong +thing. Run the `Lambda Deploy` workflow by hand with `adopt_baseline` checked, once, +with a human watching. Locally, compose sets `FLYWAY_BASELINE_ON_MIGRATE=true` so a +dev database built before Flyway is adopted without ceremony. The `migrate` job refuses to run against an **empty** target, on the assumption that it means `DB_HOST` is pointing somewhere unexpected. diff --git a/apps/backend/db/flyway.sh b/apps/backend/db/flyway.sh index 6693ad00..f1edd093 100755 --- a/apps/backend/db/flyway.sh +++ b/apps/backend/db/flyway.sh @@ -5,8 +5,16 @@ FLYWAY_IMAGE=flyway/flyway:13.5.0-alpine DB_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) # Last migration applied under kysely; already on production, so Flyway baselines here. +# Also a permanent floor: Flyway reports anything at or below it "Below Baseline" and +# never runs it. The `checks` job rejects a new migration that sorts here or lower. BASELINE_VERSION=20260907213524 +# Off unless asked. Baselining only ever fires against a non-empty schema with no +# history table, which on production means an unrecognised database -- adopting it +# silently is how a restored snapshot gets mistaken for the real one. compose sets +# it for local dev; the deploy workflow only for a deliberate one-time adoption. +BASELINE_ON_MIGRATE=${FLYWAY_BASELINE_ON_MIGRATE:-false} + # Percent-encoded credentials in DATABASE_URL are not decoded -- use DB_USER/DB_PASSWORD. if [ -n "${DATABASE_URL:-}" ]; then rest=${DATABASE_URL#*://} @@ -46,7 +54,7 @@ add_flags() { -defaultSchema=branch \ -createSchemas=true \ -outOfOrder=true \ - -baselineOnMigrate=true \ + "-baselineOnMigrate=$BASELINE_ON_MIGRATE" \ "-baselineVersion=$BASELINE_VERSION" \ -validateMigrationNaming=true \ -cleanDisabled=true \ @@ -64,8 +72,15 @@ else ssl="${ssl}/rds-ca.pem" ca_mount="--volume=$ca_path:/rds-ca.pem:ro" fi + # Docker Desktop ignores --network=host unless it is switched on, and inside the + # container `localhost` can resolve to ::1 before the host's IPv4. host-gateway + # reaches the host either way. + case "$db_host" in + localhost | 127.0.0.1 | ::1) db_host=host.docker.internal ;; + esac # shellcheck disable=SC2046 set -- docker run --rm --network=host \ + --add-host=host.docker.internal:host-gateway \ --volume="$DB_DIR/migrations:/db/migrations:ro" \ --env=FLYWAY_URL --env=FLYWAY_USER --env=FLYWAY_PASSWORD \ ${ca_mount:+"$ca_mount"} "$FLYWAY_IMAGE" \ diff --git a/apps/backend/db/testkit.ts b/apps/backend/db/testkit.ts index 295a7108..b70f948f 100644 --- a/apps/backend/db/testkit.ts +++ b/apps/backend/db/testkit.ts @@ -37,14 +37,34 @@ function migrationFiles(): string[] { .sort(); } +/** + * Flyway placeholder values for PostgreSQL. Must stay in step with the + * `-placeholders.*` flags in flyway.sh: tests apply the migration files + * directly, so nothing substitutes these for us and `${async}` would reach + * postgres as a syntax error. + */ +const PLACEHOLDERS: Record = { async: '' }; + +function substitutePlaceholders(sql: string, file: string): string { + return sql.replace(/\$\{(\w+)\}/g, (match, name: string) => { + const value = PLACEHOLDERS[name]; + if (value === undefined) { + throw new Error( + `${file}: unknown Flyway placeholder ${match} -- define it in flyway.sh and in PLACEHOLDERS here`, + ); + } + return value; + }); +} + let allSql: string | undefined; function allMigrationSql(): string { allSql ??= migrationFiles() .map( (file) => - `-- ${file}\n${fs.readFileSync( - path.join(MIGRATIONS_DIR, file), - 'utf8', + `-- ${file}\n${substitutePlaceholders( + fs.readFileSync(path.join(MIGRATIONS_DIR, file), 'utf8'), + file, )}`, ) .join('\n'); diff --git a/apps/backend/docker-compose.yml b/apps/backend/docker-compose.yml index e55699dc..55dab299 100644 --- a/apps/backend/docker-compose.yml +++ b/apps/backend/docker-compose.yml @@ -43,6 +43,9 @@ services: DB_USER: ${DB_USER:-branch_dev} DB_PASSWORD: ${DB_PASSWORD:-password} DB_NAME: ${DB_NAME:-branch_db} + # Adopt a local database built before Flyway instead of failing. Safe here + # and deliberately not the default -- see apps/backend/db/flyway.sh. + FLYWAY_BASELINE_ON_MIGRATE: 'true' command: sh -c "npm run migrate && npm run fingerprint && npm run seed -- --if-empty" depends_on: postgres: From f898f26758b687fca2caca5455599115fe913b11 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 7 Sep 2026 23:21:04 -0400 Subject: [PATCH 5/6] chore: trim comments Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/lambda-deploy.yml | 13 ++----------- .github/workflows/lambda-tests.yml | 11 +++-------- apps/backend/db/flyway.sh | 12 +++--------- apps/backend/db/testkit.ts | 7 +------ apps/backend/docker-compose.yml | 3 +-- 5 files changed, 10 insertions(+), 36 deletions(-) diff --git a/.github/workflows/lambda-deploy.yml b/.github/workflows/lambda-deploy.yml index 86cdf389..cfef17e2 100644 --- a/.github/workflows/lambda-deploy.yml +++ b/.github/workflows/lambda-deploy.yml @@ -177,8 +177,6 @@ jobs: actions: read pull-requests: read env: - # Never on for a push. Adopting an unrecognised database is a deliberate, - # human-triggered act -- see the guard below. FLYWAY_BASELINE_ON_MIGRATE: ${{ inputs.adopt_baseline == true }} steps: - uses: actions/checkout@v4 @@ -231,9 +229,7 @@ jobs: run: | set -euo pipefail npm run --silent migrate:info --prefix apps/backend/db -- -outputType=json > info.json - # Every check below reads this file. Unparseable output is a failure, not - # a reason to fall through -- `jq -e` on malformed JSON exits non-zero, - # which inside `if` would read as "the dangerous condition is absent". + # `jq -e` on malformed JSON exits non-zero, which inside `if` reads as "condition absent". jq -e 'type == "object" and has("migrations") and has("allSchemasEmpty")' \ info.json > /dev/null \ || { echo "::error::could not parse the output of flyway info"; exit 1; } @@ -243,12 +239,7 @@ jobs: echo "pending=$pending" >> "$GITHUB_OUTPUT" echo "$pending migration(s) pending" - # Production is never built by running the baseline, and it is never adopted - # by accident: baselineOnMigrate is off unless this run explicitly asks for - # it. An empty schema means DB_HOST is not the database we think it is; an - # unrecognised populated one is a restored snapshot, a clone or a hand-built - # schema. Refuse both. See apps/backend/db/README.md for the one-time - # adoption procedure. + # Empty schema means the wrong DB_HOST; populated with no history means a snapshot or clone -- refuse both. - name: Refuse to execute the baseline against production env: ADOPT: ${{ inputs.adopt_baseline }} diff --git a/.github/workflows/lambda-tests.yml b/.github/workflows/lambda-tests.yml index 5fe15a1f..9a9ff629 100644 --- a/.github/workflows/lambda-tests.yml +++ b/.github/workflows/lambda-tests.yml @@ -176,18 +176,14 @@ jobs: echo "::error file=$DIR/$f::name must be VYYYYMMDDHHMMSS__lower_snake_case.sql -- use 'make new-migration NAME=...' so it is generated for you" fail=1 fi - # Allowed at runtime (flyway.sh sets outOfOrder) for anything above - # the baseline; at or below it the next step fails the build. + # Warning only: outOfOrder is set at runtime; the next step fails anything at or below the baseline. if [ -n "${last:-}" ] && [ "$f" \< "$last" ]; then echo "::warning file=$DIR/$f::sorts before $last, which is already on main, so it will be applied out of order. Renaming it to a current UTC timestamp keeps the history readable." fi done exit $fail - # baselineOnMigrate pins production at BASELINE_VERSION, and Flyway reports - # anything at or below it "Below Baseline" -- skipped, with pending 0, a - # passing validate and an exit 0. CI builds from an empty schema, where - # baselining never fires, so nothing else can catch this. + # Flyway skips anything at or below the baseline as "Below Baseline" with exit 0; CI's empty schema never baselines, so nothing else catches it. - name: New migrations must sort above the adoption baseline if: steps.base.outputs.sha != '' env: @@ -203,8 +199,7 @@ jobs: MB=$(git merge-base "$BASE" HEAD) version_of() { basename "$1" | sed -E 's/^V0*([0-9]+)_+.*/\1/'; } - # Versions already on main, so a rename reported as an addition is not - # mistaken for a new migration. + # Renames show up as additions, so skip versions already on main. git ls-tree -r --name-only "$MB" -- "$DIR" | while read -r p; do version_of "$p"; done \ | sort -u > "$RUNNER_TEMP/known.txt" diff --git a/apps/backend/db/flyway.sh b/apps/backend/db/flyway.sh index f1edd093..63a33c4e 100755 --- a/apps/backend/db/flyway.sh +++ b/apps/backend/db/flyway.sh @@ -5,14 +5,10 @@ FLYWAY_IMAGE=flyway/flyway:13.5.0-alpine DB_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) # Last migration applied under kysely; already on production, so Flyway baselines here. -# Also a permanent floor: Flyway reports anything at or below it "Below Baseline" and -# never runs it. The `checks` job rejects a new migration that sorts here or lower. +# Also a permanent floor: anything at or below it is "Below Baseline" and never runs. BASELINE_VERSION=20260907213524 -# Off unless asked. Baselining only ever fires against a non-empty schema with no -# history table, which on production means an unrecognised database -- adopting it -# silently is how a restored snapshot gets mistaken for the real one. compose sets -# it for local dev; the deploy workflow only for a deliberate one-time adoption. +# Off by default: on production, baselining would silently adopt an unrecognised database. BASELINE_ON_MIGRATE=${FLYWAY_BASELINE_ON_MIGRATE:-false} # Percent-encoded credentials in DATABASE_URL are not decoded -- use DB_USER/DB_PASSWORD. @@ -72,9 +68,7 @@ else ssl="${ssl}/rds-ca.pem" ca_mount="--volume=$ca_path:/rds-ca.pem:ro" fi - # Docker Desktop ignores --network=host unless it is switched on, and inside the - # container `localhost` can resolve to ::1 before the host's IPv4. host-gateway - # reaches the host either way. + # Docker Desktop ignores --network=host unless enabled; host-gateway reaches the host either way. case "$db_host" in localhost | 127.0.0.1 | ::1) db_host=host.docker.internal ;; esac diff --git a/apps/backend/db/testkit.ts b/apps/backend/db/testkit.ts index b70f948f..43266b8e 100644 --- a/apps/backend/db/testkit.ts +++ b/apps/backend/db/testkit.ts @@ -37,12 +37,7 @@ function migrationFiles(): string[] { .sort(); } -/** - * Flyway placeholder values for PostgreSQL. Must stay in step with the - * `-placeholders.*` flags in flyway.sh: tests apply the migration files - * directly, so nothing substitutes these for us and `${async}` would reach - * postgres as a syntax error. - */ +// Must stay in step with the `-placeholders.*` flags in flyway.sh; tests apply the SQL directly. const PLACEHOLDERS: Record = { async: '' }; function substitutePlaceholders(sql: string, file: string): string { diff --git a/apps/backend/docker-compose.yml b/apps/backend/docker-compose.yml index 55dab299..27656482 100644 --- a/apps/backend/docker-compose.yml +++ b/apps/backend/docker-compose.yml @@ -43,8 +43,7 @@ services: DB_USER: ${DB_USER:-branch_dev} DB_PASSWORD: ${DB_PASSWORD:-password} DB_NAME: ${DB_NAME:-branch_db} - # Adopt a local database built before Flyway instead of failing. Safe here - # and deliberately not the default -- see apps/backend/db/flyway.sh. + # Local only: adopt a dev database built before Flyway. FLYWAY_BASELINE_ON_MIGRATE: 'true' command: sh -c "npm run migrate && npm run fingerprint && npm run seed -- --if-empty" depends_on: From 648b8d971cc35ce2649b721d80d37635764173d2 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 7 Sep 2026 23:25:06 -0400 Subject: [PATCH 6/6] fix(db): only reroute loopback to the gateway on Docker Desktop The unconditional rewrite broke Auto-regenerate DB Types: that workflow runs postgres on the runner itself, bound to loopback, so host.docker.internal resolves to the docker0 gateway and the connection is refused. On a native daemon --network=host is real and localhost is already correct; it is only Docker Desktop, which ignores --network=host unless switched on, that needs the gateway. Gate the rewrite on the daemon actually being Desktop. Co-Authored-By: Claude Opus 5 (1M context) --- apps/backend/db/flyway.sh | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/apps/backend/db/flyway.sh b/apps/backend/db/flyway.sh index 63a33c4e..1f42b8d6 100755 --- a/apps/backend/db/flyway.sh +++ b/apps/backend/db/flyway.sh @@ -68,10 +68,14 @@ else ssl="${ssl}/rds-ca.pem" ca_mount="--volume=$ca_path:/rds-ca.pem:ro" fi - # Docker Desktop ignores --network=host unless enabled; host-gateway reaches the host either way. - case "$db_host" in - localhost | 127.0.0.1 | ::1) db_host=host.docker.internal ;; - esac + # Docker Desktop ignores --network=host unless it is switched on, so a loopback + # host has to go via the gateway. Only there: on a native daemon --network=host + # is real, and the gateway address misses a server bound to loopback only. + if docker info --format '{{.OperatingSystem}}' 2>/dev/null | grep -qi 'docker desktop'; then + case "$db_host" in + localhost | 127.0.0.1 | ::1) db_host=host.docker.internal ;; + esac + fi # shellcheck disable=SC2046 set -- docker run --rm --network=host \ --add-host=host.docker.internal:host-gateway \