From c919d4f6368a34603bdf4bf063f7bf37cb77b987 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Fri, 26 Jun 2026 05:57:46 +0300 Subject: [PATCH] fix(page): copy shared attachments for every referencing page on duplicate (#206) attach-1: when the same attachmentId was referenced by more than one page in a duplicated subtree, the per-attachmentId map held only a single copy entry, so the last page processed clobbered the others. The downstream ownership guard (`attachment.pageId !== oldPageId`) then matched at most one page and skipped the lone DB row entirely: no blob copied, no new row, every copy's image 404'd. Key the map to a list of entries and copy one blob/row per referencing page; drop the now-incorrect ownership guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/core/page/services/page.service.ts | 96 ++++---- ...plicate-page-shared-attachment.int-spec.ts | 207 ++++++++++++++++++ 2 files changed, 260 insertions(+), 43 deletions(-) create mode 100644 apps/server/test/integration/duplicate-page-shared-attachment.int-spec.ts diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 5f16aa88..354a80fb 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -618,7 +618,13 @@ export class PageService { slugIdMap.set(entry.oldSlugId, entry); } - const attachmentMap = new Map(); + // Keyed by old attachmentId. A single attachment can be referenced by more + // than one page in the copied subtree (e.g. a block copy-pasted into a child + // page keeps the same attachmentId). Each referencing page needs its own + // fresh attachment id / row / blob copy, so the value is a LIST of copy + // entries rather than a single one — otherwise the last page's entry would + // clobber the others and their images would 404 in the copies (#206 attach-1). + const attachmentMap = new Map(); const insertablePages: InsertablePage[] = await Promise.all( pages.map(async (page) => { @@ -634,12 +640,14 @@ export class PageService { attachmentIds.forEach((attachmentId: string) => { const newPageId = pageFromMap.newPageId; const newAttachmentId = uuid7(); - attachmentMap.set(attachmentId, { + const existingEntries = attachmentMap.get(attachmentId) ?? []; + existingEntries.push({ newPageId: newPageId, oldPageId: page.id, oldAttachmentId: attachmentId, newAttachmentId: newAttachmentId, }); + attachmentMap.set(attachmentId, existingEntries); prosemirrorDoc.descendants((node: PMNode) => { if (isAttachmentNode(node.type.name)) { @@ -836,51 +844,53 @@ export class PageService { .execute(); for (const attachment of attachments) { - try { - const pageAttachment = attachmentMap.get(attachment.id); - - // make sure the copied attachment belongs to the page it was copied from - if (attachment.pageId !== pageAttachment.oldPageId) { - continue; - } - - const newAttachmentId = pageAttachment.newAttachmentId; - - const newPageId = pageAttachment.newPageId; - - const newPathFile = attachment.filePath.replace( - attachment.id, - newAttachmentId, - ); - + // One source attachment may need to be copied for several destination + // pages (it is referenced by more than one page in the subtree). Copy a + // distinct blob + row for every referencing page so each copy resolves + // (#206 attach-1). The old per-page ownership guard is gone: when the + // same attachmentId is shared, only one page would ever match the row's + // pageId, silently dropping the other copies. + const pageAttachments = attachmentMap.get(attachment.id) ?? []; + for (const pageAttachment of pageAttachments) { try { - await this.storageService.copy(attachment.filePath, newPathFile); + const newAttachmentId = pageAttachment.newAttachmentId; - await this.db - .insertInto('attachments') - .values({ - id: newAttachmentId, - type: attachment.type, - filePath: newPathFile, - fileName: attachment.fileName, - fileSize: attachment.fileSize, - mimeType: attachment.mimeType, - fileExt: attachment.fileExt, - creatorId: attachment.creatorId, - workspaceId: attachment.workspaceId, - pageId: newPageId, - spaceId: spaceId, - }) - .execute(); - } catch (err) { - this.logger.error( - `Duplicate page: failed to copy attachment ${attachment.id}`, - err, + const newPageId = pageAttachment.newPageId; + + const newPathFile = attachment.filePath.replace( + attachment.id, + newAttachmentId, ); - // Continue with other attachments even if one fails + + try { + await this.storageService.copy(attachment.filePath, newPathFile); + + await this.db + .insertInto('attachments') + .values({ + id: newAttachmentId, + type: attachment.type, + filePath: newPathFile, + fileName: attachment.fileName, + fileSize: attachment.fileSize, + mimeType: attachment.mimeType, + fileExt: attachment.fileExt, + creatorId: attachment.creatorId, + workspaceId: attachment.workspaceId, + pageId: newPageId, + spaceId: spaceId, + }) + .execute(); + } catch (err) { + this.logger.error( + `Duplicate page: failed to copy attachment ${attachment.id}`, + err, + ); + // Continue with other attachments even if one fails + } + } catch (err) { + this.logger.error(err); } - } catch (err) { - this.logger.error(err); } } } diff --git a/apps/server/test/integration/duplicate-page-shared-attachment.int-spec.ts b/apps/server/test/integration/duplicate-page-shared-attachment.int-spec.ts new file mode 100644 index 00000000..8b61fbd1 --- /dev/null +++ b/apps/server/test/integration/duplicate-page-shared-attachment.int-spec.ts @@ -0,0 +1,207 @@ +import { randomUUID } from 'node:crypto'; +import { Kysely } from 'kysely'; +import { PageRepo } from '@docmost/db/repos/page/page.repo'; +import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; +import { PageService } from 'src/core/page/services/page.service'; +import { + getTestDb, + destroyTestDb, + createWorkspace, + createSpace, + createUser, +} from './db'; + +/** + * #206 attach-1 — Duplicating a subtree where the SAME attachment is referenced + * by more than one page must copy a working blob/row for EVERY copy, not just + * the last page processed. + * + * Setup: root page A and child page B both embed the same image (attachmentId X, + * the attachment row owned by A in the DB). Duplicating A produces copies A' and + * B'. Before the fix the per-attachmentId map held a single entry, so B's entry + * clobbered A's and the row-ownership guard (`attachment.pageId !== oldPageId`) + * then skipped the only DB row entirely: zero blobs copied, zero new rows, both + * copies' images 404. The fix keys the map to a LIST and copies once per + * referencing page, dropping the broken guard. + * + * This drives the real PageService.duplicatePage against a real Postgres with a + * recording storage stub, and asserts: storage.copy called twice and two fresh + * attachment rows exist (one owned by A', one by B'), each matching the rewritten + * attachmentId in its page's content. + */ +describe('PageService.duplicatePage shared attachment [integration]', () => { + let db: Kysely; + let pageRepo: PageRepo; + let pagePermissionRepo: PagePermissionRepo; + let pageService: PageService; + let workspaceId: string; + let spaceId: string; + let userId: string; + + // Records every (source, dest) blob copy the service requests. + const copyCalls: Array<{ from: string; to: string }> = []; + const storageService = { + copy: async (from: string, to: string) => { + copyCalls.push({ from, to }); + }, + } as any; + + // Duplicate persists transclusion/reference rows in best-effort try/catch + // blocks; a no-op stub keeps the harness focused on the attachment path. + const transclusionService = { + insertTransclusionsForPages: async () => {}, + insertReferencesForPages: async () => {}, + insertTemplateReferencesForPages: async () => {}, + } as any; + + const eventEmitter = { emit: () => true } as any; + + function imageDoc(attachmentId: string) { + return { + type: 'doc', + content: [ + { + type: 'image', + attrs: { + attachmentId, + src: `/api/files/${attachmentId}/image.png`, + width: '100%', + align: 'center', + }, + }, + ], + }; + } + + beforeAll(async () => { + db = getTestDb(); + pageRepo = new PageRepo(db as any, {} as any, eventEmitter); + // filterAccessiblePageIds short-circuits to the input ids when the space has + // no restricted pages, so groupRepo/cache (2nd/3rd ctor args) are never hit. + pagePermissionRepo = new PagePermissionRepo( + db as any, + {} as any, + {} as any, + ); + pageService = new PageService( + pageRepo, + pagePermissionRepo, + undefined as any, // attachmentRepo (unused on duplicate path) + db as any, + storageService, + undefined as any, // attachmentQueue + undefined as any, // aiQueue + undefined as any, // generalQueue + eventEmitter, + undefined as any, // collaborationGateway + undefined as any, // watcherService + transclusionService, + ); + + workspaceId = (await createWorkspace(db)).id; + spaceId = (await createSpace(db, workspaceId)).id; + userId = (await createUser(db, workspaceId)).id; + }); + + afterAll(async () => { + await destroyTestDb(); + }); + + it('copies a shared attachment for every page that references it', async () => { + copyCalls.length = 0; + + const attachmentId = randomUUID(); + const pageAId = randomUUID(); + const pageBId = randomUUID(); + + // Root A and child B both embed the same attachmentId. + await db + .insertInto('pages') + .values({ + id: pageAId, + slugId: `a-${pageAId.slice(0, 8)}`, + title: 'A', + content: imageDoc(attachmentId) as any, + position: 'a0', + spaceId, + workspaceId, + creatorId: userId, + }) + .execute(); + await db + .insertInto('pages') + .values({ + id: pageBId, + slugId: `b-${pageBId.slice(0, 8)}`, + title: 'B', + content: imageDoc(attachmentId) as any, + position: 'a0', + parentPageId: pageAId, + spaceId, + workspaceId, + creatorId: userId, + }) + .execute(); + + // Single attachment row, owned by A. + await db + .insertInto('attachments') + .values({ + id: attachmentId, + type: 'image', + filePath: `${spaceId}/${attachmentId}/image.png`, + fileName: 'image.png', + fileExt: 'png', + mimeType: 'image/png', + creatorId: userId, + workspaceId, + pageId: pageAId, + spaceId, + }) + .execute(); + + const rootPage = await pageRepo.findById(pageAId); + const result = await pageService.duplicatePage( + rootPage as any, + undefined, + { id: userId, workspaceId } as any, + ); + + const newRootId = result.id; + const newChildIds = result.childPageIds; + expect(newChildIds).toHaveLength(1); + const newChildId = newChildIds[0]; + + // Both pages' images were copied: one blob per referencing page. + expect(copyCalls).toHaveLength(2); + + // Two fresh attachment rows exist, one owned by each copied page. + const newAttachments = await db + .selectFrom('attachments') + .selectAll() + .where('pageId', 'in', [newRootId, newChildId]) + .where('workspaceId', '=', workspaceId) + .execute(); + expect(newAttachments).toHaveLength(2); + + const ownerIds = newAttachments.map((a) => a.pageId).sort(); + expect(ownerIds).toEqual([newRootId, newChildId].sort()); + + // Each copied page's content points at a rewritten attachmentId that now has + // a real row (i.e. the image src resolves instead of 404ing). + for (const pageId of [newRootId, newChildId]) { + const page = await db + .selectFrom('pages') + .select(['content']) + .where('id', '=', pageId) + .executeTakeFirstOrThrow(); + const node = (page.content as any).content[0]; + expect(node.type).toBe('image'); + const referencedId = node.attrs.attachmentId; + expect(referencedId).not.toBe(attachmentId); // remapped to a fresh id + const row = newAttachments.find((a) => a.id === referencedId); + expect(row).toBeDefined(); + expect(row!.pageId).toBe(pageId); + } + }); +});