coffey-labs/ihasmail
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Update a contact on re-import rather than skipping it

Browse Source 
#228 skipped a vCard whose UID the book already held. The reporter asked for
the opposite on #174 and he is right: the reason to import a file a second
time is usually that the first one was not right, so skipping means a
corrected export corrects nothing.

A merge, not a replacement. Properties the file carries overwrite what is
here; properties it does not mention are left alone, so a phone number added
in ihasmail after the first import survives a re-import of the original file.
The cost is that a field genuinely deleted at the source stays here, which is
the better way to be wrong -- the other way round loses work nobody asked to
lose. Worth confirming with him rather than assuming.

`addressBookIds` is left off the patch. The card is already in this book, so
saying it again says nothing, and saying it on a card that is also in another
book would move it.

Creates and updates now share one batch budget. Stalwart counts every object
in a /set together, so batching the halves separately would send 300 new and
300 changed as two calls of 300 and be refused for a limit of 500 that neither
half exceeds.

LDIF is untouched and still reports look-alikes without acting on them, since
what it should match on is the question still open on #223. Both imports keep
one answer shape so a caller need not know which it called; LDIF's `updated`
is always 0, which is the honest number rather than a missing field.

The message a vCard attached to a message shows changes with it: the newer
copy now wins instead of being dropped, so it says the contact was brought up
to date rather than that nothing was added.

