fix: make breadcrumb ordering deterministic

This commit is contained in:
Philipinho
2026-09-01 22:47:05 +01:00
parent 8e35bd0a62
commit fbf87df0ca
2 changed files with 66 additions and 7 deletions
@@ -873,6 +873,7 @@ export class PageService {
'deletedAt', 'deletedAt',
sql<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'), sql<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'),
sql<boolean>`false`.as('isCycle'), sql<boolean>`false`.as('isCycle'),
sql<number>`0`.as('traversalDepth'),
]) ])
.where('id', '=', childPageId) .where('id', '=', childPageId)
.where('deletedAt', 'is', null) .where('deletedAt', 'is', null)
@@ -891,6 +892,7 @@ export class PageService {
'p.deletedAt', 'p.deletedAt',
sql<string[]>`pa.traversal_path || p.id`.as('traversalPath'), sql<string[]>`pa.traversal_path || p.id`.as('traversalPath'),
sql<boolean>`p.id = ANY(pa.traversal_path)`.as('isCycle'), sql<boolean>`p.id = ANY(pa.traversal_path)`.as('isCycle'),
sql<number>`pa.traversal_depth + 1`.as('traversalDepth'),
]) ])
.innerJoin('page_ancestors as pa', 'pa.parentPageId', 'p.id') .innerJoin('page_ancestors as pa', 'pa.parentPageId', 'p.id')
.where('p.deletedAt', 'is', null) .where('p.deletedAt', 'is', null)
@@ -921,11 +923,12 @@ export class PageService {
) )
.as('hasChildren'), .as('hasChildren'),
) )
.orderBy('traversalDepth', 'desc')
.execute(); .execute();
assertAcyclicPageTraversal(ancestors, childPageId); assertAcyclicPageTraversal(ancestors, childPageId);
return ancestors.reverse().map(stripPageTraversalMetadata); return ancestors.map(stripPageTraversalMetadata);
} }
async getRecentSpacePages( async getRecentSpacePages(
@@ -84,8 +84,12 @@ describe('cycle-safe page hierarchy reads', () => {
await withStatementTimeout(async (connection) => { await withStatementTimeout(async (connection) => {
const pageService = createPageService(connection); const pageService = createPageService(connection);
const breadcrumbs = pageService.getPageBreadCrumbs(self.id);
await expect(pageService.getPageBreadCrumbs(self.id)).rejects.toEqual( await expect(breadcrumbs).rejects.toBeInstanceOf(
PageHierarchyCycleError,
);
await expect(breadcrumbs).rejects.toEqual(
expect.objectContaining({ expect.objectContaining({
code: 'PAGE_HIERARCHY_CYCLE', code: 'PAGE_HIERARCHY_CYCLE',
rootPageId: self.id, rootPageId: self.id,
@@ -99,8 +103,12 @@ describe('cycle-safe page hierarchy reads', () => {
await withStatementTimeout(async (connection) => { await withStatementTimeout(async (connection) => {
const pageService = createPageService(connection); const pageService = createPageService(connection);
const breadcrumbs = pageService.getPageBreadCrumbs(a.id);
await expect(pageService.getPageBreadCrumbs(a.id)).rejects.toEqual( await expect(breadcrumbs).rejects.toBeInstanceOf(
PageHierarchyCycleError,
);
await expect(breadcrumbs).rejects.toEqual(
expect.objectContaining({ expect.objectContaining({
code: 'PAGE_HIERARCHY_CYCLE', code: 'PAGE_HIERARCHY_CYCLE',
rootPageId: a.id, rootPageId: a.id,
@@ -119,8 +127,9 @@ describe('cycle-safe page hierarchy reads', () => {
.where('id', '=', root.id) .where('id', '=', root.id)
.executeTakeFirstOrThrow(); .executeTakeFirstOrThrow();
let descendantId = grandchild.id; let descendantId = grandchild.id;
let nearerSharedAncestorId: string;
for (let depth = 3; depth <= 26; depth += 1) { for (let depth = 3; depth <= 52; depth += 1) {
const descendant = await db const descendant = await db
.insertInto('pages') .insertInto('pages')
.values({ .values({
@@ -133,9 +142,13 @@ describe('cycle-safe page hierarchy reads', () => {
.returning('id') .returning('id')
.executeTakeFirstOrThrow(); .executeTakeFirstOrThrow();
descendantId = descendant.id; descendantId = descendant.id;
if (depth === 26) {
nearerSharedAncestorId = descendant.id;
}
} }
const storedShare = await insertShare(root.id, true); const fartherShare = await insertShare(root.id, true);
const nearerShare = await insertShare(nearerSharedAncestorId, true);
const shareService = createShareService(db); const shareService = createShareService(db);
const share = await shareService.getShareForPage( const share = await shareService.getShareForPage(
@@ -145,11 +158,54 @@ describe('cycle-safe page hierarchy reads', () => {
expect(share).toEqual( expect(share).toEqual(
expect.objectContaining({ expect.objectContaining({
id: storedShare.id, id: nearerShare.id,
pageId: root.id, pageId: nearerSharedAncestorId,
level: 26, level: 26,
}), }),
); );
expect(share.id).not.toBe(fartherShare.id);
});
it('returns a direct share without traversing corrupt parents', async () => {
const { self } = await seedSelfCycle();
const storedShare = await insertShare(self.id, false);
await withStatementTimeout(async (connection) => {
const shareService = createShareService(connection);
await expect(
shareService.getShareForPage(self.id, storedShare.workspaceId),
).resolves.toEqual(
expect.objectContaining({
id: storedShare.id,
pageId: self.id,
level: 0,
}),
);
});
});
it('rejects a share from a different workspace', async () => {
const { root } = await seedAcyclicPageChain();
await insertShare(root.id, true);
const shareService = createShareService(db);
await expect(
shareService.getShareForPage(root.id, randomUUID()),
).resolves.toBeUndefined();
});
it('rejects an inherited share that excludes subpages', async () => {
const { root, grandchild } = await seedAcyclicPageChain();
const storedShare = await insertShare(root.id, false);
const shareService = createShareService(db);
await expect(
shareService.getShareForPage(
grandchild.id,
storedShare.workspaceId,
),
).resolves.toBeUndefined();
}); });
it('returns undefined for a cyclic chain with no reachable share', async () => { it('returns undefined for a cyclic chain with no reachable share', async () => {