From 71bb29b8c7e98e91f97cdaac8b38ba05a2c391b8 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Tue, 15 Sep 2026 11:22:33 -0400 Subject: [PATCH] [api] fix a few 2023 compat issues --- apps/accounts/src/accounts.app.ts | 105 ++++++++++++------ apps/accounts/src/openapi.ts | 10 ++ .../accounts/src/test/integration/api.test.ts | 43 +++++++ apps/api/src/reputation-db.ts | 44 +++++--- apps/api/src/test/integration/api.test.ts | 22 ++++ apps/match/src/match.app.ts | 103 ++++++++++------- apps/match/src/openapi.ts | 9 ++ apps/match/src/test/integration/api.test.ts | 36 ++++++ packages/domain/src/accounts-db.ts | 22 ++-- packages/domain/src/d1-binds.ts | 27 +++++ packages/domain/src/index.ts | 1 + packages/domain/src/presence-db.ts | 25 +++-- packages/domain/src/progression-db.ts | 21 +++- packages/domain/src/rooms-db.ts | 22 +--- 14 files changed, 360 insertions(+), 130 deletions(-) create mode 100644 packages/domain/src/d1-binds.ts diff --git a/apps/accounts/src/accounts.app.ts b/apps/accounts/src/accounts.app.ts index 2100c61..c3889fd 100644 --- a/apps/accounts/src/accounts.app.ts +++ b/apps/accounts/src/accounts.app.ts @@ -28,6 +28,7 @@ import { BannerImageRequest, BioRequest, BioResponse, + BulkIdsRequest, CreateAccountRequest, CreateAccountResult, DisplayNameRequest, @@ -51,6 +52,7 @@ import { import { resolveWhitelistedEmoji, WHITELISTED_EMOJIS } from './whitelisted-emojis' import type { Context } from 'hono' +import type { DescribeRouteOptions } from 'hono-openapi' import type { Account } from '@repo/domain' import type { App } from './context' @@ -102,6 +104,69 @@ async function formField(c: Context, name: string): Promise { return typeof value === 'string' ? value : '' } +/** + * Read a REPEATED string field from a form-urlencoded / multipart body (`id=1&id=2`). + * `parseBody` keeps only the last value for a duplicated key unless it's told to collect + * them all, which is the whole point here. + */ +async function formFields(c: Context, name: string): Promise { + const body = await c.req.parseBody({ all: true }).catch(() => ({}) as Record) + const value = body[name] + const values = Array.isArray(value) ? value : [value] + return values.filter((v): v is string => typeof v === 'string') +} + +/** + * OpenAPI spec for the two `/account/bulk` registrations, which differ only in where the + * ids ride. + */ +function bulkRouteSpec(source: 'query' | 'body'): DescribeRouteOptions { + return { + tags: ['Lookup'], + summary: 'Look up many accounts by id', + description: [ + 'Accepts repeated `id` values (query params and/or a form-urlencoded body) and', + 'comma-separated lists. Every requested id appears in the response — ids with no', + 'stored row get a synthesized default.', + ].join(' '), + ...(source === 'query' + ? { + parameters: [ + { + name: 'id', + in: 'query', + required: false, + description: 'Repeatable; each value may be a comma-separated list of ids', + schema: { type: 'array', items: { type: 'string' } }, + }, + ], + } + : { requestBody: form(BulkIdsRequest, 'Repeated `id` fields') }), + responses: { 200: json(AccountDto.array(), 'One public account per requested id') }, + } +} + +/** + * Look up many accounts at once. Ids come from repeated `id` query params AND, on the + * POST form, repeated `id` body fields; either may carry a comma-separated list. Deduped + * (first occurrence wins) so a repeated id can't double a row or waste a bound parameter. + */ +async function bulkAccounts(c: Context) { + const raw = [...(c.req.queries('id') ?? []), ...(await formFields(c, 'id'))] + const ids = [ + ...new Set( + raw + .flatMap((v) => v.split(',')) + .map((v) => Number.parseInt(v.trim(), 10)) + .filter((n) => !Number.isNaN(n)) + ), + ] + // Resolve stored accounts, synthesizing a default for any id not in the DB + // so every requested id is present in the response. + const stored = new Map((await getAccountsByIds(c.env.DB, ids)).map((a) => [a.accountId, a])) + return c.json(ids.map((id) => toAccountDto(stored.get(id) ?? defaultAccount(id)))) +} + /** * Project a stored account into the public account DTO — the client's camelCase * shape, excluding private fields like `email` (surfaced only by /account/me). @@ -286,40 +351,12 @@ const app = new Hono() // ---- Bulk / single lookup ------------------------------------------------ // Register the static `bulk` path before the `/account/:id` param route. - .get( - '/account/bulk', - describeRoute({ - tags: ['Lookup'], - summary: 'Look up many accounts by id', - description: [ - 'Accepts repeated `id` query params and/or comma-separated lists. Every requested', - 'id appears in the response — ids with no stored row get a synthesized default.', - ].join(' '), - parameters: [ - { - name: 'id', - in: 'query', - required: false, - description: 'Repeatable; each value may be a comma-separated list of ids', - schema: { type: 'array', items: { type: 'string' } }, - }, - ], - responses: { 200: json(AccountDto.array(), 'One public account per requested id') }, - }), - async (c) => { - // Reads repeated `id` query params; also accept a comma-separated list. - const ids = - c.req - .queries('id') - ?.flatMap((v) => v.split(',')) - .map((s) => Number.parseInt(s.trim(), 10)) - .filter((n) => !Number.isNaN(n)) ?? [] - // Resolve stored accounts, synthesizing a default for any id not in the DB - // so every requested id is present in the response. - const stored = new Map((await getAccountsByIds(c.env.DB, ids)).map((a) => [a.accountId, a])) - return c.json(ids.map((id) => toAccountDto(stored.get(id) ?? defaultAccount(id)))) - } - ) + .get('/account/bulk', describeRoute(bulkRouteSpec('query')), bulkAccounts) + // The 2023 client asks for its friends list as a POST with the ids in a + // form-urlencoded body — the same `id=1&id=2&…` it would put in a query string, + // moved off the URL because a few hundred friends overflow it. GET is the same + // lookup and stays for callers (and docs) that prefer it. + .post('/account/bulk', describeRoute(bulkRouteSpec('body')), bulkAccounts) .get( '/account/:id/bio', diff --git a/apps/accounts/src/openapi.ts b/apps/accounts/src/openapi.ts index 03b4883..474b000 100644 --- a/apps/accounts/src/openapi.ts +++ b/apps/accounts/src/openapi.ts @@ -212,6 +212,16 @@ export const UsernameRequest = z.object({ ), }) +/** + * `POST /account/bulk` form body. `id` repeats once per requested account (the client + * sends its whole friends list this way); each value may also be a comma-separated list. + */ +export const BulkIdsRequest = z.object({ + id: z + .union([z.string(), z.string().array()]) + .describe('Repeatable; each value may be a comma-separated list of ids'), +}) + export const EmailRequest = z.object({ email: z .string() diff --git a/apps/accounts/src/test/integration/api.test.ts b/apps/accounts/src/test/integration/api.test.ts index d708fdb..9c438ba 100644 --- a/apps/accounts/src/test/integration/api.test.ts +++ b/apps/accounts/src/test/integration/api.test.ts @@ -97,6 +97,48 @@ describe('public endpoints', () => { expect(accounts[1].username).toBe('Player2') }) + test('POST /account/bulk reads the ids from a form body', async () => { + // The 2023 client asks for its friends list as a POST — the ids are in a + // form-urlencoded body, not the query string. GET-only left it a 404. + const res = await exports.default.fetch(`${ORIGIN}/account/bulk`, { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: 'id=1&id=2&id=3', + }) + expect(res.status).toBe(200) + const accounts = (await res.json()) as Array<{ accountId: number; username: string }> + expect(accounts.map((a) => a.accountId)).toEqual([1, 2, 3]) + expect(accounts[0].username).toBe('Coach') + }) + + test('POST /account/bulk answers more ids than D1 will bind at once', async () => { + // D1 caps a statement at 100 bound parameters; a real friends list runs past that, + // and an unchunked `IN (…)` 500s. Requesting a stored id in the LAST chunk proves + // the later statements ran, not just the first. + const ids = [...Array.from({ length: 250 }, (_, i) => 1000 + i), 1] + const res = await exports.default.fetch(`${ORIGIN}/account/bulk`, { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: ids.map((id) => `id=${id}`).join('&'), + }) + expect(res.status).toBe(200) + const accounts = (await res.json()) as Array<{ accountId: number; username: string }> + expect(accounts.map((a) => a.accountId)).toEqual(ids) + expect(accounts.at(-1)?.username).toBe('Coach') + }) + + test('POST /account/bulk returns one entry per id, deduped', async () => { + const res = await exports.default.fetch(`${ORIGIN}/account/bulk`, { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + // Negative ids are real in the client's list and synthesize like any other. + body: 'id=1&id=-1&id=1', + }) + const accounts = (await res.json()) as Array<{ accountId: number; username: string }> + expect(accounts.map((a) => a.accountId)).toEqual([1, -1]) + expect(accounts[1].username).toBe('Player-1') + }) + test('GET /account/search prefix-matches usernames, returns public DTOs', async () => { const res = await exports.default.fetch(`${ORIGIN}/account/search?name=coa`) expect(res.status).toBe(200) @@ -652,6 +694,7 @@ describe('auth-gated endpoints', () => { 'GET /accountprivacysettings/{id}', 'GET /emojiConfig/whitelistedEmojis', 'GET /parentalcontrol/me', + 'POST /account/bulk', 'POST /account/create', 'POST /account/me/email', 'POST /account/me/phone', diff --git a/apps/api/src/reputation-db.ts b/apps/api/src/reputation-db.ts index 96a9361..f4705dd 100644 --- a/apps/api/src/reputation-db.ts +++ b/apps/api/src/reputation-db.ts @@ -26,6 +26,8 @@ * don't clash with the other workers' migrations that share the database). */ +import { bindPlaceholders, chunkForBinds } from '@repo/domain' + /** Schema DDL (mirror of migrations/0013_reputation.sql + 0014, folded into one CREATE). */ export const SCHEMA_DDL: string[] = [ `CREATE TABLE IF NOT EXISTS reputation ( @@ -186,15 +188,22 @@ export async function getCheerCredits( ): Promise> { const credits = new Map() if (playerIds.length === 0) return credits - const placeholders = playerIds.map((_, i) => `?${i + 2}`).join(', ') - const { results } = await db - .prepare( - `SELECT player_id, cheers_left FROM player_cheer - WHERE created > ?1 AND player_id IN (${placeholders})` + // The window cutoff is `?1`, so the ids start at `?2` and each chunk is one short of + // D1's bind cap — a bulk read over a friends list runs well past it. + const pages = await Promise.all( + chunkForBinds(playerIds, 1).map((chunk) => + db + .prepare( + `SELECT player_id, cheers_left FROM player_cheer + WHERE created > ?1 AND player_id IN (${bindPlaceholders(chunk, 1)})` + ) + .bind(windowCutoff(now), ...chunk) + .all<{ player_id: number; cheers_left: number }>() ) - .bind(windowCutoff(now), ...playerIds) - .all<{ player_id: number; cheers_left: number }>() - for (const row of results) credits.set(row.player_id, row.cheers_left) + ) + for (const row of pages.flatMap((page) => page.results)) { + credits.set(row.player_id, row.cheers_left) + } return credits } @@ -219,15 +228,20 @@ export async function getReputations( now: Date = new Date() ): Promise { if (accountIds.length === 0) return [] - const placeholders = accountIds.map((_, i) => `?${i + 1}`).join(', ') - const [{ results }, credits] = await Promise.all([ - db - .prepare(`SELECT * FROM reputation WHERE account_id IN (${placeholders})`) - .bind(...accountIds) - .all(), + const [pages, credits] = await Promise.all([ + // Chunked: the friends list behind `/api/playerReputation/v2/bulk` runs past D1's + // bind cap, and an unchunked statement fails the whole lookup rather than part of it. + Promise.all( + chunkForBinds(accountIds).map((chunk) => + db + .prepare(`SELECT * FROM reputation WHERE account_id IN (${bindPlaceholders(chunk)})`) + .bind(...chunk) + .all() + ) + ), getCheerCredits(db, accountIds, now), ]) - const stored = new Map(results.map((r) => [r.account_id, r])) + const stored = new Map(pages.flatMap((page) => page.results).map((r) => [r.account_id, r])) return accountIds.map((id) => { const credit = credits.get(id) ?? DAILY_CHEER_CREDIT const row = stored.get(id) diff --git a/apps/api/src/test/integration/api.test.ts b/apps/api/src/test/integration/api.test.ts index 8bd3e87..9da33ca 100644 --- a/apps/api/src/test/integration/api.test.ts +++ b/apps/api/src/test/integration/api.test.ts @@ -844,6 +844,28 @@ describe('public endpoints', () => { expect(body[0]).toMatchObject({ Level: 1, XP: 0 }) }) + test('the bulk profile reads answer more ids than D1 will bind at once', async () => { + // Both of these are asked about a whole friends list at once, which runs past D1's + // 100-bound-parameter cap; unchunked, the statement fails and the endpoint 500s + // rather than returning a short list. + const ids = Array.from({ length: 250 }, (_, i) => 6000 + i) + const query = ids.map((id) => `id=${id}`).join('&') + + const progression = await exports.default.fetch( + `${ORIGIN}/api/players/v2/progression/bulk?${query}` + ) + expect(progression.status).toBe(200) + const levels = (await progression.json()) as Array<{ PlayerId: number }> + expect(levels.map((p) => p.PlayerId)).toEqual(ids) + + const reputation = await exports.default.fetch( + `${ORIGIN}/api/playerReputation/v2/bulk?${query}` + ) + expect(reputation.status).toBe(200) + const reps = (await reputation.json()) as Array<{ AccountId: number }> + expect(reps.map((r) => r.AccountId)).toEqual(ids) + }) + test('progression reads back the XP game rewards banked, levelled up', async () => { // The two workers share this table; `econ` writes it when a game reward is claimed (5 XP // at a time). Granted in one lump here to exercise a multi-level climb: 25 XP from level diff --git a/apps/match/src/match.app.ts b/apps/match/src/match.app.ts index 487d327..8191a94 100644 --- a/apps/match/src/match.app.ts +++ b/apps/match/src/match.app.ts @@ -12,13 +12,13 @@ import { deleteEmptyRoomInstances, deleteExpiredPresence, deletePresence, + deleteRoomInvite, GAME_VERSION, getAccount, getClubSummary, getExpiredPresenceInstanceIds, getFriendIds, getJoinableInstance, - deleteRoomInvite, getLatestRoomInviteBetween, getMostActiveClubhouses, getOrCreateDormRoom, @@ -28,8 +28,8 @@ import { getRoomByName, getRoomInstance, getRoomInstancesByRoom, - getRoomInvite, getRoomInstanceSummariesByRoom, + getRoomInvite, getStoredRoomInstance, InviteMode, isClubMember, @@ -64,6 +64,7 @@ import { AUTHED, AvoidJuniorsRequest, AvoidJuniorsResponse, + BatchPlayerIdsRequest, ConnectionInfoResponse, CorrelationIdRequest, EMPTY_OK, @@ -91,6 +92,7 @@ import { } from './openapi' import type { Context } from 'hono' +import type { DescribeRouteOptions } from 'hono-openapi' import type { Room, StoredPresence } from '@repo/domain' import type { App, Env } from './context' @@ -991,6 +993,62 @@ async function readRequestFields(c: Context): Promise empty) } +/** + * OpenAPI spec for the two `/player` registrations, which differ only in where the ids ride. + */ +function batchPlayerRouteSpec(source: 'query' | 'body'): DescribeRouteOptions { + return { + tags: ['Presence'], + summary: 'Batch player presence lookup', + description: [ + 'Returns each requested player’s presence. `id` is repeated once per player', + '(`?id=2&id=155&id=153`, or the same fields in a form-urlencoded body) — one value', + 'each, not comma-separated. With no ids, serves a single default (online) player.', + ].join(' '), + ...(source === 'query' + ? { + parameters: [ + { + name: 'id', + in: 'query', + required: false, + description: 'Repeated once per player id (`?id=2&id=155`); not comma-separated', + schema: { type: 'array', items: { type: 'string' } }, + }, + ], + } + : { requestBody: form(BatchPlayerIdsRequest, 'Repeated `id` fields') }), + responses: { 200: json(PlayerDto.array(), 'One entry per requested player') }, + } +} + +/** + * Returns each requested player's presence. Ids come from repeated `id` query params AND, + * on the POST form, repeated `id` body fields; with none, serves the static + * getplayer.json default. Deduped (first occurrence wins) so a repeated id can't double + * an entry or waste a bound parameter. + */ +async function batchPlayers(c: Context) { + // `readRequestFields` serves the v2 client's JSON too, where an id is a real number + // rather than a form field's string — coerce rather than filtering those away. + const body = c.req.method === 'POST' ? await readRequestFields(c) : {} + const raw = body['id'] ?? body['Id'] + const ids = [ + ...new Set( + [...(c.req.queries('id') ?? []), ...(Array.isArray(raw) ? raw : [raw])] + .filter((v): v is string | number => typeof v === 'string' || typeof v === 'number') + .map((v) => Number.parseInt(String(v).trim(), 10)) + .filter((n) => !Number.isNaN(n)) + ), + ] + if (ids.length === 0) return c.json(DEFAULT_GET_PLAYER) + + // One query per 99-id chunk (D1 `WHERE account_id IN (…)`), rather than a point read + // per id as the KV store required. + const presences = await getPresences(c.env.DB, ids) + return c.json(ids.map((playerId) => playerPayload(playerId, presences.get(playerId)))) +} + /** * Look a field up case-insensitively — the client's casing isn't guaranteed across its * surfaces — taking the first value when the form encoding repeated it. @@ -1742,42 +1800,11 @@ const app = new Hono() async (c) => c.json(await getMostActiveClubhouses(c.env.DB)) ) - .get( - '/player', - describeRoute({ - tags: ['Presence'], - summary: 'Batch player presence lookup', - description: [ - 'Returns each requested player’s presence. `id` is a repeated query param', - '(`?id=2&id=155&id=153`) — one value each, not comma-separated. With no ids, serves', - 'a single default (online) player.', - ].join(' '), - parameters: [ - { - name: 'id', - in: 'query', - required: false, - description: 'Repeated once per player id (`?id=2&id=155`); not comma-separated', - schema: { type: 'array', items: { type: 'string' } }, - }, - ], - responses: { 200: json(PlayerDto.array(), 'One entry per requested player') }, - }), - async (c) => { - // Returns each requested player's presence. Reads the repeated `id` query - // param(s); with none it serves the static getplayer.json default. - const ids = c.req - .queries('id') - ?.map((s) => Number.parseInt(s.trim(), 10)) - .filter((n) => !Number.isNaN(n)) - if (!ids || ids.length === 0) return c.json(DEFAULT_GET_PLAYER) - - // One query for the whole batch (D1 `WHERE account_id IN (…)`), rather than a - // point read per id as the KV store required. - const presences = await getPresences(c.env.DB, ids) - return c.json(ids.map((playerId) => playerPayload(playerId, presences.get(playerId)))) - } - ) + .get('/player', describeRoute(batchPlayerRouteSpec('query')), batchPlayers) + // The 2023 client asks about its whole friends list at once and posts the ids in a + // form-urlencoded body rather than hanging a few hundred of them off the URL. Same + // lookup as the GET, which stays for callers that prefer a query string. + .post('/player', describeRoute(batchPlayerRouteSpec('body')), batchPlayers) .post( '/player/heartbeat', diff --git a/apps/match/src/openapi.ts b/apps/match/src/openapi.ts index 471cc8d..c79407d 100644 --- a/apps/match/src/openapi.ts +++ b/apps/match/src/openapi.ts @@ -160,6 +160,15 @@ export const MatchmakeResponse = z.object({ .describe('Echoes the request’s CorrelationId; all-zero GUID when it sent none'), }) +/** + * `POST /player` form body. `id` repeats once per player (the client asks about its whole + * friends list at once, too many ids to hang off a URL). One id per value — unlike + * `accounts`' bulk lookup, these are not comma-separated. + */ +export const BatchPlayerIdsRequest = z.object({ + id: z.union([z.string(), z.string().array()]).describe('Repeated once per player id'), +}) + /** * `GET /player/avoidjuniors` — a BARE JSON boolean (`true`/`false`), not an envelope and * not a `{ value }` wrapper. The whole body is the preference. diff --git a/apps/match/src/test/integration/api.test.ts b/apps/match/src/test/integration/api.test.ts index ff304ca..b5b1fbf 100644 --- a/apps/match/src/test/integration/api.test.ts +++ b/apps/match/src/test/integration/api.test.ts @@ -469,6 +469,41 @@ describe('public endpoints', () => { expect(players.every((p) => p.isOnline === false)).toBe(true) }) + test('POST /player reads the ids from a form body', async () => { + // The 2023 client asks about its friends list as a POST — the ids are in a + // form-urlencoded body, not the query string. GET-only left it a 404. + const res = await exports.default.fetch(`${ORIGIN}/player`, { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: 'id=1070&id=1380&id=1070', + }) + expect(res.status).toBe(200) + const players = (await res.json()) as Array<{ playerId: number; isOnline: boolean }> + // One entry per id, deduped, in request order. + expect(players.map((p) => p.playerId)).toEqual([1070, 1380]) + }) + + test('POST /player answers more ids than D1 will bind at once', async () => { + // D1 caps a statement at 100 bound parameters and the expiry check takes one of + // them, so a real friends list overruns an unchunked `IN (…)` and 500s. + const ids = Array.from({ length: 250 }, (_, i) => 4000 + i) + const res = await exports.default.fetch(`${ORIGIN}/player`, { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: ids.map((id) => `id=${id}`).join('&'), + }) + expect(res.status).toBe(200) + const players = (await res.json()) as Array<{ playerId: number }> + expect(players.map((p) => p.playerId)).toEqual(ids) + }) + + test('POST /player without an id returns the default payload', async () => { + const res = await exports.default.fetch(`${ORIGIN}/player`, { method: 'POST' }) + expect(res.status).toBe(200) + const players = (await res.json()) as Array<{ playerId: number; isOnline: boolean }> + expect(players[0]).toMatchObject({ playerId: 1, isOnline: true, appVersion: GAME_VERSION }) + }) + test('GET /player without an id returns the default payload', async () => { const res = await exports.default.fetch(`${ORIGIN}/player`) expect(res.status).toBe(200) @@ -3280,6 +3315,7 @@ describe('auth-gated endpoints', () => { 'POST /matchmake/v2/player/{playerId}', 'POST /matchmake/v2/room/{roomId}', 'POST /matchmake/v2/room/{roomId}/{subRoomId}', + 'POST /player', 'POST /player/exclusivelogin', 'POST /player/heartbeat', 'POST /player/login', diff --git a/packages/domain/src/accounts-db.ts b/packages/domain/src/accounts-db.ts index 0de8141..4f1c879 100644 --- a/packages/domain/src/accounts-db.ts +++ b/packages/domain/src/accounts-db.ts @@ -11,6 +11,8 @@ * it from `@repo/domain` (each uses the subset it needs). */ +import { bindPlaceholders, chunkForBinds } from './d1-binds' + /** * Schema DDL — the head schema, i.e. what the table looks like after every migration * (0001_accounts + 0002_avatar + 0009_account_has_plus, sans seed INSERTs; 0004 added a @@ -349,15 +351,21 @@ export async function countAccountsBySignupIp(db: D1Database, ip: string): Promi return row?.n ?? 0 } -/** Look up multiple accounts by AccountId (order not guaranteed). */ +/** + * Look up multiple accounts by AccountId (order not guaranteed). A list longer than + * {@link MAX_BOUND_PARAMS} is split across statements and the rows concatenated. + */ export async function getAccountsByIds(db: D1Database, ids: number[]): Promise { if (ids.length === 0) return [] - const placeholders = ids.map((_, i) => `?${i + 1}`).join(',') - const { results } = await db - .prepare(`SELECT data FROM account WHERE account_id IN (${placeholders})`) - .bind(...ids) - .all() - return parseAll(results) + const pages = await Promise.all( + chunkForBinds(ids).map((chunk) => + db + .prepare(`SELECT data FROM account WHERE account_id IN (${bindPlaceholders(chunk)})`) + .bind(...chunk) + .all() + ) + ) + return parseAll(pages.flatMap((page) => page.results)) } /** diff --git a/packages/domain/src/d1-binds.ts b/packages/domain/src/d1-binds.ts new file mode 100644 index 0000000..0ddac35 --- /dev/null +++ b/packages/domain/src/d1-binds.ts @@ -0,0 +1,27 @@ +/** + * D1 caps a prepared statement at 100 bound parameters — binding more fails outright with + * "variable number must be between ?1 and ?100". Every `IN (…)` list built from a caller's + * array has to respect this, which is easy to miss: the id list behind a bulk lookup (a + * friends list, a room's subrooms) stays under a hundred right up until it meets a real + * account, and then the whole endpoint 500s rather than degrading. + */ +export const MAX_BOUND_PARAMS = 100 + +/** + * Split values into chunks that fit {@link MAX_BOUND_PARAMS}, one statement each. `reserve` + * is how many binds the statement spends on something other than the list — a `now` cutoff, + * a player id — which come out of the same budget. + */ +export function chunkForBinds(values: T[], reserve = 0): T[][] { + const perChunk = MAX_BOUND_PARAMS - reserve + const chunks: T[][] = [] + for (let i = 0; i < values.length; i += perChunk) { + chunks.push(values.slice(i, i + perChunk)) + } + return chunks +} + +/** The `?1,?2,…` placeholder list for one chunk, offset past any binds reserved ahead of it. */ +export function bindPlaceholders(chunk: unknown[], offset = 0): string { + return chunk.map((_, i) => `?${i + 1 + offset}`).join(', ') +} diff --git a/packages/domain/src/index.ts b/packages/domain/src/index.ts index 65f2af1..e1e9ea5 100644 --- a/packages/domain/src/index.ts +++ b/packages/domain/src/index.ts @@ -8,6 +8,7 @@ export { } from './enums' export * from './accounts-db' export * from './clubs-db' +export * from './d1-binds' export * from './images-db' export * from './password' export * from './rooms-db' diff --git a/packages/domain/src/presence-db.ts b/packages/domain/src/presence-db.ts index 874e9e4..feb6db4 100644 --- a/packages/domain/src/presence-db.ts +++ b/packages/domain/src/presence-db.ts @@ -21,6 +21,8 @@ * {@link deleteExpiredPresence} purges them. */ +import { bindPlaceholders, chunkForBinds } from './d1-binds' + /** Presence is kept this long (s) after the last matchmake/heartbeat refresh. */ export const PRESENCE_TTL_SECONDS = 900 @@ -136,7 +138,7 @@ export async function getPresence( } /** - * Read many players' live presence in one query, keyed by account id (absent or + * Read many players' live presence in as few queries as D1 allows, keyed by account id (absent or * expired players are simply missing from the map). Replaces the N point reads the * batch `/player?id=…` lookup did against KV. */ @@ -147,15 +149,20 @@ export async function getPresences( ): Promise>> { const out = new Map>() if (accountIds.length === 0) return out - const placeholders = accountIds.map((_, i) => `?${i + 1}`).join(', ') - const { results } = await db - .prepare( - `SELECT data FROM presence - WHERE account_id IN (${placeholders}) AND expires_at > ?${accountIds.length + 1}` + // The expiry cutoff spends one of the statement's binds, so each chunk of ids runs one + // short of the cap. A friends list is routinely longer than that; unchunked, it 500s. + const pages = await Promise.all( + chunkForBinds(accountIds, 1).map((chunk) => + db + .prepare( + `SELECT data FROM presence + WHERE account_id IN (${bindPlaceholders(chunk)}) AND expires_at > ?${chunk.length + 1}` + ) + .bind(...chunk, now) + .all<{ data: string }>() ) - .bind(...accountIds, now) - .all<{ data: string }>() - for (const r of results) { + ) + for (const r of pages.flatMap((page) => page.results)) { const p = JSON.parse(r.data) as StoredPresence out.set(p.accountId, p) } diff --git a/packages/domain/src/progression-db.ts b/packages/domain/src/progression-db.ts index 088c35f..6377998 100644 --- a/packages/domain/src/progression-db.ts +++ b/packages/domain/src/progression-db.ts @@ -18,6 +18,8 @@ * the writer. */ +import { bindPlaceholders, chunkForBinds } from './d1-binds' + /** Schema DDL (mirror of apps/econ/migrations/0012_progression.sql). */ export const PROGRESSION_SCHEMA_DDL: string[] = [ `CREATE TABLE IF NOT EXISTS progression ( @@ -211,12 +213,19 @@ export async function getProgressions( accountIds: number[] ): Promise { if (accountIds.length === 0) return [] - const placeholders = accountIds.map((_, i) => `?${i + 1}`).join(', ') - const { results } = await db - .prepare(`SELECT account_id, level, xp FROM progression WHERE account_id IN (${placeholders})`) - .bind(...accountIds) - .all<{ account_id: number; level: number; xp: number }>() - const stored = new Map(results.map((r) => [r.account_id, r])) + // Chunked: the friends list behind `/api/players/v2/progression/bulk` runs past the + // bind cap, and an unchunked statement fails the whole lookup rather than part of it. + const pages = await Promise.all( + chunkForBinds(accountIds).map((chunk) => + db + .prepare( + `SELECT account_id, level, xp FROM progression WHERE account_id IN (${bindPlaceholders(chunk)})` + ) + .bind(...chunk) + .all<{ account_id: number; level: number; xp: number }>() + ) + ) + const stored = new Map(pages.flatMap((page) => page.results).map((r) => [r.account_id, r])) return accountIds.map((id) => { const row = stored.get(id) return row === undefined diff --git a/packages/domain/src/rooms-db.ts b/packages/domain/src/rooms-db.ts index 15d2262..253fb49 100644 --- a/packages/domain/src/rooms-db.ts +++ b/packages/domain/src/rooms-db.ts @@ -17,6 +17,7 @@ * database read-only and uses `getRoomById`. Each imports the subset it needs. */ +import { bindPlaceholders, chunkForBinds, MAX_BOUND_PARAMS } from './d1-binds' import { Accessibility, Role } from './enums' import { countPlayersByRoom } from './presence-db' @@ -1428,27 +1429,6 @@ const parseRow = (row: RoomRow): Room => { const parseOne = (row: RoomRow | null): Room | null => (row ? parseRow(row) : null) const parseAll = (rows: RoomRow[]): Room[] => rows.map(parseRow) -/** - * D1 caps a prepared statement at 100 bound parameters — binding more fails outright with - * "variable number must be between ?1 and ?100". Every `IN (…)` list built from a caller's - * array has to respect this, which is easy to miss: a seeded dev database has fewer than a - * hundred rooms, so an unchunked query works right up until it meets a real one. - */ -const MAX_BOUND_PARAMS = 100 - -/** - * Split values into chunks that fit {@link MAX_BOUND_PARAMS}, for the reads whose rows are - * too heavy to fetch wholesale (subroom and save blobs) and so have to page through an - * `IN (…)` rather than scan. - */ -function chunkForBinds(values: T[]): T[][] { - const chunks: T[][] = [] - for (let i = 0; i < values.length; i += MAX_BOUND_PARAMS) { - chunks.push(values.slice(i, i + MAX_BOUND_PARAMS)) - } - return chunks -} - /** * Run one `… IN (…)` query per chunk and concatenate the rows. `sql` is handed the * placeholder list for its chunk (`?1,?2,…`), which always restarts at `?1` because each