diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fe9c7f4..daa64545 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 catalog's raw files, baked into the image at build time and set per branch in CI (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) +- **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 This release makes AI chat durable and fast: assistant turns are persisted to 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 } /> 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..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'; /** @@ -7,13 +8,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 +27,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 +56,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 +72,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 +149,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,15 +239,190 @@ describe('ShareAliasService', () => { 'a-1', 'p-1', 'ws-1', + trx, ); + // 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 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); + shareAliasRepo.insert.mockRejectedValue({ code: '08006' }); // connection error + await expect( service.setAlias({ workspaceId: 'ws-1', @@ -150,7 +430,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 b70d6d3e..8b05eabb 100644 --- a/apps/server/src/core/share/share-alias.service.ts +++ b/apps/server/src/core/share/share-alias.service.ts @@ -9,9 +9,24 @@ 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, + isUniqueViolation, + violatedConstraint, +} from '@docmost/db/utils'; +import { NoResultError } from 'kysely'; -/** Postgres unique_violation; the (workspace_id, alias) constraint races here. */ -const PG_UNIQUE_VIOLATION = '23505'; +/** + * 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_PAGE_ID_INDEX = 'share_aliases_workspace_id_page_id_unique'; export interface ResolvedAliasTarget { share: NonNullable< @@ -28,16 +43,30 @@ 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). + * + * 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. @@ -57,48 +86,128 @@ 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 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, + ); } - 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; + } + // 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 (isUniqueViolation(err)) { + const constraint = violatedConstraint(err); + 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)` 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); + 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..141769fb --- /dev/null +++ b/apps/server/src/database/migrations/20260627T120000-share-aliases-one-per-page.ts @@ -0,0 +1,48 @@ +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. + * + * ⚠️ 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 + // 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/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 89ac1e52..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 @@ -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 () => { @@ -85,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); @@ -99,6 +110,60 @@ 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), + // 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); + + 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..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 @@ -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,7 +88,60 @@ export class ShareAliasRepo { .executeTakeFirst(); } - /** Retarget an existing alias to a new page (the "swap" operation). */ + /** + * 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, + 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) + .executeTakeFirstOrThrow(); + } + + /** + * 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). + * + * 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, @@ -92,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 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..b372a996 --- /dev/null +++ b/apps/server/test/integration/share-alias-one-per-page.int-spec.ts @@ -0,0 +1,344 @@ +import { Kysely, sql } from 'kysely'; +import { randomUUID } from 'node:crypto'; +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, + 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 aliasRowsForWs = (pageId: string, workspaceId: string) => + db + .selectFrom('shareAliases') + .select(['id', 'alias']) + .where('pageId', '=', pageId) + .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 }); + + 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('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; + 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('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(); + 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'); + }); + + 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); + }); +});