From 87e7aa5f91f2e37c7f05d04edf3544e493d7f0ec Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Wed, 4 Nov 2026 13:20:24 -0600 Subject: [PATCH 1/6] test: #21 run the PostgreSQL suites in CI The database-backed suites were skipped in CI because nothing set TEST_DATABASE_URL, so "27 skipped" was passing for the whole database layer. The new `integration` job creates a throwaway PostgreSQL cluster inside the runner's temp directory, socket-only, applies the migrations, runs the server suite and deletes the cluster. No shared test database, no secret to hand out, and the application's `crate` database is never in reach. `deploy` now requires the job, so a deploy cannot ignore the database tests. The cluster alone would still have left the failure mode from "skipped" to "silently empty", so `server/scripts/assert-postgres-tests.mjs` reads the JSON report and fails when any database-backed file has zero executed or any pending assertions. Verified against both reports: with the database it prints the four files and exits 0, without it fails on the three suites that skip. --- .github/workflows/ci.yml | 76 ++++++++++++++++++++- README.md | 17 ++++- docs/VM-setup.md | 6 ++ server/scripts/assert-postgres-tests.mjs | 85 ++++++++++++++++++++++++ 4 files changed, 181 insertions(+), 3 deletions(-) create mode 100644 server/scripts/assert-postgres-tests.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index df76816..cf27047 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,9 +76,83 @@ jobs: exit 1 fi + # The database-backed suites are skipped when TEST_DATABASE_URL is unset, so + # until this job existed CI reported them as "27 skipped" and stayed green. + # The tests guard the things unit tests cannot see: SQL that only fails once + # PostgreSQL parses it, keyset paging across ties and NULLs, facet counts + # against the filters, and the MusicBrainz import preserving reviews. + # + # The cluster is created here and thrown away at the end. Deliberately not the + # application database and not a shared test database: no secret to hand out, + # no state to drift, and running the destructive seed is impossible by + # construction. It listens on a unix socket only, so it cannot accept a TCP + # connection even while it exists. + integration: + name: PostgreSQL integration tests + runs-on: [self-hosted, linux, x64] + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Install pnpm + uses: pnpm/action-setup@v4 + with: + version: 10.33.0 + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: 24 + cache: pnpm + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Start a throwaway PostgreSQL cluster + run: | + set -euo pipefail + export PGDATA="${RUNNER_TEMP}/pgdata" + rm -rf "${PGDATA}" + initdb -D "${PGDATA}" --auth=trust --no-sync + # -k puts the socket in the data directory and `listen_addresses=''` + # keeps it off the network entirely. + pg_ctl -D "${PGDATA}" -l "${PGDATA}/server.log" \ + -o "-c listen_addresses='' -k ${PGDATA}" start + createdb -h "${PGDATA}" crate_test + echo "PGDATA=${PGDATA}" >> "${GITHUB_ENV}" + echo "TEST_DATABASE_URL=postgres:///crate_test?host=${PGDATA}" >> "${GITHUB_ENV}" + psql -h "${PGDATA}" -d crate_test -tAc 'select version()' + + - name: Apply migrations + run: DATABASE_URL="$TEST_DATABASE_URL" pnpm --filter @t19/server db:migrate + + - name: Run the server test suite + # The JSON report is only there for the next step; the default reporter + # still prints the normal output a reader expects. + run: | + pnpm --filter @t19/server exec vitest run \ + --reporter=default \ + --reporter=json \ + --outputFile="${RUNNER_TEMP}/vitest.json" + + # A green run is not evidence on its own: without a database every + # PostgreSQL suite skips and vitest still exits 0. See the script header. + - name: Fail if the PostgreSQL tests were skipped + run: node server/scripts/assert-postgres-tests.mjs "${RUNNER_TEMP}/vitest.json" + + - name: Remove the cluster + if: always() + run: | + if [ -f "${PGDATA}/postmaster.pid" ]; then + pg_ctl -D "${PGDATA}" -m immediate stop || true + fi + rm -rf "${PGDATA}" + deploy: name: Deploy to the group VM - needs: quality + # Both jobs have to pass. Deploying with the database layer untested is the + # same gap as deploying a server that was never built (#44). + needs: [quality, integration] if: github.ref == 'refs/heads/main' && github.event_name == 'push' # Pinned to the `project2` label on purpose. This job writes to # /var/www/html/project2 and restarts crate-api, both of which only exist diff --git a/README.md b/README.md index 71f69a2..9c15d35 100644 --- a/README.md +++ b/README.md @@ -83,8 +83,21 @@ CI also builds the project and checks the client bundle size. | `pnpm db:fetch:musicbrainz` | Rebuild the local MusicBrainz snapshot (network required) | | `pnpm db:seed:large` | Reset to a large synthetic catalog | -Tests cover frontend components and backend logic. Additional PostgreSQL tests cover search, sorting, rating updates and filters. These run when `TEST_DATABASE_URL` is configured and require a separate test database with migrations applied. The -[VM setup guide](docs/VM-setup.md) includes API smoke tests. +Tests cover frontend components and backend logic. The database-backed suites +(search, sorting, filtering and facet counts, and the MusicBrainz import) need a +real PostgreSQL server and are skipped unless `TEST_DATABASE_URL` is set. + +CI starts a temporary PostgreSQL cluster for them, applies the migrations, runs +the suites and fails if any of them was skipped instead of executed +(`server/scripts/assert-postgres-tests.mjs`). To run them locally, point +`TEST_DATABASE_URL` at a database you do not mind losing - the tests insert and +delete fixture rows: + +```bash +TEST_DATABASE_URL=postgres://t19:password@127.0.0.1:5432/crate_test pnpm --filter @t19/server test +``` + +The [VM setup guide](docs/VM-setup.md) includes API smoke tests. ## Database seed data diff --git a/docs/VM-setup.md b/docs/VM-setup.md index d560b7b..541dc63 100644 --- a/docs/VM-setup.md +++ b/docs/VM-setup.md @@ -41,6 +41,12 @@ migrations, imports the committed MusicBrainz catalog snapshot and restarts `crate-api.service`. The import is repeatable and does not reset the database or delete user data. Deployment does not run either destructive seed command. +The `integration` job runs the database-backed test suites. It starts a +temporary PostgreSQL cluster in the runner's temp directory, listening on a unix +socket only, applies the migrations to it, runs the tests and deletes it +afterwards. It never connects to the `crate` database, so the names, fixtures +and cleanup in those tests cannot affect the application data. + Apache serves the frontend at `/project2/` and forwards `/graphql` requests to the API on port 3001: diff --git a/server/scripts/assert-postgres-tests.mjs b/server/scripts/assert-postgres-tests.mjs new file mode 100644 index 0000000..ca9a08a --- /dev/null +++ b/server/scripts/assert-postgres-tests.mjs @@ -0,0 +1,85 @@ +// Fails when the PostgreSQL test files were skipped instead of executed. +// +// Every PostgreSQL suite is wrapped in `describe.skipIf(!process.env.TEST_DATABASE_URL)`, +// so a CI job that forgets the variable still exits 0 with the whole database +// layer missing. `pnpm test` would report "61 passed | 27 skipped" and nothing +// would be wrong by its own account. This script reads the JSON report from +// that run and requires every database-backed file to have executed, which is +// the difference between "the tests pass" and "the tests ran". +// +// Usage: node server/scripts/assert-postgres-tests.mjs + +import { readFileSync } from 'node:fs'; + +// Files that cannot run without a database. Adding a new one here is deliberate: +// the list is the guard, so it should grow with the suite. +const databaseBackedFiles = [ + 'src/db/musicBrainzCatalog.test.ts', + 'src/search/facets.postgres.test.ts', + 'src/search/search.postgres.test.ts', + 'src/search/sort.postgres.test.ts', +]; + +const reportPath = process.argv[2]; +if (!reportPath) { + console.error('::error::Usage: node assert-postgres-tests.mjs '); + process.exit(1); +} + +let report; +try { + report = JSON.parse(readFileSync(reportPath, 'utf8')); +} catch { + console.error(`::error::Could not read the vitest report at ${reportPath}`); + process.exit(1); +} + +// The reporter names files by absolute path. Keep only the part after `server/`. +function relativeName(name) { + const normalized = name.replaceAll('\\', '/'); + const marker = '/server/'; + const index = normalized.lastIndexOf(marker); + return index === -1 ? normalized : normalized.slice(index + marker.length); +} + +const resultsByFile = new Map( + (report.testResults ?? []).map((file) => [relativeName(file.name), file]), +); + +let failed = false; +for (const file of databaseBackedFiles) { + const result = resultsByFile.get(file); + + if (!result) { + console.error(`::error::${file} did not run at all`); + failed = true; + continue; + } + + // A skipped suite still produces a "passed" file containing only pending + // assertions, so count what actually ran rather than trusting the file + // status. A file with some tests running and some skipped also fails, which + // catches `describe.skipIf` on one block of a mixed file. + const assertions = result.assertionResults ?? []; + const executed = assertions.filter((assertion) => assertion.status === 'passed').length; + const pending = assertions.filter((assertion) => assertion.status === 'pending').length; + + if (result.status !== 'passed' || executed === 0 || pending > 0) { + console.error( + `::error::${file} did not fully run (${result.status}, ${executed} passed, ${pending} skipped) - is TEST_DATABASE_URL set?`, + ); + failed = true; + continue; + } + + console.log(`${file} ${executed} passed`); +} + +if (failed) { + console.error( + '::error::The PostgreSQL tests did not run. Point TEST_DATABASE_URL at a migrated test database.', + ); + process.exit(1); +} + +console.log(`All ${databaseBackedFiles.length} database-backed test files executed.`); From 3fd506752c0973b8222f3bb75195b935be192c5e Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Wed, 4 Nov 2026 13:35:28 -0600 Subject: [PATCH 2/6] fix: #21 find the PostgreSQL server binaries on the runner The first CI run failed with 'initdb: command not found' in 12 seconds: Debian puts the server binaries in /usr/lib/postgresql//bin and only the client on PATH, so PostgreSQL 18 being installed is not enough. Try PATH first, then the newest installed major version, and fail with a message that names the missing package if neither works. --- .github/workflows/ci.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cf27047..e36f14a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -111,6 +111,24 @@ jobs: - name: Start a throwaway PostgreSQL cluster run: | set -euo pipefail + # Debian installs the server binaries under /usr/lib/postgresql//bin + # and only puts the client on PATH, so `initdb` is not found even with + # PostgreSQL 18 installed. Use PATH when it works, otherwise the newest + # installed major version. + if ! command -v initdb >/dev/null 2>&1; then + pg_bindir='' + for dir in /usr/lib/postgresql/*/bin; do + if [ -x "${dir}/initdb" ]; then + pg_bindir="${dir}" + fi + done + if [ -z "${pg_bindir}" ]; then + echo "::error::initdb was not found. Install a PostgreSQL server package on the runner." + exit 1 + fi + export PATH="${pg_bindir}:${PATH}" + fi + initdb --version export PGDATA="${RUNNER_TEMP}/pgdata" rm -rf "${PGDATA}" initdb -D "${PGDATA}" --auth=trust --no-sync From 847f36f308e8d10d6bcdbd2ac96d973508aa2d76 Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Wed, 4 Nov 2026 13:40:11 -0600 Subject: [PATCH 3/6] feat: #76 myCrate query with keyset paging The field has been in schema.graphql since M2 and a client that asked for it got a non-null error. The resolver follows the same shape as the paginated review list: keyset on (added_at DESC, id DESC), one extra row to detect the next page, and a cursor that rejects anything it did not write. `crate_item_user_added_at_idx` already covers (user_id, added_at, id), so the page is served by the index the schema was designed with. Note that `added_at` is a Date column while `review.created_at` is stored in string mode, so the cursor rebuilds a Date on the way back in instead of passing a string to drizzle. An anonymous request gets an empty connection, not an error: CrateConnection is non-null and reads should degrade rather than fail (architecture decision 5). The decision record now says so. Seven PostgreSQL tests: newest first with a stable tie inside the same second, a full walk across the tie with no duplicates or gaps, isolation between two users who saved the same album, the anonymous case, a malformed cursor, removal, and a deleted user. --- docs/architecture.md | 7 +- server/scripts/assert-postgres-tests.mjs | 1 + server/src/crate.postgres.test.ts | 163 +++++++++++++++++++++++ server/src/resolvers.ts | 90 ++++++++++++- 4 files changed, 255 insertions(+), 6 deletions(-) create mode 100644 server/src/crate.postgres.test.ts diff --git a/docs/architecture.md b/docs/architecture.md index 0473ff6..14c0c6e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -96,11 +96,10 @@ brief explicitly wants simulated users rather than login. Not an `Authorization` header either, because nothing here is authenticated and calling it that would be misleading. -It goes on **every** request, not only mutations: `myReview` and `isInCrate` -are query fields. The same transport is intended for `myCrate`, which is declared -in the schema but has no resolver yet. +It goes on **every** request, not only mutations: `myReview`, `isInCrate` and +`myCrate` are query fields. -**Missing or invalid user ID.** If the `X-User-Id` header is missing or invalid, the application treats the user as anonymous. Their own review is returned as `null`, and albums are treated as not being in their crate. Adding albums or saving reviews requires a valid user ID. +**Missing or invalid user ID.** If the `X-User-Id` header is missing or invalid, the application treats the user as anonymous. Their own review is returned as `null`, albums are treated as not being in their crate, and `myCrate` returns an empty connection rather than an error: the field is non-null, and a read should degrade instead of failing. Adding albums or saving reviews requires a valid user ID. Reads never fail because of identity state, so a bad header cannot take the search page down. Writes require it and say so rather than attributing the diff --git a/server/scripts/assert-postgres-tests.mjs b/server/scripts/assert-postgres-tests.mjs index ca9a08a..f8ed102 100644 --- a/server/scripts/assert-postgres-tests.mjs +++ b/server/scripts/assert-postgres-tests.mjs @@ -14,6 +14,7 @@ import { readFileSync } from 'node:fs'; // Files that cannot run without a database. Adding a new one here is deliberate: // the list is the guard, so it should grow with the suite. const databaseBackedFiles = [ + 'src/crate.postgres.test.ts', 'src/db/musicBrainzCatalog.test.ts', 'src/search/facets.postgres.test.ts', 'src/search/search.postgres.test.ts', diff --git a/server/src/crate.postgres.test.ts b/server/src/crate.postgres.test.ts new file mode 100644 index 0000000..05369e7 --- /dev/null +++ b/server/src/crate.postgres.test.ts @@ -0,0 +1,163 @@ +import { randomUUID } from 'node:crypto'; +import { drizzle } from 'drizzle-orm/node-postgres'; +import { eq, inArray } from 'drizzle-orm'; +import pg from 'pg'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { album, appUser, crateItem } from './db/schema.js'; +import { createLoaders } from './loaders/index.js'; +import { resolvers } from './resolvers.js'; + +// Set TEST_DATABASE_URL to run these tests. Titles carry a random word so the +// fixture albums cannot collide with a stored catalog, and the user rows are +// deleted afterwards (crate items cascade with the user). +describe.skipIf(!process.env.TEST_DATABASE_URL)('PostgreSQL myCrate', () => { + const pool = new pg.Pool({ connectionString: process.env.TEST_DATABASE_URL }); + const db = drizzle(pool); + const word = `crate${Math.random() + .toString(36) + .replace(/[^a-z]/g, '')}`; + const owner = randomUUID(); + const stranger = randomUUID(); + const albumIds: number[] = []; + const ownerItems: number[] = []; + + function query(first: number, after?: string | null, userId: string | null = owner) { + return resolvers.Query.myCrate( + {}, + { first, after }, + { db, userId, loaders: createLoaders(db, userId) }, + ) as Promise<{ + edges: { cursor: string; node: { id: number; albumId: number } }[]; + pageInfo: { hasNextPage: boolean; endCursor: string | null }; + totalCount: number; + }>; + } + + beforeAll(async () => { + await db.insert(appUser).values([ + { id: owner, displayName: 'Crate fixture owner' }, + { id: stranger, displayName: 'Crate fixture stranger' }, + ]); + + // Three saved at distinct times and two saved in the same second, so every + // page boundary of two rows lands inside the tie at least once. + for (const index of [1, 2, 3, 4, 5]) { + const [row] = await db + .insert(album) + .values({ title: `Crate album ${index} ${word}`, releaseYear: 1990 + index }) + .returning(); + albumIds.push(row!.id); + } + + const addedAt = [ + '2026-01-01T10:00:00Z', + '2026-01-03T10:00:00Z', + '2026-01-02T10:00:00Z', + '2026-01-02T10:00:00Z', + '2026-01-02T10:00:00Z', + ]; + for (const [index, albumId] of albumIds.entries()) { + const [item] = await db + .insert(crateItem) + .values({ userId: owner, albumId, addedAt: new Date(addedAt[index]!) }) + .returning(); + ownerItems.push(item!.id); + } + + // The other user saved the same album. It must never show up above. + await db.insert(crateItem).values({ userId: stranger, albumId: albumIds[0]! }); + }); + + afterAll(async () => { + await pool.query('DELETE FROM app_user WHERE id = ANY($1)', [[owner, stranger]]); + await pool.query('DELETE FROM album WHERE id = ANY($1)', [albumIds]); + await pool.end(); + }); + + it('returns the newest items first, with a stable tie-break inside a second', async () => { + const result = await query(20); + + expect(result.totalCount).toBe(5); + // 3 Jan, then the three 2 Jan items by descending id, then 1 Jan. + expect(result.edges.map((edge) => edge.node.albumId)).toEqual([ + albumIds[1], + albumIds[4], + albumIds[3], + albumIds[2], + albumIds[0], + ]); + expect(result.pageInfo.hasNextPage).toBe(false); + }); + + it('walks every page exactly once across the tie', async () => { + const seen: number[] = []; + let after: string | null = null; + + for (let guard = 0; guard < 10; guard += 1) { + const page = await query(2, after); + seen.push(...page.edges.map((edge) => edge.node.albumId)); + + if (!page.pageInfo.hasNextPage) break; + after = page.pageInfo.endCursor; + expect(after).not.toBeNull(); + } + + expect(seen).toHaveLength(5); + expect(new Set(seen).size).toBe(5); + expect(seen).toEqual([ + albumIds[1], + albumIds[4], + albumIds[3], + albumIds[2], + albumIds[0], + ]); + }); + + it('never returns another user’s crate item', async () => { + const mine = await query(20, null, owner); + const theirs = await query(20, null, stranger); + + expect(mine.totalCount).toBe(5); + expect(mine.edges.some((edge) => edge.node.id === theirs.edges[0]?.node.id)).toBe( + false, + ); + // Both users saved albumIds[0], so a leak would show as a duplicate album. + expect(theirs.totalCount).toBe(1); + expect(theirs.edges.map((edge) => edge.node.albumId)).toEqual([albumIds[0]]); + }); + + it('gives an anonymous request an empty crate instead of an error', async () => { + const result = await query(20, null, null); + + expect(result).toEqual({ + edges: [], + pageInfo: { hasNextPage: false, endCursor: null }, + totalCount: 0, + }); + }); + + it('rejects a cursor that is not a crate cursor', async () => { + await expect(query(2, 'not-a-cursor')).rejects.toThrow('Invalid crate cursor'); + }); + + it('removes an item from the page when it leaves the crate', async () => { + await db.delete(crateItem).where(eq(crateItem.id, ownerItems[1]!)); + + const result = await query(20); + expect(result.totalCount).toBe(4); + expect(result.edges.map((edge) => edge.node.albumId)).not.toContain(albumIds[1]); + }); + + it('counts only rows that still exist after a user is removed', async () => { + const [extra] = await db + .insert(album) + .values({ title: `Crate spare ${word}` }) + .returning(); + await db.insert(crateItem).values({ userId: stranger, albumId: extra!.id }); + + expect((await query(20, null, stranger)).totalCount).toBe(2); + await db.delete(appUser).where(inArray(appUser.id, [stranger])); + expect((await query(20, null, stranger)).totalCount).toBe(0); + }); +}); diff --git a/server/src/resolvers.ts b/server/src/resolvers.ts index 4ca9a1a..16214d2 100644 --- a/server/src/resolvers.ts +++ b/server/src/resolvers.ts @@ -1,7 +1,7 @@ -import { and, desc, eq, lt, or, sql } from 'drizzle-orm'; +import { and, count, desc, eq, lt, or, sql } from 'drizzle-orm'; import { GraphQLError } from 'graphql'; import type { Db } from './db/client.js'; -import { album, review } from './db/schema.js'; +import { album, crateItem, review } from './db/schema.js'; import type { Loaders } from './loaders/index.js'; import { afterCursor, @@ -25,6 +25,7 @@ type AlbumRow = { id: number; title: string; releaseYear: number | null }; type SearchArgs = { input: AlbumSearchInput; first: number; after?: string | null }; type AlbumArgs = { id: string }; type ReviewArgs = { first: number; after?: string | null }; +type CrateArgs = { first: number; after?: string | null }; type ReviewRow = { id: number; @@ -66,6 +67,90 @@ function decodeReviewCursor(value: string): ReviewCursor { } } +type CrateCursor = { addedAt: Date; id: number }; + +function encodeCrateCursor(row: { addedAt: Date; id: number }): string { + return Buffer.from(JSON.stringify([row.addedAt.toISOString(), row.id])).toString( + 'base64url', + ); +} + +function decodeCrateCursor(value: string): CrateCursor { + try { + const parsed: unknown = JSON.parse(Buffer.from(value, 'base64url').toString('utf8')); + if (!Array.isArray(parsed) || parsed.length !== 2) throw new Error(); + const [addedAt, id] = parsed; + if ( + typeof addedAt !== 'string' || + Number.isNaN(Date.parse(addedAt)) || + !Number.isInteger(id) + ) { + throw new Error(); + } + // `added_at` is a Date column, unlike review.created_at, so the cursor has + // to rebuild the Date the column expects on the way back in. + return { addedAt: new Date(addedAt), id }; + } catch { + throw new GraphQLError('Invalid crate cursor.'); + } +} + +/** + * The user's crate, newest first, paged by `(added_at, id)` so a page boundary + * inside a run of albums saved in the same second is still stable. The + * `crate_item_user_added_at_idx` index matches the ordering exactly. + * + * An anonymous request gets an empty crate rather than an error: `CrateConnection` + * is non-null, and reads are supposed to degrade instead of failing + * (architecture decision 5). Nothing about another user's crate is reachable + * from here, because the user id comes from the request context. + */ +async function crateForUser(ctx: Context, { first, after }: CrateArgs) { + if (!ctx.userId) { + return { + edges: [], + pageInfo: { hasNextPage: false, endCursor: null }, + totalCount: 0, + }; + } + + const userId = ctx.userId; + const limit = pageSize(first); + const cursor = after ? decodeCrateCursor(after) : null; + const afterCondition = cursor + ? or( + lt(crateItem.addedAt, cursor.addedAt), + and(eq(crateItem.addedAt, cursor.addedAt), lt(crateItem.id, cursor.id)), + ) + : undefined; + + const [rows, total] = await Promise.all([ + ctx.db + .select() + .from(crateItem) + .where( + afterCondition + ? and(eq(crateItem.userId, userId), afterCondition) + : eq(crateItem.userId, userId), + ) + .orderBy(desc(crateItem.addedAt), desc(crateItem.id)) + .limit(limit + 1), + ctx.db.select({ count: count() }).from(crateItem).where(eq(crateItem.userId, userId)), + ]); + + const page = rows.slice(0, limit); + const edges = page.map((node) => ({ cursor: encodeCrateCursor(node), node })); + + return { + edges, + pageInfo: { + hasNextPage: rows.length > limit, + endCursor: edges.at(-1)?.cursor ?? null, + }, + totalCount: Number(total[0]?.count ?? 0), + }; +} + async function reviewsForAlbum( ctx: Context, albumId: number, @@ -169,6 +254,7 @@ export const resolvers = { Query: { searchAlbums: (_: unknown, args: SearchArgs, ctx: Context) => searchAlbums(ctx.db, args), + myCrate: (_: unknown, args: CrateArgs, ctx: Context) => crateForUser(ctx, args), album: async (_: unknown, { id }: AlbumArgs, ctx: Context) => { const albumId = parseNumericId(id, 'album'); const [found] = await ctx.db From d727a9aef4b28ae763741f5bda65be5c026fc0da Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Thu, 5 Nov 2026 13:29:32 -0600 Subject: [PATCH 4/6] fix: #76 keep microseconds in the crate cursor Review by evenkkl on #77 found a paging bug, reproduced here before the fix. The cursor was built from a JS Date, which keeps milliseconds only, so a save at .123900 produced a cursor at .123. The next page compared added_at against .123 and skipped every row in that millisecond that was not an exact match. Walking one item per page through four saves in one millisecond returned 2 of 5 items. The existing tests use whole-second timestamps, so they could not show it. The cursor now carries the database's own text for added_at: UTC with six fractional digits. It is produced with to_char, so the format does not depend on DateStyle or TimeZone. The next page compares against that text cast back to timestamptz, which stays exact and keeps the index usable. The GraphQL addedAt is still the Date, so the API output does not change. The cursor is checked before SQL sees it: the text must have the exact shape and must be a real calendar date, because Date.parse rolls 30 February over to 2 March. Malformed cursors still return "Invalid crate cursor.". Tests: a regression test that walks one item per page through four saves in one millisecond, including an exact microsecond tie. It fails on the previous code (returns 2 of 5) and passes now. Four malformed-timestamp cases; the 30 February case fails if the calendar check is removed. The two tests that change shared fixtures are now marked as order-dependent, as the review asked. Checked end to end: the built API, walked one item per page over HTTP, returned all five seeded items in order, and the cursor decodes to a microsecond timestamp. --- server/src/crate.postgres.test.ts | 61 ++++++++++++++++++++++++++++++- server/src/resolvers.ts | 54 +++++++++++++++++++-------- 2 files changed, 99 insertions(+), 16 deletions(-) diff --git a/server/src/crate.postgres.test.ts b/server/src/crate.postgres.test.ts index 05369e7..c9255ca 100644 --- a/server/src/crate.postgres.test.ts +++ b/server/src/crate.postgres.test.ts @@ -1,6 +1,6 @@ import { randomUUID } from 'node:crypto'; import { drizzle } from 'drizzle-orm/node-postgres'; -import { eq, inArray } from 'drizzle-orm'; +import { eq, inArray, sql } from 'drizzle-orm'; import pg from 'pg'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; @@ -141,6 +141,65 @@ describe.skipIf(!process.env.TEST_DATABASE_URL)('PostgreSQL myCrate', () => { await expect(query(2, 'not-a-cursor')).rejects.toThrow('Invalid crate cursor'); }); + it.each([ + '2026-02-30T10:00:00.000000Z', + '2026-13-01T10:00:00.000000Z', + '2026-01-02T10:00:00.123Z', + '2026-01-02 10:00:00.123900+00', + ])('rejects a cursor timestamp that is not an exact UTC instant: %s', async (text) => { + const cursor = Buffer.from(JSON.stringify([text, 1])).toString('base64url'); + + await expect(query(2, cursor)).rejects.toThrow('Invalid crate cursor'); + }); + + it('pages one item at a time through saves inside the same millisecond', async () => { + // A JS Date keeps milliseconds, but added_at keeps microseconds. Four saves + // share one millisecond, and two of those share an exact microsecond, so a + // cursor that keeps only the Date skips every row after the first. + const micro = randomUUID(); + await db.insert(appUser).values({ id: micro, displayName: 'Crate microsecond user' }); + const microAlbums = await db + .insert(album) + .values([1, 2, 3, 4, 5].map((index) => ({ title: `Crate micro ${index} ${word}` }))) + .returning({ id: album.id }); + const ids = microAlbums.map((row) => row.id); + const savedAt = [ + '2026-01-02T10:00:00.123900Z', + '2026-01-02T10:00:00.123900Z', + '2026-01-02T10:00:00.123500Z', + '2026-01-02T10:00:00.123100Z', + '2026-01-01T09:00:00.000000Z', + ]; + + try { + for (const [index, albumId] of ids.entries()) { + await db.insert(crateItem).values({ + userId: micro, + albumId, + addedAt: sql`${savedAt[index]!}::timestamptz`, + }); + } + + const seen: number[] = []; + let after: string | null = null; + for (let guard = 0; guard < 10; guard += 1) { + const page = await query(1, after, micro); + seen.push(...page.edges.map((edge) => edge.node.albumId)); + + if (!page.pageInfo.hasNextPage) break; + after = page.pageInfo.endCursor; + } + + // Newest first. The two exact ties come out by descending id. + expect(seen).toEqual([ids[1], ids[0], ids[2], ids[3], ids[4]]); + } finally { + await pool.query('DELETE FROM app_user WHERE id = $1', [micro]); + await pool.query('DELETE FROM album WHERE id = ANY($1)', [ids]); + } + }); + + // The next two tests change fixtures that the tests above count. They must stay + // last and must not run concurrently. it('removes an item from the page when it leaves the crate', async () => { await db.delete(crateItem).where(eq(crateItem.id, ownerItems[1]!)); diff --git a/server/src/resolvers.ts b/server/src/resolvers.ts index 16214d2..3febf52 100644 --- a/server/src/resolvers.ts +++ b/server/src/resolvers.ts @@ -67,14 +67,27 @@ function decodeReviewCursor(value: string): ReviewCursor { } } -type CrateCursor = { addedAt: Date; id: number }; +type CrateCursor = { addedAt: string; id: number }; -function encodeCrateCursor(row: { addedAt: Date; id: number }): string { - return Buffer.from(JSON.stringify([row.addedAt.toISOString(), row.id])).toString( - 'base64url', +// The text encodeCrateCursor writes: UTC with six fractional digits, as the +// database stores them. A JS Date would keep only milliseconds, so the cursor +// carries the database text instead, and the shape is checked before SQL sees it. +const EXACT_UTC_TIMESTAMP = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{6}Z$/; + +function isExactUtcTimestamp(text: string): boolean { + if (!EXACT_UTC_TIMESTAMP.test(text)) return false; + const milliseconds = Date.parse(text); + // Date.parse rolls 30 February over to 2 March; a real date survives the round trip. + return ( + !Number.isNaN(milliseconds) && + new Date(milliseconds).toISOString().startsWith(text.slice(0, 19)) ); } +function encodeCrateCursor(row: { addedAtCursor: string; id: number }): string { + return Buffer.from(JSON.stringify([row.addedAtCursor, row.id])).toString('base64url'); +} + function decodeCrateCursor(value: string): CrateCursor { try { const parsed: unknown = JSON.parse(Buffer.from(value, 'base64url').toString('utf8')); @@ -82,23 +95,21 @@ function decodeCrateCursor(value: string): CrateCursor { const [addedAt, id] = parsed; if ( typeof addedAt !== 'string' || - Number.isNaN(Date.parse(addedAt)) || + !isExactUtcTimestamp(addedAt) || !Number.isInteger(id) ) { throw new Error(); } - // `added_at` is a Date column, unlike review.created_at, so the cursor has - // to rebuild the Date the column expects on the way back in. - return { addedAt: new Date(addedAt), id }; + return { addedAt, id }; } catch { throw new GraphQLError('Invalid crate cursor.'); } } /** - * The user's crate, newest first, paged by `(added_at, id)` so a page boundary - * inside a run of albums saved in the same second is still stable. The - * `crate_item_user_added_at_idx` index matches the ordering exactly. + * The user's crate, newest first, paged by `(added_at, id)` at microsecond + * precision, so a page boundary inside a run of albums saved close together is + * still stable. The `crate_item_user_added_at_idx` index matches the ordering. * * An anonymous request gets an empty crate rather than an error: `CrateConnection` * is non-null, and reads are supposed to degrade instead of failing @@ -119,14 +130,24 @@ async function crateForUser(ctx: Context, { first, after }: CrateArgs) { const cursor = after ? decodeCrateCursor(after) : null; const afterCondition = cursor ? or( - lt(crateItem.addedAt, cursor.addedAt), - and(eq(crateItem.addedAt, cursor.addedAt), lt(crateItem.id, cursor.id)), + lt(crateItem.addedAt, sql`${cursor.addedAt}::timestamptz`), + and( + eq(crateItem.addedAt, sql`${cursor.addedAt}::timestamptz`), + lt(crateItem.id, cursor.id), + ), ) : undefined; const [rows, total] = await Promise.all([ ctx.db - .select() + .select({ + id: crateItem.id, + albumId: crateItem.albumId, + addedAt: crateItem.addedAt, + // The Date above keeps milliseconds. This keeps the stored microseconds + // so the next page compares the same instant. + addedAtCursor: sql`to_char(${crateItem.addedAt} AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')`, + }) .from(crateItem) .where( afterCondition @@ -139,7 +160,10 @@ async function crateForUser(ctx: Context, { first, after }: CrateArgs) { ]); const page = rows.slice(0, limit); - const edges = page.map((node) => ({ cursor: encodeCrateCursor(node), node })); + const edges = page.map(({ addedAtCursor, ...node }) => ({ + cursor: encodeCrateCursor({ addedAtCursor, id: node.id }), + node, + })); return { edges, From 8b6dd213d8d5231f504e2e6e2e1a081ec6ed4e71 Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Thu, 5 Nov 2026 13:29:32 -0600 Subject: [PATCH 5/6] docs: #76 AGENTS.md names myCrate and the empty connection Rule 5 still called myCrate "planned" and listed only null and false as the degraded reads for anonymous requests. Both are now wrong: the resolver exists, and an anonymous myCrate returns an empty connection. --- AGENTS.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 598fd1a..33db494 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,12 +45,13 @@ requirements work - do not casually change them. from a field resolver. 4. **User input is never interpolated into SQL.** Use parameterised queries. 5. **Users are simulated.** A UUID in `localStorage`, sent as an `X-User-Id` - header on **every** request, not just mutations. The implemented `myReview` - and `isInCrate` fields, as well as the planned `myCrate` query, rely on - this identity. The server reads the header once into the GraphQL context, - so no field in `schema.graphql` takes a `userId` argument. No authentication. - - Missing or malformed id means **anonymous**: reads degrade to `null` or - `false`, mutations that need identity fail with a clear error. + header on **every** request, not just mutations. `myReview`, `isInCrate` and + `myCrate` rely on this identity. The server reads the header once into the + GraphQL context, so no field in `schema.graphql` takes a `userId` argument. + No authentication. + - Missing or malformed id means **anonymous**: reads degrade to `null`, + `false` or an empty connection, mutations that need identity fail with a + clear error. - The context is **per request**. Hoisting the user id into a module-level variable or a shared cache leaks one user's data into another. - `displayName` is derived server-side from the UUID when the user row is From 8b733e8adb021ba20072eda288f42582a62f6c94 Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Thu, 5 Nov 2026 13:43:11 -0600 Subject: [PATCH 6/6] perf: #76 seek to the crate cursor with a row comparison The keyset condition was written as `added_at < $1 OR (added_at = $1 AND id < $2)`. PostgreSQL cannot use an OR as an index condition on a multi-column index, so EXPLAIN showed the user filter in the index cond and the cursor check as a Filter on every row: Index Cond: (user_id = '...'::uuid) Filter: ((added_at < '...'::timestamptz) OR ((added_at = '...') AND (id < 89))) For a large crate, a deep page walked every item already shown. The same ordering is now a row comparison, `(added_at, id) < ($1, $2)`, which PostgreSQL pushes into the index scan: Index Cond: ((user_id = '...'::uuid) AND (ROW(added_at, id) < ROW('...', 89))) Both columns are NOT NULL, so the two forms select the same rows; the twelve crate tests, including the microsecond paging walk, pass unchanged. Review by evenkkl on #77 asked for the exact database text in the cursor. That is what this builds on: the text is cast back to timestamptz inside the row, so the comparison stays exact at microsecond precision. --- server/src/resolvers.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/server/src/resolvers.ts b/server/src/resolvers.ts index 3febf52..c41e6d8 100644 --- a/server/src/resolvers.ts +++ b/server/src/resolvers.ts @@ -128,14 +128,10 @@ async function crateForUser(ctx: Context, { first, after }: CrateArgs) { const userId = ctx.userId; const limit = pageSize(first); const cursor = after ? decodeCrateCursor(after) : null; + // A row comparison rather than an OR, so PostgreSQL seeks to the cursor in the + // index instead of filtering every item the user has already seen. const afterCondition = cursor - ? or( - lt(crateItem.addedAt, sql`${cursor.addedAt}::timestamptz`), - and( - eq(crateItem.addedAt, sql`${cursor.addedAt}::timestamptz`), - lt(crateItem.id, cursor.id), - ), - ) + ? sql`(${crateItem.addedAt}, ${crateItem.id}) < (${cursor.addedAt}::timestamptz, ${cursor.id})` : undefined; const [rows, total] = await Promise.all([