From 50cbd66bfd78b9225161eefd58ea58ba8f7b5d41 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 4 Jun 2026 00:30:38 +0200 Subject: [PATCH] fix: treat blob-less FileNode as the only folder signal; migrate legacy dir-markers --- lib/jmap/client.ts | 11 +- stores/__tests__/file-store.test.ts | 126 ++++++++++++++++- stores/file-store.ts | 206 +++++++++++++++++++++++----- 3 files changed, 297 insertions(+), 46 deletions(-) diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index c8ce07ab..68f74394 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -5099,11 +5099,12 @@ export class JMAPClient implements IJMAPClient { async createFileDirectory(name: string, parentId: string | null): Promise { const accountId = this.getFilesAccountId(); - // Stalwart requires a blobId even for directories - upload an empty blob - const emptyBlob = new File([], name, { type: 'application/x-directory' }); - const { blobId } = await this.uploadBlob(emptyBlob); - - const dirProps: Record = { name, type: "d", blobId, size: 0 }; + // A FileNode is a folder (container) only when it has no content - i.e. no + // blobId, type or size, so the server stores it with `file == null`. Sending + // any of those - as older builds did (type "d" + an empty blob) - makes it a + // 0-byte FILE that nothing can ever be parented under, which is what caused + // "Parent ID does not exist or is not a folder" during the #379 migration. + const dirProps: Record = { name }; if (parentId !== null) { dirProps.parentId = parentId; } diff --git a/stores/__tests__/file-store.test.ts b/stores/__tests__/file-store.test.ts index 2972909c..b22a4da1 100644 --- a/stores/__tests__/file-store.test.ts +++ b/stores/__tests__/file-store.test.ts @@ -24,7 +24,8 @@ function makeMockClient(initial: FileNode[] = []) { return (ids === null ? nodes : nodes.filter(n => ids.includes(n.id))).map(n => ({ ...n })); }, async createFileDirectory(name: string, parentId: string | null) { - const node: FileNode = { id: `n${++seq}`, parentId, name, type: 'd', blobId: null, size: 0, created: now(), updated: now() }; + // A real folder has no content blob (this is how Stalwart marks a container). + const node: FileNode = { id: `n${++seq}`, parentId, name, type: '', blobId: null, size: 0, created: now(), updated: now() }; nodes.push(node); return { ...node }; }, @@ -39,11 +40,24 @@ function makeMockClient(initial: FileNode[] = []) { }, async updateFileNodes(updates: Record>>) { const updated: string[] = []; + const notUpdated: Record = {}; for (const [id, patch] of Object.entries(updates)) { const node = nodes.find(n => n.id === id); - if (node) { Object.assign(node, patch); updated.push(id); } + if (!node) { notUpdated[id] = 'not found'; continue; } + // Mirror Stalwart: a parentId must point at a real folder, i.e. a node + // with no content blob. A blob-backed node (file or old "folder marker") + // is not a container and is rejected. + if (patch.parentId != null) { + const parent = nodes.find(n => n.id === patch.parentId); + if (!parent || parent.blobId != null) { + notUpdated[id] = 'Parent ID does not exist or is not a folder.'; + continue; + } + } + Object.assign(node, patch); + updated.push(id); } - return { updated, notUpdated: {} as Record }; + return { updated, notUpdated }; }, async destroyFileNodes(ids: string[]) { // Emulate Stalwart's onDestroyRemoveChildren: removing a directory also @@ -80,6 +94,11 @@ function makeMockClient(initial: FileNode[] = []) { const dir = (id: string, name: string, parentId: string | null): FileNode => ({ id, parentId, name, type: 'd', blobId: null, size: 0, created: '', updated: '', }); +// An old build's "folder": a directory-typed node that is actually a blob-backed +// file, so the server won't let anything be parented under it. +const marker = (id: string, name: string, parentId: string | null): FileNode => ({ + id, parentId, name, type: 'd', blobId: `b-${id}`, size: 0, created: '', updated: '', +}); const file = (id: string, name: string, parentId: string | null): FileNode => ({ id, parentId, name, type: 'text/plain', blobId: `b-${id}`, size: 10, created: '', updated: '', }); @@ -234,6 +253,107 @@ describe('file-store hierarchy (issue #379)', () => { .toEqual(['kunden', 'night-medieval-castle-lake']); }); + it('creates missing intermediate folders instead of reparenting under a non-folder', async () => { + // The shape that produced "Parent ID does not exist or is not a folder" in + // production (#379): flat files whose path prefixes have no real directory + // node at all. The migration must materialize the folders, not skip them. + const SEP = '∕'; + const client = makeMockClient([ + file('a', `Docs${SEP}report.pdf`, null), + file('b', `Docs${SEP}Sub${SEP}notes.txt`, null), + ]); + useFileStore.getState().initClient(client); + + expect(await useFileStore.getState().migrateLegacyFlatNodes()).toBe(true); + + const nodes = client._nodes(); + const docs = nodes.find(n => n.name === 'Docs' && n.parentId === null); + expect(docs?.blobId).toBeNull(); + const sub = nodes.find(n => n.name === 'Sub' && n.parentId === docs!.id); + expect(sub?.blobId).toBeNull(); + + const byId = Object.fromEntries(nodes.map(n => [n.id, n])); + expect(byId.a).toMatchObject({ name: 'report.pdf', parentId: docs!.id }); + expect(byId.b).toMatchObject({ name: 'notes.txt', parentId: sub!.id }); + + // No leftover separators anywhere. + expect(nodes.some(n => n.name.includes(SEP))).toBe(false); + }); + + it('replaces legacy folder-marker files with real folders and reparents children', async () => { + // Old builds stored folders as 0-byte directory-typed FILES (blobId set). + // The server treats those as non-containers, which is the actual cause of + // "Parent ID does not exist or is not a folder" (#379): the migration must + // delete the markers and recreate real folders before reparenting. + const SEP = '∕'; + const client = makeMockClient([ + marker('docs', 'Documents', null), + file('a', `Documents${SEP}readme.txt`, null), + file('b', `Documents${SEP}img${SEP}logo.png`, null), + ]); + useFileStore.getState().initClient(client); + + expect(await useFileStore.getState().migrateLegacyFlatNodes()).toBe(true); + + const nodes = client._nodes(); + // The marker file is gone, replaced by a real folder (no blob). + expect(nodes.find(n => n.id === 'docs')).toBeUndefined(); + const docs = nodes.find(n => n.name === 'Documents' && n.parentId === null); + expect(docs?.blobId).toBeNull(); + const img = nodes.find(n => n.name === 'img' && n.parentId === docs!.id); + expect(img?.blobId).toBeNull(); + + const byId = Object.fromEntries(nodes.map(n => [n.id, n])); + expect(byId.a).toMatchObject({ name: 'readme.txt', parentId: docs!.id }); + expect(byId.b).toMatchObject({ name: 'logo.png', parentId: img!.id }); + expect(nodes.some(n => n.name.includes(SEP))).toBe(false); + }); + + it('handles a folder-marker whose own name is path-encoded', async () => { + // A nested old "folder" was itself stored flat ("Docs∕Sub", type d + blob). + // It is both legacy and a marker; it must be deleted (not reparented as a + // file) and replaced by a real folder holding the child. + const SEP = '∕'; + const client = makeMockClient([ + marker('docs', 'Docs', null), + marker('sub', `Docs${SEP}Sub`, null), + file('f', `Docs${SEP}Sub${SEP}file.txt`, null), + ]); + useFileStore.getState().initClient(client); + + expect(await useFileStore.getState().migrateLegacyFlatNodes()).toBe(true); + + const nodes = client._nodes(); + expect(nodes.find(n => n.id === 'docs')).toBeUndefined(); + expect(nodes.find(n => n.id === 'sub')).toBeUndefined(); + const docs = nodes.find(n => n.name === 'Docs' && n.parentId === null); + const sub = nodes.find(n => n.name === 'Sub' && n.parentId === docs!.id); + expect(sub?.blobId).toBeNull(); + expect(nodes.find(n => n.id === 'f')).toMatchObject({ name: 'file.txt', parentId: sub!.id }); + expect(nodes.some(n => n.name.includes(SEP))).toBe(false); + }); + + it('aborts and restores markers (no data loss) if the server cannot create real folders', async () => { + const SEP = '∕'; + const client = makeMockClient([ + marker('docs', 'Documents', null), + file('a', `Documents${SEP}readme.txt`, null), + ]); + // Simulate a broken/stale server: "folders" come back blob-backed (not real + // containers). The migration must detect this and undo its marker rename. + (client as unknown as { createFileDirectory: typeof client.createFileDirectory }).createFileDirectory = + async (name: string, parentId: string | null) => + ({ id: 'bad', parentId, name, type: 'd', blobId: 'b-bad', size: 0, created: '', updated: '' }); + useFileStore.getState().initClient(client); + + expect(await useFileStore.getState().migrateLegacyFlatNodes()).toBe(false); + + const byId = Object.fromEntries(client._nodes().map(n => [n.id, n])); + // Marker restored to its original name; content file untouched (still flat). + expect(byId.docs).toMatchObject({ name: 'Documents' }); + expect(byId.a).toMatchObject({ name: `Documents${SEP}readme.txt`, parentId: null }); + }); + it('migration is a no-op when no legacy nodes exist', async () => { const client = makeMockClient([dir('a', 'A', null), file('f', 'f.txt', 'a')]); useFileStore.getState().initClient(client); diff --git a/stores/file-store.ts b/stores/file-store.ts index 6c59c205..35bee40b 100644 --- a/stores/file-store.ts +++ b/stores/file-store.ts @@ -123,18 +123,29 @@ function lastLegacySepIndex(name: string): number { return idx; } +// Whether an old build's `type` marks a node as a directory. Only meaningful for +// detecting legacy "folder" nodes that were really stored as 0-byte files; it is +// NOT how a real folder is identified (see isFolder). function isDirectoryType(type: string | undefined): boolean { if (!type) return false; return DIRECTORY_TYPES.has(type) || type.includes('directory'); } +// A FileNode is a folder iff it has no content blob. This is the authoritative +// signal in the JMAP FileNode spec and in Stalwart (a node is a container when +// its `file`/`blobId` is null); a `type` of "d" is not — older builds created +// "folders" as blob-backed files, which can't hold children (#379). +function isFolder(node: Pick): boolean { + return node.blobId == null; +} + // Direct children of a given parent in the FileNode hierarchy. function childrenOf(nodes: FileNode[], parentId: string | null): FileNode[] { return nodes.filter(n => (n.parentId ?? null) === parentId); } function nodeToResource(node: FileNode): FileResource { - const isDir = isDirectoryType(node.type); + const isDir = isFolder(node); return { id: node.id, name: node.name, @@ -164,7 +175,7 @@ function resolvePathToId(nodes: FileNode[], path: string): string | null | undef const segments = path.split('/').filter(Boolean); let parentId: string | null = null; for (const segment of segments) { - const match: FileNode | undefined = childrenOf(nodes, parentId).find(n => n.name === segment && isDirectoryType(n.type)); + const match: FileNode | undefined = childrenOf(nodes, parentId).find(n => n.name === segment && isFolder(n)); if (!match) return undefined; parentId = match.id; } @@ -256,45 +267,149 @@ export const useFileStore = create((set, get) => ({ return false; } + // Split an encoded name into its real path segments, accepting any of the + // legacy separators and dropping empty segments (leading / trailing / dup + // separators). A non-legacy name yields a single segment. + const splitSegments = (name: string): string[] => { + let parts = [name]; + for (const sep of LEGACY_PATH_SEPS) parts = parts.flatMap(p => p.split(sep)); + return parts.filter(Boolean); + }; + + // A legacy "folder marker": an old build stored folders as 0-byte files with + // a directory-ish `type` and a blob. The server treats these as files, so + // nothing can be parented under them - they must be replaced by real folders. + const isLegacyDirMarker = (n: FileNode) => !isFolder(n) && isDirectoryType(n.type); + const legacy = allNodes.filter(n => lastLegacySepIndex(n.name) >= 0); - if (legacy.length === 0) return false; + const markers = allNodes.filter(isLegacyDirMarker); + if (legacy.length === 0 && markers.length === 0) return false; - // Resolve parents by their original encoded name. Building the map up front - // means the order in which we rewrite nodes does not matter: each node's - // parent is located by id, captured before any rename happens. - const idByEncodedName = new Map(); - for (const n of allNodes) idByEncodedName.set(n.name, n.id); - - // Build the per-node patches (new leaf name + resolved parentId). - const patches: { id: string; name: string; parentId: string | null }[] = []; - let skipped = 0; - for (const node of legacy) { - const idx = lastLegacySepIndex(node.name); - const parentName = node.name.slice(0, idx); - const leafName = node.name.slice(idx + 1); - const parentId = idByEncodedName.get(parentName); - // Skip orphans (missing parent) to avoid name collisions at the root; - // they keep their encoded name and remain accessible. - if (!parentId) { - skipped++; - console.warn('[Files] migration: no parent for', JSON.stringify(node.name), '(expected', JSON.stringify(parentName) + ')'); - continue; - } - patches.push({ id: node.id, name: leafName, parentId }); + // Real folders that already exist, indexed by the canonical path they + // represent, so we reuse them instead of creating duplicates. Keying on the + // JSON-encoded segment array avoids separator-collisions between levels. + const pathKey = (segs: string[]) => JSON.stringify(segs); + const existingDirByPath = new Map(); + for (const n of allNodes) { + if (!isFolder(n)) continue; + const segs = splitSegments(n.name); + if (segs.length > 0) existingDirByPath.set(pathKey(segs), n); } - set({ migrationProgress: { current: 0, total: patches.length } }); + // Per-node rename+reparent operations to apply. Crucially, every parentId + // here points at a node we have already ensured is a real folder, so the + // server's "parent must be a folder" check can't reject them (#379). + const updates: Record = {}; + const dirIdByPath = new Map(); + dirIdByPath.set('', null); // root + let skipped = 0; + let createdDirs = 0; + let creationBroken = false; + + // Ensure a real folder exists for the given path, returning its id. Reuses an + // existing folder at that path (scheduling it for rename/reparent into the + // real hierarchy) or creates a fresh one. Sequential because a create hits + // the server and deeper levels depend on its id. + const ensureDir = async (segs: string[]): Promise => { + if (segs.length === 0) return null; + const key = pathKey(segs); + if (dirIdByPath.has(key)) return dirIdByPath.get(key)!; + const parentId = await ensureDir(segs.slice(0, -1)); + const leaf = segs[segs.length - 1]; + const existing = existingDirByPath.get(key); + if (existing) { + // Reuse it; only rewrite if its name/parent isn't already correct. + if (existing.name !== leaf || (existing.parentId ?? null) !== parentId) { + updates[existing.id] = { name: leaf, parentId }; + } + dirIdByPath.set(key, existing.id); + return existing.id; + } + const created = await client.createFileDirectory(leaf, parentId); + // Safety net: a real folder has no content blob. If the server (or a stale + // build of createFileDirectory) hands back a blob-backed node, it is NOT a + // folder - abort before we delete anything irreversible (see below). + if (created.blobId != null) { + creationBroken = true; + throw new Error('createFileDirectory returned a non-folder (has a blobId)'); + } + createdDirs++; + dirIdByPath.set(key, created.id); + return created.id; + }; + + const placeDir = async (segs: string[], label: string) => { + try { + await ensureDir(segs); + } catch (err) { + skipped++; + if (!creationBroken) console.warn('[Files] migration: could not create folder', JSON.stringify(label), '→', err instanceof Error ? err.message : String(err)); + } + }; + + // Move the legacy marker files out of the way (a reversible rename) so their + // names are free for the real folders created in their place. They are only + // DELETED at the very end, once the real hierarchy is safely in place - so a + // failure can never leave a folder both gone and not recreated. + const renamedMarkers: { id: string; name: string }[] = []; + for (const m of markers) { + try { + await client.updateFileNode(m.id, { name: `__bulwark_migrating__.${m.id}` }); + renamedMarkers.push({ id: m.id, name: m.name }); + } catch (err) { + console.warn('[Files] migration: could not set aside marker', JSON.stringify(m.name), '→', err instanceof Error ? err.message : String(err)); + } + } + + // Recreate folders that existed only as markers, reparent any real folders + // that still carry an encoded name, then reparent the content files. + for (const m of markers) { + const segs = splitSegments(m.name); + if (segs.length > 0) await placeDir(segs, m.name); + } + for (const node of legacy) { + if (!isFolder(node) || isLegacyDirMarker(node)) continue; + const segs = splitSegments(node.name); + if (segs.length > 0) await placeDir(segs, node.name); + } + for (const node of legacy) { + if (isFolder(node) || isLegacyDirMarker(node)) continue; + const segs = splitSegments(node.name); + if (segs.length === 0) { skipped++; continue; } + try { + const parentId = await ensureDir(segs.slice(0, -1)); + updates[node.id] = { name: segs[segs.length - 1], parentId }; + } catch (err) { + skipped++; + if (!creationBroken) console.warn('[Files] migration: could not place', JSON.stringify(node.name), '→', err instanceof Error ? err.message : String(err)); + } + } + + // If folder creation is fundamentally broken, restore the markers we set + // aside and bail out without deleting or reparenting anything. No data lost. + if (creationBroken) { + console.error('[Files] migration aborted: the server did not return real folders ' + + '(createFileDirectory produced blob-backed nodes). Restoring markers; nothing was deleted.'); + for (const m of renamedMarkers) { + try { await client.updateFileNode(m.id, { name: m.name }); } catch { /* best effort */ } + } + set({ migrationProgress: null }); + return false; + } + + const updateIds = Object.keys(updates); + set({ migrationProgress: { current: 0, total: updateIds.length } }); let migrated = 0; let firstError: string | null = null; const CHUNK = 100; try { - for (let i = 0; i < patches.length; i += CHUNK) { - const slice = patches.slice(i, i + CHUNK); - const updates: Record = {}; - for (const p of slice) updates[p.id] = { name: p.name, parentId: p.parentId }; + for (let i = 0; i < updateIds.length; i += CHUNK) { + const slice = updateIds.slice(i, i + CHUNK); + const batch: Record = {}; + for (const id of slice) batch[id] = updates[id]; try { - const { updated, notUpdated } = await client.updateFileNodes(updates); + const { updated, notUpdated } = await client.updateFileNodes(batch); migrated += updated.length; const failedIds = Object.keys(notUpdated); if (failedIds.length > 0 && !firstError) firstError = notUpdated[failedIds[0]]; @@ -306,19 +421,34 @@ export const useFileStore = create((set, get) => ({ if (!firstError) firstError = msg; console.error('[Files] migration: batch failed →', msg); } - set({ migrationProgress: { current: Math.min(i + CHUNK, patches.length), total: patches.length } }); + set({ migrationProgress: { current: Math.min(i + CHUNK, updateIds.length), total: updateIds.length } }); } } finally { set({ migrationProgress: null }); } - if (migrated === 0) { - console.error(`[Files] migration found ${legacy.length} legacy node(s) but reparented none ` + - `(skipped ${skipped}, first error: ${firstError ?? 'none'}).`); - } else { - console.info(`[Files] migration reparented ${migrated}/${legacy.length} legacy node(s).`); + // The real hierarchy is now in place, so the set-aside marker files are empty + // and safe to delete. Done last, on purpose: until here nothing irreversible + // has happened. + let removedMarkers = 0; + if (renamedMarkers.length > 0) { + try { + const { destroyed } = await client.destroyFileNodes(renamedMarkers.map(m => m.id)); + removedMarkers = destroyed.length; + } catch (err) { + console.warn('[Files] migration: could not remove emptied markers →', err instanceof Error ? err.message : String(err)); + } } - return migrated > 0; + + const didWork = migrated > 0 || createdDirs > 0 || removedMarkers > 0; + if (!didWork && (legacy.length > 0 || markers.length > 0)) { + console.error(`[Files] migration found ${legacy.length} legacy node(s) but changed nothing ` + + `(skipped ${skipped}, first error: ${firstError ?? 'none'}).`); + } else if (didWork) { + console.info(`[Files] migration reparented ${migrated} file(s), created ${createdDirs} folder(s), ` + + `removed ${removedMarkers} legacy marker(s) (skipped ${skipped}).`); + } + return didWork; }, navigate: async (parentId: string | null, name?: string) => {