Merge pull request #218 from Coffey-Labs/fix/contact-import-batching

Set contact cards in batches the server will take

web/src/store/__tests__/contact-set-batching.test.ts

@@ -0,0 +1,179 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { CAP, client } from "@/jmap/client";
+import { useContacts } from "@/store/contacts";
+import type { ContactCard, Id, JmapSession, UploadResponse } from "@/jmap/types";
+
+/*
+ * `ContactCard/set` over the server's ceiling.
+ *
+ * Stalwart refuses a method call carrying more objects than `maxObjectsInSet`
+ * whole -- it does not take the first 500 and drop the rest, it creates nothing
+ * and answers `requestTooLarge`. The calendar import was found doing this on a
+ * real 800 KB file (#173); contacts had it in three places, and this is what
+ * would have caught them.
+ *
+ * The server below refuses the same way, which is what makes these more than an
+ * assertion about how many calls went out.
+ */
+
+const MAX = 500;
+
+interface SetArgs { create?: Record<string, Record<string, unknown>>; destroy?: Id[] }
+
+/**
+ * @param max the ceiling on objects in one call, refused whole the way Stalwart
+ *        refuses it.
+ * @param failOn which `/set` call (0-based) answers with an error instead.
+ * @param parsed what `ContactCard/parse` answers with, for the vCard import.
+ * @param notCreated refusals to hand back instead of creations.
+ */
+function server(opts: { max?: number; failOn?: number; parsed?: unknown[]; notCreated?: Record<string, unknown> } = {}) {
+  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][] };
+    const methodResponses = body.methodCalls.map(([name, args, id]) => {
+      if (name === "ContactCard/parse") {
+        const blobId = (args.blobIds as string[])[0]!;
+        return [name, { accountId: "a1", parsed: { [blobId]: opts.parsed ?? [] }, notParsable: [] }, id];
+      }
+      if (name === "ContactCard/set") {
+        const nth = sets.length;
+        const create = args.create as Record<string, Record<string, unknown>> | undefined;
+        const destroy = args.destroy as Id[] | undefined;
+        sets.push({ create, destroy });
+        const n = Object.keys(create ?? {}).length + (destroy?.length ?? 0);
+        if (opts.max != null && n > 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",
+          created: Object.fromEntries(Object.keys(create ?? {}).filter((k) => !(k in notCreated)).map((k) => [k, { id: `new-${k}` }])),
+          notCreated,
+          destroyed: destroy ?? [],
+        }, id];
+      }
+      return [name, { accountId: "a1", state: "1", list: [], notFound: [], ids: [], total: 0, queryState: "q", position: 0, canCalculateChanges: false }, id];
+    });
+    return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "1" }) } as Response;
+  });
+  vi.stubGlobal("fetch", fetchMock);
+  return sets;
+}
+
+/** An LDIF of `n` entries, each with the name and mail `cardFromLdif` needs. */
+const ldifOf = (n: number) =>
+  Array.from({ length: n }, (_, i) => `dn: cn=Person ${i}\ngivenName: Person\nsn: N${i}\ncn: Person ${i}\nmail: p${i}@example.org\n`).join("\n");
+
+/** What `ContactCard/parse` hands back for a vCard file of `n` contacts. */
+const vcardsOf = (n: number) =>
+  Array.from({ length: n }, (_, i) => ({
+    "@type": "Card", version: "1.0", uid: `uid-${i}`, kind: "individual",
+    name: { full: `Person ${i}` }, emails: { e1: { address: `p${i}@example.org` } },
+  }));
+
+/** `n` cards already in the list, ready to be deleted. */
+const cardsInState = (n: number) =>
+  Object.fromEntries(Array.from({ length: n }, (_, i) => [`c${i}`, { id: `c${i}`, name: { full: `Person ${i}` } }])) as unknown as Record<Id, ContactCard>;
+
+const sizes = (sets: SetArgs[]) => sets.map((s) => Object.keys(s.create ?? {}).length + (s.destroy?.length ?? 0));
+
+beforeEach(() => {
+  client.session = {
+    capabilities: { [CAP.core]: { maxObjectsInGet: MAX, maxObjectsInSet: MAX }, [CAP.contacts]: {} },
+    accounts: {}, primaryAccounts: {}, state: "s1",
+  } as unknown as JmapSession;
+  useContacts.setState({ accountId: "a1", available: true, books: {}, cards: {} });
+  vi.spyOn(client, "upload").mockResolvedValue({ accountId: "a1", blobId: "blob1", type: "text/vcard", size: 1 } as UploadResponse);
+});
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+  vi.restoreAllMocks();
+});
+
+describe("importing an LDIF bigger than the server will take at once", () => {
+  it("splits it into calls the server will accept, and files all of it", async () => {
+    const sets = server({ max: MAX });
+    await expect(useContacts.getState().importLdif(ldifOf(1200), "book1")).resolves.toBe(1200);
+    expect(sizes(sets)).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({ max: 40 });
+    await expect(useContacts.getState().importLdif(ldifOf(100), "book1")).resolves.toBe(100);
+    expect(sizes(sets)).toEqual([40, 40, 20]);
+  });
+
+  it("keeps every entry distinct across the split", async () => {
+    const sets = server({ max: MAX });
+    await useContacts.getState().importLdif(ldifOf(600), "book1");
+    const names = sets.flatMap((s) => Object.values(s.create!).map((c) => (c.name as { full: string }).full));
+    expect(new Set(names).size).toBe(600);
+    expect(names).toContain("Person 0");
+    expect(names).toContain("Person 599");
+  });
+
+  it("says how much got in when a later batch fails, rather than only that it failed", async () => {
+    server({ max: MAX, failOn: 2 });
+    await expect(useContacts.getState().importLdif(ldifOf(1200), "book1")).rejects.toThrow(/1000 of 1200/);
+  });
+
+  it("passes the server's own words through when the very first batch fails", async () => {
+    server({ max: MAX, failOn: 0 });
+    await expect(useContacts.getState().importLdif(ldifOf(1200), "book1")).rejects.toThrow(/roof fell in/);
+  });
+});
+
+describe("importing a vCard file bigger than the server will take at once", () => {
+  it("splits it into calls the server will accept, and files all of it", async () => {
+    const sets = server({ max: MAX, parsed: vcardsOf(1200) });
+    await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toBe(1200);
+    expect(sizes(sets)).toEqual([500, 500, 200]);
+  });
+
+  it("says how much got in when a later batch fails", async () => {
+    server({ max: MAX, failOn: 2, parsed: vcardsOf(1200) });
+    await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).rejects.toThrow(/1000 of 1200/);
+  });
+
+  /*
+   * The odd one out before this: a vCard import the server refused every card
+   * of returned 0 and the view reported importing no contacts, which reads as
+   * an empty file. The LDIF import had said why since it was written.
+   */
+  it("says why when the server accepted none of it, rather than reporting none imported", async () => {
+    server({ max: MAX, parsed: vcardsOf(2), notCreated: { c0: { type: "invalidProperties", description: "name is required" }, c1: { type: "invalidProperties" } } });
+    await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).rejects.toThrow(/name is required/);
+  });
+
+  it("counts what got in when only some of it did", async () => {
+    server({ max: MAX, parsed: vcardsOf(2), notCreated: { c1: { type: "invalidProperties" } } });
+    await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toBe(1);
+  });
+});
+
+describe("deleting more contacts than the server will take at once", () => {
+  it("splits the selection into calls the server will accept", async () => {
+    const sets = server({ max: MAX });
+    useContacts.setState({ cards: cardsInState(1200) });
+    await useContacts.getState().destroyCards(Object.keys(cardsInState(1200)));
+    expect(sizes(sets)).toEqual([500, 500, 200]);
+    expect(Object.keys(useContacts.getState().cards)).toHaveLength(0);
+  });
+
+  it("takes off the list what actually went, when a later batch fails", async () => {
+    server({ max: MAX, failOn: 2 });
+    useContacts.setState({ cards: cardsInState(1200) });
+    await expect(useContacts.getState().destroyCards(Object.keys(cardsInState(1200)))).rejects.toThrow(/roof fell in/);
+    // The two batches that succeeded are gone; the third is still there rather
+    // than vanishing from a list it was never removed from on the server.
+    expect(Object.keys(useContacts.getState().cards)).toHaveLength(200);
+  });
+});

