From 905a8712afa70bb7a6646f91e82658968be1aab2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 19:15:36 +0000 Subject: [PATCH 1/2] Keep the bundle listing under D1's bound-parameter cap listBundles and getBundleSummariesBulk bound one parameter per bundle id. D1 rejects a statement past 100 parameters, so a deployment with roughly 100 or more active bundles got SQLITE_ERROR (too many SQL variables) on the bundles page and GET /bundles. Link counts now group without an IN list and the summaries run in slices of 50 ids, merged afterwards. Regression test lists 120 bundles with and without a range. --- src/__tests__/service/bundle-service.test.ts | 32 ++++++++++++++++++++ src/db/click-repository.ts | 17 +++++++++++ src/services/bundle-management.ts | 8 ++--- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/__tests__/service/bundle-service.test.ts b/src/__tests__/service/bundle-service.test.ts index e495aba5..cbc30b7e 100644 --- a/src/__tests__/service/bundle-service.test.ts +++ b/src/__tests__/service/bundle-service.test.ts @@ -87,6 +87,38 @@ describe("listBundles", () => { }); }); +describe("listBundles past D1's bound-parameter cap", () => { + it("lists and summarises more bundles than one statement can bind", async () => { + // D1 rejects a statement with more than 100 bound parameters. The listing + // used to bind one id per bundle, so the 100th bundle broke the page and + // GET /bundles with SQLITE_ERROR. + const count = 120; + const link = await LinkRepository.create(env.DB, { url: "https://a.com", slug: "aaa", createdBy: "a@b" }); + const ids: number[] = []; + for (let i = 0; i < count; i++) { + const b = await BundleRepository.create(env.DB, { name: `B${i}`, createdBy: "a@b" }); + ids.push(b.id); + } + // Last bundle sits beyond the first 100 ids and must still get its numbers. + await BundleRepository.addLink(env.DB, ids[count - 1], link.id); + const now = Math.floor(Date.now() / 1000); + await env.DB.prepare("INSERT INTO clicks (slug, clicked_at, link_mode) VALUES (?, ?, 'link')") + .bind("aaa", now).run(); + + for (const range of [undefined, "7d" as const]) { + const res = await svc.listBundles(e, "a@b", { range, cardExtras: true }); + expect(res.ok).toBe(true); + if (!res.ok) continue; + expect(res.data).toHaveLength(count); + const last = res.data.find((b) => b.id === ids[count - 1])!; + expect(last.link_count).toBe(1); + expect(last.total_clicks).toBe(1); + expect(last.clicked_links).toBe(1); + expect(last.top_links).toEqual([{ slug: "aaa", click_count: 1 }]); + } + }); +}); + describe("getBundle / updateBundle", () => { it("returns 404 for unknown id", async () => { const res = await svc.getBundle(e, 99999, "a@b"); diff --git a/src/db/click-repository.ts b/src/db/click-repository.ts index 5983ed21..c10e867d 100644 --- a/src/db/click-repository.ts +++ b/src/db/click-repository.ts @@ -1440,6 +1440,20 @@ export class ClickRepository { // dataset-wide day span.) includeCardExtras = false, ): Promise> { + // Every query below binds one parameter per bundle id, and D1 rejects a + // statement past D1_MAX_BOUND_PARAMS. Past one chunk, summarise slices of + // the ids and merge: the summaries are per bundle, so slices are independent. + if (bundleIds.length > BUNDLE_SUMMARY_CHUNK) { + const merged = new Map(); + const ts = now ?? Math.floor(Date.now() / 1000); + for (let i = 0; i < bundleIds.length; i += BUNDLE_SUMMARY_CHUNK) { + const part = await this.getBundleSummariesBulk( + db, bundleIds.slice(i, i + BUNDLE_SUMMARY_CHUNK), ts, filters, range, includeCardExtras, + ); + for (const [id, summary] of part) merged.set(id, summary); + } + return merged; + } const out = new Map(); for (const id of bundleIds) { out.set(id, { total_clicks: 0, sparkline: [], top_links: [] }); @@ -1587,6 +1601,9 @@ export class ClickRepository { } } +/** Bundle ids bound per statement in getBundleSummariesBulk, with headroom under D1_MAX_BOUND_PARAMS for the range and filter binds. */ +const BUNDLE_SUMMARY_CHUNK = 50; + function countDistinctHosts(rows: { url: string }[]): number { const hosts = new Set(); for (const r of rows) { diff --git a/src/services/bundle-management.ts b/src/services/bundle-management.ts index 697f57a2..bad0a19b 100644 --- a/src/services/bundle-management.ts +++ b/src/services/bundle-management.ts @@ -87,12 +87,12 @@ export async function listBundles( const filters = await resolveClickFilters(env, identity); const range = opts.range ?? "all"; + // Counted per bundle without binding the ids: an IN list past D1's + // 100-parameter cap fails the whole listing, and the table is one row per + // membership, so grouping it whole costs no more than a long IN list. const [linkCounts, summaries] = await Promise.all([ env.DB - .prepare( - `SELECT bundle_id, COUNT(*) as cnt FROM bundle_links WHERE bundle_id IN (${bundleIds.map(() => "?").join(",")}) GROUP BY bundle_id`, - ) - .bind(...bundleIds) + .prepare("SELECT bundle_id, COUNT(*) as cnt FROM bundle_links GROUP BY bundle_id") .all<{ bundle_id: number; cnt: number }>(), ClickRepository.getBundleSummariesBulk(env.DB, bundleIds, undefined, filters, range, opts.cardExtras), ]); From e78e87444080acd598a76d2ae50d765435babd32 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 19:16:47 +0000 Subject: [PATCH 2/2] Validate every settings field before storing any updateAppSettings validated and wrote one field at a time, so a request rejected with 400 on a later field (an unknown theme, a non-boolean filter) had already stored the earlier ones, while telling the caller nothing was saved. All fields are now validated first and written only when the whole request is valid. Regression test sends a valid slug length and filter with an invalid theme and checks that neither is stored. --- src/__tests__/service/admin-service.test.ts | 22 +++++++++++++++++++++ src/services/admin-management.ts | 20 ++++++++++++------- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/__tests__/service/admin-service.test.ts b/src/__tests__/service/admin-service.test.ts index b1fe6c62..9a161ad0 100644 --- a/src/__tests__/service/admin-service.test.ts +++ b/src/__tests__/service/admin-service.test.ts @@ -1,6 +1,7 @@ import { beforeAll, beforeEach, describe, expect, it } from "vitest"; import { env } from "cloudflare:test"; import { applyMigrations, resetData } from "../setup"; +import { DEFAULT_SLUG_LENGTH } from "../../constants"; import { createNewApiKey, getAppSettings, @@ -278,3 +279,24 @@ describe("API key title validation", () => { expect(result.ok).toBe(true); }); }); + +describe("settings update is all or nothing", () => { + it("stores no field when a later field in the same request is invalid", async () => { + // Fields used to be validated and written one at a time, so a request + // that failed with 400 on its theme had already stored its slug length. + const result = await updateAppSettings(env as any, TEST_IDENTITY, { + slug_default_length: 9, + filter_bots: false, + theme: "neon", + }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.status).toBe(400); + + const settings = await getAppSettings(env as any, TEST_IDENTITY); + expect(settings.ok).toBe(true); + if (settings.ok) { + expect(settings.data.slug_default_length).toBe(DEFAULT_SLUG_LENGTH); + expect(settings.data.filter_bots).toBe(true); + } + }); +}); diff --git a/src/services/admin-management.ts b/src/services/admin-management.ts index 819ad490..8b05e53e 100644 --- a/src/services/admin-management.ts +++ b/src/services/admin-management.ts @@ -140,28 +140,31 @@ export async function updateAppSettings( filter_self_referrers?: boolean; }, ): Promise> { + // Validate every field before storing any, so a request rejected with 400 + // leaves the settings as they were instead of half applied. + const writes: [key: string, value: string][] = []; if (body.slug_default_length !== undefined) { const err = validateSlugLength(body.slug_default_length); if (err) return fail(400, err); - await SettingRepository.set(env.DB, identity, "slug_default_length", String(body.slug_default_length)); + writes.push(["slug_default_length", String(body.slug_default_length)]); } if (body.theme !== undefined) { if (typeof body.theme !== "string" || !(THEMES as readonly string[]).includes(body.theme)) { return fail(400, `theme must be one of: ${THEMES.join(", ")}`); } - await SettingRepository.set(env.DB, identity, "theme", body.theme); + writes.push(["theme", body.theme]); } if (body.lang !== undefined) { if (typeof body.lang !== "string" || !(SUPPORTED_LANGUAGES as readonly string[]).includes(body.lang)) { return fail(400, `lang must be one of: ${SUPPORTED_LANGUAGES.join(", ")}`); } - await SettingRepository.set(env.DB, identity, "lang", body.lang); + writes.push(["lang", body.lang]); } if (body.default_range !== undefined) { if (body.default_range === null || body.default_range === "") { - await SettingRepository.set(env.DB, identity, "default_range", ""); + writes.push(["default_range", ""]); } else if (isValidRange(body.default_range)) { - await SettingRepository.set(env.DB, identity, "default_range", body.default_range); + writes.push(["default_range", body.default_range]); } else { return fail(400, `default_range must be one of: ${TIMELINE_RANGES.join(", ")}`); } @@ -170,13 +173,16 @@ export async function updateAppSettings( if (typeof body.filter_bots !== "boolean") { return fail(400, "filter_bots must be a boolean"); } - await SettingRepository.set(env.DB, identity, "filter_bots", String(body.filter_bots)); + writes.push(["filter_bots", String(body.filter_bots)]); } if (body.filter_self_referrers !== undefined) { if (typeof body.filter_self_referrers !== "boolean") { return fail(400, "filter_self_referrers must be a boolean"); } - await SettingRepository.set(env.DB, identity, "filter_self_referrers", String(body.filter_self_referrers)); + writes.push(["filter_self_referrers", String(body.filter_self_referrers)]); + } + for (const [key, value] of writes) { + await SettingRepository.set(env.DB, identity, key, value); } return getAppSettings(env, identity);