From 115e162b86c294108fd6be393469c58464981b29 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 19:29:36 +0200 Subject: [PATCH] fix: delete large file selections in batches the server accepts Deleting files sent every selected id in one FileNode/set. A selection larger than the server's `maxObjectsInSet` was refused as a whole, so nothing was deleted. The ids now go out in batches of that size; a node already removed with its folder by an earlier batch counts as deleted, and a refused item no longer stops the remaining batches. --- src/api/__tests__/files.test.ts | 40 +++++++++++++++++++++++++++++++++ src/api/files.ts | 37 +++++++++++++++++------------- 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/src/api/__tests__/files.test.ts b/src/api/__tests__/files.test.ts index 10a5d27b..bcb2eed6 100644 --- a/src/api/__tests__/files.test.ts +++ b/src/api/__tests__/files.test.ts @@ -8,6 +8,7 @@ vi.mock('../jmap-client', () => ({ hasAccountCapability: vi.fn(() => false), getMaxSizeUpload: vi.fn(() => 0), getMaxObjectsInGet: vi.fn(() => 500), + getMaxObjectsInSet: vi.fn(() => 500), getMaxCallsInRequest: vi.fn(() => 16), currentSession: null as unknown, }, @@ -45,6 +46,7 @@ const mockRequest = jmapClient.request as ReturnType; const mockHasCapability = jmapClient.hasCapability as ReturnType; const mockMaxObjectsInGet = jmapClient.getMaxObjectsInGet as ReturnType; const mockMaxCallsInRequest = jmapClient.getMaxCallsInRequest as ReturnType; +const mockMaxObjectsInSet = jmapClient.getMaxObjectsInSet as ReturnType; function setSession(session: unknown) { (jmapClient as { currentSession: unknown }).currentSession = session; @@ -53,6 +55,7 @@ function setSession(session: unknown) { beforeEach(() => { vi.clearAllMocks(); mockMaxObjectsInGet.mockReturnValue(500); + mockMaxObjectsInSet.mockReturnValue(500); mockMaxCallsInRequest.mockReturnValue(16); mockHasCapability.mockImplementation( (urn: string) => @@ -125,6 +128,43 @@ describe('deleteFileNodes', () => { await expect(deleteFileNodes(['f1'])).rejects.toThrow('forbidden'); }); + + it("splits a large selection into batches of the server's maxObjectsInSet", async () => { + mockMaxObjectsInSet.mockReturnValue(2); + const destroyed = (ids: string[]) => ({ methodResponses: [['FileNode/set', { destroyed: ids }, '0']] }); + mockRequest + .mockResolvedValueOnce(destroyed(['a', 'b'])) + .mockResolvedValueOnce(destroyed(['c', 'd'])) + .mockResolvedValueOnce(destroyed(['e'])); + + await deleteFileNodes(['a', 'b', 'c', 'd', 'e']); + + expect(mockRequest.mock.calls.map((c) => c[0][0][1].destroy)).toEqual([['a', 'b'], ['c', 'd'], ['e']]); + expect(mockRequest.mock.calls.every((c) => c[0][0][1].onDestroyRemoveChildren === true)).toBe(true); + }); + + it('accepts a node that an earlier batch already removed with its folder', async () => { + mockMaxObjectsInSet.mockReturnValue(1); + mockRequest + .mockResolvedValueOnce({ methodResponses: [['FileNode/set', { destroyed: ['folder'] }, '0']] }) + .mockResolvedValueOnce({ + methodResponses: [['FileNode/set', { notDestroyed: { child: { type: 'notFound' } } }, '0']], + }); + + await expect(deleteFileNodes(['folder', 'child'])).resolves.toBeUndefined(); + }); + + it('still deletes the other batches when one item is refused', async () => { + mockMaxObjectsInSet.mockReturnValue(1); + mockRequest + .mockResolvedValueOnce({ + methodResponses: [['FileNode/set', { notDestroyed: { a: { type: 'forbidden', description: 'forbidden' } } }, '0']], + }) + .mockResolvedValueOnce({ methodResponses: [['FileNode/set', { destroyed: ['b'] }, '0']] }); + + await expect(deleteFileNodes(['a', 'b'])).rejects.toThrow('forbidden'); + expect(mockRequest).toHaveBeenCalledTimes(2); + }); }); describe('setFileNodeShare', () => { diff --git a/src/api/files.ts b/src/api/files.ts index bd871ebc..40c7598b 100644 --- a/src/api/files.ts +++ b/src/api/files.ts @@ -294,23 +294,28 @@ export async function moveFileNode(id: string, parentId: string | null): Promise export async function deleteFileNodes(ids: string[]): Promise { if (ids.length === 0) return; const accountId = filesAccountId(); - const res = await jmapClient.request( - [['FileNode/set', { - accountId, - destroy: ids, - // The server removes descendant nodes of destroyed folders. - onDestroyRemoveChildren: true, - }, '0']], - fileUsing(), - ); - const notDestroyed = requireMethodResult(res, '0', 'FileNode/set').notDestroyed as - | Record - | undefined; - const failedIds = Object.keys(notDestroyed ?? {}); - if (failedIds.length > 0) { - throw new Error( - notDestroyed![failedIds[0]]?.description || `Failed to delete ${failedIds.length} item(s)`, + // One FileNode/set per `maxObjectsInSet` ids: a request over the limit + // fails whole (RFC 8620 ยง3.6.1), so a large selection deleted nothing. + // Every batch runs; refusals are reported once all of them are done. + const refused: Array<{ description?: string }> = []; + for (const slice of batched(ids, jmapClient.getMaxObjectsInSet())) { + const res = await jmapClient.request( + [['FileNode/set', { + accountId, + destroy: slice, + // The server removes descendant nodes of destroyed folders. + onDestroyRemoveChildren: true, + }, '0']], + fileUsing(), ); + const notDestroyed = requireMethodResult(res, '0', 'FileNode/set').notDestroyed as + | Record + | undefined; + // A node inside a folder an earlier batch removed is already gone. + refused.push(...Object.values(notDestroyed ?? {}).filter((err) => err?.type !== 'notFound')); + } + if (refused.length > 0) { + throw new Error(refused[0]?.description || `Failed to delete ${refused.length} item(s)`); } }