Weekly defect review: bundle listing past D1 bind cap, all-or-nothing settings update

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);
+    }
+  });
+});

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");

src/db/click-repository.ts

@@ -1440,6 +1440,20 @@ export class ClickRepository {
     // dataset-wide day span.)
     includeCardExtras = false,
   ): Promise<Map<number, { total_clicks: number; delta_pct?: number; sparkline: number[]; top_links: { slug: string; click_count: number }[]; clicked_links?: number }>> {
+    // 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<number, { total_clicks: number; delta_pct?: number; sparkline: number[]; top_links: { slug: string; click_count: number }[]; clicked_links?: number }>();
+      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<number, { total_clicks: number; delta_pct?: number; sparkline: number[]; top_links: { slug: string; click_count: number }[]; clicked_links?: number }>();
     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<string>();
   for (const r of rows) {

src/services/admin-management.ts

@@ -140,28 +140,31 @@ export async function updateAppSettings(
     filter_self_referrers?: boolean;
   },
 ): Promise<ServiceResult<AppSettings>> {
+  // 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);

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),
   ]);