Merge pull request #215 from Coffey-Labs/fix/ical-import-batching

Import an iCal file in batches the server will take

web/src/store/__tests__/ics-import.test.ts

@@ -6,9 +6,10 @@ import type { JmapSession, UploadResponse } from "@/jmap/types";
 /**
  * Importing a file is not importing an invitation, and the difference is the
  * count: an emailed invite carries one event, an export carries a year of them.
- * These pin the two things that follow from that -- one round trip rather than
+ * These pin the three things that follow from that -- as few round trips as the
- * one per event, and nothing of where the events came from riding along into
+ * server will take, none of them over the ceiling it will refuse the whole call
- * the calendar they land in.
+ * for, and nothing of where the events came from riding along into the calendar
+ * they land in.
  */
 
 /** What the server hands back for a two-event file. Ids and the JMAP-only
@@ -33,8 +34,11 @@ interface SetArgs { create?: Record<string, Record<string, unknown>>; sendSchedu
  * @param parsed what `CalendarEvent/parse` answers with; a bare object rather
  *        than an array is the single-event shape, which Stalwart also returns.
  * @param notCreated refusals to hand back instead of creations.
+ * @param max the ceiling on objects in one call, refused the way Stalwart
+ *        refuses it: the whole call, creating nothing.
+ * @param failOn which `/set` call (0-based) answers with an error instead.
  */
