diff --git a/src/__tests__/service/admin-service.test.ts b/src/__tests__/service/admin-service.test.ts index b1fe6c6..9a161ad 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/__tests__/service/bundle-service.test.ts b/src/__tests__/service/bundle-service.test.ts index e495aba..cbc30b7 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 5983ed2..c10e867 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/admin-management.ts b/src/services/admin-management.ts index 819ad49..8b05e53 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); diff --git a/src/services/bundle-management.ts b/src/services/bundle-management.ts index 697f57a..bad0a19 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), ]);