From 5c484af1f78f3262274f74fe2d87922d5b0ec28a Mon Sep 17 00:00:00 2001 From: KazNIISA IT Date: Tue, 18 Aug 2026 00:45:05 +0500 Subject: [PATCH] fix(jmap): escape the JSON Pointer in keyword patches, and migrate a keyword honestly A PatchObject key is a JSON Pointer (RFC 8620 section 5.3), so a slash inside the member name it points at has to be escaped as `~1`. Keyword patches were built as `keywords/${keyword}` verbatim, which is fine for every flat keyword but wrong for a nested tag: `$label:work/clients` became the pointer `keywords/$label:work/clients`, asking for the `clients` member of the `$label:work` keyword rather than for the keyword itself. The patch is not a boolean at that path, so the server rejects or ignores it and the tag never lands. Route the keyword patch sites - setKeyword, removeKeyword, migrateKeyword and the category-tab batch patch - through `keywordPointer`, which escapes per RFC 6901 the way the calendar store already does for participant ids. Flat keywords are untouched, so `$seen`/`$flagged` and friends keep the exact pointers they had. The dev JMAP mock reads the escaping back, or nested tags would behave differently against the mock than against a real server. migrateKeyword also took any answer as success: a method-level error on the query read as "no message carries this keyword", one on the write was ignored outright, and a per-message refusal was invisible - so a rename that moved nothing still reported a clean migration. It now answers with { migrated, refused }, because the work is batched and a failure partway through cannot undo the batches already applied. A whole batch the server refuses stops the rest, which it would refuse too; a single message it refuses does not, the mail behind it being unaffected. A query error throws, nothing having been written yet, and so does a run that migrated nothing at all - the case where the caller must not rename a tag over messages that all kept the old keyword. The walk over tagged mail was not paginating safely either. It stopped at the first page shorter than the limit it asked for, but RFC 8620 section 5.5 lets a server clamp that limit, so a clamping server left most of the mail unmigrated. Positions are also only comparable within one queryState, which the walk ignored: mail arriving mid-walk shifts every later page. It now restarts when the state changes under it, and gives up - before any write, so the caller can simply repeat - once a mailbox has moved under it three times, rather than mixing ids from states that no longer agree. A page carrying no id it has not already seen ends the walk, which keeps it bounded against a server that answers every position with the same page. --- app/api/dev-jmap/[...path]/route.ts | 5 +- components/settings/keyword-settings.tsx | 8 +- lib/__tests__/jmap-keyword-pointer.test.ts | 358 +++++++++++++++++++++ lib/demo/demo-client.ts | 10 +- lib/jmap/client-interface.ts | 19 +- lib/jmap/client.ts | 105 +++++- lib/jmap/patch-pointer.ts | 29 ++ stores/message-list-tabs-store.ts | 5 +- 8 files changed, 516 insertions(+), 23 deletions(-) create mode 100644 lib/__tests__/jmap-keyword-pointer.test.ts create mode 100644 lib/jmap/patch-pointer.ts diff --git a/app/api/dev-jmap/[...path]/route.ts b/app/api/dev-jmap/[...path]/route.ts index d2d4ca958..e5cc03c2e 100644 --- a/app/api/dev-jmap/[...path]/route.ts +++ b/app/api/dev-jmap/[...path]/route.ts @@ -1,4 +1,5 @@ import { NextRequest, NextResponse } from 'next/server'; +import { pointerTokenValue } from '@/lib/jmap/patch-pointer'; /** * Mock JMAP server for local development. @@ -1725,7 +1726,9 @@ function handleEmailSet(args: MethodArgs, callId: string): MethodResult { // Patch-style keyword updates: "keywords/$seen", "keywords/$flagged", etc. for (const [key, value] of Object.entries(changes)) { if (key.startsWith('keywords/')) { - const keyword = key.slice('keywords/'.length); + // The key is a JSON Pointer, so a nested tag arrives escaped + // (`$label:work~1clients`) and has to be read back to its real name. + const keyword = pointerTokenValue(key.slice('keywords/'.length)); if (value) { email.keywords[keyword] = true; } else { diff --git a/components/settings/keyword-settings.tsx b/components/settings/keyword-settings.tsx index d55317caa..d106d22b8 100644 --- a/components/settings/keyword-settings.tsx +++ b/components/settings/keyword-settings.tsx @@ -574,9 +574,15 @@ export function KeywordSettings() { try { const oldJmapKeyword = `$label:${oldId}`; const newJmapKeyword = `$label:${keyword.id}`; - await client.migrateKeyword(oldJmapKeyword, newJmapKeyword); + const migration = await client.migrateKeyword(oldJmapKeyword, newJmapKeyword); renameKeyword(oldId, keyword); fetchTagCounts(client); + if (migration.refused > 0) { + // Some mail kept the old keyword. The definition still follows the + // messages that did move; the rest is what the warning is about. + const toastModule = await import('sonner'); + toastModule.toast.warning(t("migration_error")); + } } catch (error) { console.error("Failed to migrate keyword:", error); const toastModule = await import('sonner'); diff --git a/lib/__tests__/jmap-keyword-pointer.test.ts b/lib/__tests__/jmap-keyword-pointer.test.ts new file mode 100644 index 000000000..02b0b483f --- /dev/null +++ b/lib/__tests__/jmap-keyword-pointer.test.ts @@ -0,0 +1,358 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { JMAPClient } from '../jmap/client'; +import { keywordPointer, pointerToken, pointerTokenValue } from '../jmap/patch-pointer'; + +// A nested tag's id contains a slash (`work/clients`), so its keyword is +// `$label:work/clients`. PatchObject keys are JSON Pointers (RFC 8620 section +// 5.3), where an unescaped slash starts a new path segment - the server reads +// `keywords/$label:work/clients` as "the `clients` member of the `$label:work` +// keyword", which is not a boolean and is rejected or ignored. Nested tags can +// therefore only be written with the slash escaped as `~1`. + +function createClient(): JMAPClient { + const client = new JMAPClient('https://jmap.example.com', 'user@example.com', 'pass'); + Object.assign(client, { + apiUrl: 'https://jmap.example.com/api', + accountId: 'primary-account', + username: 'user@example.com', + }); + return client; +} + +interface JMAPMethodCall { + 0: string; + 1: Record; + 2: string; +} + +/** + * Answers Email/query with one id and then nothing - the walk ends on an empty + * page - and Email/set with a plain success. + */ +function mockJmap() { + const captured: JMAPMethodCall[] = []; + let queried = false; + const fetchSpy = vi.spyOn(globalThis, 'fetch'); + fetchSpy.mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + captured.push(...body.methodCalls); + const methodResponses = body.methodCalls.map((call) => { + if (call[0] !== 'Email/query') return ['Email/set', { updated: { m1: null } }, call[2]]; + const ids = queried ? [] : ['m1']; + queried = true; + return ['Email/query', { ids }, call[2]]; + }); + return new Response(JSON.stringify({ methodResponses }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + return { captured, fetchSpy }; +} + +function patchOf(captured: JMAPMethodCall[], emailId = 'm1'): Record { + const set = captured.find((call) => call[0] === 'Email/set'); + const update = set?.[1].update as Record> | undefined; + return update?.[emailId] ?? {}; +} + +describe('pointerToken', () => { + it('escapes a slash as ~1', () => { + expect(pointerToken('work/clients')).toBe('work~1clients'); + }); + + it('escapes a tilde as ~0, and does so before slashes', () => { + expect(pointerToken('a~/b')).toBe('a~0~1b'); + expect(pointerToken('a~1b')).toBe('a~01b'); + }); + + it('leaves a token with nothing to escape alone', () => { + expect(keywordPointer('$label:red')).toBe('keywords/$label:red'); + }); +}); + +describe('pointerTokenValue', () => { + it('reads back what pointerToken wrote', () => { + for (const value of ['work/clients', 'a~/b', 'a~1b', '$label:red', '~0/~1']) { + expect(pointerTokenValue(pointerToken(value))).toBe(value); + } + }); + + it('reads ~1 before ~0, so an escaped tilde stays literal', () => { + expect(pointerTokenValue('~01')).toBe('~1'); + }); +}); + +describe('keyword writes escape the JSON Pointer', () => { + beforeEach(() => vi.restoreAllMocks()); + afterEach(() => vi.restoreAllMocks()); + + it('setKeyword escapes a nested tag keyword', async () => { + const { captured } = mockJmap(); + await createClient().setKeyword('m1', '$label:work/clients'); + expect(patchOf(captured)).toEqual({ 'keywords/$label:work~1clients': true }); + }); + + it('removeKeyword escapes a nested tag keyword', async () => { + const { captured } = mockJmap(); + await createClient().removeKeyword('m1', '$label:work/clients'); + expect(patchOf(captured)).toEqual({ 'keywords/$label:work~1clients': null }); + }); + + it('migrateKeyword escapes both the old and the new keyword', async () => { + const { captured } = mockJmap(); + await createClient().migrateKeyword('$label:work/clients', '$label:work/acme'); + expect(patchOf(captured)).toEqual({ + 'keywords/$label:work~1clients': null, + 'keywords/$label:work~1acme': true, + }); + }); + + it('leaves a flat keyword unchanged', async () => { + const { captured } = mockJmap(); + await createClient().setKeyword('m1', '$label:red'); + expect(patchOf(captured)).toEqual({ 'keywords/$label:red': true }); + }); +}); + +describe('migrateKeyword reports a server that refused', () => { + beforeEach(() => vi.restoreAllMocks()); + afterEach(() => vi.restoreAllMocks()); + + /** + * Answers queries from `pages` in order (an empty page ends the walk) and + * every Email/set with `setResult`. A page given as an error tuple is + * returned as the method response itself. + */ + function mockRun( + pages: (string[] | ['error', Record])[], + setResult: Record | ['error', Record] = { updated: {} }, + ) { + let page = 0; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + const methodResponses = body.methodCalls.map((call) => { + if (call[0] === 'Email/query') { + const next = pages[Math.min(page, pages.length - 1)]; + page++; + if (Array.isArray(next) && next[0] === 'error') { + return ['error', next[1] as Record, call[2]]; + } + return ['Email/query', { ids: next as string[] }, call[2]]; + } + if (Array.isArray(setResult) && setResult[0] === 'error') { + return ['error', setResult[1] as Record, call[2]]; + } + return ['Email/set', setResult as Record, call[2]]; + }); + return new Response(JSON.stringify({ methodResponses }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + } + + it('throws when the query for tagged mail fails instead of reading it as none', async () => { + mockRun([['error', { type: 'serverFail', description: 'boom' }]]); + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).rejects.toThrow('boom'); + }); + + it('throws when the update fails outright', async () => { + mockRun([['m1'], []], ['error', { type: 'serverFail', description: 'no writes today' }]); + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).rejects.toThrow( + 'no writes today', + ); + }); + + it('counts only the messages the server actually updated', async () => { + mockRun([['m1', 'm2'], []], { updated: { m1: null }, notUpdated: { m2: { type: 'notFound' } } }); + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).resolves.toEqual({ + migrated: 1, + refused: 0, + }); + }); + + it('reports a message refused for a reason other than being gone', async () => { + mockRun([['m1', 'm2'], []], { updated: { m1: null }, notUpdated: { m2: { type: 'forbidden' } } }); + // Throwing would abandon m1, which this same call already migrated. + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).resolves.toEqual({ + migrated: 1, + refused: 1, + }); + }); + + it('keeps what it migrated when a later batch fails outright', async () => { + // One message per Email/set, so the first write lands before the second + // fails: throwing would strand m1 under the new keyword with the tag + // definition still on the old one. + const client = createClient(); + Object.assign(client, { capabilities: { 'urn:ietf:params:jmap:core': { maxObjectsInSet: 1 } } }); + let set = 0; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + const methodResponses = body.methodCalls.map((call) => { + if (call[0] === 'Email/query') { + return ['Email/query', { ids: set === 0 ? ['m1', 'm2'] : [], queryState: 's1' }, call[2]]; + } + set++; + return set === 1 + ? ['Email/set', { updated: { m1: null } }, call[2]] + : ['error', { type: 'serverFail', description: 'gave up' }, call[2]]; + }); + return new Response(JSON.stringify({ methodResponses }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + + await expect(client.migrateKeyword('$label:red', '$label:iso')).resolves.toEqual({ + migrated: 1, + refused: 1, + }); + }); + + it('throws when nothing at all could be migrated', async () => { + mockRun([['m1'], []], { notUpdated: { m1: { type: 'forbidden' } } }); + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).rejects.toThrow( + 'forbidden', + ); + }); + + it('restarts the walk when the query state changes under it', async () => { + // Positions only line up within one queryState. The first page belongs to a + // state that is gone by the next request; a walk that kept its position (and + // the ids it had collected) would migrate mail the tag is no longer on and + // miss the mail it is. + const current = ['m1', 'm2']; + let queries = 0; + const touched: string[] = []; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + const methodResponses = body.methodCalls.map((call) => { + if (call[0] === 'Email/query') { + queries++; + if (queries === 1) { + return ['Email/query', { ids: ['stale1', 'stale2'], queryState: 's1' }, call[2]]; + } + const position = call[1].position as number; + const limit = call[1].limit as number; + return ['Email/query', { ids: current.slice(position, position + limit), queryState: 's2' }, call[2]]; + } + const update = call[1].update as Record; + touched.push(...Object.keys(update)); + return ['Email/set', { updated: Object.fromEntries(Object.keys(update).map((id) => [id, null])) }, call[2]]; + }); + return new Response(JSON.stringify({ methodResponses }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).resolves.toEqual({ + migrated: 2, + refused: 0, + }); + expect(touched).toEqual(['m1', 'm2']); + }); + + it('keeps migrating later batches after one message is refused', async () => { + // A refusal is per message: the mail behind it in the queue is unaffected, + // and skipping it would leave far more behind than the server refused. + const client = createClient(); + Object.assign(client, { capabilities: { 'urn:ietf:params:jmap:core': { maxObjectsInSet: 1 } } }); + const touched: string[] = []; + let queried = false; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + const methodResponses = body.methodCalls.map((call) => { + if (call[0] === 'Email/query') { + const ids = queried ? [] : ['m1', 'm2']; + queried = true; + return ['Email/query', { ids, queryState: 's1' }, call[2]]; + } + const id = Object.keys(call[1].update as Record)[0]; + touched.push(id); + return id === 'm1' + ? ['Email/set', { notUpdated: { m1: { type: 'forbidden' } } }, call[2]] + : ['Email/set', { updated: { [id]: null } }, call[2]]; + }); + return new Response(JSON.stringify({ methodResponses }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + + await expect(client.migrateKeyword('$label:red', '$label:iso')).resolves.toEqual({ + migrated: 1, + refused: 1, + }); + expect(touched).toEqual(['m1', 'm2']); + }); + + it('gives up before writing anything when the mailbox will not hold still', async () => { + // Each query reports a new state, so no page ever belongs with the last. + // Mixing them would retag mail the tag has since been taken off. + let queries = 0; + const sets: string[] = []; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + const methodResponses = body.methodCalls.map((call) => { + if (call[0] === 'Email/query') { + queries++; + return ['Email/query', { ids: [`m${queries}`], queryState: `s${queries}` }, call[2]]; + } + sets.push(call[2]); + return ['Email/set', { updated: {} }, call[2]]; + }); + return new Response(JSON.stringify({ methodResponses }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).rejects.toThrow( + /kept changing/, + ); + expect(sets).toEqual([]); + }); + + it('ends the walk when a page repeats ids instead of paginating', async () => { + // A server that answers every position with the same page would otherwise + // be paged forever. + mockRun([['m1']]); + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).resolves.toEqual({ + migrated: 1, + refused: 0, + }); + }); + + it('keeps paging when the server returns fewer ids than the limit asked for', async () => { + // RFC 8620 section 5.5 lets a server clamp the limit. Treating a short page + // as the last one would silently migrate only the first page. + const pages = [['m1', 'm2'], ['m3'], []]; + let query = 0; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + const methodResponses = body.methodCalls.map((call) => { + if (call[0] === 'Email/query') { + const ids = pages[Math.min(query, pages.length - 1)]; + query++; + return ['Email/query', { ids }, call[2]]; + } + const update = call[1].update as Record; + const updated = Object.fromEntries(Object.keys(update).map((id) => [id, null])); + return ['Email/set', { updated }, call[2]]; + }); + return new Response(JSON.stringify({ methodResponses }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + + await expect(createClient().migrateKeyword('$label:red', '$label:iso')).resolves.toEqual({ + migrated: 3, + refused: 0, + }); + expect(query).toBe(3); + }); +}); diff --git a/lib/demo/demo-client.ts b/lib/demo/demo-client.ts index 25acd36ac..d9133b5f4 100644 --- a/lib/demo/demo-client.ts +++ b/lib/demo/demo-client.ts @@ -1,4 +1,4 @@ -import type { IJMAPClient, KeywordDiscoveryResult } from '@/lib/jmap/client-interface'; +import type { IJMAPClient, KeywordDiscoveryResult, KeywordMigration } from '@/lib/jmap/client-interface'; import type { Email, Mailbox, StateChange, AccountStates, Thread, Identity, EmailAddress, ContactCard, AddressBook, VacationResponse, Calendar, CalendarEvent, CalendarEventFilter, CalendarTask, FileNode, ScheduledEmail, SendEmailResult, SharedAccount } from '@/lib/jmap/types'; import type { SieveScript, SieveCapabilities } from '@/lib/jmap/sieve-types'; import { getDemoData, type DemoData } from './demo-data'; @@ -382,16 +382,16 @@ export class DemoJMAPClient implements IJMAPClient { } } - async migrateKeyword(oldKeyword: string, newKeyword: string): Promise { - let count = 0; + async migrateKeyword(oldKeyword: string, newKeyword: string): Promise { + let migrated = 0; for (const email of this.data.emails) { if (email.keywords[oldKeyword]) { delete email.keywords[oldKeyword]; email.keywords[newKeyword] = true; - count++; + migrated++; } } - return count; + return { migrated, refused: 0 }; } async deleteEmail(emailId: string): Promise { diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index d38348f28..961028843 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -1,6 +1,17 @@ import type { Email, Mailbox, StateChange, AccountStates, Thread, Identity, EmailAddress, ContactCard, AddressBook, AddressBookRights, VacationResponse, Calendar, CalendarRights, CalendarEvent, CalendarEventFilter, CalendarTask, FileNode, FileNodeRights, Principal, PushSubscription, ScheduledEmail, SendEmailResult, SharedAccount } from "./types"; import type { SieveScript, SieveCapabilities } from "./sieve-types"; +/** What `migrateKeyword` managed to do. */ +export interface KeywordMigration { + /** Messages now carrying the new keyword. */ + migrated: number; + /** + * Messages the server refused to update for a reason other than being gone. + * Those still carry the old keyword. + */ + refused: number; +} + export interface KeywordInfo { id: string; name: string; @@ -153,7 +164,13 @@ export interface IJMAPClient { removeKeyword(emailId: string, keyword: string, accountId?: string): Promise; /** Apply one `keywords/` patch fragment (true=add, null=remove) to many messages in a single Email/set. */ batchUpdateKeywords(emailIds: string[], patch: Record, accountId?: string): Promise; - migrateKeyword(oldKeyword: string, newKeyword: string): Promise; + /** + * Rewrite one keyword to another on every message that carries it, in + * batches. Fails outright only when the server refuses the whole call; + * per-message refusals are reported in the result, since the messages + * migrated alongside them stay migrated. + */ + migrateKeyword(oldKeyword: string, newKeyword: string): Promise; deleteEmail(emailId: string, accountId?: string): Promise; moveToTrash(emailId: string, trashMailboxId: string, accountId?: string, markAsRead?: boolean): Promise; batchDeleteEmails(emailIds: string[], accountId?: string): Promise; diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index da88a2751..8d1cb4274 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -1,8 +1,9 @@ import type { Email, Mailbox, StateChange, AccountStates, Thread, Identity, EmailAddress, ContactCard, AddressBook, AddressBookRights, VacationResponse, Calendar, CalendarRights, CalendarEvent, CalendarEventFilter, CalendarTask, FileNode, FileNodeFilter, FileNodeRights, Principal, PushSubscription, EmailSubmission, ScheduledEmail, SendEmailResult, SharedAccount } from "./types"; import type { SieveScript, SieveCapabilities } from "./sieve-types"; -import type { IJMAPClient, KeywordDiscoveryResult, KeywordInfo } from "./client-interface"; +import type { IJMAPClient, KeywordDiscoveryResult, KeywordInfo, KeywordMigration } from "./client-interface"; import { toWildcardQuery } from "./search-utils"; import { batched, itemsPerRequest } from "./request-limits"; +import { keywordPointer } from "./patch-pointer"; import { debug } from "@/lib/debug"; import { normalizeCalendarEventLike } from "@/lib/calendar-event-normalization"; import { findTasksOnlyCalendarIds, isTaskLikeObject, type ScannedCalendarObject } from "@/lib/calendar-component-detection"; @@ -1756,7 +1757,7 @@ export class JMAPClient implements IJMAPClient { accountId: accountId || this.accountId, update: { [emailId]: { - [`keywords/${keyword}`]: true, + [keywordPointer(keyword)]: true, }, }, }, "0"], @@ -1769,7 +1770,7 @@ export class JMAPClient implements IJMAPClient { accountId: accountId || this.accountId, update: { [emailId]: { - [`keywords/${keyword}`]: null, + [keywordPointer(keyword)]: null, }, }, }, "0"], @@ -1791,11 +1792,18 @@ export class JMAPClient implements IJMAPClient { } } - async migrateKeyword(oldKeyword: string, newKeyword: string): Promise { + async migrateKeyword(oldKeyword: string, newKeyword: string): Promise { // Query all email IDs that have the old keyword + const seen = new Set(); const allIds: string[] = []; let position = 0; const batchSize = 100; + // Positions only line up within one queryState (RFC 8620 section 5.5). + // Mail arriving mid-walk shifts every later page, so a changed state + // restarts the walk - bounded, since a busy mailbox could change forever. + let queryState: string | undefined; + let restarts = 0; + const maxRestarts = 3; while (true) { const response = await this.request([ @@ -1807,35 +1815,106 @@ export class JMAPClient implements IJMAPClient { }, "0"], ]); - const queryResult = response.methodResponses?.[0]?.[1]; - const ids: string[] = queryResult?.ids || []; - allIds.push(...ids); + const [queryMethod, queryResult] = response.methodResponses?.[0] || []; + if (queryMethod === "error") { + // Reading a refusal as "no messages carry the tag" would report a + // clean migration of mail that still has the old keyword. + throw new Error(queryResult?.description || "Failed to list emails with the keyword"); + } + const state = queryResult?.queryState as string | undefined; + if (queryState === undefined) { + queryState = state; + } else if (state !== undefined && state !== queryState) { + if (restarts >= maxRestarts) { + // Carrying on would mix ids from states that no longer agree, and + // retagging a message the tag has since been taken off is worse than + // not retagging at all. Nothing has been written yet, so this is a + // clean failure the caller can simply repeat. + throw new Error("The mailbox kept changing while listing the messages to retag"); + } + restarts++; + queryState = state; + seen.clear(); + allIds.length = 0; + position = 0; + continue; + } - if (ids.length < batchSize) break; + const ids: string[] = queryResult?.ids || []; + const fresh = ids.filter(id => !seen.has(id)); + for (const id of fresh) seen.add(id); + allIds.push(...fresh); + + // A server may return fewer ids than the limit asked for (RFC 8620 + // section 5.5 lets it clamp), so a short page does not end the walk. + // A page carrying nothing new under an unchanged state does: the walk + // is not advancing, and continuing would loop for as long as the server + // keeps answering. Position counts the server's own page, not the ids + // this walk had not seen before. + if (fresh.length === 0) break; position += ids.length; } - if (allIds.length === 0) return 0; + if (allIds.length === 0) return { migrated: 0, refused: 0 }; // Batch update: remove old keyword, add new keyword using per-property patches + let migrated = 0; + let refusals = 0; + // A whole batch the server refused to take: it will not take the next one + // either. A single message it refused: the rest of the mail is unaffected, + // and only the reason is worth keeping. + let stopped: Error | null = null; + let refusal: Error | null = null; for (const batch of batched(allIds, this.getMaxObjectsInSet())) { + if (stopped) { + refusals += batch.length; + continue; + } + const update: Record> = {}; for (const id of batch) { update[id] = { - [`keywords/${oldKeyword}`]: null, - [`keywords/${newKeyword}`]: true, + [keywordPointer(oldKeyword)]: null, + [keywordPointer(newKeyword)]: true, }; } - await this.request([ + const response = await this.request([ ["Email/set", { accountId: this.accountId, update, }, "0"], ]); + + const [setMethod, setResult] = response.methodResponses?.[0] || []; + if (setMethod === "error") { + stopped = new Error(setResult?.description || "Failed to migrate keyword"); + refusals += batch.length; + continue; + } + // Email/set applies per message, so a refusal is never all-or-nothing: + // throwing here would abandon messages this very call already migrated. + // Report instead - a message deleted since the query (notFound) is + // nothing to report, any other refusal leaves the old keyword in place + // on mail the caller would otherwise treat as migrated. + const notUpdated = (setResult?.notUpdated || {}) as Record; + const failed = Object.entries(notUpdated); + const refused = failed.filter(([, error]) => error?.type !== 'notFound'); + if (refused.length > 0) { + debug.warn('keywords', 'Keyword migration refused for', refused.length, 'message(s):', refused[0][1]); + refusal = refusal || new Error(refused[0][1]?.description || refused[0][1]?.type || "Failed to migrate keyword"); + } + refusals += refused.length; + migrated += batch.length - failed.length; } - return allIds.length; + // Nothing moved: the caller must not rename the tag over messages that all + // kept the old keyword. Something moved: that is a partial migration the + // caller reports, not an error - the messages that did move are migrated + // and renaming the definition is what keeps them named. + if (migrated === 0 && (stopped || refusal)) throw (stopped || refusal) as Error; + + return { migrated, refused: refusals }; } async deleteEmail(emailId: string, accountId?: string): Promise { diff --git a/lib/jmap/patch-pointer.ts b/lib/jmap/patch-pointer.ts new file mode 100644 index 000000000..af860631b --- /dev/null +++ b/lib/jmap/patch-pointer.ts @@ -0,0 +1,29 @@ +/** + * Naming one member of an object in a JMAP PatchObject. + * + * A PatchObject key is a JSON Pointer (RFC 8620 section 5.3), so a slash in the + * member's own name starts a new path segment unless it is escaped. That bites + * keywords: a nested tag is stored as `$label:work/clients`, and the unescaped + * pointer `keywords/$label:work/clients` asks for the `clients` member of the + * `$label:work` keyword - not a boolean, so the patch is rejected or ignored + * and the tag never lands. + */ + +/** One JSON Pointer reference token: `~` -> `~0`, `/` -> `~1` (RFC 6901). */ +export function pointerToken(value: string): string { + return value.replace(/~/g, '~0').replace(/\//g, '~1'); +} + +/** + * The member name a reference token stands for: the inverse of + * `pointerToken`. `~1` is read before `~0`, per RFC 6901 - the other order + * would turn the escaped `~1` of `~01` back into a slash. + */ +export function pointerTokenValue(token: string): string { + return token.replace(/~1/g, '/').replace(/~0/g, '~'); +} + +/** The PatchObject key that sets or clears the keyword `keyword`. */ +export function keywordPointer(keyword: string): string { + return `keywords/${pointerToken(keyword)}`; +} diff --git a/stores/message-list-tabs-store.ts b/stores/message-list-tabs-store.ts index 7b0e74157..8bae86d2f 100644 --- a/stores/message-list-tabs-store.ts +++ b/stores/message-list-tabs-store.ts @@ -15,6 +15,7 @@ import type { Email } from '@/lib/jmap/types'; import type { MessageListTab, MessageListTabsConfig } from '@/lib/plugin-types'; import { MAX_MESSAGE_LIST_TABS } from '@/lib/plugin-types'; import { messageListTabHooks } from '@/lib/plugin-hooks'; +import { keywordPointer } from '@/lib/jmap/patch-pointer'; import { useEmailStore } from './email-store'; // ─── Validation ────────────────────────────────────────────── @@ -296,8 +297,8 @@ export const useMessageListTabsStore = create()((set, get) // One PatchObject per message: drop every other category keyword, add the // target's (removals of absent keywords are no-ops per RFC 8620 §5.3). const patch: Record = {}; - for (const k of keywordsRemoved) patch[`keywords/${k}`] = null; - if (keywordAdded) patch[`keywords/${keywordAdded}`] = true; + for (const k of keywordsRemoved) patch[keywordPointer(k)] = null; + if (keywordAdded) patch[keywordPointer(keywordAdded)] = true; // Group by owning account so shared-mailbox messages patch correctly. const byAccount = new Map();