-function server(parsed: unknown, opts: { notCreated?: Record<string, unknown> } = {}) {
+function server(parsed: unknown, opts: { notCreated?: Record<string, unknown>; max?: number; failOn?: number } = {}) {
   const sets: SetArgs[] = [];
   const fetchMock = vi.fn(async (_url: string, init: RequestInit) => {
     const body = JSON.parse(init.body as string) as { methodCalls: [string, Record<string, unknown>, string][] };
@@ -44,8 +48,14 @@ function server(parsed: unknown, opts: { notCreated?: Record<string, unknown> }
         return [name, { accountId: "a1", parsed: parsed === null ? {} : { [blobIds[0]!]: parsed }, notParsable: [] }, id];
       }
       if (name === "CalendarEvent/set") {
+        const nth = sets.length;
         sets.push({ create: args.create as Record<string, Record<string, unknown>>, sendSchedulingMessages: args.sendSchedulingMessages as boolean });
         const keys = Object.keys((args.create ?? {}) as object);
+        // Whole-call refusals, both of them: nothing in this call is created.
+        if (opts.max != null && keys.length > opts.max) {
+          return ["error", { type: "requestTooLarge", description: "The number of ids requested by the client exceeds the maximum number the server is willing to process in a single method call." }, id];
+        }
+        if (opts.failOn === nth) return ["error", { type: "serverFail", description: "the roof fell in" }, id];
         const notCreated = opts.notCreated ?? {};
         return [name, {
           accountId: "a1", oldState: "1", newState: "2",
@@ -62,13 +72,15 @@ function server(parsed: unknown, opts: { notCreated?: Record<string, unknown> }
 }
 
 let uploaded: { type?: string; text: string } | null = null;
+/** Two tests stand a mock in for it; put the store's own back afterwards. */
+const realInvalidate = useCalendar.getState().invalidate;
 
 beforeEach(() => {
   client.session = {
     capabilities: { [CAP.core]: { maxObjectsInGet: 500, maxObjectsInSet: 500 }, [CAP.calendars]: {} },
     accounts: {}, primaryAccounts: {}, state: "s1",
   } as unknown as JmapSession;
-  useCalendar.setState({ accountId: "a1", available: true, calendars: {}, events: {}, ranges: {} });
+  useCalendar.setState({ accountId: "a1", available: true, calendars: {}, events: {}, ranges: {}, invalidate: realInvalidate });
   uploaded = null;
   // XHR, not fetch, so it is stubbed at the client rather than at the network.
   // jsdom's Blob has no `text()`, hence the reader.
@@ -96,7 +108,7 @@ describe("importing an .ics file", () => {
     expect(uploaded?.text).toContain("BEGIN:VCALENDAR");
   });
 
-  it("creates every event in one call, not one call each", async () => {
+  it("creates every event in one call when the file fits in one, not one call each", async () => {
     const sets = server(PARSED);
     const n = await useCalendar.getState().importIcs("x", "cal1");
     expect(n).toBe(2);
@@ -159,3 +171,69 @@ describe("importing an .ics file", () => {
     await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toBe(1);
   });
 });
+
+/*
+ * A real export, rather than the two-event file above.
+ *
+ * `CalendarEvent/set` is refused whole over `maxObjectsInSet` -- the server
+ * does not take the first 500 and drop the rest, it creates nothing and answers
+ * `requestTooLarge` -- so a file large enough to cross the ceiling used to
+ * import no events at all. The server here refuses the same way, which is what
+ * makes these more than an assertion about call counts.
+ */
+describe("importing a file bigger than the server will take at once", () => {
+  const MAX = 500;
+  const many = (n: number) =>
+    Array.from({ length: n }, (_, i) => ({
+      "@type": "Event", uid: `uid-${i}@example.org`, title: `Event ${i}`,
+      start: "2026-09-02T09:00:00", duration: "PT1H", timeZone: "Etc/UTC",
+    }));
+
+  it("splits it into calls the server will accept, and files all of it", async () => {
+    const sets = server(many(1200), { max: MAX });
+    await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toBe(1200);
+    expect(sets.map((s) => Object.keys(s.create!).length)).toEqual([500, 500, 200]);
+  });
+
+  it("splits by what the session advertises, not by a number of its own", async () => {
+    client.session!.capabilities[CAP.core] = { maxObjectsInGet: 40, maxObjectsInSet: 40 };
+    const sets = server(many(100), { max: 40 });
+    await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toBe(100);
+    expect(sets.map((s) => Object.keys(s.create!).length)).toEqual([40, 40, 20]);
+  });
+
+  it("keeps every event distinct across the split", async () => {
+    const sets = server(many(600), { max: MAX });
+    await useCalendar.getState().importIcs("x", "cal1");
+    const uids = sets.flatMap((s) => Object.values(s.create!).map((e) => e.uid));
+    expect(new Set(uids).size).toBe(600);
+    expect(uids).toContain("[email protected]");
+    expect(uids).toContain("[email protected]");
+  });
+
+  it("re-reads the calendar once, not once per batch", async () => {
+    server(many(1200), { max: MAX });
+    const invalidate = vi.fn();
+    useCalendar.setState({ invalidate });
+    await useCalendar.getState().importIcs("x", "cal1");
+    expect(invalidate).toHaveBeenCalledTimes(1);
+  });
+
+  it("says how much got in when a later batch fails, rather than only that it failed", async () => {
+    server(many(1200), { max: MAX, failOn: 2 });
+    await expect(useCalendar.getState().importIcs("x", "cal1")).rejects.toThrow(/1000 of 1200/);
+  });
+
+  it("leaves what did get in visible when a later batch fails", async () => {
+    server(many(1200), { max: MAX, failOn: 2 });
+    const invalidate = vi.fn();
+    useCalendar.setState({ invalidate });
+    await expect(useCalendar.getState().importIcs("x", "cal1")).rejects.toThrow();
+    expect(invalidate).toHaveBeenCalledTimes(1);
+  });
+
+  it("passes the server's own words through when the very first batch fails", async () => {
+    server(many(1200), { max: MAX, failOn: 0 });
+    await expect(useCalendar.getState().importIcs("x", "cal1")).rejects.toThrow(/roof fell in/);
+  });
+});

web/src/store/calendar.ts

@@ -1,6 +1,6 @@
 import { create } from "zustand";
-import { CAP, client, setErrorMessage } from "@/jmap/client";
+import { CAP, chunk, client, setErrorMessage } from "@/jmap/client";
-import type { BusyPeriod, Calendar, CalendarEvent, EmailAddress, GetResponse, Id, JSCalendarParticipant, JSCalendarRecurrenceRule, ParticipantIdentity, QueryResponse, SetResponse } from "@/jmap/types";
+import type { BusyPeriod, Calendar, CalendarEvent, EmailAddress, GetResponse, Id, JSCalendarParticipant, JSCalendarRecurrenceRule, ParticipantIdentity, QueryResponse, SetError, SetResponse } from "@/jmap/types";
 import { toUTCDate, toLocalDateTime, zonedToDate, parseDuration, DAY_MS, browserTimeZone } from "@/lib/dates";
 import { t } from "@/lib/i18n";
 import { useContacts } from "./contacts";
@@ -808,11 +808,16 @@ export const useCalendar = create<CalendarState>((set, get) => ({
    * a browser, and the one already in Stalwart handles what a hand-rolled
    * parser would not.
    *
-   * Every event goes out in one `CalendarEvent/set` rather than a call each.
+   * The events go out `maxObjectsInSet` at a time -- the ceiling the session
-   * The round trips are the smaller half of the reason: `createEvent`
+   * advertises, 500 where a server does not say. A call carrying more than that
-   * invalidates on the way out, and invalidating re-fetches every cached range,
+   * is refused whole with `requestTooLarge` and creates nothing, so a real
-   * so importing a year of events one at a time would refetch the calendar a
+   * export -- an 800 KB file is thousands of events -- imported nothing at all
-   * few hundred times.
+   * while this went out in a single call.
+   *
+   * Batches rather than a call per event, though: `createEvent` invalidates on
+   * the way out, and invalidating re-fetches every cached range, so importing a
+   * year of events one at a time would refetch the calendar a few hundred
+   * times. One invalidate here, after the last batch.
    *
    * No scheduling messages. Importing a file is filing something you already
    * have, and mailing its participants would be a surprise to everyone.
@@ -830,15 +835,29 @@ export const useCalendar = create<CalendarState>((set, get) => ({
       // invented, and an event with no UID is not one anything can match to.
       create[`e${i}`] = { "@type": "Event", ...rest, uid: rest.uid || crypto.randomUUID(), calendarIds: { [calendarId]: true } };
     });
-    const res = await client.call<SetResponse<CalendarEvent>>("CalendarEvent/set", { accountId, create, sendSchedulingMessages: false });
+    const keys = Object.keys(create);
-    get().invalidate();
+    let created = 0;
-    const created = Object.keys(res.created ?? {}).length;
+    let refused: SetError | undefined;
+    try {
+      for (const part of chunk(keys, client.maxObjectsInSet)) {
+        const sub: Record<string, unknown> = {};
+        for (const k of part) sub[k] = create[k];
+        const res = await client.call<SetResponse<CalendarEvent>>("CalendarEvent/set", { accountId, create: sub, sendSchedulingMessages: false });
+        created += Object.keys(res.created ?? {}).length;
+        refused ??= Object.values(res.notCreated ?? {})[0];
+      }
+    } catch (err) {
+      // A batch that failed with earlier ones already filed: those events are
+      // in the calendar, and an error saying only that the import failed sends
+      // someone looking for events that are already there.
+      if (!created) throw err;
+      throw new Error(`${created} of ${keys.length} events were imported before this happened: ${(err as Error).message}`);
+    } finally {
+      if (created) get().invalidate();
+    }
     // Nothing at all got in: say why rather than report importing zero events
     // as though the file had been empty.
-    if (!created) {
+    if (!created) throw new Error(refused ? setErrorMessage(refused) : "the server did not accept any of its events");
-      const first = Object.values(res.notCreated ?? {})[0];
-      throw new Error(first ? setErrorMessage(first) : "the server did not accept any of its events");
-    }
     return created;
   },