Merge pull request 'fix(share): custom address edit renames in place instead of duplicating (#226)' (#227) from fix/share-alias-rename into develop

Reviewed-on: #227
2026-06-27 03:53:31 +03:00
parent 2a4ef9267e 767ac9e7e2
commit 39113c9dbf
14 changed files with 1239 additions and 70 deletions
@@ -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);
    });
  });

@@ -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/<alias> 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/<old>` 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/<alias> 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 ?? '<unknown>'}`,
+        );
+        // `(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). */