Refs #223.
This commit is contained in:
jcoffey-dev
2026-09-02 15:06:04 -07:00
parent af0e8b2be2
commit 9f4bd65fd5
16 changed files with 210 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/store/__tests__/contact-set-batching.test.ts
+14 -9
View File
@@ -18,7 +18,7 @@ import type { ContactCard, Id, JmapSession, UploadResponse } from "@/jmap/types"
const MAX = 500;
interface SetArgs { create?: Record<string, Record<string, unknown>>; destroy?: Id[] }
interface SetArgs { create?: Record<string, Record<string, unknown>>; update?: Record<string, Record<string, unknown>>; destroy?: Id[] }
/**
* @param max the ceiling on objects in one call, refused whole the way Stalwart
@@ -39,9 +39,12 @@ function server(opts: { max?: number; failOn?: number; parsed?: unknown[]; notCr
if (name === "ContactCard/set") {
const nth = sets.length;
const create = args.create as Record<string, Record<string, unknown>> | undefined;
const update = args.update 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);
sets.push({ create, update, destroy });
/* Everything in the call counts against the ceiling, the way Stalwart
counts it -- creates and updates share one budget. */
const n = Object.keys(create ?? {}).length + Object.keys(update ?? {}).length + (destroy?.length ?? 0);
if (opts.max != null && n > opts.max) {
return [
"error",
@@ -54,7 +57,8 @@ function server(opts: { max?: number; failOn?: number; parsed?: unknown[]; notCr
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,
updated: Object.fromEntries(Object.keys(update ?? {}).map((k) => [k, null])),
notCreated, notUpdated: {},
destroyed: destroy ?? [],
}, id];
}
@@ -81,7 +85,8 @@ const vcardsOf = (n: number) =>
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));
const sizes = (sets: SetArgs[]) =>
sets.map((s) => Object.keys(s.create ?? {}).length + Object.keys(s.update ?? {}).length + (s.destroy?.length ?? 0));
beforeEach(() => {
client.session = {
@@ -100,14 +105,14 @@ afterEach(() => {
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.toEqual({ created: 1200, skipped: 0, alike: 0 });
await expect(useContacts.getState().importLdif(ldifOf(1200), "book1")).resolves.toEqual({ created: 1200, updated: 0, alike: 0 });
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.toEqual({ created: 100, skipped: 0, alike: 0 });
await expect(useContacts.getState().importLdif(ldifOf(100), "book1")).resolves.toEqual({ created: 100, updated: 0, alike: 0 });
expect(sizes(sets)).toEqual([40, 40, 20]);
});
@@ -134,7 +139,7 @@ describe("importing an LDIF bigger than the server will take at once", () => {
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.toEqual({ created: 1200, skipped: 0, alike: 0 });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1200, updated: 0, alike: 0 });
expect(sizes(sets)).toEqual([500, 500, 200]);
});
@@ -155,7 +160,7 @@ describe("importing a vCard file bigger than the server will take at once", () =
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.toEqual({ created: 1, skipped: 0, alike: 0 });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, updated: 0, alike: 0 });
});
});
web/src/store/__tests__/ldif-import.test.ts
+3 -3
View File
@@ -63,7 +63,7 @@ describe("importing an LDIF address book", () => {
it("creates every entry in one call, not one call each", async () => {
const sets = server();
const n = await useContacts.getState().importLdif(TWO, "book1");
expect(n).toEqual({ created: 2, skipped: 0, alike: 0 });
expect(n).toEqual({ created: 2, updated: 0, alike: 0 });
expect(sets).toHaveLength(1);
expect(Object.keys(sets[0]!.create!)).toEqual(["c0", "c1"]);
});
@@ -105,7 +105,7 @@ describe("importing an LDIF address book", () => {
it("skips entries too empty to be a person, and imports the rest", async () => {
const sets = server();
const n = await useContacts.getState().importLdif(`${TWO}\ndn: cn=Nobody\nobjectClass: top\n`, "book1");
expect(n).toEqual({ created: 2, skipped: 0, alike: 0 });
expect(n).toEqual({ created: 2, updated: 0, alike: 0 });
expect(Object.keys(sets[0]!.create!)).toHaveLength(2);
});
@@ -116,6 +116,6 @@ describe("importing an LDIF address book", () => {
it("counts what got in when only some of it did", async () => {
server({ notCreated: { c1: { type: "invalidProperties" } } });
await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 1, skipped: 0, alike: 0 });
await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 1, updated: 0, alike: 0 });
});
});
web/src/store/__tests__/ldif-likeness.test.ts
+3 -3
View File
@@ -72,7 +72,7 @@ describe("telling somebody what an LDIF re-import duplicated", () => {
it("counts an entry that matches an existing card on name and address", async () => {
server([card("c1", "Jane Doe", "[email protected]")]);
const r = await useContacts.getState().importLdif(entry("Jane Doe", "[email protected]"), "book1");
expect(r).toEqual({ created: 1, skipped: 0, alike: 1 });
expect(r).toEqual({ created: 1, updated: 0, alike: 1 });
});
it("imports it anyway, which is the whole point of counting rather than matching", async () => {
@@ -124,7 +124,7 @@ describe("telling somebody what an LDIF re-import duplicated", () => {
it("counts nothing against an empty book", async () => {
server([]);
const r = await useContacts.getState().importLdif(entry("Jane Doe", "[email protected]"), "book1");
expect(r).toEqual({ created: 1, skipped: 0, alike: 0 });
expect(r).toEqual({ created: 1, updated: 0, alike: 0 });
});
it("does not count the file against itself", async () => {
@@ -133,6 +133,6 @@ describe("telling somebody what an LDIF re-import duplicated", () => {
server([]);
const twice = entry("Jane Doe", "[email protected]") + "\n" + entry("Jane Doe", "[email protected]");
const r = await useContacts.getState().importLdif(twice, "book1");
expect(r).toEqual({ created: 2, skipped: 0, alike: 0 });
expect(r).toEqual({ created: 2, updated: 0, alike: 0 });
});
});
web/src/store/__tests__/vcard-dedupe.test.ts
+79 -13
View File
@@ -15,9 +15,9 @@ import type { ContactCard, JmapSession, UploadResponse } from "@/jmap/types";
const MAX = 500;
interface SetArgs { create?: Record<string, Record<string, unknown>> }
interface SetArgs { create?: Record<string, Record<string, unknown>>; update?: Record<string, Record<string, unknown>> }
function server(opts: { parsed?: unknown[]; existing?: Array<{ id: string; uid: string; addressBookIds: Record<string, boolean> }> } = {}) {
function server(opts: { parsed?: unknown[]; existing?: Array<{ id: string; uid: string; addressBookIds: Record<string, boolean> }>; max?: number } = {}) {
const sets: SetArgs[] = [];
const existing = opts.existing ?? [];
const fetchMock = vi.fn(async (_url: string, init: RequestInit) => {
@@ -36,11 +36,21 @@ function server(opts: { parsed?: unknown[]; existing?: Array<{ id: string; uid:
return [name, { accountId: "a1", state: "1", list: existing.filter((c) => want.has(c.id)), notFound: [] }, id];
}
if (name === "ContactCard/set") {
sets.push({ create: args.create as Record<string, Record<string, unknown>> });
sets.push({
create: args.create as Record<string, Record<string, unknown>>,
update: args.update as Record<string, Record<string, unknown>>,
});
/* Refused whole over the ceiling, the way Stalwart refuses it, and
counting creates and updates together the way Stalwart counts. */
const n = Object.keys((args.create ?? {}) as object).length + Object.keys((args.update ?? {}) as object).length;
if (opts.max != null && n > opts.max) {
return ["error", { type: "requestTooLarge", description: "too many objects" }, id];
}
return [name, {
accountId: "a1", oldState: "1", newState: "2",
created: Object.fromEntries(Object.keys((args.create ?? {}) as object).map((k) => [k, { id: `new-${k}` }])),
notCreated: {},
updated: Object.fromEntries(Object.keys((args.update ?? {}) as object).map((k) => [k, null])),
notCreated: {}, notUpdated: {},
}, id];
}
return [name, { accountId: "a1", state: "1", list: [], notFound: [], ids: [], total: 0, queryState: "q", position: 0, canCalculateChanges: false }, id];
@@ -71,36 +81,66 @@ afterEach(() => {
});
describe("re-importing vCards the book already has", () => {
it("skips a card whose uid is already in this book", async () => {
it("updates a card whose uid is already in this book, and creates the rest", async () => {
/*
* It used to skip. The reporter asked for the opposite on #174 and he is
* right: the reason to import a file twice is usually that the first one
* was wrong, and skipping means a corrected export corrects nothing.
*/
const sets = server({ parsed: [card("ada@x", "Ada"), card("alan@x", "Alan")], existing: [here("ada@x")] });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 1, alike: 0 });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, updated: 1, alike: 0 });
expect(Object.values(sets[0]!.create!).map((c) => (c.name as { full: string }).full)).toEqual(["Alan"]);
// Addressed by the id already here, not by a client-side key.
expect(Object.keys(sets[0]!.update!)).toEqual(["srv-ada@x"]);
});
it("does not move an updated card into the book it is being imported into", async () => {
// The card is already in this book; sending addressBookIds again would say
// nothing, and sending it on a card shared into another book would move it.
const sets = server({ parsed: [card("ada@x", "Ada")], existing: [here("ada@x")] });
await useContacts.getState().importVCard("BEGIN:VCARD", "book1");
expect(Object.values(sets[0]!.update!)[0]).not.toHaveProperty("addressBookIds");
});
it("leaves properties the file does not mention alone", async () => {
/*
* A merge rather than a replacement: a phone number added in ihasmail after
* the first import survives a re-import of the original file. The cost is
* that a field deleted at the source stays here, which is the better way to
* be wrong.
*/
const sets = server({ parsed: [card("ada@x", "Ada")], existing: [here("ada@x")] });
await useContacts.getState().importVCard("BEGIN:VCARD", "book1");
const patch = Object.values(sets[0]!.update!)[0]!;
expect(patch).not.toHaveProperty("phones");
expect(patch.name).toEqual({ full: "Ada" });
});
it("imports a card whose uid is in a different book", async () => {
// The same person legitimately filed in two address books is not a
// duplicate, any more than the same event in two calendars is.
const sets = server({ parsed: [card("ada@x", "Ada")], existing: [here("ada@x", "book2")] });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0, alike: 0 });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, updated: 0, alike: 0 });
expect(Object.keys(sets[0]!.create!)).toHaveLength(1);
});
it("imports a card that arrived with no uid, rather than guessing at one", async () => {
const noUid = { "@type": "Card", version: "1.0", kind: "individual", name: { full: "Anon" } };
const sets = server({ parsed: [noUid], existing: [here("ada@x")] });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0, alike: 0 });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, updated: 0, alike: 0 });
expect(Object.values(sets[0]!.create!)[0]!.uid).toEqual(expect.any(String));
});
it("sends nothing at all when the whole file is already here", async () => {
it("updates the lot when the whole file is already here, creating none", async () => {
const sets = server({ parsed: [card("ada@x", "Ada"), card("alan@x", "Alan")], existing: [here("ada@x"), here("alan@x")] });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 0, skipped: 2, alike: 0 });
expect(sets).toHaveLength(0);
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 0, updated: 2, alike: 0 });
expect(Object.keys(sets[0]!.create ?? {})).toHaveLength(0);
expect(Object.keys(sets[0]!.update!)).toHaveLength(2);
});
it("imports everything into an empty book", async () => {
const sets = server({ parsed: [card("ada@x", "Ada"), card("alan@x", "Alan")] });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 2, skipped: 0, alike: 0 });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 2, updated: 0, alike: 0 });
expect(Object.keys(sets[0]!.create!)).toHaveLength(2);
});
});
@@ -116,6 +156,32 @@ describe("LDIF, which has nothing to match on", () => {
* name and an address instead is the open question on #223.
*/
server({ existing: [here("anything")] });
await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 2, skipped: 0, alike: 0 });
await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 2, updated: 0, alike: 0 });
});
});
/*
* Creates and updates share the ceiling.
*
* Now that a re-import updates rather than skips, one file can carry both.
* Stalwart counts every object in a `/set` against `maxObjectsInSet` together,
* so batching the halves separately would send 300 new and 300 changed as two
* calls of 300 and be refused for a limit of 500 that neither half exceeds.
*/
describe("a file that both creates and updates", () => {
it("counts them against one budget, not one each", async () => {
const MAX = 500;
const existing = Array.from({ length: 300 }, (_, i) => here(`old-${i}@x`));
const parsed = [
...Array.from({ length: 300 }, (_, i) => card(`old-${i}@x`, `Old ${i}`)),
...Array.from({ length: 300 }, (_, i) => card(`new-${i}@x`, `New ${i}`)),
];
const sets = server({ max: MAX, parsed, existing });
await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 300, updated: 300, alike: 0 });
for (const s of sets) {
const n = Object.keys(s.create ?? {}).length + Object.keys(s.update ?? {}).length;
expect(n).toBeLessThanOrEqual(MAX);
}
expect(sets).toHaveLength(2);
});
});