diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1986edb43..86bee9632 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -311,6 +311,12 @@ jobs: # through tests/helpers/factories (MikroORM) instead. run: cd server && npm run lint:test-sql + - name: QueryBuilder calls in the repositories may only shrink + # Counts this.qb(...)/createQueryBuilder(...) per file under + # src/db/repositories/ against scripts/query-api-baseline.json. New + # code uses Kysely for join reads and the entity methods for CRUD. + run: cd server && npm run lint:query-api + - name: Open request schemas may only shrink # Needs the shared build above. Counts open shapes (records of # unknown, passthrough, any) in shared's request schemas. diff --git a/CLAUDE.md b/CLAUDE.md index 7e66c3989..bef14f338 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,6 +52,7 @@ cd server && npm run lint:dialect # SQLite-only SQL spellings m cd server && npm run probe:pg # dialect helpers + the repository statements it reaches against Postgres (CI job; needs TREK_PG_PROBE_URL) cd server && npm run lint:tx # no new method writing twice outside one transaction (CI gate) cd server && npm run lint:test-sql # raw SQL fixtures in tests/ may only shrink; seed through tests/helpers/factories (CI gate) +cd server && npm run lint:query-api # MikroORM QueryBuilder calls in the repositories may only shrink (CI gate) cd client && npm run theme:lint # flag styling that bypasses appearance tokens (local only) npm run typecheck --workspace=server # tsc --noEmit (also client/shared; server also has typecheck:tests) ``` @@ -126,7 +127,7 @@ These principles come out of a full-repo audit and shape all new code: ## Quality gates in CI -`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client and server through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:strict`, `lint:boundaries`, `lint:tx`, `lint:dialect` and `lint:test-sql`, the client's and server's `lint:format`, `lint:layers`, `lint:offline`, `lint:dup`, `lint:pairs`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and the statements every repository method reaches with sample arguments against a Postgres service; the SQL errors per method and the list of methods it cannot measure are held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `lint:format` on `shared/`, both check-only. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. +`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client and server through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:strict`, `lint:boundaries`, `lint:tx`, `lint:dialect`, `lint:test-sql` and `lint:query-api`, the client's and server's `lint:format`, `lint:layers`, `lint:offline`, `lint:dup`, `lint:pairs`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and the statements every repository method reaches with sample arguments against a Postgres service; the SQL errors per method and the list of methods it cannot measure are held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `lint:format` on `shared/`, both check-only. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. - **`ci-ok` is the required check.** The branch ruleset requires `ci-ok` (plus `lint` and `check-target` from their own workflows), and `ci-ok` needs every job above except the scan. It fails if any of them failed or was cancelled and passes if they passed or were skipped. A new gate job counts only once it is listed in `ci-ok`'s `needs:`. - **Guides name paths that exist**: the `changes` job runs `scripts/ci/doc-paths.mjs`, which resolves every backtick path in the CLAUDE.md files, `README.md`, `server/src/nest/README.md` and the wiki against the tracked files; the missing ones are listed in `scripts/ci/doc-paths-baseline.json`, which only shrinks (`--update` after a fix). diff --git a/server/CLAUDE.md b/server/CLAUDE.md index c866bd851..1bf35508b 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -28,6 +28,7 @@ npm run lint:dialect # SQLite-only SQL spellings under src/ may only shrink npm run probe:pg # dialect helpers + the repository statements it reaches against Postgres (CI job postgres-probe; needs TREK_PG_PROBE_URL) npm run lint:tx # methods writing more than once outside one transaction may only shrink (CI gate) npm run lint:test-sql # raw better-sqlite3 statements in tests/ may only shrink (CI gate) +npm run lint:query-api # MikroORM QueryBuilder calls in the repositories may only shrink (CI gate) npm run contracts:dto-open # open shapes in the createZodDto schemas may only shrink (runs in the unit tests) ``` @@ -43,6 +44,8 @@ npm run contracts:dto-open # open shapes in the createZodDto schemas may only sh **`lint:test-sql`** (`scripts/test-sql-ratchet.mjs`) counts every `.prepare(` under `tests/` per file against `scripts/test-sql-baseline.json`: a file may hold at most its entry, a file without one none. Growth fails, and so does an entry above what its file holds now, so a conversion lands with `npm run lint:test-sql -- --update` (it only lowers and drops entries). Files whose subject is raw SQL (the dialect functions, PRAGMA introspection, the legacy upgrade fixtures) sit in the baseline's `exempt` list with a reason; a single statement that must stay raw carries a `test-sql-allow: ` comment on its line or the line above. `tests/unit/scripts/test-sql-ratchet.test.ts` also runs it on the tree. +**`lint:query-api`** (`scripts/query-api-ratchet.mjs`) counts every `.qb(` and `createQueryBuilder(` under `src/db/repositories/` per file (the `TrekRepository` base that defines `qb` is left out) against `scripts/query-api-baseline.json`: a file may hold at most its entry, a file without one none, and an entry above what its file holds now fails until `npm run lint:query-api -- --update` lowers it (it never raises or adds one). The rule it holds is "Which query API" under SQL dialect below. + Single test: `npx vitest run tests/unit/nest/weather.controller.test.ts`, or `npx vitest run -t "returns 401 without cookie"`. **Build emits even with type errors** (`scripts/build.mjs` catches `tsc` failures and warns). `npm run build` succeeding does **not** mean the code typechecks — run `npm run typecheck`. @@ -79,12 +82,17 @@ Nest owns everything. Every domain is a DI module under `src/nest//` (`c SQLite is the engine today and Postgres is the planned second one. Code under `src/` is written so that a Postgres port only adds a branch to the dialect layer, never a sweep through the repositories: -- **Build statements, don't spell them.** Use the MikroORM QueryBuilder or entity methods, or `this.kysely>()` on the generated table types. Repositories and seeders may not hold SQL text at all (ESLint), and `lint:dialect` holds every other file to its baseline. +- **Build statements, don't spell them.** Use the entity methods or `this.kysely>()` on the generated table types. Repositories and seeders may not hold SQL text at all (ESLint), and `lint:dialect` holds every other file to its baseline. +- **Which query API.** New repository code picks by the shape of the statement, not by what the file already uses: + - **Kysely** (`this.kysely>()`) for every read that joins, aggregates, groups or projects columns across tables, and for set-based writes (a statement over many rows, a subquery, `RETURNING`). Its types come from the migrated schema, so a renamed column fails `tsc`. + - **The entity methods** (`findOne`/`find`/`count`/`insert`/`nativeUpdate`/`nativeDelete`/`upsert`) for single-table CRUD by key or by a plain filter. They carry the `TrekRepository` request-context check and the identity-map default. + - **No MikroORM QueryBuilder** (`this.qb(...)`, `createQueryBuilder(...)`) in new code. The existing calls stay until their method is touched for another reason, then move to one of the two above; there is no sweep. `lint:query-api` holds the count per file. - **Insert or ignore / upsert**: `.onConflict((oc) => oc.columns([...]).doNothing())` or `.doUpdateSet({...})`, or `em.upsert`. Name the conflict target where there is one. Never `INSERT OR IGNORE`/`INSERT OR REPLACE`. - **The new id**: `.returning('id').executeTakeFirstOrThrow()`. Never `InsertResult.insertId` or `last_insert_rowid()`; both are SQLite's rowid and undefined on Postgres. - **Time**: `currentTimestamp`/`currentTimestampKysely`, `nowMinusDays`, `nowMinusHours`, `nowPlusSeconds`/`nowPlusSecondsKysely`, `nowDateOffset`, `dateOf`, `dateAdd`, `unixEpochToIsoKysely` from `src/db/dialect/sql-functions.ts` (the MikroORM `raw()` helpers) and `src/db/dialect/kysely-functions.ts` (their Kysely-expression twins). Never `datetime('now')`, `strftime` or `julianday`. - **Strings and casts**: `concat`/`concatKysely` instead of `||`, `substring`/`substringKysely`, `lower`/`lowerTrim`, `collateNoCase`, `castInteger`/`castIntegerKysely`, and `startsWithIsoDate`/`startsWithIsoDateKysely` instead of `GLOB`. -- **JSON columns** are read whole and parsed in the service; no `json_extract` in SQL. +- **JSON columns** are read whole and parsed in the service; no `json_extract` in SQL. Each one is declared once in `src/db/json-columns.ts` (its Zod shape and the fallback a reader gets for an empty or broken row) and read with `decodeJson`/`decodeJsonResult`, written from a server-built value with `encodeJson` (`src/utils/json-column.ts`), never with a bare `JSON.parse` at the call site: one column, one answer to a broken row, logged. +- **Timestamps** in a DATETIME column are the canonical `YYYY-MM-DD HH:MM:SS` UTC text, never `toISOString()`: in SQL the two spellings do not compare. A value written from JS is a `DbTimestamp` from `dbNow()` (`src/db/types`), and a stored value is read back as an instant with `parseDbTimestamp()` (a bare `new Date()` reads the canonical text as local time). Where a response has always carried the ISO spelling, convert on the way out (`utcSuffix`), not in the column. - **PRAGMA** belongs to the `src/db/` lifecycle files (`database.ts`, `connection.ts`, `durability.ts`, the migrations). `MaintenanceRepository` is the baselined SQLite maintenance adapter (`wal_checkpoint`, `VACUUM INTO`). - **A missing helper** goes into `sql-functions.ts` (or `kysely-functions.ts` for a Kysely expression), dispatching on the live platform through `src/db/dialect/platform.ts`: a SQLite branch and a Postgres branch (one spelling under `isKnownPlatform` where both engines read it alike), `unsupported(platform)` for anything else. The Postgres branch keeps the SQLite storage model, so a clock helper renders the same `YYYY-MM-DD HH:MM:SS` UTC text. Pin the Postgres text in `tests/unit/db/dialect/sql-functions.postgres.test.ts` and add a case to `scripts/pg-probe/helper-cases.ts` so the probe runs it on both engines. `src/db/dialect/` is the only place allowed to spell an engine's own SQL. - **Migrations** that have shipped are frozen as they are. A new migration's data statements follow the rules above; its DDL is written for the engine it runs on. diff --git a/server/package.json b/server/package.json index 2bf627fd8..9a0a610eb 100644 --- a/server/package.json +++ b/server/package.json @@ -23,6 +23,7 @@ "lint:dialect": "node scripts/dialect-lint.mjs", "lint:tx": "node scripts/tx-writes.mjs", "lint:test-sql": "node scripts/test-sql-ratchet.mjs", + "lint:query-api": "node scripts/query-api-ratchet.mjs", "contracts:dto-open": "node scripts/dto-open-shapes.mjs", "gen:plugin-facts": "node --import tsx scripts/gen-plugin-facts.ts", "check:plugin-facts": "node --import tsx scripts/gen-plugin-facts.ts --check", diff --git a/server/scripts/query-api-baseline.json b/server/scripts/query-api-baseline.json new file mode 100644 index 000000000..9ef394cb0 --- /dev/null +++ b/server/scripts/query-api-baseline.json @@ -0,0 +1,49 @@ +{ + "counts": { + "src/db/repositories/_shared/packing-visibility.ts": 1, + "src/db/repositories/_shared/reservation-travelers-owns.ts": 1, + "src/db/repositories/AssignmentParticipants.repository.ts": 3, + "src/db/repositories/BudgetItems.repository.ts": 1, + "src/db/repositories/Categories.repository.ts": 1, + "src/db/repositories/CollabMessageReactions.repository.ts": 2, + "src/db/repositories/CollabPollVotes.repository.ts": 1, + "src/db/repositories/DawarichConnections.repository.ts": 4, + "src/db/repositories/DawarichVisitSuggestions.repository.ts": 4, + "src/db/repositories/DayAccommodations.repository.ts": 12, + "src/db/repositories/DayAssignments.repository.ts": 20, + "src/db/repositories/DayNotes.repository.ts": 6, + "src/db/repositories/Days.repository.ts": 7, + "src/db/repositories/InviteTokens.repository.ts": 3, + "src/db/repositories/JourneyBooks.repository.ts": 3, + "src/db/repositories/JourneyContributors.repository.ts": 3, + "src/db/repositories/JourneyEntries.repository.ts": 9, + "src/db/repositories/JourneyPhotos.repository.ts": 6, + "src/db/repositories/Journeys.repository.ts": 4, + "src/db/repositories/JourneyShareTokens.repository.ts": 4, + "src/db/repositories/JourneyTrips.repository.ts": 1, + "src/db/repositories/McpTokens.repository.ts": 5, + "src/db/repositories/OauthTokens.repository.ts": 3, + "src/db/repositories/PlaceRatings.repository.ts": 1, + "src/db/repositories/Places.repository.ts": 28, + "src/db/repositories/PlaceShadowPicks.repository.ts": 3, + "src/db/repositories/PushSubscriptions.repository.ts": 1, + "src/db/repositories/ReservationDayPositions.repository.ts": 1, + "src/db/repositories/ReservationEndpoints.repository.ts": 5, + "src/db/repositories/Reservations.repository.ts": 14, + "src/db/repositories/ReservationTravelers.repository.ts": 2, + "src/db/repositories/RoadtripDayBoundaries.repository.ts": 3, + "src/db/repositories/RoadtripDayTracks.repository.ts": 1, + "src/db/repositories/RoadtripPreferences.repository.ts": 1, + "src/db/repositories/RoadtripVias.repository.ts": 6, + "src/db/repositories/Tags.repository.ts": 3, + "src/db/repositories/TodoItems.repository.ts": 2, + "src/db/repositories/TripDocumentLinks.repository.ts": 5, + "src/db/repositories/TripInviteTokens.repository.ts": 4, + "src/db/repositories/TripMembers.repository.ts": 8, + "src/db/repositories/TripPhotos.repository.ts": 1, + "src/db/repositories/Trips.repository.ts": 14, + "src/db/repositories/Users.repository.ts": 1, + "src/db/repositories/VacayPlanMembers.repository.ts": 3, + "src/db/repositories/VacayShares.repository.ts": 2 + } +} diff --git a/server/scripts/query-api-ratchet.mjs b/server/scripts/query-api-ratchet.mjs new file mode 100644 index 000000000..2f01c837b --- /dev/null +++ b/server/scripts/query-api-ratchet.mjs @@ -0,0 +1,155 @@ +#!/usr/bin/env node +/* + * lint:query-api: MikroORM QueryBuilder calls in the repositories may only shrink. + * + * A repository has three ways to build a statement: the entity methods + * (findOne/find/insert/nativeUpdate/nativeDelete/upsert/count), Kysely on the + * generated table types (`this.kysely>()`), and MikroORM's + * QueryBuilder (`this.qb(...)`, `createQueryBuilder(...)`). server/CLAUDE.md + * ("Which query API") settles which one new code uses: Kysely for a read that + * joins, aggregates or projects, the entity methods for single-table CRUD, + * and no QueryBuilder. The ones already written stay until their method is + * touched; this check makes sure they only recede. + * + * Every `.qb(` and `createQueryBuilder(` under src/db/repositories/ is + * counted per file (the TrekRepository base that defines `qb` is left out). + * A file may hold at most its entry in scripts/query-api-baseline.json, and a + * file without an entry none. An entry above what its file holds now, or for + * a file that is gone, fails too until --update lowers it. + * + * npm run lint:query-api check against the baseline (CI) + * npm run lint:query-api -- --update lower the counts to what the files hold now; + * it never raises an entry and never adds one + * + * --dir= points the check at another server root (the unit tests use it). + */ +import { existsSync, readdirSync, readFileSync, realpathSync, statSync, writeFileSync } from 'node:fs'; +import { join, relative, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** The tree the check walks, relative to the server root. */ +export const ROOT = 'src/db/repositories'; + +/** The baseline, relative to the server root. */ +export const BASELINE = 'scripts/query-api-baseline.json'; + +/** Files that define the QueryBuilder entry point rather than use it. */ +export const IGNORED = new Set(['src/db/repositories/_shared/trek-repository.ts']); + +const SOURCE = /\.ts$/; +const QB_CALL = /\.qb\(|\bcreateQueryBuilder\(/g; + +/** The QueryBuilder calls in one file's text, comments included. */ +export function countQueryBuilderCalls(text) { + return text.match(QB_CALL)?.length ?? 0; +} + +function walk(dir, files = []) { + for (const name of readdirSync(dir)) { + const path = join(dir, name); + if (statSync(path).isDirectory()) walk(path, files); + else if (SOURCE.test(name)) files.push(path); + } + return files; +} + +/** QueryBuilder counts keyed by the server-relative POSIX path, files without any left out. */ +export function scan(serverDir) { + const dir = join(serverDir, ROOT); + if (!existsSync(dir) || !statSync(dir).isDirectory()) { + throw new Error(`${ROOT}/ does not exist under ${serverDir}: the check would pass without looking at anything`); + } + const counts = {}; + for (const path of walk(dir)) { + const file = relative(serverDir, path).split('\\').join('/'); + if (IGNORED.has(file)) continue; + const n = countQueryBuilderCalls(readFileSync(path, 'utf8')); + if (n > 0) counts[file] = n; + } + return counts; +} + +/** The baseline as committed; missing or malformed stops the run instead of reading as empty. */ +export function readBaseline(path) { + let parsed; + try { + parsed = JSON.parse(readFileSync(path, 'utf8')); + } catch (err) { + throw new Error(`${BASELINE} cannot be read: ${err.message}`); + } + const isObject = (v) => v !== null && typeof v === 'object' && !Array.isArray(v); + if (!isObject(parsed) || !isObject(parsed.counts)) { + throw new Error(`${BASELINE} must be an object with a "counts" object`); + } + for (const [file, n] of Object.entries(parsed.counts)) { + if (!Number.isInteger(n) || n <= 0) { + throw new Error(`${BASELINE}: ${file} holds ${JSON.stringify(n)}, expected a whole number above 0`); + } + } + return parsed; +} + +/** The counts lowered to what the files hold now. Never raises an entry, never adds one. */ +export function lowerCounts(baseline, counts) { + const lowered = {}; + for (const [file, allowed] of Object.entries(baseline.counts)) { + const now = counts[file] ?? 0; + if (now > 0) lowered[file] = Math.min(allowed, now); + } + return Object.fromEntries(Object.entries(lowered).sort(([a], [b]) => a.localeCompare(b))); +} + +/** Files over their allowance, and entries above what their file holds now. */ +export function compare(baseline, counts) { + const grown = Object.entries(counts).filter(([file, n]) => n > (baseline.counts[file] ?? 0)); + const lowered = lowerCounts(baseline, counts); + const stale = Object.entries(baseline.counts).filter(([file, n]) => lowered[file] !== n); + return { grown, stale }; +} + +function main(argv) { + const dirArg = argv.find((a) => a.startsWith('--dir=')); + const serverDir = dirArg ? resolve(dirArg.slice('--dir='.length)) : fileURLToPath(new URL('..', import.meta.url)); + const baselinePath = join(serverDir, BASELINE); + + const counts = scan(serverDir); + const baseline = readBaseline(baselinePath); + if (argv.includes('--update')) { + baseline.counts = lowerCounts(baseline, counts); + writeFileSync(baselinePath, JSON.stringify(baseline, null, 2) + '\n'); + } + + const { grown, stale } = compare(baseline, counts); + for (const [file, n] of grown) { + const entry = baseline.counts[file]; + console.error( + `FAIL ${file}: ${n} QueryBuilder call(s), ` + + (entry ? `its baseline is ${entry}. ` : 'a file without a baseline entry may hold none. ') + + 'Use Kysely for a join or aggregate read and the entity methods for single-table CRUD (server/CLAUDE.md).', + ); + } + for (const [file, entry] of stale) { + console.error( + `FAIL ${file} is held at ${entry} in ${BASELINE}, ` + + (file in counts ? `but it has ${counts[file]} now.` : 'but it has none left (or the file is gone).'), + ); + } + if (stale.length) { + console.error('Run npm run lint:query-api -- --update to lower the baseline with the change that made it smaller.'); + } + const total = Object.values(counts).reduce((sum, n) => sum + n, 0); + console.log(`query-api: ${total} QueryBuilder call(s) in ${Object.keys(counts).length} file(s) held at their baseline`); + return grown.length || stale.length ? 1 : 0; +} + +// Compared by real path, so the check still runs when the script is started through a symlink. +const isCli = + Boolean(process.argv[1]) && existsSync(process.argv[1]) && realpathSync(process.argv[1]) === fileURLToPath(import.meta.url); +if (isCli) { + try { + process.exitCode = main(process.argv.slice(2)); + } catch (err) { + console.error(`FAIL ${err.message}`); + process.exitCode = 1; + } +} diff --git a/server/src/db/repositories/_shared/trek-repository.ts b/server/src/db/repositories/_shared/trek-repository.ts index 6caba40a4..6ced110ee 100644 --- a/server/src/db/repositories/_shared/trek-repository.ts +++ b/server/src/db/repositories/_shared/trek-repository.ts @@ -70,12 +70,15 @@ import { * option itself any more. See * https://mikro-orm.io/docs/entity-manager#disableidentitymap. * - * **Kysely.** `em.getKysely()` (used for the one or two statements per - * repository the QueryBuilder cannot express — a recursive CTE, a - * `DELETE … RETURNING`) resolves its `EntityManager` with `getContext(false)` - * too (`node_modules/@mikro-orm/sql/SqlEntityManager.js`), so it has the - * exact same gap as the write paths above. {@link TrekRepository.kysely} is - * the one place a repository reaches Kysely from now on, instead of calling + * **Kysely.** `em.getKysely()` is the query API for every read that joins, + * aggregates or projects across tables and for set-based writes, typed + * against the generated `DB` from `db/kysely/db.ts`; the entity methods cover + * single-table CRUD, and new code adds no QueryBuilder calls (server/CLAUDE.md, + * "Which query API"; `lint:query-api` holds the existing ones). It resolves its + * `EntityManager` with `getContext(false)` too + * (`node_modules/@mikro-orm/sql/SqlEntityManager.js`), so it has the exact + * same gap as the write paths above. {@link TrekRepository.kysely} is the one + * place a repository reaches Kysely, instead of calling * `this.getEntityManager().getKysely()` directly. * * See https://mikro-orm.io/docs/repositories (custom repositories) and @@ -95,11 +98,9 @@ export abstract class TrekRepository extends EntityReposi } /** - * `em.getKysely()`, validated first. The QueryBuilder has no recursive-CTE - * support and its `DELETE … RETURNING` is silently dropped - * (`WebauthnChallenges.repository.ts::claimChallenge`'s docstring), so a - * handful of methods across the program need this escape hatch — always - * through here now, never `this.getEntityManager().getKysely()` directly. + * `em.getKysely()`, validated first: the query API for join, aggregate and + * set-based statements (see the class docstring), always reached through + * here, never through `this.getEntityManager().getKysely()` directly. * No explicit return-type annotation: `SqlEntityManager.getKysely`'s * return type is expressed in terms of `this` (the entity list the calling * `EntityManager` was constructed with), which does not survive being diff --git a/server/tests/unit/scripts/query-api-ratchet.test.ts b/server/tests/unit/scripts/query-api-ratchet.test.ts new file mode 100644 index 000000000..597107a3d --- /dev/null +++ b/server/tests/unit/scripts/query-api-ratchet.test.ts @@ -0,0 +1,106 @@ +/** + * scripts/query-api-ratchet.mjs holds the MikroORM QueryBuilder calls in the + * repositories to their baseline (server/CLAUDE.md, "Which query API"). It + * runs the real script against throwaway server roots (--dir) and once on this + * repository, so the committed baseline is checked by the unit suite too. + */ +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; + +const SCRIPT = path.join(__dirname, '../../../scripts/query-api-ratchet.mjs'); +const SERVER_ROOT = path.join(__dirname, '../../..'); +const REPOS = 'src/db/repositories'; + +// Spelled in pieces so this file reads as what it is, a fixture. +const QB = ['this.', 'qb(', "'r')"].join(''); +const calls = (n: number) => `await ${QB}.select('*').execute();\n`.repeat(n); + +const roots: string[] = []; + +function serverRoot(files: Record, baseline: unknown = { counts: {} }): string { + const dir = mkdtempSync(path.join(tmpdir(), 'trek-query-api-')); + roots.push(dir); + mkdirSync(path.join(dir, REPOS), { recursive: true }); + mkdirSync(path.join(dir, 'scripts'), { recursive: true }); + for (const [file, text] of Object.entries(files)) { + mkdirSync(path.dirname(path.join(dir, file)), { recursive: true }); + writeFileSync(path.join(dir, file), text); + } + if (baseline !== null) { + writeFileSync(path.join(dir, 'scripts/query-api-baseline.json'), typeof baseline === 'string' ? baseline : JSON.stringify(baseline)); + } + return dir; +} + +function run(dir: string, ...args: string[]): { status: number | null; out: string } { + try { + const stdout = execFileSync(process.execPath, [SCRIPT, `--dir=${dir}`, ...args], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + return { status: 0, out: stdout }; + } catch (err) { + const { status, stdout, stderr } = err as { status: number | null; stdout: string; stderr: string }; + return { status, out: `${stdout}${stderr}` }; + } +} + +afterEach(() => { + for (const dir of roots.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +describe('query-api-ratchet.mjs', () => { + it('QAPI-001: passes when no repository builds through the QueryBuilder', () => { + const { status, out } = run(serverRoot({ [`${REPOS}/A.repository.ts`]: 'await this.findOne({ id });\n' })); + expect(status).toBe(0); + expect(out).toContain('query-api: 0 QueryBuilder call(s) in 0 file(s)'); + }); + + it('QAPI-002: fails a new file holding one, and counts createQueryBuilder too', () => { + const { status, out } = run(serverRoot({ [`${REPOS}/New.repository.ts`]: `${calls(1)}em.createQueryBuilder(X);\n` })); + expect(status).toBe(1); + expect(out).toContain(`FAIL ${REPOS}/New.repository.ts: 2 QueryBuilder call(s), a file without a baseline entry may hold none.`); + }); + + it('QAPI-003: holds a baselined file to its entry', () => { + const baseline = { counts: { [`${REPOS}/A.repository.ts`]: 2 } }; + const grown = run(serverRoot({ [`${REPOS}/A.repository.ts`]: calls(3) }, baseline)); + expect(grown.status).toBe(1); + expect(grown.out).toContain('its baseline is 2.'); + expect(run(serverRoot({ [`${REPOS}/A.repository.ts`]: calls(2) }, baseline)).status).toBe(0); + }); + + it('QAPI-004: the base class that defines qb is not counted', () => { + expect(run(serverRoot({ [`${REPOS}/_shared/trek-repository.ts`]: 'return super.qb(alias);\n' })).status).toBe(0); + }); + + it('QAPI-005: an entry above its file fails until --update lowers it, and --update never raises or adds one', () => { + const dir = serverRoot( + { [`${REPOS}/A.repository.ts`]: calls(1), [`${REPOS}/B.repository.ts`]: calls(4) }, + { counts: { [`${REPOS}/A.repository.ts`]: 3, [`${REPOS}/Gone.repository.ts`]: 1, [`${REPOS}/B.repository.ts`]: 2 } }, + ); + const stale = run(dir); + expect(stale.status).toBe(1); + expect(stale.out).toContain(`FAIL ${REPOS}/A.repository.ts is held at 3`); + expect(stale.out).toContain(`FAIL ${REPOS}/Gone.repository.ts is held at 1`); + + run(dir, '--update'); + const lowered = JSON.parse(readFileSync(path.join(dir, 'scripts/query-api-baseline.json'), 'utf8')) as { counts: Record }; + expect(lowered.counts).toEqual({ [`${REPOS}/A.repository.ts`]: 1, [`${REPOS}/B.repository.ts`]: 2 }); + }); + + it('QAPI-006: a missing or malformed baseline, or no repositories folder, stops the run', () => { + expect(run(serverRoot({}, null)).out).toContain('cannot be read'); + expect(run(serverRoot({}, '{"counts":{"x":0}}')).out).toContain('expected a whole number above 0'); + expect(run(serverRoot({}, '[]')).out).toContain('must be an object with a "counts" object'); + const empty = mkdtempSync(path.join(tmpdir(), 'trek-query-api-')); + roots.push(empty); + expect(run(empty).out).toContain('does not exist'); + }); + + it('QAPI-007: this repository holds its committed baseline', () => { + const { status, out } = run(SERVER_ROOT); + expect(out).toContain('query-api:'); + expect(status).toBe(0); + }); +}); diff --git a/wiki/Development-environment.md b/wiki/Development-environment.md index e91a0b769..91a94166d 100644 --- a/wiki/Development-environment.md +++ b/wiki/Development-environment.md @@ -182,6 +182,7 @@ The `@trek/shared` package is the single source of truth for code shared between | `npm run lint:boundaries` | Import cycles and domain boundaries may not grow past their baseline (CI gate) | | `npm run lint:tx` | Methods that write more than once outside one transaction may not grow past their baseline (CI gate) | | `npm run lint:test-sql` | Raw SQL fixtures in `tests/` may not grow past their baseline (CI gate) | +| `npm run lint:query-api` | MikroORM QueryBuilder calls in the repositories may not grow past their baseline (CI gate) | | `npm run lint:dialect` | SQLite-only SQL spellings under `src/` may not grow past their baseline (CI gate) | | `npm run gen:db-types` | Regenerate the Kysely table types in `src/db/kysely/` from the migrated schema | | `npm run check:db-types` | Verify the generated Kysely table types match the migrations (CI gate) | @@ -250,7 +251,7 @@ Then open a Pull Request from your fork to `liketrek/TREK` targeting the `dev` b - Always branch off from an up-to-date `dev` — run `git fetch upstream && git rebase upstream/dev` before starting new work. - Run tests before pushing: `npm test` at the repo root runs all workspaces. That alone is not the full CI gate. With `shared` built, these are the checks CI runs before any test (`.github/workflows/test.yml` is the source of truth): - in `shared/`: `npm run typecheck && npm run contracts:open` - - in `server/`: `npm run build && npm run typecheck && npm run typecheck:tests && npm run typecheck:scripts && npm run check:entities && npm run check:db-types && npm run lint:check && npm run db:call-graph -- --sync --tx && npm run lint:size && npm run lint:boundaries && npm run lint:tx && npm run lint:test-sql && npm run lint:dialect && npm run check:plugin-facts` + - in `server/`: `npm run build && npm run typecheck && npm run typecheck:tests && npm run typecheck:scripts && npm run check:entities && npm run check:db-types && npm run lint:check && npm run db:call-graph -- --sync --tx && npm run lint:size && npm run lint:boundaries && npm run lint:tx && npm run lint:test-sql && npm run lint:query-api && npm run lint:dialect && npm run check:plugin-facts` - in `client/`: `npm run typecheck && npm run lint:warnings && npm run lint:pages && npm run lint:rtl && npm run lint:size && npm run lint:format && npm run lint:layers && npm run lint:offline && npm run lint:dup && npm run lint:pairs && npm run lint:skips && npm run lint:i18n-keys` - at the root, if you touched translations: `npm run i18n:parity:strict --workspace=shared` - Follow the commit message conventions described in the [[Contributing]] guidelines.