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); + }); });