From 9d2bec8eb8021cda56f0885070608f2a261d042c Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 27 Jun 2026 00:56:37 +0300 Subject: [PATCH 1/4] fix(share): keep exactly one custom address per page on alias edit (#226) Editing an existing share alias (e.g. slug `te` -> `ted`) failed to update the displayed `/l/` link: `setAlias()` looked the requested slug up by name and, if free, INSERTed a brand-new row, leaving the page with multiple alias rows. The modal then read via `findByPageId().executeTakeFirst()` with no `ORDER BY`, so Postgres returned an arbitrary (in practice the oldest, stale) row. Every edit also spawned an orphan row that kept a live `/l/` link forever. Regression of #205. Enforce the invariant "a page has EXACTLY ONE custom address": - `setAlias()` now resolves the page's current alias row and RENAMES it in place when the requested name is free (insert only when the page has none), keeps the same-name no-op and the cross-page 409 `ALIAS_REASSIGN_REQUIRED` + confirmed-retarget flow, and after any successful write DELETEs all other alias rows for the page (self-heal). Runs in one transaction so the page is never transiently empty or duplicated. - repo: add `updateAlias` (rename) and `deleteOthersForPage`; make `findByPageId` deterministic with `ORDER BY created_at DESC, id DESC`. - migration: dedup existing rows (keep newest per page) + a PARTIAL unique index `(workspace_id, page_id) WHERE page_id IS NOT NULL` so dangling aliases still coexist while live ones are one-per-page. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/share/share-alias.service.spec.ts | 130 ++++++++++- .../src/core/share/share-alias.service.ts | 142 ++++++++---- ...60627T120000-share-aliases-one-per-page.ts | 41 ++++ .../share-alias/share-alias.repo.spec.ts | 65 +++++- .../repos/share-alias/share-alias.repo.ts | 50 +++- .../share-alias-one-per-page.int-spec.ts | 215 ++++++++++++++++++ 6 files changed, 587 insertions(+), 56 deletions(-) create mode 100644 apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts create mode 100644 apps/server/test/integration/share-alias-one-per-page.int-spec.ts diff --git a/apps/server/src/core/share/share-alias.service.spec.ts b/apps/server/src/core/share/share-alias.service.spec.ts index 349142e4..d2c5a269 100644 --- a/apps/server/src/core/share/share-alias.service.spec.ts +++ b/apps/server/src/core/share/share-alias.service.spec.ts @@ -7,13 +7,18 @@ import { ShareAliasService } from './share-alias.service'; * request-time readable-target resolution (which re-runs the share boundary). */ describe('ShareAliasService', () => { + // Sentinel handed to repo calls so tests can assert they ran inside the tx. + const trx = { __trx: true }; + function makeService() { const shareAliasRepo = { findByAliasAndWorkspace: jest.fn(), findByPageId: jest.fn(), findById: jest.fn(), insert: jest.fn(), + updateAlias: jest.fn(), updatePageId: jest.fn(), + deleteOthersForPage: jest.fn(), delete: jest.fn(), }; const pageRepo = { findById: jest.fn() }; @@ -21,12 +26,19 @@ describe('ShareAliasService', () => { resolveReadableSharePage: jest.fn(), isSharingAllowed: jest.fn(), }; + // Fake kysely db: only .transaction().execute(cb) is used by setAlias. + const db = { + transaction: jest.fn(() => ({ + execute: jest.fn(async (cb: any) => cb(trx)), + })), + }; const service = new ShareAliasService( shareAliasRepo as any, pageRepo as any, shareService as any, + db as any, ); - return { service, shareAliasRepo, pageRepo, shareService }; + return { service, shareAliasRepo, pageRepo, shareService, db }; } describe('setAlias', () => { @@ -43,9 +55,10 @@ describe('ShareAliasService', () => { expect(shareAliasRepo.findByAliasAndWorkspace).not.toHaveBeenCalled(); }); - it('normalizes then inserts a brand-new alias', async () => { + it('normalizes then inserts a brand-new alias (page has none yet)', async () => { const { service, shareAliasRepo } = makeService(); shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + shareAliasRepo.findByPageId.mockResolvedValue(undefined); shareAliasRepo.insert.mockResolvedValue({ id: 'a-1', alias: 'my-page' }); const res = await service.setAlias({ @@ -58,17 +71,70 @@ describe('ShareAliasService', () => { expect(shareAliasRepo.findByAliasAndWorkspace).toHaveBeenCalledWith( 'my-page', 'ws-1', + trx, + ); + expect(shareAliasRepo.insert).toHaveBeenCalledWith( + { + workspaceId: 'ws-1', + alias: 'my-page', + pageId: 'p-1', + creatorId: 'u-1', + }, + trx, + ); + expect(shareAliasRepo.updateAlias).not.toHaveBeenCalled(); + // self-heal still runs, keeping just the inserted row + expect(shareAliasRepo.deleteOthersForPage).toHaveBeenCalledWith( + 'p-1', + 'a-1', + 'ws-1', + trx, ); - expect(shareAliasRepo.insert).toHaveBeenCalledWith({ - workspaceId: 'ws-1', - alias: 'my-page', - pageId: 'p-1', - creatorId: 'u-1', - }); expect(res).toMatchObject({ id: 'a-1' }); }); - it('is a no-op when the alias already points at the same page', async () => { + it('renames the existing row in place when editing to a free name (te -> ted)', async () => { + const { service, shareAliasRepo } = makeService(); + // The new slug is free... + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + // ...but the page already owns an alias named `te`. + shareAliasRepo.findByPageId.mockResolvedValue({ + id: 'a-1', + alias: 'te', + pageId: 'p-1', + }); + shareAliasRepo.updateAlias.mockResolvedValue({ + id: 'a-1', + alias: 'ted', + pageId: 'p-1', + }); + + const res = await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'ted', + }); + + // RENAME, not INSERT a second row. + expect(shareAliasRepo.insert).not.toHaveBeenCalled(); + expect(shareAliasRepo.updateAlias).toHaveBeenCalledWith( + 'a-1', + 'ted', + 'ws-1', + trx, + ); + // ...and any other row for the page is reaped, so `te` cannot survive. + expect(shareAliasRepo.deleteOthersForPage).toHaveBeenCalledWith( + 'p-1', + 'a-1', + 'ws-1', + trx, + ); + expect(res).toMatchObject({ id: 'a-1', alias: 'ted' }); + }); + + it('is a no-op when the alias already points at the same page (and self-heals)', async () => { const { service, shareAliasRepo } = makeService(); const existing = { id: 'a-1', alias: 'foo', pageId: 'p-1' }; shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(existing); @@ -82,7 +148,45 @@ describe('ShareAliasService', () => { expect(res).toBe(existing); expect(shareAliasRepo.insert).not.toHaveBeenCalled(); + expect(shareAliasRepo.updateAlias).not.toHaveBeenCalled(); expect(shareAliasRepo.updatePageId).not.toHaveBeenCalled(); + // self-heal reaps any legacy duplicate rows for the page + expect(shareAliasRepo.deleteOthersForPage).toHaveBeenCalledWith( + 'p-1', + 'a-1', + 'ws-1', + trx, + ); + }); + + it('self-heals a page with pre-existing duplicate rows down to one', async () => { + const { service, shareAliasRepo } = makeService(); + // Name free; the page already has a (legacy) alias row we rename. + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + shareAliasRepo.findByPageId.mockResolvedValue({ + id: 'a-keep', + alias: 'old', + pageId: 'p-1', + }); + shareAliasRepo.updateAlias.mockResolvedValue({ + id: 'a-keep', + alias: 'new', + pageId: 'p-1', + }); + + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'new', + }); + + expect(shareAliasRepo.deleteOthersForPage).toHaveBeenCalledWith( + 'p-1', + 'a-keep', + 'ws-1', + trx, + ); }); it('throws 409 with current target when name is taken and not confirmed', async () => { @@ -134,6 +238,14 @@ describe('ShareAliasService', () => { 'a-1', 'p-1', 'ws-1', + trx, + ); + // the page's previous alias row(s) are reaped after the swap + expect(shareAliasRepo.deleteOthersForPage).toHaveBeenCalledWith( + 'p-1', + 'a-1', + 'ws-1', + trx, ); expect(res).toMatchObject({ pageId: 'p-1' }); }); diff --git a/apps/server/src/core/share/share-alias.service.ts b/apps/server/src/core/share/share-alias.service.ts index b70d6d3e..e31a35cd 100644 --- a/apps/server/src/core/share/share-alias.service.ts +++ b/apps/server/src/core/share/share-alias.service.ts @@ -9,6 +9,9 @@ import { PageRepo } from '@docmost/db/repos/page/page.repo'; import { ShareService } from './share.service'; import { Page, ShareAlias } from '@docmost/db/types/entity.types'; import { isValidShareAlias, normalizeShareAlias } from './share-alias.util'; +import { InjectKysely } from 'nestjs-kysely'; +import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { executeTx } from '@docmost/db/utils'; /** Postgres unique_violation; the (workspace_id, alias) constraint races here. */ const PG_UNIQUE_VIOLATION = '23505'; @@ -28,16 +31,26 @@ export class ShareAliasService { private readonly shareAliasRepo: ShareAliasRepo, private readonly pageRepo: PageRepo, private readonly shareService: ShareService, + @InjectKysely() private readonly db: KyselyDB, ) {} /** - * Create or retarget a vanity alias. The alias is workspace-scoped: - * - no row for this name -> INSERT a new pointer - * - row already points at pageId -> no-op (idempotent) - * - row points elsewhere -> the "swap". Without confirmReassign we - * throw 409 carrying the current target so the client can confirm; with - * it we UPDATE the single row's page_id (every /l/ link follows the - * 302 to the new page instantly — no stale 301 cache). + * Create, RENAME or retarget a page's vanity alias. INVARIANT: a page has + * EXACTLY ONE custom address. The alias name is workspace-scoped: + * - name free, page has no alias yet -> INSERT a new pointer + * - name free, page already has one -> RENAME that row in place (the slug + * edit, e.g. `te` -> `ted`); we never spawn a second row, so no orphan + * `/l/` link survives + * - name already points at pageId -> no-op (idempotent) + * - name points at ANOTHER page -> the "swap". Without confirmReassign + * we throw 409 carrying the current target so the client can confirm; + * with it we UPDATE the single row's page_id (every /l/ link + * follows the 302 to the new page instantly — no stale cache). + * + * After ANY successful write we DELETE every other alias row still pointing + * at this page (the previous name after a rename/retarget, plus any legacy + * duplicates) so the invariant self-heals. The whole thing runs in one + * transaction so the page never transiently has zero or duplicate rows. * * Caller is responsible for authorizing the page (edit rights + public * readability); this method owns only the alias-name semantics. @@ -57,48 +70,91 @@ export class ShareAliasService { ); } - const existing = await this.shareAliasRepo.findByAliasAndWorkspace( - alias, - workspaceId, - ); - - if (!existing) { - try { - return await this.shareAliasRepo.insert({ - workspaceId, + try { + return await executeTx(this.db, async (trx) => { + const byName = await this.shareAliasRepo.findByAliasAndWorkspace( alias, - pageId, - creatorId, - }); - } catch (err: any) { - // Lost a uniqueness race: another request claimed the name first. - if (err?.code === PG_UNIQUE_VIOLATION) { - throw new ConflictException({ message: 'Alias already taken' }); + workspaceId, + trx, + ); + + // The name is occupied by a DIFFERENT (or dangling) target page. + if (byName && byName.pageId !== pageId) { + if (!confirmReassign) { + const currentPage = byName.pageId + ? await this.pageRepo.findById(byName.pageId) + : null; + throw new ConflictException({ + message: 'Alias already in use', + code: 'ALIAS_REASSIGN_REQUIRED', + currentPageId: byName.pageId, + currentPageTitle: currentPage?.title ?? null, + }); + } + // Confirmed: claim the existing name row for this page, then drop the + // page's previous alias row(s) so it ends with exactly this one. + const retargeted = await this.shareAliasRepo.updatePageId( + byName.id, + pageId, + workspaceId, + trx, + ); + await this.shareAliasRepo.deleteOthersForPage( + pageId, + retargeted.id, + workspaceId, + trx, + ); + return retargeted; } - this.logger.error(err); - throw new BadRequestException('Failed to set alias'); - } - } - // Already points at this page -> nothing to do. - if (existing.pageId === pageId) { - return existing; - } + // The name is FREE, or already points at THIS page. Ensure the page has + // a single row carrying this name: rename its current one, or insert. + const current = + byName ?? + (await this.shareAliasRepo.findByPageId(pageId, workspaceId, trx)); - // Name occupied by a different (or dangling) target: require confirmation. - if (!confirmReassign) { - const currentPage = existing.pageId - ? await this.pageRepo.findById(existing.pageId) - : null; - throw new ConflictException({ - message: 'Alias already in use', - code: 'ALIAS_REASSIGN_REQUIRED', - currentPageId: existing.pageId, - currentPageTitle: currentPage?.title ?? null, + let row: ShareAlias; + if (current) { + row = + current.alias === alias + ? current // same-name no-op + : await this.shareAliasRepo.updateAlias( + current.id, + alias, + workspaceId, + trx, + ); + } else { + row = await this.shareAliasRepo.insert( + { workspaceId, alias, pageId, creatorId }, + trx, + ); + } + + // Self-heal: a page keeps EXACTLY ONE custom address. + await this.shareAliasRepo.deleteOthersForPage( + pageId, + row.id, + workspaceId, + trx, + ); + return row; }); + } catch (err: any) { + if ( + err instanceof ConflictException || + err instanceof BadRequestException + ) { + throw err; + } + // Lost a uniqueness race: another request claimed the name first. + if (err?.code === PG_UNIQUE_VIOLATION) { + throw new ConflictException({ message: 'Alias already taken' }); + } + this.logger.error(err); + throw new BadRequestException('Failed to set alias'); } - - return this.shareAliasRepo.updatePageId(existing.id, pageId, workspaceId); } /** Free a vanity name (no history kept). */ diff --git a/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts b/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts new file mode 100644 index 00000000..58042e4f --- /dev/null +++ b/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts @@ -0,0 +1,41 @@ +import { type Kysely, sql } from 'kysely'; + +/** + * Enforce "a page has EXACTLY ONE custom address" at the DB level. The original + * `share_aliases` table only had a unique index on `(workspace_id, alias)`, so a + * page could accumulate several alias rows (every slug edit used to INSERT a new + * one), leaving orphan `/l/` links live forever and making the share + * modal's `findByPageId` lookup nondeterministic. + * + * We first dedup any pre-existing rows (keeping the NEWEST per page — the same + * "current" choice the read path now makes), then add a PARTIAL unique index on + * `(workspace_id, page_id)`. It is partial (`WHERE page_id IS NOT NULL`) so that + * multiple DANGLING aliases (target page deleted -> `page_id` SET NULL) can + * still coexist without colliding. + */ +export async function up(db: Kysely): Promise { + // Reap legacy duplicates: for each (workspace_id, page_id) keep only the row + // with the greatest (created_at, id) — matches ShareAliasRepo.findByPageId. + await sql` + DELETE FROM share_aliases sa + USING share_aliases keep + WHERE sa.page_id IS NOT NULL + AND sa.workspace_id = keep.workspace_id + AND sa.page_id = keep.page_id + AND (keep.created_at, keep.id) > (sa.created_at, sa.id) + `.execute(db); + + await db.schema + .createIndex('share_aliases_workspace_id_page_id_unique') + .on('share_aliases') + .columns(['workspace_id', 'page_id']) + .unique() + .where('page_id', 'is not', null) + .execute(); +} + +export async function down(db: Kysely): Promise { + await db.schema + .dropIndex('share_aliases_workspace_id_page_id_unique') + .execute(); +} diff --git a/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts b/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts index 89ac1e52..505ba9b5 100644 --- a/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts +++ b/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts @@ -10,16 +10,21 @@ import type { KyselyDB } from '../../types/kysely.types'; describe('ShareAliasRepo', () => { function makeSelectRepo(result: unknown) { const where = jest.fn(); + const orderBy = jest.fn(); const builder: any = { select: jest.fn(() => builder), where: jest.fn((...args: unknown[]) => { where(...args); return builder; }), + orderBy: jest.fn((...args: unknown[]) => { + orderBy(...args); + return builder; + }), executeTakeFirst: jest.fn().mockResolvedValue(result), }; const db = { selectFrom: jest.fn(() => builder) } as unknown as KyselyDB; - return { repo: new ShareAliasRepo(db), db, where, builder }; + return { repo: new ShareAliasRepo(db), db, where, orderBy, builder }; } it('findByAliasAndWorkspace scopes by alias AND workspace', async () => { @@ -34,11 +39,15 @@ describe('ShareAliasRepo', () => { expect(where).toHaveBeenCalledWith('workspaceId', '=', 'ws-1'); }); - it('findByPageId scopes by page AND workspace', async () => { - const { repo, where } = makeSelectRepo(undefined); + it('findByPageId scopes by page AND workspace, deterministically ordered', async () => { + const { repo, where, orderBy } = makeSelectRepo(undefined); await repo.findByPageId('p-1', 'ws-1'); expect(where).toHaveBeenCalledWith('pageId', '=', 'p-1'); expect(where).toHaveBeenCalledWith('workspaceId', '=', 'ws-1'); + // Explicit ORDER BY removes the nondeterministic heap order for any legacy + // duplicate rows (newest createdAt wins, id as a stable tiebreak). + expect(orderBy).toHaveBeenCalledWith('createdAt', 'desc'); + expect(orderBy).toHaveBeenCalledWith('id', 'desc'); }); it('insert writes the provided columns and returns the row', async () => { @@ -99,6 +108,56 @@ describe('ShareAliasRepo', () => { expect(where).toHaveBeenCalledWith('workspaceId', '=', 'ws-1'); }); + it('updateAlias renames a single row scoped by id + workspace', async () => { + const set = jest.fn(); + const where = jest.fn(); + const builder: any = { + set: jest.fn((s: unknown) => { + set(s); + return builder; + }), + where: jest.fn((...args: unknown[]) => { + where(...args); + return builder; + }), + returning: jest.fn(() => builder), + executeTakeFirst: jest.fn().mockResolvedValue({ id: 'a-1', alias: 'ted' }), + }; + const db = { updateTable: jest.fn(() => builder) } as unknown as KyselyDB; + const repo = new ShareAliasRepo(db); + + const res = await repo.updateAlias('a-1', 'ted', 'ws-1'); + + expect(db.updateTable).toHaveBeenCalledWith('shareAliases'); + expect(set.mock.calls[0][0].alias).toBe('ted'); + expect(set.mock.calls[0][0].updatedAt).toBeInstanceOf(Date); + // a rename must NOT touch page_id (the page's pointer is preserved) + expect(set.mock.calls[0][0]).not.toHaveProperty('pageId'); + expect(where).toHaveBeenCalledWith('id', '=', 'a-1'); + expect(where).toHaveBeenCalledWith('workspaceId', '=', 'ws-1'); + expect(res).toMatchObject({ alias: 'ted' }); + }); + + it('deleteOthersForPage reaps every row for the page except keepId', async () => { + const where = jest.fn(); + const builder: any = { + where: jest.fn((...args: unknown[]) => { + where(...args); + return builder; + }), + execute: jest.fn().mockResolvedValue(undefined), + }; + const db = { deleteFrom: jest.fn(() => builder) } as unknown as KyselyDB; + const repo = new ShareAliasRepo(db); + + await repo.deleteOthersForPage('p-1', 'a-keep', 'ws-1'); + + expect(db.deleteFrom).toHaveBeenCalledWith('shareAliases'); + expect(where).toHaveBeenCalledWith('pageId', '=', 'p-1'); + expect(where).toHaveBeenCalledWith('workspaceId', '=', 'ws-1'); + expect(where).toHaveBeenCalledWith('id', '!=', 'a-keep'); + }); + it('delete scopes by id + workspace', async () => { const where = jest.fn(); const builder: any = { diff --git a/apps/server/src/database/repos/share-alias/share-alias.repo.ts b/apps/server/src/database/repos/share-alias/share-alias.repo.ts index 4c24f33c..39c94a81 100644 --- a/apps/server/src/database/repos/share-alias/share-alias.repo.ts +++ b/apps/server/src/database/repos/share-alias/share-alias.repo.ts @@ -41,7 +41,14 @@ export class ShareAliasRepo { .executeTakeFirst(); } - /** The alias currently pointing at a page (for the share modal). */ + /** + * The alias currently pointing at a page (for the share modal). The service + * enforces a single alias row per page, but legacy rows (pre-invariant) may + * still exist until self-healed; the explicit ORDER BY makes the "current" + * choice DETERMINISTIC (newest wins — i.e. the most recently created address, + * which is the one the user last asked for) instead of an arbitrary Postgres + * heap order. + */ async findByPageId( pageId: string, workspaceId: string, @@ -52,6 +59,8 @@ export class ShareAliasRepo { .select(this.baseFields) .where('pageId', '=', pageId) .where('workspaceId', '=', workspaceId) + .orderBy('createdAt', 'desc') + .orderBy('id', 'desc') .executeTakeFirst(); } @@ -79,6 +88,45 @@ export class ShareAliasRepo { .executeTakeFirst(); } + /** + * Rename an existing alias row in place (the vanity-slug edit, e.g. + * `te` -> `ted`). Keeps the row's id/page_id/creator so the page's single + * alias pointer is preserved — only the human-readable name changes. + */ + async updateAlias( + id: string, + alias: string, + workspaceId: string, + trx?: KyselyTransaction, + ): Promise { + return dbOrTx(this.db, trx) + .updateTable('shareAliases') + .set({ alias, updatedAt: new Date() }) + .where('id', '=', id) + .where('workspaceId', '=', workspaceId) + .returning(this.baseFields) + .executeTakeFirst(); + } + + /** + * Self-heal helper: drop every OTHER alias row still pointing at a page, + * keeping only `keepId`. Enforces the "exactly one custom address per page" + * invariant after a rename/retarget and reaps any legacy duplicates. + */ + async deleteOthersForPage( + pageId: string, + keepId: string, + workspaceId: string, + trx?: KyselyTransaction, + ): Promise { + await dbOrTx(this.db, trx) + .deleteFrom('shareAliases') + .where('pageId', '=', pageId) + .where('workspaceId', '=', workspaceId) + .where('id', '!=', keepId) + .execute(); + } + /** Retarget an existing alias to a new page (the "swap" operation). */ async updatePageId( id: string, diff --git a/apps/server/test/integration/share-alias-one-per-page.int-spec.ts b/apps/server/test/integration/share-alias-one-per-page.int-spec.ts new file mode 100644 index 00000000..e0829583 --- /dev/null +++ b/apps/server/test/integration/share-alias-one-per-page.int-spec.ts @@ -0,0 +1,215 @@ +import { Kysely, sql } from 'kysely'; +import { randomUUID } from 'node:crypto'; +import { ConflictException } from '@nestjs/common'; +import { ShareAliasRepo } from '@docmost/db/repos/share-alias/share-alias.repo'; +import { ShareAliasService } from 'src/core/share/share-alias.service'; +import { + getTestDb, + destroyTestDb, + createWorkspace, + createSpace, + createPage, +} from './db'; + +/** + * Issue #226 (regression of #205): "a page has EXACTLY ONE custom address". + * Exercises against real Postgres: + * - the partial unique index `(workspace_id, page_id) WHERE page_id IS NOT NULL` + * (migration 20260627T120000) — one alias per page, but dangling aliases + * (page_id NULL) may coexist; + * - the migration's dedup DELETE keeps the NEWEST row per page; + * - ShareAliasService.setAlias renames in place (te -> ted) instead of + * spawning a second row, and self-heals the page down to one alias. + */ +describe('share_aliases one-per-page invariant [integration]', () => { + let db: Kysely; + let repo: ShareAliasRepo; + let service: ShareAliasService; + let wsId: string; + let spaceId: string; + + // setAlias only consults pageRepo on the unconfirmed-reassign (409) path. + const pageRepo = { + findById: async (id: string) => ({ id, title: `title-${id}` }), + }; + + beforeAll(async () => { + db = getTestDb(); + repo = new ShareAliasRepo(db as any); + service = new ShareAliasService( + repo as any, + pageRepo as any, + {} as any, // shareService — unused by setAlias + db as any, + ); + wsId = (await createWorkspace(db)).id; + spaceId = (await createSpace(db, wsId)).id; + }); + + afterAll(async () => { + await destroyTestDb(); + }); + + const newPage = async (): Promise => + (await createPage(db, { workspaceId: wsId, spaceId })).id; + + const aliasRowsFor = (pageId: string) => + db + .selectFrom('shareAliases') + .select(['id', 'alias']) + .where('pageId', '=', pageId) + .where('workspaceId', '=', wsId) + .orderBy('alias') + .execute(); + + it('partial unique index rejects a second alias for the same page (23505)', async () => { + const pageId = await newPage(); + await repo.insert({ workspaceId: wsId, alias: 'first', pageId }); + + let code: string | undefined; + try { + await repo.insert({ workspaceId: wsId, alias: 'second', pageId }); + } catch (err: any) { + code = err?.code ?? err?.cause?.code; + } + expect(code).toBe('23505'); + }); + + it('allows multiple DANGLING aliases (page_id NULL) — partial index excludes them', async () => { + const a = await repo.insert({ + workspaceId: wsId, + alias: `dangling-${randomUUID().slice(0, 8)}`, + pageId: null as any, + }); + const b = await repo.insert({ + workspaceId: wsId, + alias: `dangling-${randomUUID().slice(0, 8)}`, + pageId: null as any, + }); + expect(a.id).toBeDefined(); + expect(b.id).toBeDefined(); + expect(a.id).not.toBe(b.id); + }); + + it("migration dedup DELETE keeps the page's NEWEST alias row", async () => { + const pageId = await newPage(); + // Temporarily drop the guard so we can seed the legacy duplicate shape. + await sql`DROP INDEX share_aliases_workspace_id_page_id_unique`.execute(db); + try { + const mk = async (alias: string, createdAt: string): Promise => { + const id = randomUUID(); + await db + .insertInto('shareAliases') + .values({ id, workspaceId: wsId, alias, pageId, createdAt }) + .execute(); + return id; + }; + await mk('oldest', '2026-01-01T00:00:00Z'); + await mk('middle', '2026-02-01T00:00:00Z'); + const newest = await mk('newest', '2026-03-01T00:00:00Z'); + + // Exact dedup statement from the migration. + await sql` + DELETE FROM share_aliases sa + USING share_aliases keep + WHERE sa.page_id IS NOT NULL + AND sa.workspace_id = keep.workspace_id + AND sa.page_id = keep.page_id + AND (keep.created_at, keep.id) > (sa.created_at, sa.id) + `.execute(db); + + const rows = await aliasRowsFor(pageId); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ id: newest, alias: 'newest' }); + } finally { + await sql` + CREATE UNIQUE INDEX share_aliases_workspace_id_page_id_unique + ON share_aliases (workspace_id, page_id) + WHERE page_id IS NOT NULL + `.execute(db); + } + }); + + it('setAlias renames te -> ted in place: page ends with ONE row named ted', async () => { + const pageId = await newPage(); + const creatorId = null as any; + const first = await service.setAlias({ + workspaceId: wsId, + pageId, + creatorId, + alias: 'te', + }); + expect(first.alias).toBe('te'); + + const renamed = await service.setAlias({ + workspaceId: wsId, + pageId, + creatorId, + alias: 'ted', + }); + // Same row id — a RENAME, not a new insert. + expect(renamed.id).toBe(first.id); + expect(renamed.alias).toBe('ted'); + + const rows = await aliasRowsFor(pageId); + expect(rows).toHaveLength(1); + expect(rows[0].alias).toBe('ted'); // the stale `te` row is gone + + // The modal read resolves the current (only) row deterministically. + const shown = await service.getAliasForPage(pageId, wsId); + expect(shown?.alias).toBe('ted'); + }); + + it('setAlias inserts the first alias, then is a no-op for the same name', async () => { + const pageId = await newPage(); + const inserted = await service.setAlias({ + workspaceId: wsId, + pageId, + creatorId: null as any, + alias: 'hello', + }); + const again = await service.setAlias({ + workspaceId: wsId, + pageId, + creatorId: null as any, + alias: 'hello', + }); + expect(again.id).toBe(inserted.id); + expect(await aliasRowsFor(pageId)).toHaveLength(1); + }); + + it('cross-page collision throws 409, and confirmReassign moves the single row', async () => { + const pageA = await newPage(); + const pageB = await newPage(); + await service.setAlias({ + workspaceId: wsId, + pageId: pageA, + creatorId: null as any, + alias: 'shared', + }); + + await expect( + service.setAlias({ + workspaceId: wsId, + pageId: pageB, + creatorId: null as any, + alias: 'shared', + }), + ).rejects.toBeInstanceOf(ConflictException); + + const moved = await service.setAlias({ + workspaceId: wsId, + pageId: pageB, + creatorId: null as any, + alias: 'shared', + confirmReassign: true, + }); + expect(moved.alias).toBe('shared'); + + // The name now belongs to pageB only; pageA has no alias. + expect(await aliasRowsFor(pageA)).toHaveLength(0); + const bRows = await aliasRowsFor(pageB); + expect(bRows).toHaveLength(1); + expect(bRows[0].alias).toBe('shared'); + }); +}); From e682bbccd1200c2b9a123bd97bcae2148d202f15 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 27 Jun 2026 02:49:48 +0300 Subject: [PATCH 2/4] fix(share): order swap delete-before-update and distinguish unique violations Addresses review on PR #227. - setAlias confirmed-reassign branch: DELETE the target page's existing alias row(s) BEFORE retargeting `byName` onto the page, instead of after. The new partial unique index `(workspace_id, page_id)` is non-deferrable and checked at each statement, so retargeting first momentarily left two rows for the page -> immediate 23505 -> rolled-back tx surfaced as a misleading "Alias already taken" (regressing a previously-working swap onto a page that already had its own alias). The reordered branch needs no trailing self-heal. JSDoc updated to describe the real ordering. - catch block: the postgres@3.x driver exposes the violated index as `err.constraint_name` (with `.constraint` as a fallback). Map `share_aliases_workspace_id_alias_unique` -> "Alias already taken" and the new `share_aliases_workspace_id_page_id_unique` -> a distinct ALIAS_PAGE_RACE outcome (a concurrent same-page write, not a name clash). Always log the constraint name on any 23505 so the race is diagnosable. - migration 20260627T120000: document that the dedup DELETE is intended, irreversible data loss (old duplicate `/l/` links start 404ing after upgrade; `down()` cannot restore the rows). Same note added to CHANGELOG [Unreleased] Fixed. Tests: - integration: confirmed reassign onto a page that ALREADY has its own alias (RED before the reorder); migration up() dedup scoping across pages and a second workspace; mid-transaction error -> BadRequest with clean rollback. - unit: constraint_name distinguishing (alias index, page_id index, fallback `.constraint`, no-info default) and non-unique error -> BadRequest; retarget test now asserts delete-before-update order. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 14 ++ .../core/share/share-alias.service.spec.ts | 111 +++++++++++++- .../src/core/share/share-alias.service.ts | 78 +++++++--- ...60627T120000-share-aliases-one-per-page.ts | 7 + .../share-alias-one-per-page.int-spec.ts | 135 +++++++++++++++++- 5 files changed, 323 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c74d97c..b4580f75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 remotely; otherwise a local directory; empty defaults to the in-repo `agent-roles-catalog/` folder — see `.env.example`). (#222) +### Fixed + +- **A shared page now keeps EXACTLY ONE custom address (`/l/:alias`).** Editing a + page's vanity slug previously inserted a second `share_aliases` row instead of + renaming the existing one, leaving the old `/l/` link live forever and + making the share modal's lookup nondeterministic. Slug edits and confirmed + reassigns now rename/retarget the single row, and a new partial unique index on + `(workspace_id, page_id)` enforces the invariant in the database. **Upgrade + note:** the accompanying migration `20260627T120000` IRREVERSIBLY deletes the + orphaned duplicate alias rows the old bug created (keeping the newest per + page), so any previously-live duplicate `/l/` link begins returning the + generic 404 after upgrade — intended, but not undoable by `down()`. (#226, + #227) + ## [0.94.0] - 2026-06-26 This release makes AI chat durable and fast: assistant turns are persisted to diff --git a/apps/server/src/core/share/share-alias.service.spec.ts b/apps/server/src/core/share/share-alias.service.spec.ts index d2c5a269..236fd1ec 100644 --- a/apps/server/src/core/share/share-alias.service.spec.ts +++ b/apps/server/src/core/share/share-alias.service.spec.ts @@ -240,21 +240,126 @@ describe('ShareAliasService', () => { 'ws-1', trx, ); - // the page's previous alias row(s) are reaped after the swap + // ORDER MATTERS: the target page's existing alias row(s) are reaped BEFORE + // the retarget, so the non-deferrable (workspace_id, page_id) index never + // sees two rows for the page mid-statement. There is no trailing self-heal. expect(shareAliasRepo.deleteOthersForPage).toHaveBeenCalledWith( 'p-1', 'a-1', 'ws-1', trx, ); + expect(shareAliasRepo.deleteOthersForPage).toHaveBeenCalledTimes(1); + const deleteOrder = + shareAliasRepo.deleteOthersForPage.mock.invocationCallOrder[0]; + const updateOrder = + shareAliasRepo.updatePageId.mock.invocationCallOrder[0]; + expect(deleteOrder).toBeLessThan(updateOrder); expect(res).toMatchObject({ pageId: 'p-1' }); }); - it('maps a unique-violation race to 409', async () => { + it('maps a unique-violation race (no constraint info) to 409 "Alias already taken"', async () => { const { service, shareAliasRepo } = makeService(); shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); shareAliasRepo.insert.mockRejectedValue({ code: '23505' }); + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'foo', + }); + fail('expected ConflictException'); + } catch (err) { + expect(err).toBeInstanceOf(ConflictException); + expect((err as ConflictException).getResponse()).toMatchObject({ + message: 'Alias already taken', + }); + } + }); + + it('maps the (workspace_id, alias) index violation to "Alias already taken"', async () => { + const { service, shareAliasRepo } = makeService(); + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + // postgres@3.x driver exposes the index name as `constraint_name`. + shareAliasRepo.insert.mockRejectedValue({ + code: '23505', + constraint_name: 'share_aliases_workspace_id_alias_unique', + }); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'foo', + }); + fail('expected ConflictException'); + } catch (err) { + expect((err as ConflictException).getResponse()).toMatchObject({ + message: 'Alias already taken', + }); + } + }); + + it('maps the (workspace_id, page_id) index violation to a DISTINCT page-race outcome', async () => { + const { service, shareAliasRepo } = makeService(); + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + shareAliasRepo.insert.mockRejectedValue({ + code: '23505', + constraint_name: 'share_aliases_workspace_id_page_id_unique', + }); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'foo', + }); + fail('expected ConflictException'); + } catch (err) { + expect(err).toBeInstanceOf(ConflictException); + // NOT the misleading "Alias already taken" — a separate, page-scoped code. + expect((err as ConflictException).getResponse()).toMatchObject({ + code: 'ALIAS_PAGE_RACE', + }); + expect((err as ConflictException).getResponse()).not.toMatchObject({ + message: 'Alias already taken', + }); + } + }); + + it('reads the index name from `.constraint` when `.constraint_name` is absent', async () => { + const { service, shareAliasRepo } = makeService(); + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + // Fallback path for non-postgres@3.x drivers. + shareAliasRepo.insert.mockRejectedValue({ + code: '23505', + constraint: 'share_aliases_workspace_id_page_id_unique', + }); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'foo', + }); + fail('expected ConflictException'); + } catch (err) { + expect((err as ConflictException).getResponse()).toMatchObject({ + code: 'ALIAS_PAGE_RACE', + }); + } + }); + + it('maps a non-unique-violation db error to BadRequest (Failed to set alias)', async () => { + const { service, shareAliasRepo } = makeService(); + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + shareAliasRepo.insert.mockRejectedValue({ code: '08006' }); // connection error + await expect( service.setAlias({ workspaceId: 'ws-1', @@ -262,7 +367,7 @@ describe('ShareAliasService', () => { creatorId: 'u-1', alias: 'foo', }), - ).rejects.toBeInstanceOf(ConflictException); + ).rejects.toBeInstanceOf(BadRequestException); }); }); diff --git a/apps/server/src/core/share/share-alias.service.ts b/apps/server/src/core/share/share-alias.service.ts index e31a35cd..dceae71b 100644 --- a/apps/server/src/core/share/share-alias.service.ts +++ b/apps/server/src/core/share/share-alias.service.ts @@ -13,9 +13,21 @@ import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB } from '@docmost/db/types/kysely.types'; import { executeTx } from '@docmost/db/utils'; -/** Postgres unique_violation; the (workspace_id, alias) constraint races here. */ +/** Postgres unique_violation. Two unique indexes can raise it on this table. */ const PG_UNIQUE_VIOLATION = '23505'; +/** + * Unique index names from the share_aliases migrations. The `postgres@3.x` + * driver (kysely-postgres-js) surfaces the violated constraint as + * `err.constraint_name` (NOT `.constraint`); we keep `.constraint` only as a + * defensive fallback for other drivers. + * - ALIAS: `(workspace_id, alias)` -> the vanity NAME is taken. + * - PAGE_ID: partial `(workspace_id, page_id) WHERE page_id IS NOT NULL` + * -> a concurrent writer already gave THIS page an alias. + */ +const UNIQUE_ALIAS_INDEX = 'share_aliases_workspace_id_alias_unique'; +const UNIQUE_PAGE_ID_INDEX = 'share_aliases_workspace_id_page_id_unique'; + export interface ResolvedAliasTarget { share: NonNullable< Awaited> @@ -47,10 +59,14 @@ export class ShareAliasService { * with it we UPDATE the single row's page_id (every /l/ link * follows the 302 to the new page instantly — no stale cache). * - * After ANY successful write we DELETE every other alias row still pointing - * at this page (the previous name after a rename/retarget, plus any legacy - * duplicates) so the invariant self-heals. The whole thing runs in one - * transaction so the page never transiently has zero or duplicate rows. + * To keep the invariant self-healing we DELETE every other alias row still + * pointing at this page (a legacy duplicate, or the target page's own former + * alias during a swap). The whole thing runs in one transaction. Because the + * `(workspace_id, page_id)` unique index is NON-deferrable (checked at the end + * of each statement), the swap branch DELETEs the target page's existing row + * BEFORE retargeting, so the page is never transiently carried by two rows; + * the other branches self-heal AFTER their write. Either way the page never + * ends a statement with duplicate rows. * * Caller is responsible for authorizing the page (edit rights + public * readability); this method owns only the alias-name semantics. @@ -91,21 +107,28 @@ export class ShareAliasService { currentPageTitle: currentPage?.title ?? null, }); } - // Confirmed: claim the existing name row for this page, then drop the - // page's previous alias row(s) so it ends with exactly this one. - const retargeted = await this.shareAliasRepo.updatePageId( + // Confirmed swap. ORDER MATTERS: the partial unique index on + // `(workspace_id, page_id)` is NON-deferrable, so it is checked at the + // end of EVERY statement. If we retargeted `byName` onto `pageId` + // first while `pageId` still had its OWN alias row, there would + // momentarily be two rows with this page_id -> immediate 23505 and a + // rolled-back tx (a misleading "Alias already taken"). So we FIRST drop + // the target page's existing alias row(s), THEN retarget. `byName.id` + // still points at its old page here, so excluding it via `keepId` is + // harmless; after the retarget it is the page's only row, so no + // trailing self-heal is needed. + await this.shareAliasRepo.deleteOthersForPage( + pageId, + byName.id, + workspaceId, + trx, + ); + return await this.shareAliasRepo.updatePageId( byName.id, pageId, workspaceId, trx, ); - await this.shareAliasRepo.deleteOthersForPage( - pageId, - retargeted.id, - workspaceId, - trx, - ); - return retargeted; } // The name is FREE, or already points at THIS page. Ensure the page has @@ -148,8 +171,31 @@ export class ShareAliasService { ) { throw err; } - // Lost a uniqueness race: another request claimed the name first. + // A unique index fired. Which one decides the message — always log the + // constraint so the race is diagnosable. if (err?.code === PG_UNIQUE_VIOLATION) { + const constraint: string | undefined = + err?.constraint_name ?? err?.constraint; + this.logger.warn( + `share alias unique violation on ${constraint ?? ''}`, + ); + // `(workspace_id, page_id)`: a concurrent request already gave this page + // an alias. The page still has exactly one custom address (the racing + // writer's), so this is not a user-facing name clash — surface a + // distinct, non-misleading message instead of "Alias already taken". + if (constraint === UNIQUE_PAGE_ID_INDEX) { + throw new ConflictException({ + message: 'This page is being given an address by another request', + code: 'ALIAS_PAGE_RACE', + }); + } + // `(workspace_id, alias)` (UNIQUE_ALIAS_INDEX) or any other/unknown + // unique index: treat as the vanity name being claimed first. + if (constraint && constraint !== UNIQUE_ALIAS_INDEX) { + this.logger.warn( + `unexpected unique index ${constraint} mapped to "Alias already taken"`, + ); + } throw new ConflictException({ message: 'Alias already taken' }); } this.logger.error(err); diff --git a/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts b/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts index 58042e4f..141769fb 100644 --- a/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts +++ b/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts @@ -12,6 +12,13 @@ import { type Kysely, sql } from 'kysely'; * `(workspace_id, page_id)`. It is partial (`WHERE page_id IS NOT NULL`) so that * multiple DANGLING aliases (target page deleted -> `page_id` SET NULL) can * still coexist without colliding. + * + * ⚠️ IRREVERSIBLE DATA LOSS (intended): the dedup DELETE below permanently drops + * every alias row but the newest per page. Those duplicates were live `/l/` + * pointers (resolved by name via `findByAliasAndWorkspace`, not by page), so + * after this upgrade any such OLD vanity link starts returning the SPA 404. This + * is the point — it kills the orphan rows the pre-invariant bug accumulated — + * but `down()` only drops the unique index; it CANNOT restore the deleted rows. */ export async function up(db: Kysely): Promise { // Reap legacy duplicates: for each (workspace_id, page_id) keep only the row diff --git a/apps/server/test/integration/share-alias-one-per-page.int-spec.ts b/apps/server/test/integration/share-alias-one-per-page.int-spec.ts index e0829583..b372a996 100644 --- a/apps/server/test/integration/share-alias-one-per-page.int-spec.ts +++ b/apps/server/test/integration/share-alias-one-per-page.int-spec.ts @@ -1,8 +1,9 @@ import { Kysely, sql } from 'kysely'; import { randomUUID } from 'node:crypto'; -import { ConflictException } from '@nestjs/common'; +import { BadRequestException, ConflictException } from '@nestjs/common'; import { ShareAliasRepo } from '@docmost/db/repos/share-alias/share-alias.repo'; import { ShareAliasService } from 'src/core/share/share-alias.service'; +import * as onePerPageMigration from 'src/database/migrations/20260627T120000-share-aliases-one-per-page'; import { getTestDb, destroyTestDb, @@ -53,15 +54,17 @@ describe('share_aliases one-per-page invariant [integration]', () => { const newPage = async (): Promise => (await createPage(db, { workspaceId: wsId, spaceId })).id; - const aliasRowsFor = (pageId: string) => + const aliasRowsForWs = (pageId: string, workspaceId: string) => db .selectFrom('shareAliases') .select(['id', 'alias']) .where('pageId', '=', pageId) - .where('workspaceId', '=', wsId) + .where('workspaceId', '=', workspaceId) .orderBy('alias') .execute(); + const aliasRowsFor = (pageId: string) => aliasRowsForWs(pageId, wsId); + it('partial unique index rejects a second alias for the same page (23505)', async () => { const pageId = await newPage(); await repo.insert({ workspaceId: wsId, alias: 'first', pageId }); @@ -130,6 +133,54 @@ describe('share_aliases one-per-page invariant [integration]', () => { } }); + it('migration up() dedups per page and leaves OTHER pages and workspaces untouched', async () => { + // Seed legacy duplicates for two pages in this workspace AND a page in a + // SECOND workspace, then run the real migration up() (not an inlined copy of + // its SQL) and assert it scopes the DELETE to (workspace_id, page_id). + const ws2 = (await createWorkspace(db)).id; + const space2 = (await createSpace(db, ws2)).id; + const pageA = await newPage(); + const pageB = await newPage(); + const pageC = (await createPage(db, { workspaceId: ws2, spaceId: space2 })) + .id; + + // Drop the guard so we can seed the pre-invariant duplicate shape. + await sql`DROP INDEX share_aliases_workspace_id_page_id_unique`.execute(db); + const seed = async ( + workspaceId: string, + pageId: string, + alias: string, + createdAt: string, + ): Promise => { + const id = randomUUID(); + await db + .insertInto('shareAliases') + .values({ id, workspaceId, alias, pageId, createdAt }) + .execute(); + return id; + }; + await seed(wsId, pageA, 'a-old', '2026-01-01T00:00:00Z'); + const aNew = await seed(wsId, pageA, 'a-new', '2026-03-01T00:00:00Z'); + await seed(wsId, pageB, 'b-old', '2026-01-01T00:00:00Z'); + const bNew = await seed(wsId, pageB, 'b-new', '2026-03-01T00:00:00Z'); + await seed(ws2, pageC, 'c-old', '2026-01-01T00:00:00Z'); + const cNew = await seed(ws2, pageC, 'c-new', '2026-03-01T00:00:00Z'); + + // Run the migration. It dedups AND recreates the unique index. + await onePerPageMigration.up(db as any); + + const aliasesOf = async (pageId: string) => + (await aliasRowsForWs(pageId, wsId)).map((r) => r.alias); + const aRows = await aliasRowsForWs(pageA, wsId); + expect(aRows).toEqual([{ id: aNew, alias: 'a-new' }]); + const bRows = await aliasRowsForWs(pageB, wsId); + expect(bRows).toEqual([{ id: bNew, alias: 'b-new' }]); + // The other workspace's page keeps only ITS newest row, untouched by wsId. + const cRows = await aliasRowsForWs(pageC, ws2); + expect(cRows).toEqual([{ id: cNew, alias: 'c-new' }]); + expect(await aliasesOf(pageA)).toEqual(['a-new']); + }); + it('setAlias renames te -> ted in place: page ends with ONE row named ted', async () => { const pageId = await newPage(); const creatorId = null as any; @@ -178,6 +229,45 @@ describe('share_aliases one-per-page invariant [integration]', () => { expect(await aliasRowsFor(pageId)).toHaveLength(1); }); + it('a mid-transaction error becomes BadRequestException and rolls back cleanly', async () => { + // A non-23505 failure inside the tx must surface as BadRequest AND leave NO + // partial alias state behind (the whole executeTx unit rolls back). + const pageId = await newPage(); + const boom = new Error('disk on fire'); // not a unique-violation + // Wrap the real repo so the INSERT succeeds but the trailing self-heal + // throws — the row inserted earlier in the tx must not survive. + const flakyRepo = Object.create(repo); + flakyRepo.deleteOthersForPage = async () => { + throw boom; + }; + const flakyService = new ShareAliasService( + flakyRepo as any, + pageRepo as any, + {} as any, + db as any, + ); + + await expect( + flakyService.setAlias({ + workspaceId: wsId, + pageId, + creatorId: null as any, + alias: 'rollback-me', + }), + ).rejects.toBeInstanceOf(BadRequestException); + + // Rolled back: neither the page nor the name has any row. + expect(await aliasRowsFor(pageId)).toHaveLength(0); + expect( + await db + .selectFrom('shareAliases') + .select('id') + .where('alias', '=', 'rollback-me') + .where('workspaceId', '=', wsId) + .execute(), + ).toHaveLength(0); + }); + it('cross-page collision throws 409, and confirmReassign moves the single row', async () => { const pageA = await newPage(); const pageB = await newPage(); @@ -212,4 +302,43 @@ describe('share_aliases one-per-page invariant [integration]', () => { expect(bRows).toHaveLength(1); expect(bRows[0].alias).toBe('shared'); }); + + it('confirmReassign onto a page that ALREADY has its own alias: target ends with ONE row', async () => { + // Regression guard for the operation-order bug: A has `shared`, B has its + // OWN alias `bee`. Moving `shared` onto B must FIRST drop B's `bee` row, + // THEN retarget, or the NON-deferrable (workspace_id, page_id) index fires a + // 23505 mid-transaction (two rows momentarily carry page_id = B) and the tx + // rolls back into a misleading "Alias already taken". + // Distinct names: the workspace is shared across tests, so reuse of an + // earlier test's `shared` would trip the 409 guard before we get here. + const pageA = await newPage(); + const pageB = await newPage(); + await service.setAlias({ + workspaceId: wsId, + pageId: pageA, + creatorId: null as any, + alias: 'shared-target', + }); + await service.setAlias({ + workspaceId: wsId, + pageId: pageB, + creatorId: null as any, + alias: 'bee', + }); + + const moved = await service.setAlias({ + workspaceId: wsId, + pageId: pageB, + creatorId: null as any, + alias: 'shared-target', + confirmReassign: true, + }); + expect(moved.alias).toBe('shared-target'); + + // B now carries exactly `shared-target` (its old `bee` is gone); A has none. + const bRows = await aliasRowsFor(pageB); + expect(bRows).toHaveLength(1); + expect(bRows[0].alias).toBe('shared-target'); + expect(await aliasRowsFor(pageA)).toHaveLength(0); + }); }); From 309719abc600104e456c070d9092d8f18090dd25 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 27 Jun 2026 03:24:00 +0300 Subject: [PATCH 3/4] fix(share): show reassign hint instead of dead-end error for a taken custom address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The share modal flagged a custom address already owned by another page with a red "This address is already in use" error driven by the availability probe. That reads as terminal even though Save actually triggers the server's 409 `ALIAS_REASSIGN_REQUIRED` and opens the "Move custom address?" confirm modal that retargets the address to the current page — so the reassign path was hidden behind what looked like a hard stop. Replace the red error with an informational description hint ("This address is in use. Saving will move it to this page.") and keep Save enabled, so the existing confirm-reassign flow is discoverable. Renaming to a FREE name was already correct (the probe returns available -> no error -> server renames the single row in place); this only changes the taken-name presentation. Verified end-to-end in a real browser against a live stand on this branch: - A (free rename `test`->`test2`): 200, same alias row renamed in place, link becomes `/l/test2`, no error, exactly one DB row for the page. - B (`test2` owned by another page): hint shown (no dead-end error), Save -> 409 ALIAS_REASSIGN_REQUIRED -> "Move custom address?" modal -> confirm -> 200, the single row retargets, one row each. - C (same-name re-save): Save disabled (no-op); first-time set inserts. Add a client component test covering both branches (taken name -> hint not error + Save enabled; 409 -> reassign modal -> confirm sends confirmReassign). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 7 + .../public/locales/en-US/translation.json | 1 + .../public/locales/ru-RU/translation.json | 1 + .../components/share-alias-section.test.tsx | 149 ++++++++++++++++++ .../share/components/share-alias-section.tsx | 18 ++- 5 files changed, 171 insertions(+), 5 deletions(-) create mode 100644 apps/client/src/features/share/components/share-alias-section.test.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index b4580f75..27a00954 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 page), so any previously-live duplicate `/l/` link begins returning the generic 404 after upgrade — intended, but not undoable by `down()`. (#226, #227) +- **Typing a custom address already used by another page no longer looks like a + dead end.** The share modal previously flagged such a name with a red "This + address is already in use" error, hiding the fact that saving offers to MOVE + the address to the current page. The field now shows an informational hint — + "This address is in use. Saving will move it to this page." — and keeps Save + enabled, so the existing reassign-confirm flow (`409 ALIAS_REASSIGN_REQUIRED` → + "Move custom address?") is discoverable instead of reading as terminal. (#227) ## [0.94.0] - 2026-06-26 diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 95ebdd15..ffbfd0cb 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -1333,6 +1333,7 @@ "A short, memorable link you can point at any shared page.": "A short, memorable link you can point at any shared page.", "Use 2-60 lowercase letters, digits and hyphens": "Use 2-60 lowercase letters, digits and hyphens", "This address is already in use": "This address is already in use", + "This address is in use. Saving will move it to this page.": "This address is in use. Saving will move it to this page.", "Move custom address?": "Move custom address?", "Move here": "Move here", "The address \"{{alias}}\" currently points to \"{{title}}\". Move it to this page?": "The address \"{{alias}}\" currently points to \"{{title}}\". Move it to this page?", diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index acd1a7c3..f0b99071 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -1190,6 +1190,7 @@ "A short, memorable link you can point at any shared page.": "Короткая запоминающаяся ссылка, которую можно направить на любую опубликованную страницу.", "Use 2-60 lowercase letters, digits and hyphens": "Используйте 2–60 строчных букв, цифр и дефисов", "This address is already in use": "Этот адрес уже занят", + "This address is in use. Saving will move it to this page.": "Этот адрес уже используется. При сохранении он будет перемещён на эту страницу.", "Move custom address?": "Переместить пользовательский адрес?", "Move here": "Переместить сюда", "The address \"{{alias}}\" currently points to \"{{title}}\". Move it to this page?": "Адрес «{{alias}}» сейчас указывает на «{{title}}». Переместить его на эту страницу?", diff --git a/apps/client/src/features/share/components/share-alias-section.test.tsx b/apps/client/src/features/share/components/share-alias-section.test.tsx new file mode 100644 index 00000000..f83e00c7 --- /dev/null +++ b/apps/client/src/features/share/components/share-alias-section.test.tsx @@ -0,0 +1,149 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; +import type { IShareAlias } from "@/features/share/types/share.types"; + +// matchMedia / storage are stubbed globally in vitest.setup.ts. + +// The mutation + query hooks reach react-query/network; the availability probe +// hits the API. Stub them so the section renders in isolation and we can drive +// the exact branches (taken name -> hint, 409 -> reassign modal). +const setMutateAsync = vi.fn(); +let currentAlias: IShareAlias | null = null; +let availabilityResult: { + valid: boolean; + available: boolean; + currentPageId: string | null; +} = { valid: true, available: true, currentPageId: null }; + +vi.mock("@/features/share/queries/share-query.ts", () => ({ + useShareAliasForPageQuery: () => ({ data: currentAlias }), + useSetShareAliasMutation: () => ({ + mutateAsync: setMutateAsync, + isPending: false, + }), + useRemoveShareAliasMutation: () => ({ + mutateAsync: vi.fn(), + isPending: false, + }), +})); + +vi.mock("@/features/share/services/share-service.ts", () => ({ + checkShareAliasAvailability: vi.fn(async () => availabilityResult), +})); + +import ShareAliasSection from "./share-alias-section"; + +const aliasRow = (alias: string, pageId: string): IShareAlias => ({ + id: `alias-${alias}`, + workspaceId: "ws-1", + alias, + pageId, + creatorId: "user-1", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), +}); + +function renderSection(pageId = "page-Y") { + return render( + + + , + ); +} + +describe("ShareAliasSection — taken-name handling is never a dead end", () => { + beforeEach(() => { + setMutateAsync.mockReset(); + currentAlias = null; + availabilityResult = { valid: true, available: true, currentPageId: null }; + }); + + it("shows a 'will move it here' HINT (not a terminal error) when the name belongs to another page, and keeps Save enabled", async () => { + // Page Y already owns "bee"; the user retypes a name owned by page X. + currentAlias = aliasRow("bee", "page-Y"); + availabilityResult = { + valid: true, + available: false, + currentPageId: "page-X", + }; + + renderSection("page-Y"); + const input = screen.getByPlaceholderText("my-page") as HTMLInputElement; + fireEvent.change(input, { target: { value: "test2" } }); + + // The reassign hint replaces the old dead-end red error. + await waitFor( + () => + expect( + screen.getByText( + "This address is in use. Saving will move it to this page.", + ), + ).toBeDefined(), + { timeout: 2000 }, + ); + // The old terminal "already in use" error must NOT be shown. + expect(screen.queryByText("This address is already in use")).toBeNull(); + + // Save stays enabled so the confirm-reassign flow can run. + const saveBtn = screen.getByRole("button", { + name: "Save", + }) as HTMLButtonElement; + expect(saveBtn.disabled).toBe(false); + }); + + it("opens the reassign-confirm modal on a 409 ALIAS_REASSIGN_REQUIRED (path forward, not a dead end)", async () => { + currentAlias = aliasRow("bee", "page-Y"); + availabilityResult = { + valid: true, + available: false, + currentPageId: "page-X", + }; + // The server rejects the un-confirmed save asking the client to confirm. + setMutateAsync.mockRejectedValueOnce({ + status: 409, + response: { + status: 409, + data: { + code: "ALIAS_REASSIGN_REQUIRED", + currentPageId: "page-X", + currentPageTitle: "Alias Test Page X", + }, + }, + }); + + renderSection("page-Y"); + const input = screen.getByPlaceholderText("my-page") as HTMLInputElement; + fireEvent.change(input, { target: { value: "test2" } }); + + const saveBtn = screen.getByRole("button", { + name: "Save", + }) as HTMLButtonElement; + await waitFor(() => expect(saveBtn.disabled).toBe(false), { + timeout: 2000, + }); + fireEvent.click(saveBtn); + + // First save sent WITHOUT confirmReassign. + await waitFor(() => + expect(setMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ alias: "test2", confirmReassign: false }), + ), + ); + + // The "Move custom address?" confirm modal must appear (the path forward). + await waitFor(() => + expect(screen.getByText("Move custom address?")).toBeDefined(), + ); + expect(screen.getByRole("button", { name: "Move here" })).toBeDefined(); + + // Confirming retries WITH confirmReassign: true. + setMutateAsync.mockResolvedValueOnce(aliasRow("test2", "page-Y")); + fireEvent.click(screen.getByRole("button", { name: "Move here" })); + await waitFor(() => + expect(setMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ alias: "test2", confirmReassign: true }), + ), + ); + }); +}); diff --git a/apps/client/src/features/share/components/share-alias-section.tsx b/apps/client/src/features/share/components/share-alias-section.tsx index 082b9c23..a3938548 100644 --- a/apps/client/src/features/share/components/share-alias-section.tsx +++ b/apps/client/src/features/share/components/share-alias-section.tsx @@ -120,8 +120,13 @@ export default function ShareAliasSection({ }; const showInvalid = normalized.length > 0 && !isValid; - const showTaken = - isValid && !unchanged && availability && !availability.available; + // The typed name is already in use by ANOTHER page. This is NOT a dead end: + // hitting Save triggers the server's 409 `ALIAS_REASSIGN_REQUIRED` and opens + // the "Move custom address?" confirm modal that retargets the address here. + // So surface it as an informational hint (not a terminal red error) and keep + // Save enabled, instead of looking like the address is unusable. + const reassignable = + isValid && !unchanged && !!availability && !availability.available; // The slug prefix (e.g. "docs.example.com/l/") is static for the session. const prefixLabel = aliasPrefixLabel(); @@ -198,9 +203,12 @@ export default function ShareAliasSection({ error={ showInvalid ? t("Use 2-60 lowercase letters, digits and hyphens") - : showTaken - ? t("This address is already in use") - : undefined + : undefined + } + description={ + reassignable + ? t("This address is in use. Saving will move it to this page.") + : undefined } /> From 767ac9e7e2b0d801577514290bc04c5d63815dc2 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 27 Jun 2026 03:33:33 +0300 Subject: [PATCH 4/4] fix(share): guard alias swap/rename against concurrent-delete race; share unique-violation helpers Address PR #227 re-review (comment 2193). - Stability: `updatePageId`/`updateAlias` now `executeTakeFirstOrThrow`, so a row reaped by a concurrent `removeAlias` between the read and the UPDATE (READ COMMITTED) raises `NoResultError` instead of returning `undefined`. The service maps that to a retryable `ConflictException` (`ALIAS_PAGE_RACE`) rather than a 200-without-alias (swap) or a generic 400 from `undefined.id` (rename). Tests cover both branches. - Simplification: drop the redundant secondary "unexpected unique index" warn and the now-unused `UNIQUE_ALIAS_INDEX` const (the constraint name is already logged unconditionally; both index branches still distinguish "Alias already taken" vs ALIAS_PAGE_RACE). - Architecture: extract `isUniqueViolation`/`violatedConstraint` into database/utils.ts; adopt them in the share-alias service and favorite.repo (the bare `23505` check). ai-agent-roles (#222) is on a separate unmerged branch and should adopt them after #227 merges (noted at the helpers). Helper unit test added. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/share/share-alias.service.spec.ts | 63 +++++++++++++++++++ .../src/core/share/share-alias.service.ts | 47 ++++++++------ .../database/repos/favorite/favorite.repo.ts | 5 +- .../share-alias/share-alias.repo.spec.ts | 10 ++- .../repos/share-alias/share-alias.repo.ts | 20 +++++- .../src/database/unique-violation.spec.ts | 51 +++++++++++++++ apps/server/src/database/utils.ts | 29 +++++++++ 7 files changed, 198 insertions(+), 27 deletions(-) create mode 100644 apps/server/src/database/unique-violation.spec.ts diff --git a/apps/server/src/core/share/share-alias.service.spec.ts b/apps/server/src/core/share/share-alias.service.spec.ts index 236fd1ec..471ccf8e 100644 --- a/apps/server/src/core/share/share-alias.service.spec.ts +++ b/apps/server/src/core/share/share-alias.service.spec.ts @@ -1,4 +1,5 @@ import { BadRequestException, ConflictException } from '@nestjs/common'; +import { NoResultError } from 'kysely'; import { ShareAliasService } from './share-alias.service'; /** @@ -355,6 +356,68 @@ describe('ShareAliasService', () => { } }); + it('maps a concurrent-delete race in the SWAP branch to a retryable 409 (not a 200-without-alias)', async () => { + const { service, shareAliasRepo } = makeService(); + // Name points at another page; reassign confirmed -> swap branch. + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue({ + id: 'a-1', + alias: 'foo', + pageId: 'p-other', + }); + // A concurrent removeAlias deleted the row between read and UPDATE, so the + // repo's executeTakeFirstOrThrow finds 0 rows and throws NoResultError. + shareAliasRepo.updatePageId.mockRejectedValue( + new NoResultError({} as any), + ); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'foo', + confirmReassign: true, + }); + fail('expected ConflictException'); + } catch (err) { + // Crucially NOT a resolved 200 carrying `undefined` as the alias. + expect(err).toBeInstanceOf(ConflictException); + expect((err as ConflictException).getResponse()).toMatchObject({ + code: 'ALIAS_PAGE_RACE', + }); + } + }); + + it('maps a concurrent-delete race in the RENAME branch to a retryable 409 (not a generic 400)', async () => { + const { service, shareAliasRepo } = makeService(); + // New slug is free, but the page already owns an alias we rename in place. + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + shareAliasRepo.findByPageId.mockResolvedValue({ + id: 'a-1', + alias: 'te', + pageId: 'p-1', + }); + // The row vanished before the UPDATE; repo throws NoResultError rather + // than returning undefined (which would dereference undefined.id -> 400). + shareAliasRepo.updateAlias.mockRejectedValue(new NoResultError({} as any)); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'ted', + }); + fail('expected ConflictException'); + } catch (err) { + expect(err).toBeInstanceOf(ConflictException); + expect(err).not.toBeInstanceOf(BadRequestException); + expect((err as ConflictException).getResponse()).toMatchObject({ + code: 'ALIAS_PAGE_RACE', + }); + } + }); + it('maps a non-unique-violation db error to BadRequest (Failed to set alias)', async () => { const { service, shareAliasRepo } = makeService(); shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); diff --git a/apps/server/src/core/share/share-alias.service.ts b/apps/server/src/core/share/share-alias.service.ts index dceae71b..8b05eabb 100644 --- a/apps/server/src/core/share/share-alias.service.ts +++ b/apps/server/src/core/share/share-alias.service.ts @@ -11,21 +11,21 @@ import { Page, ShareAlias } from '@docmost/db/types/entity.types'; import { isValidShareAlias, normalizeShareAlias } from './share-alias.util'; import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB } from '@docmost/db/types/kysely.types'; -import { executeTx } from '@docmost/db/utils'; - -/** Postgres unique_violation. Two unique indexes can raise it on this table. */ -const PG_UNIQUE_VIOLATION = '23505'; +import { + executeTx, + isUniqueViolation, + violatedConstraint, +} from '@docmost/db/utils'; +import { NoResultError } from 'kysely'; /** - * Unique index names from the share_aliases migrations. The `postgres@3.x` - * driver (kysely-postgres-js) surfaces the violated constraint as - * `err.constraint_name` (NOT `.constraint`); we keep `.constraint` only as a - * defensive fallback for other drivers. - * - ALIAS: `(workspace_id, alias)` -> the vanity NAME is taken. + * Unique index name from the share_aliases migrations whose violation we map to + * a DISTINCT, non-misleading outcome: * - PAGE_ID: partial `(workspace_id, page_id) WHERE page_id IS NOT NULL` * -> a concurrent writer already gave THIS page an alias. + * The `(workspace_id, alias)` index (the vanity NAME being taken) needs no + * constant: it is the default "Alias already taken" mapping. */ -const UNIQUE_ALIAS_INDEX = 'share_aliases_workspace_id_alias_unique'; const UNIQUE_PAGE_ID_INDEX = 'share_aliases_workspace_id_page_id_unique'; export interface ResolvedAliasTarget { @@ -171,11 +171,23 @@ export class ShareAliasService { ) { throw err; } + // The row we read was deleted (concurrent `removeAlias`) before our UPDATE + // matched it, so `executeTakeFirstOrThrow` found no row. Surface a + // retryable conflict instead of a 200-without-alias (swap branch) or a + // generic 400 from dereferencing `undefined.id` (rename branch). + if (err instanceof NoResultError) { + this.logger.warn( + 'share alias update matched no row (concurrent-delete race)', + ); + throw new ConflictException({ + message: 'The address changed concurrently, please retry', + code: 'ALIAS_PAGE_RACE', + }); + } // A unique index fired. Which one decides the message — always log the // constraint so the race is diagnosable. - if (err?.code === PG_UNIQUE_VIOLATION) { - const constraint: string | undefined = - err?.constraint_name ?? err?.constraint; + if (isUniqueViolation(err)) { + const constraint = violatedConstraint(err); this.logger.warn( `share alias unique violation on ${constraint ?? ''}`, ); @@ -189,13 +201,8 @@ export class ShareAliasService { code: 'ALIAS_PAGE_RACE', }); } - // `(workspace_id, alias)` (UNIQUE_ALIAS_INDEX) or any other/unknown - // unique index: treat as the vanity name being claimed first. - if (constraint && constraint !== UNIQUE_ALIAS_INDEX) { - this.logger.warn( - `unexpected unique index ${constraint} mapped to "Alias already taken"`, - ); - } + // `(workspace_id, alias)` or any other/unknown unique index: treat as + // the vanity name being claimed first. throw new ConflictException({ message: 'Alias already taken' }); } this.logger.error(err); diff --git a/apps/server/src/database/repos/favorite/favorite.repo.ts b/apps/server/src/database/repos/favorite/favorite.repo.ts index 54b21135..8f152e60 100644 --- a/apps/server/src/database/repos/favorite/favorite.repo.ts +++ b/apps/server/src/database/repos/favorite/favorite.repo.ts @@ -7,7 +7,7 @@ import { executeWithCursorPagination } from '@docmost/db/pagination/cursor-pagin import { jsonObjectFrom } from 'kysely/helpers/postgres'; import { ExpressionBuilder, SelectQueryBuilder, sql } from 'kysely'; import { DB } from '@docmost/db/types/db'; -import { dbOrTx } from '@docmost/db/utils'; +import { dbOrTx, isUniqueViolation } from '@docmost/db/utils'; export const FavoriteType = { PAGE: 'page', @@ -29,7 +29,8 @@ export class FavoriteRepo { .returningAll() .executeTakeFirst(); } catch (err: any) { - if (err?.code === '23505') return undefined; + // Idempotent favorite: a duplicate (already-favorited) is not an error. + if (isUniqueViolation(err)) return undefined; throw err; } } diff --git a/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts b/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts index 505ba9b5..42f296a7 100644 --- a/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts +++ b/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts @@ -94,7 +94,9 @@ describe('ShareAliasRepo', () => { return builder; }), returning: jest.fn(() => builder), - executeTakeFirst: jest.fn().mockResolvedValue({ id: 'a-1' }), + // Retarget uses executeTakeFirstOrThrow so a row reaped by a concurrent + // delete (0 rows matched) raises NoResultError instead of returning undefined. + executeTakeFirstOrThrow: jest.fn().mockResolvedValue({ id: 'a-1' }), }; const db = { updateTable: jest.fn(() => builder) } as unknown as KyselyDB; const repo = new ShareAliasRepo(db); @@ -121,7 +123,11 @@ describe('ShareAliasRepo', () => { return builder; }), returning: jest.fn(() => builder), - executeTakeFirst: jest.fn().mockResolvedValue({ id: 'a-1', alias: 'ted' }), + // Rename uses executeTakeFirstOrThrow so a row reaped by a concurrent + // delete (0 rows matched) raises NoResultError instead of returning undefined. + executeTakeFirstOrThrow: jest + .fn() + .mockResolvedValue({ id: 'a-1', alias: 'ted' }), }; const db = { updateTable: jest.fn(() => builder) } as unknown as KyselyDB; const repo = new ShareAliasRepo(db); diff --git a/apps/server/src/database/repos/share-alias/share-alias.repo.ts b/apps/server/src/database/repos/share-alias/share-alias.repo.ts index 39c94a81..dedea832 100644 --- a/apps/server/src/database/repos/share-alias/share-alias.repo.ts +++ b/apps/server/src/database/repos/share-alias/share-alias.repo.ts @@ -92,6 +92,12 @@ export class ShareAliasRepo { * Rename an existing alias row in place (the vanity-slug edit, e.g. * `te` -> `ted`). Keeps the row's id/page_id/creator so the page's single * alias pointer is preserved — only the human-readable name changes. + * + * Uses `executeTakeFirstOrThrow`: if a concurrent `delete` reaps this row + * between the service's read and this UPDATE (READ COMMITTED), the UPDATE + * matches 0 rows and kysely throws `NoResultError` rather than returning + * `undefined` for a `Promise`. The service maps that to a + * retryable conflict instead of dereferencing `undefined.id`. */ async updateAlias( id: string, @@ -105,7 +111,7 @@ export class ShareAliasRepo { .where('id', '=', id) .where('workspaceId', '=', workspaceId) .returning(this.baseFields) - .executeTakeFirst(); + .executeTakeFirstOrThrow(); } /** @@ -127,7 +133,15 @@ export class ShareAliasRepo { .execute(); } - /** Retarget an existing alias to a new page (the "swap" operation). */ + /** + * Retarget an existing alias to a new page (the "swap" operation). + * + * Uses `executeTakeFirstOrThrow`: if a concurrent `delete` reaps this row + * between the service's read and this UPDATE, the UPDATE matches 0 rows and + * kysely throws `NoResultError` instead of returning `undefined` into the 200 + * response (a "success" with no alias). The service maps that to a retryable + * conflict. + */ async updatePageId( id: string, pageId: string, @@ -140,7 +154,7 @@ export class ShareAliasRepo { .where('id', '=', id) .where('workspaceId', '=', workspaceId) .returning(this.baseFields) - .executeTakeFirst(); + .executeTakeFirstOrThrow(); } async delete( diff --git a/apps/server/src/database/unique-violation.spec.ts b/apps/server/src/database/unique-violation.spec.ts new file mode 100644 index 00000000..2567942a --- /dev/null +++ b/apps/server/src/database/unique-violation.spec.ts @@ -0,0 +1,51 @@ +import { isUniqueViolation, violatedConstraint } from './utils'; + +/** + * Unit tests for the driver-bound Postgres unique-violation helpers extracted + * from the share-alias service (and now shared with favorite.repo). They encode + * two `kysely-postgres-js` / `postgres@3.x` quirks: the SQLSTATE is the string + * `'23505'`, and the violated index name arrives as `constraint_name` (with + * `constraint` only a fallback for other drivers). + */ +describe('isUniqueViolation', () => { + it('is true for a 23505 error', () => { + expect(isUniqueViolation({ code: '23505' })).toBe(true); + }); + + it('is false for any other code', () => { + expect(isUniqueViolation({ code: '08006' })).toBe(false); + }); + + it('is false when there is no code / not an object', () => { + expect(isUniqueViolation({})).toBe(false); + expect(isUniqueViolation(null)).toBe(false); + expect(isUniqueViolation(undefined)).toBe(false); + expect(isUniqueViolation(new Error('boom'))).toBe(false); + }); +}); + +describe('violatedConstraint', () => { + it('reads the postgres@3.x `constraint_name` field', () => { + expect( + violatedConstraint({ code: '23505', constraint_name: 'idx_a' }), + ).toBe('idx_a'); + }); + + it('falls back to `constraint` when `constraint_name` is absent', () => { + expect(violatedConstraint({ code: '23505', constraint: 'idx_b' })).toBe( + 'idx_b', + ); + }); + + it('prefers `constraint_name` over `constraint` when both are present', () => { + expect( + violatedConstraint({ constraint_name: 'idx_a', constraint: 'idx_b' }), + ).toBe('idx_a'); + }); + + it('is undefined when neither field is present', () => { + expect(violatedConstraint({ code: '23505' })).toBeUndefined(); + expect(violatedConstraint(null)).toBeUndefined(); + expect(violatedConstraint(undefined)).toBeUndefined(); + }); +}); diff --git a/apps/server/src/database/utils.ts b/apps/server/src/database/utils.ts index 9ed16cbb..84c3693b 100644 --- a/apps/server/src/database/utils.ts +++ b/apps/server/src/database/utils.ts @@ -33,6 +33,35 @@ export function dbOrTx( } } +/** Postgres `unique_violation` SQLSTATE — raised when a write hits a UNIQUE index. */ +const PG_UNIQUE_VIOLATION = '23505'; + +/** + * Whether `err` is a Postgres unique-violation (SQLSTATE `23505`). THE single + * check so repos/services stop re-hardcoding the magic code. + * + * NOTE (#222): `core/ai-chat/roles/ai-agent-roles.service.ts` still carries its + * own inline `23505` check on a separate, unmerged branch; it should adopt this + * helper (and {@link violatedConstraint}) after #227 lands. + */ +export function isUniqueViolation(err: unknown): boolean { + return (err as { code?: unknown } | null | undefined)?.code === PG_UNIQUE_VIOLATION; +} + +/** + * The name of the UNIQUE index/constraint a `23505` error violated, or + * undefined. The `kysely-postgres-js` / `postgres@3.x` driver surfaces it as + * `err.constraint_name` (NOT `.constraint`); `.constraint` is kept only as a + * defensive fallback for other drivers. + */ +export function violatedConstraint(err: unknown): string | undefined { + const e = err as + | { constraint_name?: string; constraint?: string } + | null + | undefined; + return e?.constraint_name ?? e?.constraint; +} + /** * Bind a JS array/object as a `jsonb` column value, working around a postgres * driver double-encoding quirk. THE single implementation — repos that persist