Merge pull request #274 from Coffey-Labs/ldif-dedupe-on-dn

Match an LDIF re-import on the entry's dn
This commit is contained in:
Coffey Labs
2026-09-04 07:54:42 -07:00
committed by GitHub
9 changed files with 330 additions and 53 deletions
+8
View File
@@ -724,6 +724,14 @@ JMAP Contacts and JSContact.
title, nickname, web pages and the custom fields all come across. The import
control takes either format and decides by what is in the file, not by what it
is called.
- **Re-importing updates rather than duplicates.** A vCard is recognised by its
UID; an LDIF entry, whose schema has none, by its distinguished name. The card
already here is merged with the file's version -- what the file carries wins,
what it does not mention is left alone -- so a corrected export can correct
what the first attempt got wrong. Matching is per address book, which is also
how two directories that each hold a `cn=John Smith` stay two people. An entry
no longer recognisable, because its `dn` moved between exports, is imported
again and counted: *"3 of them look like contacts you already had."*
[ldif-schema]: https://wiki.mozilla.org/MailNews:Mozilla_LDAP_Address_Book_Schema
- **Directory lookup** through `Principal/query`, so colleagues on the server
+43 -1
View File
@@ -1,5 +1,5 @@
import { describe, expect, it } from "vitest";
import { parseLdif } from "@/lib/ldif";
import { parseLdif, uidFromDn } from "@/lib/ldif";
/** The example from issue #174, as SOGo exports it -- lowercased attribute names and all. */
const SOGO = `dn: cn=Jane Doe
@@ -103,3 +103,45 @@ describe("parseLdif", () => {
expect(r!.attrs.sn).toEqual(["Y"]);
});
});
describe("an identity for an entry, from its distinguished name", () => {
it("gives the same dn the same identity, which is the whole point", () => {
expect(uidFromDn("cn=Jane Doe,ou=People")).toBe(uidFromDn("cn=Jane Doe,ou=People"));
});
it("gives two entries two identities", () => {
expect(uidFromDn("cn=Jane Doe")).not.toBe(uidFromDn("cn=Alan Turing"));
});
it("ignores the case and spacing two exports of one directory differ in", () => {
// LDAP matches attribute types without regard to case, and exporters lay
// a dn out differently. Neither is a different person.
const canonical = uidFromDn("cn=Jane Doe,ou=People");
expect(uidFromDn("CN=Jane Doe,OU=People")).toBe(canonical);
expect(uidFromDn("cn = Jane Doe , ou = People")).toBe(canonical);
expect(uidFromDn(" cn=Jane Doe,ou=People ")).toBe(canonical);
});
it("does not run together words inside a value", () => {
expect(uidFromDn("cn=Jane Doe")).not.toBe(uidFromDn("cn=JaneDoe"));
});
it("says so plainly that it came from an LDIF entry", () => {
// It becomes the card's uid, where a vCard's own UID also lives. The
// namespace is what keeps one from being read as the other.
expect(uidFromDn("cn=Jane Doe")).toMatch(/^urn:x-ihasmail:ldif:/);
});
it("survives a dn a URI would otherwise choke on", () => {
const uid = uidFromDn("cn=Ünter Straße \\+ Söhne,ou=Übersicht")!;
expect(uid.startsWith("urn:x-ihasmail:ldif:")).toBe(true);
expect(uid).not.toMatch(/[\s?#]/);
});
it("has nothing to offer for an entry with no dn", () => {
// Such an entry gets an identity of its own instead, and duplicates on
// re-import as everything did before there was a dn to match on.
expect(uidFromDn("")).toBeNull();
expect(uidFromDn(" ")).toBeNull();
});
});
+33
View File
@@ -105,3 +105,36 @@ export function parseLdif(text: string): LdifRecord[] {
return !change || change === "add";
});
}
/**
* An identity for an entry, derived from its distinguished name.
*
* Mozilla's schema has no UID, so a re-import had nothing to be recognised by
* and duplicated everything (#223). The `dn` is what the file actually carries,
* and it does not need to be a durable identity to answer the only question
* being asked of it: have I imported this exact entry before? A migration is
* import, notice something wrong, correct the export, import again -- and the
* `dn` does not change in the ten minutes between two attempts, which is the
* interval that matters. An import is not a sync.
*
* Namespaced rather than stored raw, because it becomes the card's `uid` and
* must not be mistaken for a UID a vCard author meant. The one way this can be
* wrong: two directories that both contain `cn=John Smith`, imported into the
* *same* address book, are one contact afterwards. Matching is per book, so
* filing two directories in two books keeps them apart.
*
* Normalised for case and for the spacing exporters differ in, which costs
* nothing when a file is compared against itself and helps when it is compared
* against a differently-produced export of the same directory.
*
* Null for an entry with no usable `dn`: that entry gets an identity of its own
* and duplicates on re-import, as everything did before.
*/
export function uidFromDn(dn: string): string | null {
const normalised = dn
.trim()
.toLowerCase()
.replace(/\s+/g, " ")
.replace(/\s*([,=])\s*/g, "$1");
return normalised ? `urn:x-ihasmail:ldif:${encodeURIComponent(normalised)}` : null;
}
+154
View File
@@ -0,0 +1,154 @@
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { CAP, client } from "@/jmap/client";
import { useContacts } from "@/store/contacts";
import type { ContactCard, JmapSession } from "@/jmap/types";
/*
* Re-importing an LDIF address book you already have.
*
* Mozilla's schema has no UID, so for a long time an LDIF re-import duplicated
* everything -- reported on #174, tracked on #223, and reported again once the
* vCard half shipped without it. The entry's `dn` is the identity the file
* actually carries: not durable enough to be anyone's identity forever, and
* unchanged across the ten minutes between importing a migration, spotting a
* mistake, correcting the export and importing again, which is the only
* interval an import has to survive.
*
* Updated rather than skipped, because the reason to import a file twice is
* that the first attempt was not right.
*/
interface SetArgs { create?: Record<string, Record<string, unknown>>; update?: Record<string, Record<string, unknown>> }
/** A card as the server holds it: what `scanBook` asks for, and nothing else. */
const here = (id: string, uid: string, full: string, bookId = "book1") => ({
id, uid, addressBookIds: { [bookId]: true }, name: { full }, emails: {},
}) as unknown as Partial<ContactCard> & { id: string };
/** What `uidFromDn` makes of a `dn`, spelled out rather than imported, so a
change to the scheme has to be a deliberate one. */
const uidFor = (dn: string) => `urn:x-ihasmail:ldif:${encodeURIComponent(dn)}`;
function server(existing: Array<Partial<ContactCard> & { id: string }> = []) {
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/query") {
const position = (args.position as number) ?? 0;
return [name, { accountId: "a1", queryState: "1", canCalculateChanges: false, position, ids: position ? [] : existing.map((c) => c.id), total: existing.length }, id];
}
if (name === "ContactCard/get") {
const want = new Set((args.ids as string[]) ?? []);
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>>,
update: args.update as Record<string, Record<string, unknown>>,
});
return [name, {
accountId: "a1", oldState: "1", newState: "2",
created: Object.fromEntries(Object.keys((args.create ?? {}) as object).map((k) => [k, { id: `new-${k}` }])),
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];
});
return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "1" }) } as Response;
});
vi.stubGlobal("fetch", fetchMock);
return sets;
}
/** The same card, carrying the address likeness is read from. */
const withEmail = (c: Partial<ContactCard> & { id: string }, address = "[email protected]") =>
({ ...c, emails: { e0: { address } } }) as unknown as Partial<ContactCard> & { id: string };
const JANE = "dn: cn=Jane Doe,ou=People\ngivenName: Jane\nsn: Doe\ncn: Jane Doe\nmail: [email protected]\n";
beforeEach(() => {
client.session = {
capabilities: { [CAP.core]: { maxObjectsInGet: 500, maxObjectsInSet: 500 }, [CAP.contacts]: {} },
accounts: {}, primaryAccounts: {}, state: "s1",
} as unknown as JmapSession;
useContacts.setState({ accountId: "a1", available: true, books: {}, cards: {} as Record<string, ContactCard> });
});
afterEach(() => {
vi.unstubAllGlobals();
vi.restoreAllMocks();
});
describe("re-importing an LDIF address book", () => {
it("updates the card an entry's dn already names, rather than adding a second", async () => {
const sets = server([here("c1", uidFor("cn=jane doe,ou=people"), "Jane Doe")]);
const r = await useContacts.getState().importLdif(JANE, "book1");
expect(r).toEqual({ created: 0, updated: 1, alike: 0 });
expect(sets[0]!.create).toEqual({});
expect(Object.keys(sets[0]!.update!)).toEqual(["c1"]);
});
it("carries the file's version of the entry into the existing card", async () => {
const sets = server([here("c1", uidFor("cn=jane doe,ou=people"), "Jane Doe")]);
await useContacts.getState().importLdif(JANE.replace("sn: Doe", "sn: Doe-Smith").replace("cn: Jane Doe", "cn: Jane Doe-Smith"), "book1");
expect(sets[0]!.update!.c1!.name).toMatchObject({ full: "Jane Doe-Smith" });
});
it("does not move the card into the book being imported into", async () => {
// A contact filed in two books stays filed in both. Naming the target book
// in an update would quietly refile it.
const sets = server([here("c1", uidFor("cn=jane doe,ou=people"), "Jane Doe")]);
await useContacts.getState().importLdif(JANE, "book1");
expect(sets[0]!.update!.c1!).not.toHaveProperty("addressBookIds");
});
it("creates an entry whose dn is not here yet", async () => {
const sets = server([here("c1", uidFor("cn=someone else,ou=people"), "Someone Else")]);
const r = await useContacts.getState().importLdif(JANE, "book1");
expect(r).toEqual({ created: 1, updated: 0, alike: 0 });
expect(Object.keys(sets[0]!.create!)).toHaveLength(1);
});
it("ignores the case and spacing two exports of one directory differ in", async () => {
const sets = server([here("c1", uidFor("cn=jane doe,ou=people"), "Jane Doe")]);
const spaced = JANE.replace("dn: cn=Jane Doe,ou=People", "dn: CN = Jane Doe , OU = People");
await useContacts.getState().importLdif(spaced, "book1");
expect(Object.keys(sets[0]!.update!)).toEqual(["c1"]);
});
it("matches only inside the book being imported into", async () => {
// Two customers' directories can each hold a cn=John Smith. Filed in two
// books they stay two people; this is the escape hatch for that.
const sets = server([here("c1", uidFor("cn=jane doe,ou=people"), "Jane Doe", "book2")]);
const r = await useContacts.getState().importLdif(JANE, "book1");
expect(r.created).toBe(1);
expect(sets[0]!.update).toEqual({});
});
it("still counts a look-alike whose dn moved, and imports it anyway", async () => {
// The same person under a different branch of the directory: nothing to
// match on, so it arrives as a new card. Reported, never merged -- name
// plus address is a guess, and a merge made on a guess cannot be undone.
server([withEmail(here("c1", uidFor("cn=jane doe,ou=staff"), "Jane Doe"))]);
const r = await useContacts.getState().importLdif(JANE, "book1");
expect(r).toEqual({ created: 1, updated: 0, alike: 1 });
});
it("does not also count an entry it matched as a look-alike", async () => {
// It is not a card that looks like this one; it is this one.
server([withEmail(here("c1", uidFor("cn=jane doe,ou=people"), "Jane Doe"))]);
const r = await useContacts.getState().importLdif(JANE, "book1");
expect(r).toEqual({ created: 0, updated: 1, alike: 0 });
});
it("reports created and updated apart when a file holds both", async () => {
const sets = server([here("c1", uidFor("cn=jane doe,ou=people"), "Jane Doe")]);
const both = `${JANE}\ndn: cn=Alan Turing,ou=People\ncn: Alan Turing\nsn: Turing\nmail: [email protected]\n`;
const r = await useContacts.getState().importLdif(both, "book1");
expect(r).toEqual({ created: 1, updated: 1, alike: 0 });
// One call, not one per kind: creates and updates share Stalwart's budget.
expect(sets).toHaveLength(1);
});
});
+12 -5
View File
@@ -86,15 +86,22 @@ describe("importing an LDIF address book", () => {
expect(first.name).toMatchObject({ full: "Jane Doe" });
});
it("gives each contact an identity of its own, not the entry's directory name", async () => {
it("gives each contact an identity derived from its entry, and a distinct one", async () => {
const sets = server();
await useContacts.getState().importLdif(TWO, "book1");
const uids = Object.values(sets[0]!.create!).map((c) => c.uid as string);
expect(uids.every((u) => typeof u === "string" && u.length > 0)).toBe(true);
expect(new Set(uids).size).toBe(2);
// A distinguished name says where an entry sat in somebody else's
// directory, and must not become the contact's identity here.
expect(uids.some((u) => u.includes("cn="))).toBe(false);
// Namespaced, so it is never mistaken for a UID a vCard author meant, and
// stable, so importing the same file again recognises these.
expect(uids.every((u) => u.startsWith("urn:x-ihasmail:ldif:"))).toBe(true);
});
it("gives an entry with no usable dn an identity of its own", async () => {
const sets = server();
await useContacts.getState().importLdif("dn:\ncn: Nameless Place\nmail: [email protected]\n", "book1");
const uid = Object.values(sets[0]!.create!)[0]!.uid as string;
expect(uid).not.toContain("urn:x-ihasmail:ldif:");
expect(uid.length).toBeGreaterThan(0);
});
it("says a file held no contacts rather than reporting none imported", async () => {
+15 -3
View File
@@ -128,11 +128,23 @@ describe("telling somebody what an LDIF re-import duplicated", () => {
});
it("does not count the file against itself", async () => {
// Two of the same person in one file are two new cards, not a duplicate of
// Two people in one file are two new cards, neither a duplicate of
// something that was already here. The scan is read before anything lands.
server([]);
const twice = entry("Jane Doe", "[email protected]") + "\n" + entry("Jane Doe", "jane@example.com");
const r = await useContacts.getState().importLdif(twice, "book1");
const two = entry("Jane Doe", "[email protected]") + "\n" + entry("Alan Turing", "alan@example.org");
const r = await useContacts.getState().importLdif(two, "book1");
expect(r).toEqual({ created: 2, updated: 0, alike: 0 });
});
it("makes one card of two entries in a file that share a dn", async () => {
// A directory cannot hold two entries under one name, so a file that does
// is malformed -- and must not produce two cards sharing an identity,
// which is the duplication this all exists to prevent. The later wins.
const sets = 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: 1, updated: 0, alike: 0 });
const only = Object.values(sets[0]!.create!)[0]!;
expect(Object.values(only.emails as Record<string, { address: string }>)[0]!.address).toBe("[email protected]");
});
});
+2 -2
View File
@@ -9,8 +9,8 @@ import type { ContactCard, JmapSession, UploadResponse } from "@/jmap/types";
* A vCard UID is an identity its author meant, so a card whose UID a book
* already holds is that card and importing it again used to leave a second
* copy. Reported on #174 by the reporter's colleague, decided on #173 for
* events, tracked as #223. The LDIF half is deliberately absent -- Mozilla's
* schema has no UID, so the import invents one and there is nothing to match.
* events, tracked as #223. The LDIF half matches on the entry's `dn` instead,
* since Mozilla's schema has no UID; it is tested in `ldif-dedupe.test.ts`.
*/
const MAX = 500;
+59 -39
View File
@@ -3,7 +3,7 @@ import { accountKey, loadRaw, saveJson } from "@/lib/storage";
import { CAP, chunk, client, setErrorMessage } from "@/jmap/client";
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 { parseLdif, uidFromDn } from "@/lib/ldif";
import { cardFromLdif } from "@/lib/mozillaAb";
import { useSettings } from "./settings";
import { useSession } from "./session";
@@ -73,14 +73,14 @@ async function scanBook(accountId: Id, addressBookId: Id): Promise<{ byUid: Map<
* wrong in both directions: two colleagues who share a name and a shared alias
* collapse into one, and somebody whose address changed since the last export
* looks like a stranger. Either mistake is silent and one of them is
* unrecoverable, which is why #223 leaves the decision open.
* unrecoverable, so it counts and never acts.
*
* Counting is a different act from acting. An LDIF re-import duplicates
* everything -- Mozilla's schema has no UID, so the import invents one and
* nothing can match -- and the reported harm was confusion rather than data
* loss: somebody imports a file twice and cannot tell what happened. Being told
* "40 of these look like contacts you already had" answers that without
* touching a single card.
* What is left for it to count, now that an LDIF re-import matches on the
* entry's `dn`, is the entries that matching could not catch: one whose `dn`
* moved between exports, and anything imported before there was a `dn` to match
* on. Those arrive as new cards, and saying "40 of these look like contacts you
* already had" is the honest half of the answer -- the reported harm was
* confusion rather than duplication, and being told costs nothing.
*
* One key per address, so a person whose second address matches is still
* recognised.
@@ -211,10 +211,12 @@ interface ContactsState {
/**
* Import an address book in LDIF, read against Mozilla's schema.
*
* `updated` is always 0: Mozilla's schema has no UID, so there is nothing to
* recognise a re-import by and everything arrives as new. `alike` says how
* many look like cards already here without acting on it. Answered in the
* same shape as the vCard import so the caller need not know which it called.
* Mozilla's schema has no UID, so a re-import is recognised by the entry's
* `dn` instead -- the same update-rather-than-duplicate rule the vCard import
* follows, on the only identity the file carries. `alike` is what is left
* over: entries that were created and still look like somebody already here,
* which is what a changed `dn` produces. Answered in the same shape as the
* vCard import so the caller need not know which it called.
*/
importLdif(text: string, addressBookId: Id): Promise<{ created: number; updated: number; alike: number }>;
loadPrincipals(): Promise<void>;
@@ -568,43 +570,61 @@ export const useContacts = create<ContactsState>((set, get) => ({
*/
async importLdif(text, addressBookId) {
const accountId = get().accountId!;
const cards = parseLdif(text).map(cardFromLdif).filter((c): c is Partial<ContactCard> => c !== null);
if (!cards.length) throw new Error("it has no contacts in it");
/* The record and not just the card: the `dn` is the entry's identity and
`cardFromLdif` deliberately does not carry it into the card. */
const entries = parseLdif(text)
.map((rec) => ({ uid: uidFromDn(rec.dn), card: cardFromLdif(rec) }))
.filter((e): e is { uid: string | null; card: Partial<ContactCard> } => e.card !== null);
if (!entries.length) throw new Error("it has no contacts in it");
/*
* Read before anything is created, so "already had" means before this
* import rather than including it. Every card here is imported either way;
* this only counts.
* Read before anything is written, so "already had" means before this
* import rather than including it.
*/
const before = await scanBook(accountId, addressBookId);
let alike = 0;
const create: Record<string, unknown> = {};
cards.forEach((c, i) => {
if (likenessKeys(c).some((k) => before.likeness.has(k))) alike++;
// Built here rather than read from the file: LDIF identifies an entry by
// its distinguished name, which says where it sat in somebody's
// 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 update: Record<Id, unknown> = {};
/* Where in `create` an entry from this same file already landed. A
directory cannot hold two entries under one `dn`, so a file that does is
malformed -- but it must not become two cards sharing a uid, which is a
duplicate of exactly the kind being fixed here. The later one wins, as it
would in the directory. */
const pending = new Map<string, string>();
entries.forEach(({ uid, card }, i) => {
/*
* An entry whose `dn` this book already holds is that entry, and the
* newer version of it wins -- a merge, as the vCard import does it:
* properties the file carries overwrite what is here, properties it does
* not mention are left alone. The reason to import a file twice is
* usually that the first attempt was not right, so skipping would mean a
* corrected export corrects nothing (#174).
*/
const existing = uid ? before.byUid.get(uid) : undefined;
if (existing) {
update[existing] = card;
return;
}
const seen = uid ? pending.get(uid) : undefined;
const key = seen ?? `c${i}`;
if (uid) pending.set(uid, key);
/*
* Only what is actually being created can look like a duplicate: what
* matched above is not a look-alike but the same entry. So this counts
* what `dn` matching could not catch -- an entry whose `dn` moved, or one
* imported before there was anything to match on -- and still only
* counts, because name-plus-email is a guess wrong in both directions and
* a merge made on a guess cannot be undone.
*/
if (!seen && likenessKeys(card).some((k) => before.likeness.has(k))) alike++;
create[key] = { "@type": "Card", version: "1.0", ...card, uid: uid ?? crypto.randomUUID(), addressBookIds: { [addressBookId]: true } };
});
let created: number;
try {
const r = await writeCards(accountId, create);
created = r.created;
if (!created) throw new Error(r.refused ? setErrorMessage(r.refused) : "the server did not accept any of its contacts");
const { created, updated, refused } = await writeCards(accountId, create, update);
if (!created && !updated) throw new Error(refused ? setErrorMessage(refused) : "the server did not accept any of its contacts");
return { created, updated, alike };
} finally {
await get().loadAll();
}
/*
* Nothing skipped, and nothing that could be. The UID above is invented
* here because Mozilla's schema does not define one, so a re-import has no
* identity to be recognised by -- see #223, where whether to guess at one
* from a name and an address is still an open question.
*
* `alike` is what can be said without answering it: how many of these look
* like contacts that were already here. Reporting is not matching -- every
* card was imported -- and it is the confusion rather than the duplication
* that was reported as the harm.
*/
return { created, updated: 0, alike };
},
async loadPrincipals() {
+4 -3
View File
@@ -153,9 +153,10 @@ export function ContactsView({ id }: { id?: string }) {
else toast.success(imported);
/*
* Said separately, and after, because it is a different kind of fact.
* LDIF has no UID to match on, so nothing was updated and nothing was
* merged -- these are simply here twice now, and saying so is the whole
* of what can honestly be said without guessing (#223).
* These were not matched and are here twice now -- an LDIF entry whose
* `dn` moved between exports, or one imported before there was a `dn` to
* match on. Name-plus-email is enough to notice that and not enough to
* merge on, so it is reported and left alone (#223).
*/
if (alike) {
toast.show(plural(alike, {