web/src/store/contacts.ts

@@ -1,7 +1,7 @@
 import { create } from "zustand";
 import { accountKey, loadRaw, saveJson } from "@/lib/storage";
-import { CAP, client, setErrorMessage } from "@/jmap/client";
+import { CAP, chunk, client, setErrorMessage } from "@/jmap/client";
-import type { AddressBook, ContactCard, EmailAddress, GetResponse, Id, Principal, QueryResponse, SetResponse } from "@/jmap/types";
+import type { AddressBook, ContactCard, EmailAddress, GetResponse, Id, Principal, QueryResponse, SetError, SetResponse } from "@/jmap/types";
 import { contactDisplayName, contactEmails, sortKey } from "@/lib/contacts";
 import { parseLdif } from "@/lib/ldif";
 import { cardFromLdif } from "@/lib/mozillaAb";
@@ -9,6 +9,48 @@ import { useSettings } from "./settings";
 import { useSession } from "./session";
 import { useMail } from "./mail";
 
+/**
+ * Create cards in batches the server will take.
+ *
+ * `ContactCard/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 an address book big enough to cross the ceiling
+ * imported nothing at all. The same bug the calendar import had, found on a
+ * real 800 KB export ([#173]).
+ *
+ * `maxObjectsInSet` is what the session advertises and 500 where a server does
+ * not say; splitting by it rather than by a constant follows a deployment that
+ * has tuned the limit.
+ *
+ * Both imports come through here, which is what the LDIF import's "from
+ * `ContactCard/set` down they are the same" was always claiming and is now
+ * true of.
+ *
+ * [#173]: https://github.com/Coffey-Labs/ihasmail/issues/173
+ */
+async function createCards(accountId: Id, create: Record<string, unknown>): Promise<{ created: number; refused?: SetError }> {
+  const keys = Object.keys(create);
+  let created = 0;
+  let refused: SetError | undefined;
+  for (const part of chunk(keys, client.maxObjectsInSet)) {
+    const sub: Record<string, unknown> = {};
+    for (const k of part) sub[k] = create[k];
+    let res: SetResponse<ContactCard>;
+    try {
+      res = await client.call<SetResponse<ContactCard>>("ContactCard/set", { accountId, create: sub });
+    } catch (err) {
+      // A batch that failed with earlier ones already filed: those contacts are
+      // in the address book, and an error saying only that the import failed
+      // sends someone looking for contacts that are already there.
+      if (!created) throw err;
+      throw new Error(`${created} of ${keys.length} contacts were imported before this happened: ${(err as Error).message}`);
+    }
+    created += Object.keys(res.created ?? {}).length;
+    refused ??= Object.values(res.notCreated ?? {})[0];
+  }
+  return { created, refused };
+}
+
 export interface Suggestion {
   name: string | null;
   email: string;
@@ -316,16 +358,35 @@ export const useContacts = create<ContactsState>((set, get) => ({
     await get().getCard(id);
   },
 
+  /*
+   * Batched for the same reason the imports are: a selection larger than
+   * `maxObjectsInSet` is refused whole, so "select all" over a big address book
+   * deleted nothing and said why in JMAP's words.
+   *
+   * The ids that actually went are what leaves the list, rather than everything
+   * that was asked for. A batch that fails after earlier ones succeeded must
+   * not leave deleted contacts on screen, and must not take live ones off it.
+   */
   async destroyCards(ids) {
     const accountId = get().accountId!;
-    const res = await client.call<SetResponse>("ContactCard/set", { accountId, destroy: ids });
+    const gone: Id[] = [];
-    const failed = Object.values(res.notDestroyed ?? {})[0];
+    let failed: SetError | undefined;
+    try {
+      for (const part of chunk(ids, client.maxObjectsInSet)) {
+        const res = await client.call<SetResponse>("ContactCard/set", { accountId, destroy: part });
+        gone.push(...(res.destroyed ?? []));
+        failed ??= Object.values(res.notDestroyed ?? {})[0];
+      }
+    } finally {
+      if (gone.length) {
+        set((s) => {
+          const cards = { ...s.cards };
+          for (const id of gone) delete cards[id];
+          return { cards };
+        });
+      }
+    }
     if (failed) throw new Error(setErrorMessage(failed));
-    set((s) => {
-      const cards = { ...s.cards };
-      for (const id of ids) delete cards[id];
-      return { cards };
-    });
   },
 
   async createBook(name) {
@@ -366,9 +427,16 @@ export const useContacts = create<ContactsState>((set, get) => ({
       const { id: _id, addressBookIds: _ab, ...rest } = c as ContactCard & { id?: Id };
       create[`c${i}`] = { ...rest, uid: rest.uid || crypto.randomUUID(), addressBookIds: { [addressBookId]: true } };
     });
-    const res = await client.call<SetResponse<ContactCard>>("ContactCard/set", { accountId, create });
+    try {
-    await get().loadAll();
+      const { created, refused } = await createCards(accountId, create);
-    return Object.keys(res.created ?? {}).length;
+      // Nothing at all got in: say why rather than report importing none as
+      // though the file had been empty. The LDIF import said this already; a
+      // vCard import that quietly returned 0 was the odd one out.
+      if (!created) throw new Error(refused ? setErrorMessage(refused) : "the server did not accept any of its contacts");
+      return created;
+    } finally {
+      await get().loadAll();
+    }
   },
 
   /*
@@ -391,12 +459,13 @@ export const useContacts = create<ContactsState>((set, get) => ({
       // directory and is no use as a contact's identity anywhere else.
       create[`c${i}`] = { "@type": "Card", version: "1.0", ...c, uid: crypto.randomUUID(), addressBookIds: { [addressBookId]: true } };
     });
-    const res = await client.call<SetResponse<ContactCard>>("ContactCard/set", { accountId, create });
+    let created: number;
-    await get().loadAll();
+    try {
-    const created = Object.keys(res.created ?? {}).length;
+      const r = await createCards(accountId, create);
-    if (!created) {
+      created = r.created;
-      const first = Object.values(res.notCreated ?? {})[0];
+      if (!created) throw new Error(r.refused ? setErrorMessage(r.refused) : "the server did not accept any of its contacts");
-      throw new Error(first ? setErrorMessage(first) : "the server did not accept any of its contacts");
+    } finally {
+      await get().loadAll();
     }
     return created;
   },