Compare commits
3 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| e609832ae4 | |||
| ee03da4018 | |||
| 28251b1e08 |
@@ -1,3 +1,10 @@
|
||||
export const HISTORY_INTERVAL = 5 * 60 * 1000;
|
||||
export const HISTORY_FAST_INTERVAL = 60 * 1000;
|
||||
export const HISTORY_FAST_THRESHOLD = 5 * 60 * 1000;
|
||||
|
||||
// #348 — debounce window for the per-page RAG re-embed job. Repeated saves
|
||||
// within this window collapse to a single delayed job (coalesced by a stable
|
||||
// jobId), so active editing does not pile up expensive re-embeds (external API
|
||||
// + page_embeddings rewrite, concurrency 1). The worker reads the CURRENT page
|
||||
// state at run time, so the last content within the window wins.
|
||||
export const EMBED_DEBOUNCE_MS = 30 * 1000;
|
||||
|
||||
@@ -431,7 +431,17 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot'
|
||||
it('uses the canonical page.id (not the slugId doc name) for post-store side effects (#260)', async () => {
|
||||
const SLUG = 'slug-1'; // persistedHumanPage.slugId; findById resolves it
|
||||
const document = ydocFor(doc('NEW AGENT CONTENT'));
|
||||
pageRepo.findById.mockResolvedValue(persistedHumanPage('NEW AGENT CONTENT'));
|
||||
// #348 — the transclusion sync now runs only when the new OR the previously
|
||||
// persisted content carries a transclusion-family node. Give the persisted
|
||||
// (old) content a pageEmbed so the sync path is exercised and the #260
|
||||
// UUID-vs-slugId contract asserted below is still verified.
|
||||
pageRepo.findById.mockResolvedValue({
|
||||
...persistedHumanPage('NEW AGENT CONTENT'),
|
||||
content: {
|
||||
type: 'doc',
|
||||
content: [{ type: 'pageEmbed', attrs: { sourcePageId: 'src-1' } }],
|
||||
},
|
||||
});
|
||||
pageHistoryRepo.findPageLastHistory.mockResolvedValue(null);
|
||||
|
||||
// A `page.<slugId>` document name (the bug's smoking gun), agent store over
|
||||
|
||||
@@ -36,6 +36,7 @@ import {
|
||||
import { Page } from '@docmost/db/types/entity.types';
|
||||
import { CollabHistoryService } from '../services/collab-history.service';
|
||||
import {
|
||||
EMBED_DEBOUNCE_MS,
|
||||
HISTORY_FAST_INTERVAL,
|
||||
HISTORY_FAST_THRESHOLD,
|
||||
HISTORY_INTERVAL,
|
||||
@@ -45,6 +46,7 @@ import {
|
||||
observeCollabLoad,
|
||||
observeCollabStore,
|
||||
} from '../../integrations/metrics/metrics.registry';
|
||||
import { hasTransclusionFamilyNodes } from '../../core/page/transclusion/utils/transclusion-prosemirror.util';
|
||||
|
||||
/**
|
||||
* #251 — wire format of the client→server stateless message that signals a
|
||||
@@ -450,7 +452,18 @@ export class PersistenceExtension implements Extension {
|
||||
// Use the canonical page UUID (page.id), not the doc-name id, which may be
|
||||
// a slugId for a `page.<slugId>` doc (#260). The transclusion/reference
|
||||
// syncs write uuid-typed columns, so a slugId here threw Postgres 22P02.
|
||||
await this.syncTransclusion(page.id, page.workspaceId, tiptapJson);
|
||||
//
|
||||
// #348 — skip the three sync SELECTs when neither the new content nor the
|
||||
// previously-persisted content has any transclusion/reference/pageEmbed
|
||||
// node: nothing to insert, and (the DB mirrors the old content) nothing to
|
||||
// delete. Whenever either side has one, run the idempotent sync exactly as
|
||||
// before so removals are still reconciled.
|
||||
if (
|
||||
hasTransclusionFamilyNodes(tiptapJson) ||
|
||||
hasTransclusionFamilyNodes(page.content)
|
||||
) {
|
||||
await this.syncTransclusion(page.id, page.workspaceId, tiptapJson);
|
||||
}
|
||||
}
|
||||
|
||||
if (page) {
|
||||
@@ -466,7 +479,17 @@ export class PersistenceExtension implements Extension {
|
||||
(m) => m.entityId,
|
||||
);
|
||||
|
||||
if (userMentions.length > 0) {
|
||||
// #348 — only enqueue when the mentioned-user set actually GAINED a member.
|
||||
// The processor (processPageMention) already no-ops when every current
|
||||
// mention was present before (newMentions.length === 0), so skipping the
|
||||
// enqueue in that case is behavior-identical and avoids piling up no-op jobs
|
||||
// on every save of a page that merely CONTAINS (unchanged) mentions.
|
||||
const oldMentionedUserIdSet = new Set(oldMentionedUserIds);
|
||||
const hasNewMentionedUser = userMentions.some(
|
||||
(m) => !oldMentionedUserIdSet.has(m.entityId),
|
||||
);
|
||||
|
||||
if (hasNewMentionedUser) {
|
||||
await this.notificationQueue.add(QueueJob.PAGE_MENTION_NOTIFICATION, {
|
||||
userMentions: userMentions.map((m) => ({
|
||||
userId: m.entityId,
|
||||
@@ -481,12 +504,23 @@ export class PersistenceExtension implements Extension {
|
||||
} as IPageMentionNotificationJob);
|
||||
}
|
||||
|
||||
await this.aiQueue.add(QueueJob.PAGE_CONTENT_UPDATED, {
|
||||
// Canonical UUID: the embedding reindex resolves pages by uuid, so a
|
||||
// slugId here threw Postgres 22P02 invalid-uuid (#260).
|
||||
pageIds: [page.id],
|
||||
workspaceId: page.workspaceId,
|
||||
});
|
||||
await this.aiQueue.add(
|
||||
QueueJob.PAGE_CONTENT_UPDATED,
|
||||
{
|
||||
// Canonical UUID: the embedding reindex resolves pages by uuid, so a
|
||||
// slugId here threw Postgres 22P02 invalid-uuid (#260).
|
||||
pageIds: [page.id],
|
||||
workspaceId: page.workspaceId,
|
||||
},
|
||||
// #348 — coalesce re-embeds during active editing. A stable per-page
|
||||
// jobId + delay means repeated saves within EMBED_DEBOUNCE_MS collapse
|
||||
// to one delayed job instead of one expensive re-embed per save. The
|
||||
// worker reads the current page state at run time, so last content wins.
|
||||
// BullMQ forbids ':' in custom job ids (Redis key separator), so '-' is
|
||||
// used; page.id is a UUID, so the id is unique per page. removeOnComplete
|
||||
// (queue.module) frees the id after each run so the next window re-arms.
|
||||
{ jobId: `embed-${page.id}`, delay: EMBED_DEBOUNCE_MS },
|
||||
);
|
||||
|
||||
await this.enqueuePageHistory(page, lastUpdatedSource);
|
||||
}
|
||||
|
||||
@@ -220,6 +220,13 @@ export class RedisSyncExtension<TCE extends CustomEvents> implements Extension {
|
||||
};
|
||||
|
||||
async maintainLock(documentName: string) {
|
||||
// #348 — clear any existing timer for this document before installing a new
|
||||
// one. Without this, a second maintainLock for the same document (a
|
||||
// reload-without-unload) overwrites this.locks[documentName] and leaks the
|
||||
// previous interval, which keeps firing SET forever with no way to clear it.
|
||||
if (this.locks[documentName]) {
|
||||
clearInterval(this.locks[documentName]);
|
||||
}
|
||||
this.locks[documentName] = setInterval(() => {
|
||||
this.pub.set(
|
||||
this.getKey(documentName),
|
||||
|
||||
@@ -4,8 +4,21 @@ export const CacheKey = {
|
||||
`perm:space-roles:${userId}:${spaceId}`,
|
||||
PAGE_CAN_EDIT: (userId: string, pageId: string) =>
|
||||
`perm:can-edit:${userId}:${pageId}`,
|
||||
// #348 — DomainMiddleware workspace resolution. Self-hosted resolves the single
|
||||
// workspace (constant key); cloud resolves by the request subdomain (lowercased
|
||||
// to match the case-insensitive `LOWER(hostname)` lookup). Every WorkspaceRepo
|
||||
// mutator busts these, so staleness is bounded by both explicit invalidation and
|
||||
// the short TTL below.
|
||||
WORKSPACE_SELF_HOSTED: 'workspace:self-hosted',
|
||||
WORKSPACE_BY_HOST: (subdomain: string) =>
|
||||
`workspace:byhost:${subdomain.toLowerCase()}`,
|
||||
};
|
||||
|
||||
// Permission caches dedupe repeated checks within and across short request bursts.
|
||||
// 5s keeps staleness on revocations bounded.
|
||||
export const PERMISSION_CACHE_TTL_MS = 5_000;
|
||||
|
||||
// #348 — workspace row changes rarely; a short TTL bounds staleness of
|
||||
// security-relevant fields (enforceSso/enforceMfa/status) even if an explicit
|
||||
// bust is ever missed, while still removing the per-request workspace query.
|
||||
export const WORKSPACE_CACHE_TTL_MS = 15_000;
|
||||
|
||||
@@ -1,13 +1,42 @@
|
||||
import { Injectable, NestMiddleware, NotFoundException } from '@nestjs/common';
|
||||
import { Inject, Injectable, NestMiddleware } from '@nestjs/common';
|
||||
import { FastifyRequest, FastifyReply } from 'fastify';
|
||||
import { CACHE_MANAGER } from '@nestjs/cache-manager';
|
||||
import { Cache } from 'cache-manager';
|
||||
import { EnvironmentService } from '../../integrations/environment/environment.service';
|
||||
import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo';
|
||||
import { Workspace } from '@docmost/db/types/entity.types';
|
||||
import { withCache } from '../helpers/with-cache';
|
||||
import { CacheKey, WORKSPACE_CACHE_TTL_MS } from '../helpers/cache-keys';
|
||||
|
||||
// #348 — timestamptz columns on the workspace row. The cache store (Keyv/Redis)
|
||||
// JSON-serializes values, so a cached workspace comes back with these fields as
|
||||
// ISO strings. Reviving them to Date keeps the cached path byte-identical to the
|
||||
// direct DB path (postgres.js returns Date), so nothing downstream can observe a
|
||||
// cache hit vs miss. Idempotent: `new Date(date)` on an already-Date value is a
|
||||
// no-op-equivalent. Keep in sync with the workspace timestamptz columns.
|
||||
const WORKSPACE_DATE_FIELDS: Array<keyof Workspace> = [
|
||||
'createdAt',
|
||||
'updatedAt',
|
||||
'deletedAt',
|
||||
'trialEndAt',
|
||||
];
|
||||
|
||||
function reviveWorkspaceDates(workspace: Workspace): Workspace {
|
||||
for (const field of WORKSPACE_DATE_FIELDS) {
|
||||
const value = workspace[field];
|
||||
if (value != null) {
|
||||
(workspace as any)[field] = new Date(value as any);
|
||||
}
|
||||
}
|
||||
return workspace;
|
||||
}
|
||||
|
||||
@Injectable()
|
||||
export class DomainMiddleware implements NestMiddleware {
|
||||
constructor(
|
||||
private workspaceRepo: WorkspaceRepo,
|
||||
private environmentService: EnvironmentService,
|
||||
@Inject(CACHE_MANAGER) private readonly cacheManager: Cache,
|
||||
) {}
|
||||
async use(
|
||||
req: FastifyRequest['raw'],
|
||||
@@ -15,13 +44,21 @@ export class DomainMiddleware implements NestMiddleware {
|
||||
next: () => void,
|
||||
) {
|
||||
if (this.environmentService.isSelfHosted()) {
|
||||
const workspace = await this.workspaceRepo.findFirst();
|
||||
// #348 — cache the single-workspace lookup that runs on every request.
|
||||
// Invalidated by every WorkspaceRepo mutator (see bustWorkspaceCache).
|
||||
const workspace = await withCache(
|
||||
this.cacheManager,
|
||||
CacheKey.WORKSPACE_SELF_HOSTED,
|
||||
WORKSPACE_CACHE_TTL_MS,
|
||||
() => this.workspaceRepo.findFirst(),
|
||||
);
|
||||
if (!workspace) {
|
||||
//throw new NotFoundException('Workspace not found');
|
||||
(req as any).workspaceId = null;
|
||||
return next();
|
||||
}
|
||||
|
||||
reviveWorkspaceDates(workspace);
|
||||
// TODO: unify
|
||||
(req as any).workspaceId = workspace.id;
|
||||
(req as any).workspace = workspace;
|
||||
@@ -29,13 +66,21 @@ export class DomainMiddleware implements NestMiddleware {
|
||||
const header = req.headers.host;
|
||||
const subdomain = header.split('.')[0];
|
||||
|
||||
const workspace = await this.workspaceRepo.findByHostname(subdomain);
|
||||
// #348 — cache per-subdomain workspace resolution. Keyed by subdomain (the
|
||||
// hostname column); busted per hostname by every WorkspaceRepo mutator.
|
||||
const workspace = await withCache(
|
||||
this.cacheManager,
|
||||
CacheKey.WORKSPACE_BY_HOST(subdomain),
|
||||
WORKSPACE_CACHE_TTL_MS,
|
||||
() => this.workspaceRepo.findByHostname(subdomain),
|
||||
);
|
||||
|
||||
if (!workspace) {
|
||||
(req as any).workspaceId = null;
|
||||
return next();
|
||||
}
|
||||
|
||||
reviveWorkspaceDates(workspace);
|
||||
(req as any).workspaceId = workspace.id;
|
||||
(req as any).workspace = workspace;
|
||||
}
|
||||
|
||||
@@ -729,35 +729,6 @@ export class AiChatToolsService {
|
||||
}),
|
||||
),
|
||||
|
||||
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
|
||||
// meta.hash in the result is the baseHash drawioUpdate requires.
|
||||
drawioGet: sharedTool(
|
||||
sharedToolSpecs.drawioGet,
|
||||
async ({ pageId, node, format }) =>
|
||||
await client.drawioGet(pageId, node, format ?? 'xml'),
|
||||
),
|
||||
|
||||
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
|
||||
// The flat schema fields are regrouped into the client's `where` object.
|
||||
drawioCreate: sharedTool(
|
||||
sharedToolSpecs.drawioCreate,
|
||||
async ({ pageId, xml, position, anchorNodeId, anchorText, title }) =>
|
||||
await client.drawioCreate(
|
||||
pageId,
|
||||
{ position, anchorNodeId, anchorText },
|
||||
xml,
|
||||
title,
|
||||
),
|
||||
),
|
||||
|
||||
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
|
||||
// baseHash is the optimistic lock: mismatch => structured conflict error.
|
||||
drawioUpdate: sharedTool(
|
||||
sharedToolSpecs.drawioUpdate,
|
||||
async ({ pageId, node, xml, baseHash }) =>
|
||||
await client.drawioUpdate(pageId, node, xml, baseHash),
|
||||
),
|
||||
|
||||
// Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294).
|
||||
// The table reference parameter was unified to `table` (was `tableRef`).
|
||||
tableInsertRow: sharedTool(
|
||||
|
||||
@@ -168,32 +168,6 @@ export interface DocmostClientLike {
|
||||
url: string,
|
||||
opts?: { align?: 'left' | 'center' | 'right'; alt?: string },
|
||||
): Promise<Record<string, unknown>>;
|
||||
// --- draw.io diagrams (#423, stage 1) ---
|
||||
// Read a diagram as decoded mxGraph XML (default) or the raw .drawio.svg.
|
||||
// meta.hash is the optimistic-lock key drawioUpdate expects as baseHash.
|
||||
drawioGet(
|
||||
pageId: string,
|
||||
node: string,
|
||||
format?: 'xml' | 'svg',
|
||||
): Promise<Record<string, unknown>>;
|
||||
// Lint mxGraph XML, build the .drawio.svg attachment and insert a drawio node.
|
||||
drawioCreate(
|
||||
pageId: string,
|
||||
where: {
|
||||
position: 'before' | 'after' | 'append';
|
||||
anchorNodeId?: string;
|
||||
anchorText?: string;
|
||||
},
|
||||
xml: string,
|
||||
title?: string,
|
||||
): Promise<Record<string, unknown>>;
|
||||
// Optimistic-locked full replacement of a diagram (baseHash from drawioGet).
|
||||
drawioUpdate(
|
||||
pageId: string,
|
||||
node: string,
|
||||
xml: string,
|
||||
baseHash: string,
|
||||
): Promise<Record<string, unknown>>;
|
||||
tableInsertRow(
|
||||
pageId: string,
|
||||
tableRef: string,
|
||||
|
||||
@@ -51,7 +51,21 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
|
||||
throw new UnauthorizedException();
|
||||
}
|
||||
|
||||
const workspace = await this.workspaceRepo.findById(payload.workspaceId);
|
||||
// #348 — reuse the workspace DomainMiddleware already loaded for this request
|
||||
// instead of re-querying it. `validate()` above has confirmed
|
||||
// `req.raw.workspaceId === payload.workspaceId` (or that it is unset), and the
|
||||
// middleware sets `req.raw.workspace` alongside `req.raw.workspaceId` from the
|
||||
// SAME workspace row, so when the ids match this is that row. NOTE it is the
|
||||
// middleware's `selectAll` object (a superset of the fallback `findById` base
|
||||
// fields — it also carries licenseKey/auditRetentionDays); that is harmless
|
||||
// here because every consumer reads this workspace via the AuthWorkspace
|
||||
// decorator, which already preferred `req.raw.workspace` (the selectAll object)
|
||||
// over `req.user.workspace` before this change. Fall back to the query if the
|
||||
// middleware did not populate it (a path that bypasses DomainMiddleware).
|
||||
const workspace =
|
||||
req.raw.workspace && req.raw.workspaceId === payload.workspaceId
|
||||
? req.raw.workspace
|
||||
: await this.workspaceRepo.findById(payload.workspaceId);
|
||||
|
||||
if (!workspace) {
|
||||
throw new UnauthorizedException();
|
||||
|
||||
@@ -5,16 +5,6 @@ import {
|
||||
} from '@nestjs/common';
|
||||
import { CommentService } from './comment.service';
|
||||
import { AuditEvent, AuditResource } from '../../common/events/audit-events';
|
||||
import { QueueJob } from '../../integrations/queue/constants';
|
||||
|
||||
// #399: the resolve/unresolve flip and the ephemeral anchor removal are enqueued
|
||||
// as COMMENT_MARK_UPDATE jobs (off the HTTP path), NOT awaited against the collab
|
||||
// gateway. applyCommentSuggestion (the document TEXT edit) is untouched — it
|
||||
// still runs synchronously via the gateway.
|
||||
const markJob = (generalQueue: any, action: string) =>
|
||||
generalQueue.add.mock.calls.find(
|
||||
(c: any[]) => c[0] === QueueJob.COMMENT_MARK_UPDATE && c[1]?.action === action,
|
||||
);
|
||||
|
||||
/**
|
||||
* Focused coverage for CommentService.applySuggestion (comment.service.ts).
|
||||
@@ -69,7 +59,6 @@ describe('CommentService — applySuggestion', () => {
|
||||
commentRepo,
|
||||
wsService,
|
||||
collaborationGateway,
|
||||
generalQueue,
|
||||
auditService,
|
||||
};
|
||||
}
|
||||
@@ -97,15 +86,9 @@ describe('CommentService — applySuggestion', () => {
|
||||
|
||||
// --- no replies → ephemeral delete branch -------------------------------
|
||||
|
||||
it('applied=true, no replies → replaces text, hard-deletes, enqueues the anchor-mark removal, audits APPLIED, outcome=deleted', async () => {
|
||||
const {
|
||||
service,
|
||||
commentRepo,
|
||||
wsService,
|
||||
collaborationGateway,
|
||||
generalQueue,
|
||||
auditService,
|
||||
} = makeService({ applied: true, currentText: 'new text' });
|
||||
it('applied=true, no replies → replaces text, hard-deletes, strips the anchor mark, audits APPLIED, outcome=deleted', async () => {
|
||||
const { service, commentRepo, wsService, collaborationGateway, auditService } =
|
||||
makeService({ applied: true, currentText: 'new text' });
|
||||
|
||||
const result = await service.applySuggestion(suggestionComment(), user());
|
||||
|
||||
@@ -122,20 +105,12 @@ describe('CommentService — applySuggestion', () => {
|
||||
);
|
||||
|
||||
// Ephemeral: the redundant comment is hard-deleted (atomic-conditional) and
|
||||
// its inline anchor mark removal is ENQUEUED (#399), no longer a sync gateway
|
||||
// call. The gateway was only touched for the applyCommentSuggestion text edit.
|
||||
// its inline anchor mark removed via the deleteCommentMark collab event.
|
||||
expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1');
|
||||
const del = markJob(generalQueue, 'delete');
|
||||
expect(del).toBeDefined();
|
||||
expect(del[1]).toMatchObject({
|
||||
documentName: 'page.page-1',
|
||||
commentId: 'c-1',
|
||||
userId: 'user-1',
|
||||
});
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith(
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'deleteCommentMark',
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
'page.page-1',
|
||||
expect.objectContaining({ commentId: 'c-1', user: expect.any(Object) }),
|
||||
);
|
||||
// No applied stamps are written for a row about to be deleted.
|
||||
expect(appliedPatch(commentRepo)).toBeUndefined();
|
||||
@@ -283,7 +258,7 @@ describe('CommentService — applySuggestion', () => {
|
||||
// The suggested text is already applied to the document, but between the
|
||||
// hasChildren read and the atomic delete a reply landed. The parent must NOT
|
||||
// be hard-deleted (cascade would destroy the reply); resolve the thread.
|
||||
const { service, commentRepo, wsService, generalQueue } =
|
||||
const { service, commentRepo, wsService, collaborationGateway } =
|
||||
makeService({ applied: true, currentText: 'new text' }, false, 0);
|
||||
|
||||
const result = await service.applySuggestion(suggestionComment(), user());
|
||||
@@ -300,8 +275,11 @@ describe('CommentService — applySuggestion', () => {
|
||||
.map((c: any[]) => c[0])
|
||||
.find((p: any) => 'resolvedAt' in p);
|
||||
expect(resolvePatch.resolvedAt).toBeInstanceOf(Date);
|
||||
// The resolve mark is enqueued (#399), not a sync gateway call.
|
||||
expect(markJob(generalQueue, 'resolve')).toBeDefined();
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'resolveCommentMark',
|
||||
'page.page-1',
|
||||
expect.objectContaining({ commentId: 'c-1', resolved: true }),
|
||||
);
|
||||
expect(result.outcome).toBe('resolved');
|
||||
});
|
||||
|
||||
|
||||
@@ -313,15 +313,11 @@ describe('CommentService — behavior', () => {
|
||||
});
|
||||
|
||||
const [patch] = commentRepo.updateComment.mock.calls[0];
|
||||
// #399: resolve/unresolve now also stamps updatedAt (the async mark
|
||||
// worker's race-guard reads it to order out-of-order events). The
|
||||
// resolve-state fields are still cleared to null on unresolve.
|
||||
expect(patch).toMatchObject({
|
||||
expect(patch).toEqual({
|
||||
resolvedAt: null,
|
||||
resolvedById: null,
|
||||
resolvedSource: null,
|
||||
});
|
||||
expect(patch.updatedAt).toBeInstanceOf(Date);
|
||||
});
|
||||
|
||||
it("notifies the author when SOMEONE ELSE resolves their comment", async () => {
|
||||
|
||||
@@ -1,15 +1,6 @@
|
||||
import { BadRequestException } from '@nestjs/common';
|
||||
import { CommentService } from './comment.service';
|
||||
import { AuditEvent, AuditResource } from '../../common/events/audit-events';
|
||||
import { QueueJob } from '../../integrations/queue/constants';
|
||||
|
||||
// #399: the inline comment-mark op (resolve flip / ephemeral-suggestion anchor
|
||||
// removal) is now enqueued as a COMMENT_MARK_UPDATE job instead of being awaited
|
||||
// against the collab gateway on the HTTP path. Find that job by action.
|
||||
const markJob = (generalQueue: any, action: string) =>
|
||||
generalQueue.add.mock.calls.find(
|
||||
(c: any[]) => c[0] === QueueJob.COMMENT_MARK_UPDATE && c[1]?.action === action,
|
||||
);
|
||||
|
||||
/**
|
||||
* Coverage for CommentService.dismissSuggestion (#329). Dismiss ("Не применять")
|
||||
@@ -53,14 +44,7 @@ describe('CommentService — dismissSuggestion', () => {
|
||||
auditService,
|
||||
);
|
||||
|
||||
return {
|
||||
service,
|
||||
commentRepo,
|
||||
wsService,
|
||||
collaborationGateway,
|
||||
generalQueue,
|
||||
auditService,
|
||||
};
|
||||
return { service, commentRepo, wsService, collaborationGateway, auditService };
|
||||
}
|
||||
|
||||
const suggestionComment = (over?: Partial<any>): any => ({
|
||||
@@ -78,30 +62,25 @@ describe('CommentService — dismissSuggestion', () => {
|
||||
});
|
||||
const user = (over?: Partial<any>): any => ({ id: 'user-1', ...over });
|
||||
|
||||
it('no replies → hard-deletes, enqueues the anchor-mark removal, does NOT touch page text, audits DISMISSED, outcome=deleted', async () => {
|
||||
const {
|
||||
service,
|
||||
commentRepo,
|
||||
wsService,
|
||||
collaborationGateway,
|
||||
generalQueue,
|
||||
auditService,
|
||||
} = makeService(false);
|
||||
it('no replies → hard-deletes, strips the anchor mark, does NOT touch page text, audits DISMISSED, outcome=deleted', async () => {
|
||||
const { service, commentRepo, wsService, collaborationGateway, auditService } =
|
||||
makeService(false);
|
||||
|
||||
const result = await service.dismissSuggestion(suggestionComment(), user());
|
||||
|
||||
// Never applies the suggestion to the document (no sync gateway call at all
|
||||
// now — the mark op is off the HTTP path, #399).
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
// Hard-delete (atomic-conditional) + enqueue the anchor-mark strip.
|
||||
// Never applies the suggestion to the document.
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith(
|
||||
'applyCommentSuggestion',
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
);
|
||||
// Hard-delete (atomic-conditional) + strip mark.
|
||||
expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1');
|
||||
const del = markJob(generalQueue, 'delete');
|
||||
expect(del).toBeDefined();
|
||||
expect(del[1]).toMatchObject({
|
||||
documentName: 'page.page-1',
|
||||
commentId: 'c-1',
|
||||
userId: 'user-1',
|
||||
});
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'deleteCommentMark',
|
||||
'page.page-1',
|
||||
expect.objectContaining({ commentId: 'c-1', user: expect.any(Object) }),
|
||||
);
|
||||
expect(wsService.emitCommentEvent).toHaveBeenCalledWith(
|
||||
'space-1',
|
||||
'page-1',
|
||||
@@ -117,20 +96,20 @@ describe('CommentService — dismissSuggestion', () => {
|
||||
expect(result.outcome).toBe('deleted');
|
||||
});
|
||||
|
||||
it('no replies → if the anchor-mark ENQUEUE FAILS, the row is NOT deleted and the error propagates (#329/#399: no orphan anchor)', async () => {
|
||||
const { service, commentRepo, wsService, generalQueue } = makeService(false);
|
||||
// #399: the mark removal now runs async in a worker, but the ENQUEUE is
|
||||
// awaited BEFORE the irreversible row delete — so the anchor-removal job is
|
||||
// durably scheduled before the row can vanish. If even the enqueue fails
|
||||
// (e.g. Redis down), the whole operation aborts, leaving row + mark
|
||||
// consistent — never a deleted row with an orphan anchor reporting success.
|
||||
generalQueue.add = jest.fn(async () => {
|
||||
throw new Error('queue add failed: no redis');
|
||||
it('no replies → if the anchor-mark removal FAILS, the row is NOT deleted and the error propagates (#329: no orphan anchor)', async () => {
|
||||
const { service, commentRepo, wsService, collaborationGateway } =
|
||||
makeService(false);
|
||||
// Mark removal is FATAL and runs BEFORE the irreversible row delete: a collab
|
||||
// failure (e.g. COLLAB_DISABLE_REDIS "no live instance") must abort the whole
|
||||
// operation, leaving row + mark consistent — never a deleted row with an
|
||||
// orphan anchor left in the document reporting success.
|
||||
collaborationGateway.handleYjsEvent = jest.fn(async () => {
|
||||
throw new Error('requires a live collaboration instance');
|
||||
});
|
||||
|
||||
await expect(
|
||||
service.dismissSuggestion(suggestionComment(), user()),
|
||||
).rejects.toThrow(/queue add failed/);
|
||||
).rejects.toThrow(/live collaboration/);
|
||||
|
||||
expect(commentRepo.deleteCommentIfChildless).not.toHaveBeenCalled();
|
||||
expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith(
|
||||
@@ -141,29 +120,23 @@ describe('CommentService — dismissSuggestion', () => {
|
||||
});
|
||||
|
||||
it('WITH replies → resolves (not delete), does NOT apply, audits DISMISSED, outcome=resolved', async () => {
|
||||
const {
|
||||
service,
|
||||
commentRepo,
|
||||
collaborationGateway,
|
||||
generalQueue,
|
||||
auditService,
|
||||
} = makeService(true);
|
||||
const { service, commentRepo, wsService, collaborationGateway, auditService } =
|
||||
makeService(true);
|
||||
|
||||
const result = await service.dismissSuggestion(suggestionComment(), user());
|
||||
|
||||
// Resolved via resolveComment (resolve patch + enqueued resolve mark), NOT
|
||||
// deleted.
|
||||
// Resolved via resolveComment (resolve patch + resolve mark), NOT deleted.
|
||||
const resolvePatch = commentRepo.updateComment.mock.calls
|
||||
.map((c: any[]) => c[0])
|
||||
.find((p: any) => 'resolvedAt' in p);
|
||||
expect(resolvePatch.resolvedAt).toBeInstanceOf(Date);
|
||||
expect(resolvePatch.resolvedById).toBe('user-1');
|
||||
expect(commentRepo.deleteComment).not.toHaveBeenCalled();
|
||||
// No sync gateway call; the resolve mark is enqueued (#399).
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
const res = markJob(generalQueue, 'resolve');
|
||||
expect(res).toBeDefined();
|
||||
expect(res[1]).toMatchObject({ documentName: 'page.page-1', commentId: 'c-1' });
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'resolveCommentMark',
|
||||
'page.page-1',
|
||||
expect.objectContaining({ commentId: 'c-1', resolved: true }),
|
||||
);
|
||||
// No applied stamp — dismiss does not apply the edit.
|
||||
const appliedPatch = commentRepo.updateComment.mock.calls
|
||||
.map((c: any[]) => c[0])
|
||||
@@ -183,7 +156,8 @@ describe('CommentService — dismissSuggestion', () => {
|
||||
// but the atomic delete matches 0 rows because a reply landed in the window
|
||||
// between that read and the delete. The parent must NOT be hard-deleted
|
||||
// (a cascade would destroy the just-added reply); the thread is resolved.
|
||||
const { service, commentRepo, wsService, generalQueue } = makeService(false, 0);
|
||||
const { service, commentRepo, wsService, collaborationGateway } =
|
||||
makeService(false, 0);
|
||||
|
||||
const result = await service.dismissSuggestion(suggestionComment(), user());
|
||||
|
||||
@@ -201,9 +175,11 @@ describe('CommentService — dismissSuggestion', () => {
|
||||
.find((p: any) => 'resolvedAt' in p);
|
||||
expect(resolvePatch.resolvedAt).toBeInstanceOf(Date);
|
||||
expect(resolvePatch.resolvedById).toBe('user-1');
|
||||
// A resolve mark job is enqueued (the anchor was already delete-marked; the
|
||||
// resolve mirror is idempotent — #399).
|
||||
expect(markJob(generalQueue, 'resolve')).toBeDefined();
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'resolveCommentMark',
|
||||
'page.page-1',
|
||||
expect.objectContaining({ commentId: 'c-1', resolved: true }),
|
||||
);
|
||||
expect(result.outcome).toBe('resolved');
|
||||
});
|
||||
|
||||
|
||||
@@ -1,179 +0,0 @@
|
||||
import { Logger } from '@nestjs/common';
|
||||
import { CommentService } from './comment.service';
|
||||
import { QueueJob } from '../../integrations/queue/constants';
|
||||
|
||||
// Flush pending microtasks so a fire-and-forget `.catch(...)` runs before we assert.
|
||||
const flushMicrotasks = () => new Promise((r) => setImmediate(r));
|
||||
|
||||
/**
|
||||
* #399: the comment inline-mark update is moved OFF the HTTP critical path.
|
||||
* resolveComment / unresolve / the ephemeral-suggestion delete must NO LONGER
|
||||
* await CollaborationGateway.handleYjsEvent (which loaded the whole Y.Doc and
|
||||
* ran the store pipeline synchronously, ~4.5s p95). Instead they enqueue an
|
||||
* idempotent COMMENT_MARK_UPDATE job onto the GENERAL_QUEUE with the payload the
|
||||
* worker replays.
|
||||
*
|
||||
* The service is constructed directly with jest mocks (the @InjectQueue tokens
|
||||
* cannot be resolved by Test.createTestingModule — see comment.service.spec.ts).
|
||||
*/
|
||||
describe('CommentService — async comment mark (#399)', () => {
|
||||
function makeService() {
|
||||
const commentRepo: any = {
|
||||
findById: jest.fn(async (id: string) => ({
|
||||
id,
|
||||
content: {},
|
||||
spaceId: 'space-1',
|
||||
pageId: 'page-1',
|
||||
})),
|
||||
updateComment: jest.fn(async () => undefined),
|
||||
hasChildren: jest.fn(async () => false),
|
||||
deleteCommentIfChildless: jest.fn(async () => 1),
|
||||
};
|
||||
const pageRepo: any = {};
|
||||
const wsService: any = { emitCommentEvent: jest.fn() };
|
||||
// The gateway MUST NOT be touched on the HTTP path anymore.
|
||||
const collaborationGateway: any = {
|
||||
handleYjsEvent: jest.fn(async () => undefined),
|
||||
};
|
||||
const generalQueue: any = { add: jest.fn(() => Promise.resolve()) };
|
||||
const notificationQueue: any = { add: jest.fn(async () => undefined) };
|
||||
const auditService: any = { log: jest.fn() };
|
||||
|
||||
const service = new CommentService(
|
||||
commentRepo,
|
||||
pageRepo,
|
||||
wsService,
|
||||
collaborationGateway,
|
||||
generalQueue,
|
||||
notificationQueue,
|
||||
auditService,
|
||||
);
|
||||
return {
|
||||
service,
|
||||
commentRepo,
|
||||
collaborationGateway,
|
||||
generalQueue,
|
||||
auditService,
|
||||
};
|
||||
}
|
||||
|
||||
const comment = (over?: Partial<any>): any => ({
|
||||
id: 'c-1',
|
||||
creatorId: 'user-1',
|
||||
pageId: 'page-1',
|
||||
spaceId: 'space-1',
|
||||
workspaceId: 'ws-1',
|
||||
...over,
|
||||
});
|
||||
const user = (over?: Partial<any>): any => ({ id: 'user-1', ...over });
|
||||
|
||||
const markJob = (generalQueue: any) =>
|
||||
generalQueue.add.mock.calls.find(
|
||||
(c: any[]) => c[0] === QueueJob.COMMENT_MARK_UPDATE,
|
||||
);
|
||||
|
||||
it('resolveComment does NOT call the gateway synchronously, and enqueues a resolve mark job', async () => {
|
||||
const { service, collaborationGateway, generalQueue } = makeService();
|
||||
|
||||
await service.resolveComment(comment(), true, user());
|
||||
|
||||
// The whole point of #399: the Y.Doc mark op is off the HTTP path.
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
|
||||
const job = markJob(generalQueue);
|
||||
expect(job).toBeDefined();
|
||||
expect(job[0]).toBe(QueueJob.COMMENT_MARK_UPDATE);
|
||||
expect(job[1]).toMatchObject({
|
||||
documentName: 'page.page-1',
|
||||
commentId: 'c-1',
|
||||
action: 'resolve',
|
||||
userId: 'user-1',
|
||||
});
|
||||
expect(typeof job[1].ts).toBe('number');
|
||||
// ts equals the resolvedAt stamp written to the row (shared timestamp).
|
||||
const [patch] = (service as any).commentRepo.updateComment.mock.calls[0];
|
||||
expect(job[1].ts).toBe((patch.resolvedAt as Date).getTime());
|
||||
expect(job[1].ts).toBe((patch.updatedAt as Date).getTime());
|
||||
});
|
||||
|
||||
it('unresolve enqueues an unresolve mark job (action mapped from resolved=false)', async () => {
|
||||
const { service, collaborationGateway, generalQueue } = makeService();
|
||||
|
||||
await service.resolveComment(comment(), false, user());
|
||||
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
const job = markJob(generalQueue);
|
||||
expect(job[1]).toMatchObject({
|
||||
documentName: 'page.page-1',
|
||||
commentId: 'c-1',
|
||||
action: 'unresolve',
|
||||
userId: 'user-1',
|
||||
});
|
||||
});
|
||||
|
||||
it('dismissing a childless ephemeral suggestion enqueues a delete mark job (not a sync gateway call)', async () => {
|
||||
const { service, collaborationGateway, generalQueue } = makeService();
|
||||
|
||||
await service.dismissSuggestion(
|
||||
comment({ suggestedText: 'new text', selection: 'old', resolvedAt: null }),
|
||||
user(),
|
||||
);
|
||||
|
||||
// The anchor removal is queued, not awaited against the gateway.
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
const job = markJob(generalQueue);
|
||||
expect(job).toBeDefined();
|
||||
expect(job[1]).toMatchObject({
|
||||
documentName: 'page.page-1',
|
||||
commentId: 'c-1',
|
||||
action: 'delete',
|
||||
userId: 'user-1',
|
||||
});
|
||||
expect(typeof job[1].ts).toBe('number');
|
||||
});
|
||||
|
||||
it('awaits the delete ENQUEUE before the irreversible row hard-delete (ordering preserved)', async () => {
|
||||
const { service, generalQueue, commentRepo } = makeService();
|
||||
const order: string[] = [];
|
||||
generalQueue.add.mockImplementation(async (name: string) => {
|
||||
order.push(`enqueue:${name}`);
|
||||
});
|
||||
commentRepo.deleteCommentIfChildless.mockImplementation(async () => {
|
||||
order.push('delete-row');
|
||||
return 1;
|
||||
});
|
||||
|
||||
await service.dismissSuggestion(
|
||||
comment({ suggestedText: 'new text', selection: 'old', resolvedAt: null }),
|
||||
user(),
|
||||
);
|
||||
|
||||
// The mark-removal job must be durably queued BEFORE the row disappears.
|
||||
expect(order).toEqual([
|
||||
`enqueue:${QueueJob.COMMENT_MARK_UPDATE}`,
|
||||
'delete-row',
|
||||
]);
|
||||
});
|
||||
|
||||
it('resolve is fire-and-forget: a queue-add rejection does NOT fail the HTTP call (best-effort warn)', async () => {
|
||||
const { service, generalQueue } = makeService();
|
||||
// The queue is unavailable — the whole point of #399 is that this must NOT
|
||||
// propagate out of resolveComment onto the HTTP request.
|
||||
const queueErr = new Error('queue is down');
|
||||
generalQueue.add.mockRejectedValue(queueErr);
|
||||
const warnSpy = jest
|
||||
.spyOn(Logger.prototype, 'warn')
|
||||
.mockImplementation(() => undefined);
|
||||
|
||||
// Must resolve, never throw, even though the enqueue rejects.
|
||||
await expect(service.resolveComment(comment(), true, user())).resolves.not.toThrow();
|
||||
|
||||
// The rejection is swallowed on a microtask AFTER the method returns; flush it.
|
||||
await flushMicrotasks();
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Failed to enqueue comment mark update for comment c-1'),
|
||||
queueErr,
|
||||
);
|
||||
warnSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
@@ -21,7 +21,6 @@ import { CursorPaginationResult } from '@docmost/db/pagination/cursor-pagination
|
||||
import { QueueJob, QueueName } from '../../integrations/queue/constants';
|
||||
import { extractUserMentionIdsFromJson } from '../../common/helpers/prosemirror/utils';
|
||||
import {
|
||||
ICommentMarkUpdateJob,
|
||||
ICommentNotificationJob,
|
||||
ICommentResolvedNotificationJob,
|
||||
} from '../../integrations/queue/constants/queue.interface';
|
||||
@@ -299,11 +298,7 @@ export class CommentService {
|
||||
// source is cleared alongside resolvedAt/resolvedById.
|
||||
provenance?: AuthProvenanceData,
|
||||
): Promise<Comment> {
|
||||
// One shared timestamp: it stamps resolvedAt AND updatedAt on the row and is
|
||||
// carried as the mark job's `ts`, so the worker's race-guard can order this
|
||||
// event against the row's authoritative resolve-state mutation time (#399).
|
||||
const now = new Date();
|
||||
const resolvedAt = resolved ? now : null;
|
||||
const resolvedAt = resolved ? new Date() : null;
|
||||
const resolvedById = resolved ? authUser.id : null;
|
||||
const isAgent = provenance?.actor === 'agent';
|
||||
// Set the agent marker only when resolving; on unresolve clear it back to
|
||||
@@ -312,33 +307,25 @@ export class CommentService {
|
||||
const resolvedSource = resolved && isAgent ? 'agent' : null;
|
||||
|
||||
await this.commentRepo.updateComment(
|
||||
// Bump updatedAt (not editedAt — that drives the "edited" badge) so the
|
||||
// row records WHEN the resolve state last changed; the async mark worker
|
||||
// compares its job ts against this to skip a superseded out-of-order event.
|
||||
{ resolvedAt, resolvedById, resolvedSource, updatedAt: now },
|
||||
{ resolvedAt, resolvedById, resolvedSource },
|
||||
comment.id,
|
||||
);
|
||||
|
||||
// #399: mirror the resolved state onto the inline comment mark OFF the HTTP
|
||||
// critical path. The DB row above is the source of truth (updated in ms); the
|
||||
// mark is an eventual mirror for connected clients, and its failure was
|
||||
// ALREADY swallowed (best-effort warn) — so instead of awaiting the whole
|
||||
// Y.Doc load + immediate store pipeline (~4.5s p95), enqueue an idempotent,
|
||||
// retryable COMMENT_MARK_UPDATE job. (Store-pipeline cost itself is #348's
|
||||
// scope, not duplicated here.)
|
||||
// Reflect the resolved state on the inline comment mark in the
|
||||
// collaborative document so all connected clients stay in sync.
|
||||
const documentName = `page.${comment.pageId}`;
|
||||
void this.enqueueCommentMarkUpdate(
|
||||
documentName,
|
||||
comment.id,
|
||||
resolved ? 'resolve' : 'unresolve',
|
||||
now.getTime(),
|
||||
authUser.id,
|
||||
).catch((error) =>
|
||||
try {
|
||||
await this.collaborationGateway.handleYjsEvent(
|
||||
'resolveCommentMark',
|
||||
documentName,
|
||||
{ commentId: comment.id, resolved, user: authUser },
|
||||
);
|
||||
} catch (error) {
|
||||
this.logger.warn(
|
||||
`Failed to enqueue comment mark update for comment ${comment.id}`,
|
||||
`Failed to update comment mark for comment ${comment.id}`,
|
||||
error,
|
||||
),
|
||||
);
|
||||
);
|
||||
}
|
||||
|
||||
// Notify the comment author when someone else resolves their comment.
|
||||
if (resolved && comment.creatorId !== authUser.id) {
|
||||
@@ -684,54 +671,23 @@ export class CommentService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Schedule removal of the inline `comment` anchor mark from the collaborative
|
||||
* document (ephemeral suggestion #329), OFF the HTTP critical path (#399).
|
||||
*
|
||||
* ORDERING PRESERVED: we `await` the ENQUEUE (a fast Redis add), not the mark
|
||||
* op, and the caller only proceeds to the irreversible row hard-delete after
|
||||
* this resolves. So the anchor-removal job is DURABLY queued before the row
|
||||
* vanishes — a queue-add failure throws here and aborts the delete (row + mark
|
||||
* stay consistent), preserving the invariant the old FATAL sync call gave. The
|
||||
* mark op itself now runs async in the worker: it is idempotent and retried
|
||||
* (3 attempts), so a transient collab failure self-heals; only an exhausted-
|
||||
* retries job leaves a DB↔mark divergence, now VISIBLE via BullMQ failed-job
|
||||
* metrics (was a hard 5xx before). Delete carries no state guard — the row is
|
||||
* being removed, and stripping an absent mark is a no-op.
|
||||
* Remove the inline `comment` mark for a comment from the collaborative
|
||||
* document. FATAL, NOT best-effort: unlike resolveComment (which keeps the row,
|
||||
* so a failed mark update is recoverable), this is used before an irreversible
|
||||
* hard-delete, so the mark removal MUST succeed or throw. Under
|
||||
* COLLAB_DISABLE_REDIS the gateway invokes the deleteCommentMark handler
|
||||
* directly (never a silent no-op) and a missing live instance surfaces as a
|
||||
* thrown error, which we let propagate so the caller aborts before deleting.
|
||||
*/
|
||||
private async deleteCommentMark(comment: Comment, user: User): Promise<void> {
|
||||
const documentName = `page.${comment.pageId}`;
|
||||
await this.enqueueCommentMarkUpdate(
|
||||
await this.collaborationGateway.handleYjsEvent(
|
||||
'deleteCommentMark',
|
||||
documentName,
|
||||
comment.id,
|
||||
'delete',
|
||||
Date.now(),
|
||||
user.id,
|
||||
{ commentId: comment.id, user },
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Enqueue an idempotent COMMENT_MARK_UPDATE job (#399) — the single path that
|
||||
* mirrors a comment's inline-mark state into the collab Y.Doc off the HTTP
|
||||
* response. The worker (GeneralQueueProcessor) runs the SAME handleYjsEvent
|
||||
* the sync code used, so the mark op is byte-identical.
|
||||
*/
|
||||
private enqueueCommentMarkUpdate(
|
||||
documentName: string,
|
||||
commentId: string,
|
||||
action: 'resolve' | 'unresolve' | 'delete',
|
||||
ts: number,
|
||||
userId: string,
|
||||
): Promise<unknown> {
|
||||
const jobData: ICommentMarkUpdateJob = {
|
||||
documentName,
|
||||
commentId,
|
||||
action,
|
||||
ts,
|
||||
userId,
|
||||
};
|
||||
return this.generalQueue.add(QueueJob.COMMENT_MARK_UPDATE, jobData);
|
||||
}
|
||||
|
||||
private async queueCommentNotification(
|
||||
content: any,
|
||||
oldMentionIds: string[],
|
||||
|
||||
@@ -38,6 +38,8 @@ export class FavoriteService {
|
||||
await this.pagePermissionRepo.filterAccessiblePageIds({
|
||||
pageIds: result.items,
|
||||
userId,
|
||||
// #348 — favorites load at app-start; enable the workspace short-circuit.
|
||||
workspaceId,
|
||||
});
|
||||
const accessibleSet = new Set(accessibleIds);
|
||||
result.items = result.items.filter((id) => accessibleSet.has(id));
|
||||
@@ -125,6 +127,8 @@ export class FavoriteService {
|
||||
await this.pagePermissionRepo.filterAccessiblePageIds({
|
||||
pageIds,
|
||||
userId,
|
||||
// #348 — workspace-level short-circuit for the favorites list.
|
||||
workspaceId,
|
||||
});
|
||||
accessiblePageSet = new Set(accessibleIds);
|
||||
}
|
||||
|
||||
@@ -23,7 +23,12 @@ export class NotificationController {
|
||||
@Body() dto: ListNotificationsDto,
|
||||
@AuthUser() user: User,
|
||||
) {
|
||||
return this.notificationService.findByUserId(user.id, dto, dto.type);
|
||||
return this.notificationService.findByUserId(
|
||||
user.id,
|
||||
dto,
|
||||
dto.type,
|
||||
user.workspaceId,
|
||||
);
|
||||
}
|
||||
|
||||
@HttpCode(HttpStatus.OK)
|
||||
|
||||
@@ -45,6 +45,7 @@ export class NotificationService {
|
||||
userId: string,
|
||||
pagination: PaginationOptions,
|
||||
type: NotificationTab = 'all',
|
||||
workspaceId?: string | null,
|
||||
) {
|
||||
const result = await this.notificationRepo.findByUserId(
|
||||
userId,
|
||||
@@ -61,6 +62,8 @@ export class NotificationService {
|
||||
await this.pagePermissionRepo.filterAccessiblePageIds({
|
||||
pageIds,
|
||||
userId,
|
||||
// #348 — notifications list; enable the workspace short-circuit.
|
||||
workspaceId,
|
||||
});
|
||||
const accessibleSet = new Set(accessiblePageIds);
|
||||
|
||||
|
||||
@@ -446,7 +446,11 @@ export class PageController {
|
||||
);
|
||||
}
|
||||
|
||||
return this.pageService.getRecentPages(user.id, pagination);
|
||||
return this.pageService.getRecentPages(
|
||||
user.id,
|
||||
pagination,
|
||||
user.workspaceId,
|
||||
);
|
||||
}
|
||||
|
||||
@HttpCode(HttpStatus.OK)
|
||||
@@ -469,7 +473,13 @@ export class PageController {
|
||||
}
|
||||
}
|
||||
|
||||
return this.pageService.getCreatedByPages(targetUserId, user.id, pagination, dto.spaceId);
|
||||
return this.pageService.getCreatedByPages(
|
||||
targetUserId,
|
||||
user.id,
|
||||
pagination,
|
||||
dto.spaceId,
|
||||
user.workspaceId,
|
||||
);
|
||||
}
|
||||
|
||||
@HttpCode(HttpStatus.OK)
|
||||
|
||||
@@ -1165,6 +1165,7 @@ export class PageService {
|
||||
async getRecentPages(
|
||||
userId: string,
|
||||
pagination: PaginationOptions,
|
||||
workspaceId?: string | null,
|
||||
): Promise<CursorPaginationResult<Page>> {
|
||||
const result = await this.pageRepo.getRecentPages(userId, pagination);
|
||||
|
||||
@@ -1174,6 +1175,8 @@ export class PageService {
|
||||
await this.pagePermissionRepo.filterAccessiblePageIds({
|
||||
pageIds,
|
||||
userId,
|
||||
// #348 — cross-space "recent"; enable the workspace short-circuit.
|
||||
workspaceId,
|
||||
});
|
||||
const accessibleSet = new Set(accessibleIds);
|
||||
result.items = result.items.filter((p) => accessibleSet.has(p.id));
|
||||
@@ -1187,6 +1190,7 @@ export class PageService {
|
||||
requestingUserId: string,
|
||||
pagination: PaginationOptions,
|
||||
spaceId?: string,
|
||||
workspaceId?: string | null,
|
||||
): Promise<CursorPaginationResult<Page>> {
|
||||
const result = await this.pageRepo.getCreatedByPages(
|
||||
creatorId,
|
||||
@@ -1201,6 +1205,9 @@ export class PageService {
|
||||
await this.pagePermissionRepo.filterAccessiblePageIds({
|
||||
pageIds,
|
||||
userId: requestingUserId,
|
||||
spaceId,
|
||||
// #348 — enable the workspace short-circuit when not space-scoped.
|
||||
workspaceId,
|
||||
});
|
||||
const accessibleSet = new Set(accessibleIds);
|
||||
result.items = result.items.filter((p) => accessibleSet.has(p.id));
|
||||
|
||||
@@ -93,6 +93,41 @@ function collectNodes<T>(
|
||||
return Array.from(byKey.values());
|
||||
}
|
||||
|
||||
/**
|
||||
* #348 — cheap early-exit probe: does this doc contain ANY node the transclusion
|
||||
* syncs care about (`transclusionSource` / `transclusionReference` / `pageEmbed`)?
|
||||
* Lets the collab store skip the three sync SELECTs when neither the previous nor
|
||||
* the new content has any such node — there is nothing to insert, and (since the
|
||||
* DB mirrors the previously-persisted content) nothing to delete. Walks once and
|
||||
* short-circuits on the first match; uses the same depth ceiling as the
|
||||
* collectors. Deliberately does NOT skip `transclusionSource` subtrees: it only
|
||||
* answers "any node present?", so descending everywhere is strictly conservative
|
||||
* (it can never wrongly report "none").
|
||||
*/
|
||||
export function hasTransclusionFamilyNodes(doc: unknown): boolean {
|
||||
const visit = (node: any, depth: number): boolean => {
|
||||
if (!node || typeof node !== 'object') return false;
|
||||
if (depth > MAX_PM_WALK_DEPTH) return false;
|
||||
|
||||
if (
|
||||
node.type === TRANSCLUSION_TYPE ||
|
||||
node.type === REFERENCE_TYPE ||
|
||||
node.type === PAGE_EMBED_TYPE
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (Array.isArray(node.content)) {
|
||||
for (const child of node.content) {
|
||||
if (visit(child, depth + 1)) return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
};
|
||||
|
||||
return visit(doc, 0);
|
||||
}
|
||||
|
||||
/**
|
||||
* Walks a ProseMirror JSON document and returns one snapshot per top-level
|
||||
* `transclusion` node. Does not recurse into transclusions (schema disallows
|
||||
|
||||
@@ -155,6 +155,8 @@ export class SearchService {
|
||||
pageIds,
|
||||
userId: opts.userId,
|
||||
spaceId: searchParams.spaceId,
|
||||
// #348 — enables the workspace-level short-circuit when not space-scoped.
|
||||
workspaceId: opts.workspaceId,
|
||||
});
|
||||
const accessibleSet = new Set(accessibleIds);
|
||||
results = results.filter((r: any) => accessibleSet.has(r.id));
|
||||
@@ -266,6 +268,8 @@ export class SearchService {
|
||||
await this.pagePermissionRepo.filterAccessiblePageIds({
|
||||
pageIds,
|
||||
userId,
|
||||
// #348 — workspace-level short-circuit for the suggest path.
|
||||
workspaceId,
|
||||
});
|
||||
const accessibleSet = new Set(accessibleIds);
|
||||
pages = pages.filter((p) => accessibleSet.has(p.id));
|
||||
|
||||
@@ -0,0 +1,118 @@
|
||||
import { type Kysely, sql } from 'kysely';
|
||||
|
||||
/**
|
||||
* #348 — targeted hot-path indexes.
|
||||
*
|
||||
* 1. GIN trigram indexes for `/search/suggest`. That endpoint runs a
|
||||
* leading-wildcard `LOWER(f_unaccent(col)) LIKE '%q%'` per keystroke, which
|
||||
* is a sequential scan without a trigram index. The index EXPRESSIONS below
|
||||
* are `LOWER(f_unaccent(title|name))`, matching the predicates in
|
||||
* search.service.ts exactly so the planner uses them (verified with EXPLAIN:
|
||||
* the suggest predicate resolves to a Bitmap Index Scan on these indexes).
|
||||
*
|
||||
* IMMUTABLE-wrapper fix (required for the index to build): `f_unaccent` was
|
||||
* defined as `SELECT unaccent('unaccent', $1)` (the two-arg, dictionary-named
|
||||
* unaccent). That body CANNOT be used in an index expression: when Postgres
|
||||
* inlines the IMMUTABLE SQL wrapper while building the index it fails to
|
||||
* resolve the two-arg call (`function unaccent(unknown, text) does not exist`,
|
||||
* the `'unaccent'` literal loses its regdictionary coercion). The single-arg
|
||||
* `unaccent($1)` is the same operation (the default text-search dictionary IS
|
||||
* `unaccent`; verified byte-equal on accented samples), and — crucially —
|
||||
* SCHEMA-QUALIFIED as `public.unaccent($1)` it inlines cleanly, so the index
|
||||
* builds. We therefore `CREATE OR REPLACE` `f_unaccent` to the qualified
|
||||
* single-arg body. This is output-identical for every existing caller (the
|
||||
* tsvector trigger, the main `tsv @@` search, and the suggest LIKE), so no
|
||||
* reindex/backfill is needed; `down()` restores the original two-arg body.
|
||||
* (The `unaccent` extension is installed in `public` in this codebase, which
|
||||
* is why `public.unaccent` is the correct qualification.)
|
||||
*
|
||||
* 2. Composite indexes for two ORDER-BY-only-on-id queries that currently sort
|
||||
* on top of a created_at index:
|
||||
* - page_history: `findPageHistoryByPageId` does WHERE page_id ORDER BY id
|
||||
* DESC, but only `(page_id, created_at DESC)` exists → extra sort.
|
||||
* - comments: `findPageComments` does WHERE page_id ORDER BY id ASC, but only
|
||||
* `(page_id)` exists → extra sort.
|
||||
*
|
||||
* DEPLOY-TIME LOCK WARNING: these are plain (non-CONCURRENT) CREATE INDEX
|
||||
* statements — CONCURRENTLY is impossible because Kysely runs each migration in a
|
||||
* transaction. They take a SHARE lock that BLOCKS writes (INSERT/UPDATE/DELETE) on
|
||||
* pages/users/groups/comments/page_history for the duration of the build. The two
|
||||
* GIN trigram builds on pages.title / users.name are the slow ones and can take
|
||||
* minutes on a large tenant → a write-outage window during the deploy migration.
|
||||
* For large installations, run this migration in a maintenance window, or build
|
||||
* the trigram indexes out-of-band with CREATE INDEX CONCURRENTLY before deploying
|
||||
* (then this migration's `IF NOT EXISTS` is a no-op). Small/typical tenants are
|
||||
* unaffected.
|
||||
*/
|
||||
export async function up(db: Kysely<any>): Promise<void> {
|
||||
// Index-compatible, output-identical redefinition of f_unaccent (see header).
|
||||
await sql`
|
||||
CREATE OR REPLACE FUNCTION f_unaccent(text)
|
||||
RETURNS text
|
||||
LANGUAGE sql
|
||||
IMMUTABLE PARALLEL SAFE STRICT
|
||||
AS $func$
|
||||
SELECT public.unaccent($1);
|
||||
$func$
|
||||
`.execute(db);
|
||||
|
||||
// Search-suggest trigram indexes. Expressions match search.service.ts.
|
||||
await sql`
|
||||
CREATE INDEX IF NOT EXISTS idx_pages_title_trgm
|
||||
ON pages USING gin ((LOWER(f_unaccent(title))) gin_trgm_ops)
|
||||
`.execute(db);
|
||||
|
||||
await sql`
|
||||
CREATE INDEX IF NOT EXISTS idx_users_name_trgm
|
||||
ON users USING gin ((LOWER(f_unaccent(name))) gin_trgm_ops)
|
||||
`.execute(db);
|
||||
|
||||
await sql`
|
||||
CREATE INDEX IF NOT EXISTS idx_groups_name_trgm
|
||||
ON groups USING gin ((LOWER(f_unaccent(name))) gin_trgm_ops)
|
||||
`.execute(db);
|
||||
|
||||
// page_history: WHERE page_id ORDER BY id DESC (findPageHistoryByPageId).
|
||||
await sql`
|
||||
CREATE INDEX IF NOT EXISTS idx_page_history_page_id
|
||||
ON page_history (page_id, id DESC)
|
||||
`.execute(db);
|
||||
|
||||
// comments: WHERE page_id ORDER BY id ASC (findPageComments).
|
||||
await sql`
|
||||
CREATE INDEX IF NOT EXISTS idx_comments_page_id_id
|
||||
ON comments (page_id, id)
|
||||
`.execute(db);
|
||||
|
||||
// page_access(workspace_id): #348 made hasRestrictedPagesInWorkspace uncached
|
||||
// (F1 fix), so `EXISTS(SELECT 1 FROM page_access WHERE workspace_id=?)` now runs
|
||||
// per-request on every whole-workspace list endpoint (global search + suggest,
|
||||
// favorites, notifications, recent, created-by). page_access only had a
|
||||
// space_id index → that EXISTS was a seq scan in the common zero-restriction
|
||||
// case. This index makes it an index-only existence probe.
|
||||
await sql`
|
||||
CREATE INDEX IF NOT EXISTS idx_page_access_workspace_id
|
||||
ON page_access (workspace_id)
|
||||
`.execute(db);
|
||||
}
|
||||
|
||||
export async function down(db: Kysely<any>): Promise<void> {
|
||||
// Drop the expression indexes before restoring the function body.
|
||||
await sql`DROP INDEX IF EXISTS idx_pages_title_trgm`.execute(db);
|
||||
await sql`DROP INDEX IF EXISTS idx_users_name_trgm`.execute(db);
|
||||
await sql`DROP INDEX IF EXISTS idx_groups_name_trgm`.execute(db);
|
||||
await sql`DROP INDEX IF EXISTS idx_page_history_page_id`.execute(db);
|
||||
await sql`DROP INDEX IF EXISTS idx_comments_page_id_id`.execute(db);
|
||||
await sql`DROP INDEX IF EXISTS idx_page_access_workspace_id`.execute(db);
|
||||
|
||||
// Restore the original two-arg (dictionary-named) f_unaccent body.
|
||||
await sql`
|
||||
CREATE OR REPLACE FUNCTION f_unaccent(text)
|
||||
RETURNS text
|
||||
LANGUAGE sql
|
||||
IMMUTABLE PARALLEL SAFE STRICT
|
||||
AS $func$
|
||||
SELECT unaccent('unaccent', $1);
|
||||
$func$
|
||||
`.execute(db);
|
||||
}
|
||||
@@ -657,8 +657,9 @@ export class PagePermissionRepo {
|
||||
pageIds: string[];
|
||||
userId: string;
|
||||
spaceId?: string;
|
||||
workspaceId?: string | null;
|
||||
}): Promise<string[]> {
|
||||
const { pageIds, userId, spaceId } = opts;
|
||||
const { pageIds, userId, spaceId, workspaceId } = opts;
|
||||
if (pageIds.length === 0) return [];
|
||||
|
||||
if (spaceId) {
|
||||
@@ -666,6 +667,17 @@ export class PagePermissionRepo {
|
||||
if (!hasRestrictions) {
|
||||
return pageIds;
|
||||
}
|
||||
} else if (workspaceId) {
|
||||
// #348 — whole-workspace callers (no spaceId: favorites, notifications,
|
||||
// recent, created-by, global search) skip the recursive-ancestor CTE + anti
|
||||
// -join entirely when the workspace has ZERO restricted pages. When any
|
||||
// restriction DOES exist, fall through to the identical CTE below, so
|
||||
// behavior is unchanged whenever restrictions are present.
|
||||
const hasRestrictions =
|
||||
await this.hasRestrictedPagesInWorkspace(workspaceId);
|
||||
if (!hasRestrictions) {
|
||||
return pageIds;
|
||||
}
|
||||
}
|
||||
|
||||
const results = await this.db
|
||||
@@ -903,6 +915,39 @@ export class PagePermissionRepo {
|
||||
return Boolean(result?.exists);
|
||||
}
|
||||
|
||||
/**
|
||||
* Workspace-level analogue of hasRestrictedPagesInSpace: does ANY page in the
|
||||
* whole workspace carry a restriction? Lets whole-workspace access filters
|
||||
* short-circuit the recursive-ancestor CTE when nothing is restricted at all.
|
||||
*
|
||||
* UNCACHED (like the sibling hasRestrictedPagesInSpace) — a single cheap
|
||||
* `EXISTS(pageAccess WHERE workspaceId=?)` per call. This is an ACCESS-CONTROL
|
||||
* gate on whole-workspace list endpoints, so it must never go stale: caching it
|
||||
* (even 5s) reintroduced a leak the space-path never had — a concurrent
|
||||
* whole-workspace read in the insert->commit window of the FIRST restricted page
|
||||
* could re-populate `false` under withCache (read-then-set, no del-during-read
|
||||
* guard) and override the insert bust, leaking that page to unauthorized users
|
||||
* for up to the TTL (#348 review F1). An uncached EXISTS removes both the
|
||||
* cache/DB asymmetry with hasRestrictedPagesInSpace and that race; the space
|
||||
* path already accepts this exact per-call cost.
|
||||
*/
|
||||
async hasRestrictedPagesInWorkspace(workspaceId: string): Promise<boolean> {
|
||||
const result = await this.db
|
||||
.selectNoFrom((eb) =>
|
||||
eb
|
||||
.exists(
|
||||
eb
|
||||
.selectFrom('pageAccess')
|
||||
.select(sql`1`.as('one'))
|
||||
.where('pageAccess.workspaceId', '=', workspaceId),
|
||||
)
|
||||
.as('exists'),
|
||||
)
|
||||
.executeTakeFirst();
|
||||
|
||||
return Boolean(result?.exists);
|
||||
}
|
||||
|
||||
/**
|
||||
* Given a list of parent page IDs, return which ones have at least one accessible child.
|
||||
* Efficient batch query for sidebar hasChildren calculation.
|
||||
|
||||
@@ -581,6 +581,9 @@ export class PageRepo {
|
||||
const query = this.db
|
||||
.selectFrom('pages')
|
||||
.select(this.baseFields)
|
||||
// NOTE: `content` IS needed here — the trash UI reads page.content to render
|
||||
// the deleted-page preview modal (trash.tsx handlePageClick ->
|
||||
// TrashPageContentModal pageContent). Do NOT drop it (see #348 review F3).
|
||||
.select('content')
|
||||
.select((eb) => this.withSpace(eb))
|
||||
.select((eb) => this.withDeletedBy(eb))
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { Inject, Injectable } from '@nestjs/common';
|
||||
import { CACHE_MANAGER } from '@nestjs/cache-manager';
|
||||
import { Cache } from 'cache-manager';
|
||||
import { InjectKysely } from 'nestjs-kysely';
|
||||
import { KyselyDB, KyselyTransaction } from '../../types/kysely.types';
|
||||
import { dbOrTx } from '../../utils';
|
||||
@@ -9,6 +11,7 @@ import {
|
||||
} from '@docmost/db/types/entity.types';
|
||||
import { ExpressionBuilder, sql } from 'kysely';
|
||||
import { DB, Workspaces } from '@docmost/db/types/db';
|
||||
import { CacheKey } from '../../../common/helpers/cache-keys';
|
||||
|
||||
/**
|
||||
* Writable `settings.ai.provider` keys, enforced at this generic SQL layer. This
|
||||
@@ -61,7 +64,34 @@ export class WorkspaceRepo {
|
||||
'temporaryNoteHours',
|
||||
'isScimEnabled',
|
||||
];
|
||||
constructor(@InjectKysely() private readonly db: KyselyDB) {}
|
||||
constructor(
|
||||
@InjectKysely() private readonly db: KyselyDB,
|
||||
@Inject(CACHE_MANAGER) private readonly cacheManager: Cache,
|
||||
) {}
|
||||
|
||||
/**
|
||||
* #348 — bust the DomainMiddleware workspace caches after any workspace write.
|
||||
* Deletes BOTH the self-hosted (constant) key and the cloud per-hostname key so
|
||||
* a single implementation covers either deployment mode (the irrelevant key is a
|
||||
* harmless no-op). Best-effort: a cache error must never fail the write, and a
|
||||
* missed bust is bounded by WORKSPACE_CACHE_TTL_MS. Note: a hostname RENAME only
|
||||
* busts the NEW hostname's key (the row returned here carries the new hostname);
|
||||
* the old key expires via TTL.
|
||||
*/
|
||||
private async bustWorkspaceCache(
|
||||
workspace?: Pick<Workspace, 'hostname'> | undefined,
|
||||
): Promise<void> {
|
||||
try {
|
||||
await this.cacheManager.del(CacheKey.WORKSPACE_SELF_HOSTED);
|
||||
if (workspace?.hostname) {
|
||||
await this.cacheManager.del(
|
||||
CacheKey.WORKSPACE_BY_HOST(workspace.hostname),
|
||||
);
|
||||
}
|
||||
} catch {
|
||||
// cache is best-effort; TTL is the backstop
|
||||
}
|
||||
}
|
||||
|
||||
async findById(
|
||||
workspaceId: string,
|
||||
@@ -144,12 +174,14 @@ export class WorkspaceRepo {
|
||||
trx?: KyselyTransaction,
|
||||
): Promise<Workspace> {
|
||||
const db = dbOrTx(this.db, trx);
|
||||
return db
|
||||
const workspace = await db
|
||||
.updateTable('workspaces')
|
||||
.set({ ...updatableWorkspace, updatedAt: new Date() })
|
||||
.where('id', '=', workspaceId)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
async insertWorkspace(
|
||||
@@ -157,11 +189,14 @@ export class WorkspaceRepo {
|
||||
trx?: KyselyTransaction,
|
||||
): Promise<Workspace> {
|
||||
const db = dbOrTx(this.db, trx);
|
||||
return db
|
||||
const workspace = await db
|
||||
.insertInto('workspaces')
|
||||
.values(insertableWorkspace)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
// Bust the cached "not found" so a fresh install / new tenant is seen at once.
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
async count(): Promise<number> {
|
||||
@@ -203,7 +238,7 @@ export class WorkspaceRepo {
|
||||
trx?: KyselyTransaction,
|
||||
) {
|
||||
const db = dbOrTx(this.db, trx);
|
||||
return db
|
||||
const workspace = await db
|
||||
.updateTable('workspaces')
|
||||
.set({
|
||||
settings: sql`COALESCE(settings, '{}'::jsonb)
|
||||
@@ -214,6 +249,8 @@ export class WorkspaceRepo {
|
||||
.where('id', '=', workspaceId)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
async updateAiSettings(
|
||||
@@ -223,7 +260,7 @@ export class WorkspaceRepo {
|
||||
trx?: KyselyTransaction,
|
||||
) {
|
||||
const db = dbOrTx(this.db, trx);
|
||||
return db
|
||||
const workspace = await db
|
||||
.updateTable('workspaces')
|
||||
.set({
|
||||
settings: sql`COALESCE(settings, '{}'::jsonb)
|
||||
@@ -234,6 +271,8 @@ export class WorkspaceRepo {
|
||||
.where('id', '=', workspaceId)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -272,7 +311,7 @@ export class WorkspaceRepo {
|
||||
entries.flatMap(([k, v]) => [sql.lit(k), sql`${v}::text`]),
|
||||
)})`
|
||||
: sql`'{}'::jsonb`;
|
||||
return db
|
||||
const workspace = await db
|
||||
.updateTable('workspaces')
|
||||
.set({
|
||||
settings: sql`COALESCE(settings, '{}'::jsonb) || jsonb_build_object(
|
||||
@@ -287,6 +326,8 @@ export class WorkspaceRepo {
|
||||
.where('id', '=', workspaceId)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -303,7 +344,7 @@ export class WorkspaceRepo {
|
||||
trx?: KyselyTransaction,
|
||||
) {
|
||||
const db = dbOrTx(this.db, trx);
|
||||
return db
|
||||
const workspace = await db
|
||||
.updateTable('workspaces')
|
||||
.set({
|
||||
settings: sql`COALESCE(settings, '{}'::jsonb)
|
||||
@@ -313,6 +354,8 @@ export class WorkspaceRepo {
|
||||
.where('id', '=', workspaceId)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
async updateSharingSettings(
|
||||
@@ -322,7 +365,7 @@ export class WorkspaceRepo {
|
||||
trx?: KyselyTransaction,
|
||||
) {
|
||||
const db = dbOrTx(this.db, trx);
|
||||
return db
|
||||
const workspace = await db
|
||||
.updateTable('workspaces')
|
||||
.set({
|
||||
settings: sql`COALESCE(settings, '{}'::jsonb)
|
||||
@@ -333,6 +376,8 @@ export class WorkspaceRepo {
|
||||
.where('id', '=', workspaceId)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
async updateTemplateSettings(
|
||||
@@ -342,7 +387,7 @@ export class WorkspaceRepo {
|
||||
trx?: KyselyTransaction,
|
||||
) {
|
||||
const db = dbOrTx(this.db, trx);
|
||||
return db
|
||||
const workspace = await db
|
||||
.updateTable('workspaces')
|
||||
.set({
|
||||
settings: sql`COALESCE(settings, '{}'::jsonb)
|
||||
@@ -353,6 +398,8 @@ export class WorkspaceRepo {
|
||||
.where('id', '=', workspaceId)
|
||||
.returning(this.baseFields)
|
||||
.executeTakeFirst();
|
||||
await this.bustWorkspaceCache(workspace);
|
||||
return workspace;
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -61,9 +61,6 @@ export enum QueueJob {
|
||||
|
||||
COMMENT_NOTIFICATION = 'comment-notification',
|
||||
COMMENT_RESOLVED_NOTIFICATION = 'comment-resolved-notification',
|
||||
// #399: off-critical-path mirror of a comment's inline mark into the collab
|
||||
// Y.Doc (resolve/unresolve flip, or ephemeral-suggestion anchor removal).
|
||||
COMMENT_MARK_UPDATE = 'comment-mark-update',
|
||||
PAGE_MENTION_NOTIFICATION = 'page-mention-notification',
|
||||
PAGE_PERMISSION_GRANTED = 'page-permission-granted',
|
||||
PAGE_UPDATE_DIGEST = 'page-update-digest',
|
||||
|
||||
@@ -63,33 +63,6 @@ export interface ICommentNotificationJob {
|
||||
notifyWatchers: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* GENERAL_QUEUE payload for the off-critical-path comment inline-mark mirror
|
||||
* (#399). The comment DB row is the source of truth and is already updated
|
||||
* synchronously (ms); this job flips/removes the inline `comment` mark in the
|
||||
* collaborative Y.Doc for connected clients, OFF the HTTP response path, so
|
||||
* `POST /api/comments/resolve` no longer waits the whole Y.Doc load + store
|
||||
* pipeline (was ~4.5s p95). The mark op is idempotent, so BullMQ retries are
|
||||
* safe.
|
||||
*
|
||||
* `action`:
|
||||
* - 'resolve' / 'unresolve' → flip the mark's `resolved` attribute (exactly
|
||||
* what the synchronous resolveCommentMark path did);
|
||||
* - 'delete' → strip the anchor mark entirely (ephemeral suggestion #329).
|
||||
* `ts` is the DB-mutation timestamp (ms). The worker's race-guard uses it (with
|
||||
* the row's authoritative resolved state) to skip a resolve/unresolve event
|
||||
* that a newer, opposite event has already superseded (out-of-order drain).
|
||||
* `userId` supplies the connection-context user the store pipeline attributes
|
||||
* the change to (persistence.extension reads context.user.id).
|
||||
*/
|
||||
export interface ICommentMarkUpdateJob {
|
||||
documentName: string;
|
||||
commentId: string;
|
||||
action: 'resolve' | 'unresolve' | 'delete';
|
||||
ts: number;
|
||||
userId: string;
|
||||
}
|
||||
|
||||
export interface ICommentResolvedNotificationJob {
|
||||
commentId: string;
|
||||
commentCreatorId: string;
|
||||
|
||||
-151
@@ -1,151 +0,0 @@
|
||||
import { Job } from 'bullmq';
|
||||
import { GeneralQueueProcessor } from './general-queue.processor';
|
||||
import { QueueJob } from '../constants';
|
||||
import { ICommentMarkUpdateJob } from '../constants/queue.interface';
|
||||
|
||||
/**
|
||||
* #399: the GENERAL_QUEUE worker replays the comment inline-mark op that used to
|
||||
* run synchronously on the HTTP path. It must call the SAME gateway handler with
|
||||
* the SAME semantics (resolve/unresolve → flip the `resolved` attribute; delete
|
||||
* → strip the anchor), and its timestamp race-guard must skip an event a newer,
|
||||
* opposite event already superseded.
|
||||
*/
|
||||
describe('GeneralQueueProcessor — COMMENT_MARK_UPDATE (#399)', () => {
|
||||
function makeProc() {
|
||||
const collaborationGateway: any = {
|
||||
handleYjsEvent: jest.fn(async () => undefined),
|
||||
};
|
||||
const commentRepo: any = { findById: jest.fn() };
|
||||
// #399: the processor resolves CollaborationGateway lazily via ModuleRef
|
||||
// (strict:false) to avoid a DI cycle; the fake returns our gateway spy.
|
||||
const moduleRef: any = { get: jest.fn(() => collaborationGateway) };
|
||||
const proc = new GeneralQueueProcessor(
|
||||
{} as any, // db
|
||||
{} as any, // backlinkRepo
|
||||
{} as any, // watcherRepo
|
||||
commentRepo,
|
||||
moduleRef,
|
||||
);
|
||||
return { proc, collaborationGateway, commentRepo };
|
||||
}
|
||||
|
||||
const job = (data: ICommentMarkUpdateJob): Job =>
|
||||
({ name: QueueJob.COMMENT_MARK_UPDATE, data }) as unknown as Job;
|
||||
|
||||
const base = {
|
||||
documentName: 'page.page-1',
|
||||
commentId: 'c-1',
|
||||
userId: 'user-1',
|
||||
};
|
||||
|
||||
it('resolve → resolveCommentMark with resolved:true and the same-shape args', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
const ts = 1000;
|
||||
// Row reflects the resolve (source of truth), stamped at the same ts.
|
||||
commentRepo.findById.mockResolvedValue({
|
||||
id: 'c-1',
|
||||
resolvedAt: new Date(ts),
|
||||
updatedAt: new Date(ts),
|
||||
});
|
||||
|
||||
await proc.process(job({ ...base, action: 'resolve', ts }));
|
||||
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledTimes(1);
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'resolveCommentMark',
|
||||
'page.page-1',
|
||||
{ commentId: 'c-1', resolved: true, user: { id: 'user-1' } },
|
||||
);
|
||||
});
|
||||
|
||||
it('unresolve → resolveCommentMark with resolved:false', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
const ts = 2000;
|
||||
commentRepo.findById.mockResolvedValue({
|
||||
id: 'c-1',
|
||||
resolvedAt: null,
|
||||
updatedAt: new Date(ts),
|
||||
});
|
||||
|
||||
await proc.process(job({ ...base, action: 'unresolve', ts }));
|
||||
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'resolveCommentMark',
|
||||
'page.page-1',
|
||||
{ commentId: 'c-1', resolved: false, user: { id: 'user-1' } },
|
||||
);
|
||||
});
|
||||
|
||||
it('delete → deleteCommentMark (strip the anchor), no row lookup / no state guard', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
|
||||
await proc.process(job({ ...base, action: 'delete', ts: 123 }));
|
||||
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'deleteCommentMark',
|
||||
'page.page-1',
|
||||
{ commentId: 'c-1', user: { id: 'user-1' } },
|
||||
);
|
||||
// Delete carries no state guard — the row is (being) removed.
|
||||
expect(commentRepo.findById).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('SKIPS a stale resolve superseded by a newer unresolve (row unresolved, job ts older)', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
// A later unresolve already set the row: resolvedAt null, updatedAt = 5000.
|
||||
commentRepo.findById.mockResolvedValue({
|
||||
id: 'c-1',
|
||||
resolvedAt: null,
|
||||
updatedAt: new Date(5000),
|
||||
});
|
||||
|
||||
// Stale resolve job enqueued at ts=1000 (< 5000), intends resolved=true,
|
||||
// but the row's authoritative state is unresolved → skip.
|
||||
await proc.process(job({ ...base, action: 'resolve', ts: 1000 }));
|
||||
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('SKIPS a stale unresolve superseded by a newer resolve', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
commentRepo.findById.mockResolvedValue({
|
||||
id: 'c-1',
|
||||
resolvedAt: new Date(5000),
|
||||
updatedAt: new Date(5000),
|
||||
});
|
||||
|
||||
await proc.process(job({ ...base, action: 'unresolve', ts: 1000 }));
|
||||
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('applies when the row state agrees even if ts is older (idempotent, not a stale flip)', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
// Row is resolved and its updatedAt is newer than the job ts, but the state
|
||||
// AGREES with the job → this is a harmless idempotent replay, not a stale
|
||||
// opposite event, so it must still apply.
|
||||
commentRepo.findById.mockResolvedValue({
|
||||
id: 'c-1',
|
||||
resolvedAt: new Date(9000),
|
||||
updatedAt: new Date(9000),
|
||||
});
|
||||
|
||||
await proc.process(job({ ...base, action: 'resolve', ts: 1000 }));
|
||||
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'resolveCommentMark',
|
||||
'page.page-1',
|
||||
{ commentId: 'c-1', resolved: true, user: { id: 'user-1' } },
|
||||
);
|
||||
});
|
||||
|
||||
it('skips (no throw) when the comment row has vanished', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
commentRepo.findById.mockResolvedValue(undefined);
|
||||
|
||||
await expect(
|
||||
proc.process(job({ ...base, action: 'resolve', ts: 1000 })),
|
||||
).resolves.toBeUndefined();
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -4,7 +4,6 @@ import { Job } from 'bullmq';
|
||||
import { QueueJob, QueueName } from '../constants';
|
||||
import {
|
||||
IAddPageWatchersJob,
|
||||
ICommentMarkUpdateJob,
|
||||
IPageBacklinkJob,
|
||||
} from '../constants/queue.interface';
|
||||
import { InjectKysely } from 'nestjs-kysely';
|
||||
@@ -14,11 +13,8 @@ import {
|
||||
WatcherRepo,
|
||||
WatcherType,
|
||||
} from '@docmost/db/repos/watcher/watcher.repo';
|
||||
import { InsertableWatcher, User } from '@docmost/db/types/entity.types';
|
||||
import { InsertableWatcher } from '@docmost/db/types/entity.types';
|
||||
import { processBacklinks } from '../tasks/backlinks.task';
|
||||
import { ModuleRef } from '@nestjs/core';
|
||||
import { CollaborationGateway } from '../../../collaboration/collaboration.gateway';
|
||||
import { CommentRepo } from '@docmost/db/repos/comment/comment.repo';
|
||||
|
||||
@Processor(QueueName.GENERAL_QUEUE)
|
||||
export class GeneralQueueProcessor
|
||||
@@ -26,32 +22,14 @@ export class GeneralQueueProcessor
|
||||
implements OnModuleDestroy
|
||||
{
|
||||
private readonly logger = new Logger(GeneralQueueProcessor.name);
|
||||
// #399: CollaborationGateway lives in CollaborationModule. We resolve it lazily
|
||||
// via ModuleRef instead of importing that module into the @Global QueueModule —
|
||||
// CollaborationModule's own HistoryProcessor injects this module's global
|
||||
// GENERAL_QUEUE token, so a static import edge here would form a DI cycle. A
|
||||
// lazy strict:false lookup (cached) sidesteps it; the gateway is a singleton in
|
||||
// both the API-server and collab processes that run this worker.
|
||||
private collaborationGateway?: CollaborationGateway;
|
||||
constructor(
|
||||
@InjectKysely() private readonly db: KyselyDB,
|
||||
private readonly backlinkRepo: BacklinkRepo,
|
||||
private readonly watcherRepo: WatcherRepo,
|
||||
private readonly commentRepo: CommentRepo,
|
||||
private readonly moduleRef: ModuleRef,
|
||||
) {
|
||||
super();
|
||||
}
|
||||
|
||||
private getCollaborationGateway(): CollaborationGateway {
|
||||
if (!this.collaborationGateway) {
|
||||
this.collaborationGateway = this.moduleRef.get(CollaborationGateway, {
|
||||
strict: false,
|
||||
});
|
||||
}
|
||||
return this.collaborationGateway;
|
||||
}
|
||||
|
||||
async process(job: Job): Promise<void> {
|
||||
try {
|
||||
switch (job.name) {
|
||||
@@ -78,87 +56,12 @@ export class GeneralQueueProcessor
|
||||
);
|
||||
break;
|
||||
}
|
||||
|
||||
case QueueJob.COMMENT_MARK_UPDATE: {
|
||||
await this.processCommentMarkUpdate(
|
||||
job.data as ICommentMarkUpdateJob,
|
||||
);
|
||||
break;
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* #399: apply a comment's inline-mark mirror in the collab Y.Doc, off the HTTP
|
||||
* critical path. Runs the SAME gateway path the synchronous comment.service
|
||||
* code used (byte-identical mark op):
|
||||
* - resolve / unresolve → resolveCommentMark (flip the `resolved` attribute);
|
||||
* - delete → deleteCommentMark (strip the ephemeral-suggestion anchor #329).
|
||||
* The op is idempotent, so a BullMQ retry is safe. Throwing propagates to
|
||||
* WorkerHost → the job is retried and, on exhaustion, surfaces in failed-job
|
||||
* metrics (the divergence is now visible rather than a silently-swallowed warn).
|
||||
*/
|
||||
private async processCommentMarkUpdate(
|
||||
data: ICommentMarkUpdateJob,
|
||||
): Promise<void> {
|
||||
const { documentName, commentId, action, ts, userId } = data;
|
||||
// Minimal connection-context user: the store pipeline reads context.user.id
|
||||
// to attribute the change (persistence.extension). The mark mutation itself
|
||||
// does not depend on the user, so the op stays byte-identical. Deliberate
|
||||
// trade-off: the store pipeline's transient `page.updated` broadcast carries
|
||||
// only { id } here, so its live "who edited" badge loses name/avatarUrl for
|
||||
// this async mark replay. lastUpdatedById is still set correctly; the diff is
|
||||
// cosmetic and self-heals on the next real edit — worth it to stay off the
|
||||
// HTTP path and avoid re-loading the users row.
|
||||
const user = { id: userId } as User;
|
||||
|
||||
if (action === 'delete') {
|
||||
await this.getCollaborationGateway().handleYjsEvent(
|
||||
'deleteCommentMark',
|
||||
documentName,
|
||||
{ commentId, user },
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
// resolve / unresolve. The comment row is written SYNCHRONOUSLY before this
|
||||
// job is enqueued, so it is the source of truth for the final resolved state
|
||||
// and its updatedAt records when that state last changed. Race-guard: if a
|
||||
// newer, OPPOSITE event has already superseded this one (its ts is older than
|
||||
// the row's last resolve-state mutation AND the row's current resolved state
|
||||
// disagrees with what this job intends — e.g. an unresolve that drained ahead
|
||||
// of this resolve), skip it rather than flip the mark to a stale state.
|
||||
const comment = await this.commentRepo.findById(commentId);
|
||||
if (!comment) {
|
||||
// The comment vanished (e.g. hard-deleted) → nothing left to mirror.
|
||||
return;
|
||||
}
|
||||
const wantResolved = action === 'resolve';
|
||||
const rowResolved = comment.resolvedAt != null;
|
||||
const rowMutatedAt = new Date(comment.updatedAt).getTime();
|
||||
// `<=`, not `<`: on a sub-millisecond tie (two opposite toggles stamped in
|
||||
// the same ms) skip the disagreeing job rather than let queue order decide.
|
||||
// The consistent job (whose intent matches the row) short-circuits on the
|
||||
// first condition, so a real update is never dropped; only a mark that both
|
||||
// disagrees with the row AND is no newer than it is discarded.
|
||||
if (rowResolved !== wantResolved && ts <= rowMutatedAt) {
|
||||
this.logger.debug(
|
||||
`Skipping stale comment mark '${action}' for ${commentId} ` +
|
||||
`(job ts ${ts} < row ${rowMutatedAt}, row resolved=${rowResolved})`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
await this.getCollaborationGateway().handleYjsEvent(
|
||||
'resolveCommentMark',
|
||||
documentName,
|
||||
{ commentId, resolved: wantResolved, user },
|
||||
);
|
||||
}
|
||||
|
||||
@OnWorkerEvent('active')
|
||||
onActive(job: Job) {
|
||||
this.logger.debug(`Processing ${job.name} job`);
|
||||
|
||||
@@ -0,0 +1,113 @@
|
||||
import { Kysely } from 'kysely';
|
||||
import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo';
|
||||
import { GroupRepo } from '@docmost/db/repos/group/group.repo';
|
||||
import {
|
||||
getTestDb,
|
||||
destroyTestDb,
|
||||
createWorkspace,
|
||||
createSpace,
|
||||
createUser,
|
||||
createPage,
|
||||
} from './db';
|
||||
|
||||
/**
|
||||
* #348 — the whole-workspace access-filter short-circuit is an ACCESS-CONTROL
|
||||
* path, so it must produce the SAME result as the full recursive-ancestor CTE.
|
||||
*
|
||||
* filterAccessiblePageIds({ workspaceId }) (no spaceId — the favorites /
|
||||
* notifications / recent / created-by / global-search callers) skips the CTE only
|
||||
* when the workspace has ZERO restricted pages. A page is "restricted &
|
||||
* inaccessible" when it (or an ancestor) has a `pageAccess` row and the user has
|
||||
* no matching `pagePermissions`. Driven against real Postgres, asserts:
|
||||
* 1. zero restrictions -> short-circuit returns the full input set;
|
||||
* 2. a restriction present -> the CTE runs and drops the page the user can't
|
||||
* reach while keeping the reachable ones (behavior unchanged);
|
||||
* 3. inserting the FIRST pageAccess flips hasRestrictedPagesInWorkspace
|
||||
* false -> true immediately (the 0->1 transition — now uncached, no stale
|
||||
* window, review F1); it is scoped per workspace.
|
||||
*/
|
||||
describe('#348 filterAccessiblePageIds workspace short-circuit (real PG)', () => {
|
||||
let db: Kysely<any>;
|
||||
let repo: PagePermissionRepo;
|
||||
let workspaceId: string;
|
||||
let otherWorkspaceId: string;
|
||||
let userId: string;
|
||||
let spaceId: string;
|
||||
|
||||
beforeAll(async () => {
|
||||
db = getTestDb();
|
||||
// hasRestrictedPagesInWorkspace is now uncached, and no other cached
|
||||
// permission path is exercised here, so a no-op cache stub suffices.
|
||||
const cacheStub = {
|
||||
get: async () => undefined,
|
||||
set: async () => undefined,
|
||||
del: async () => undefined,
|
||||
} as never;
|
||||
repo = new PagePermissionRepo(db, new GroupRepo(db), cacheStub);
|
||||
|
||||
const ws = await createWorkspace(db);
|
||||
workspaceId = ws.id;
|
||||
const other = await createWorkspace(db);
|
||||
otherWorkspaceId = other.id;
|
||||
const user = await createUser(db, workspaceId);
|
||||
userId = user.id;
|
||||
const space = await createSpace(db, workspaceId);
|
||||
spaceId = space.id;
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await destroyTestDb();
|
||||
});
|
||||
|
||||
it('zero restrictions: short-circuit returns the full input set', async () => {
|
||||
const p1 = await createPage(db, { workspaceId, spaceId });
|
||||
const p2 = await createPage(db, { workspaceId, spaceId });
|
||||
|
||||
expect(await repo.hasRestrictedPagesInWorkspace(workspaceId)).toBe(false);
|
||||
|
||||
const ids = [p1.id, p2.id];
|
||||
const filtered = await repo.filterAccessiblePageIds({
|
||||
pageIds: ids,
|
||||
userId,
|
||||
workspaceId,
|
||||
});
|
||||
expect(new Set(filtered)).toEqual(new Set(ids));
|
||||
});
|
||||
|
||||
it('a restriction present: filters out the page the user cannot reach', async () => {
|
||||
const openPage = await createPage(db, { workspaceId, spaceId });
|
||||
const restrictedPage = await createPage(db, { workspaceId, spaceId });
|
||||
|
||||
// Add a pageAccess row on restrictedPage with NO matching pagePermissions for
|
||||
// `userId` → the CTE anti-join marks it inaccessible for this user.
|
||||
await db
|
||||
.insertInto('pageAccess')
|
||||
.values({
|
||||
pageId: restrictedPage.id,
|
||||
workspaceId,
|
||||
spaceId,
|
||||
accessLevel: 'read',
|
||||
creatorId: userId,
|
||||
})
|
||||
.execute();
|
||||
|
||||
// 0->1 transition is reflected immediately (uncached).
|
||||
expect(await repo.hasRestrictedPagesInWorkspace(workspaceId)).toBe(true);
|
||||
|
||||
const filtered = await repo.filterAccessiblePageIds({
|
||||
pageIds: [openPage.id, restrictedPage.id],
|
||||
userId,
|
||||
workspaceId,
|
||||
});
|
||||
expect(filtered).toContain(openPage.id);
|
||||
expect(filtered).not.toContain(restrictedPage.id);
|
||||
});
|
||||
|
||||
it('hasRestrictedPagesInWorkspace is scoped per workspace', async () => {
|
||||
// The other workspace has no pageAccess rows → still false, unaffected by the
|
||||
// restriction added above in `workspaceId`.
|
||||
expect(await repo.hasRestrictedPagesInWorkspace(otherWorkspaceId)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -1,7 +1,25 @@
|
||||
import { Kysely } from 'kysely';
|
||||
import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo';
|
||||
import { CacheKey } from 'src/common/helpers/cache-keys';
|
||||
import { getTestDb, destroyTestDb, createWorkspace } from './db';
|
||||
|
||||
// A minimal Map-backed cache double with a working `del` (the previous `{}` stub
|
||||
// made bustWorkspaceCache's `del` throw into its own try/catch, so the #348
|
||||
// invalidation was never actually exercised — review F6).
|
||||
function makeCacheDouble() {
|
||||
const store = new Map<string, unknown>();
|
||||
return {
|
||||
store,
|
||||
get: async (k: string) => store.get(k),
|
||||
set: async (k: string, v: unknown) => {
|
||||
store.set(k, v);
|
||||
},
|
||||
del: async (k: string) => {
|
||||
store.delete(k);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* A — WorkspaceRepo.updateSetting jsonb-MERGE (the html-embed kill-switch
|
||||
* write-half). Setting a single top-level key must NOT clobber sibling
|
||||
@@ -15,7 +33,9 @@ describe('WorkspaceRepo.updateSetting (jsonb merge) [integration]', () => {
|
||||
beforeAll(() => {
|
||||
db = getTestDb();
|
||||
// Repos are plain classes taking @InjectKysely() db — instantiate directly.
|
||||
repo = new WorkspaceRepo(db as any);
|
||||
// 2nd arg is CACHE_MANAGER (used only to bust the #348 workspace cache); a
|
||||
// stub is fine here since bustWorkspaceCache is best-effort (try/catch).
|
||||
repo = new WorkspaceRepo(db as any, {} as any);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
@@ -58,3 +78,62 @@ describe('WorkspaceRepo.updateSetting (jsonb merge) [integration]', () => {
|
||||
expect(updated.settings).toEqual({ htmlEmbed: false });
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* #348 F6 — the DomainMiddleware workspace cache (WORKSPACE_SELF_HOSTED /
|
||||
* WORKSPACE_BY_HOST, 15s TTL) caches security-relevant fields (enforceSso/
|
||||
* enforceMfa/status). Its correctness rests entirely on bustWorkspaceCache being
|
||||
* called from every mutator. This exercises the real invalidation with a working
|
||||
* cache double (not the {} stub, whose del throws-and-swallows): warm the cache
|
||||
* like DomainMiddleware, mutate, and assert the busted key is gone so a stale
|
||||
* workspace row can't outlive the mutation.
|
||||
*/
|
||||
describe('WorkspaceRepo bustWorkspaceCache invalidation [integration]', () => {
|
||||
let db: Kysely<any>;
|
||||
|
||||
beforeAll(() => {
|
||||
db = getTestDb();
|
||||
});
|
||||
afterAll(async () => {
|
||||
await destroyTestDb();
|
||||
});
|
||||
|
||||
it('updateSetting busts the self-hosted workspace cache key', async () => {
|
||||
const cache = makeCacheDouble();
|
||||
const repo = new WorkspaceRepo(db as any, cache as any);
|
||||
const ws = await createWorkspace(db, { settings: {} });
|
||||
|
||||
// Warm the cache as DomainMiddleware would (self-hosted key).
|
||||
cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws);
|
||||
expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(true);
|
||||
|
||||
await repo.updateSetting(ws.id, 'htmlEmbed', true);
|
||||
|
||||
// The mutation must have invalidated the cached row.
|
||||
expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false);
|
||||
});
|
||||
|
||||
it('updateSharingSettings busts the by-host workspace cache key too', async () => {
|
||||
const cache = makeCacheDouble();
|
||||
const repo = new WorkspaceRepo(db as any, cache as any);
|
||||
const ws = await createWorkspace(db, { settings: {} });
|
||||
// createWorkspace assigns a unique hostname; read it back for the by-host key.
|
||||
const { hostname } = await db
|
||||
.selectFrom('workspaces')
|
||||
.select(['hostname'])
|
||||
.where('id', '=', ws.id)
|
||||
.executeTakeFirstOrThrow();
|
||||
|
||||
// Warm BOTH keys (self-hosted + by-host); the by-host bust needs the row's
|
||||
// hostname, which the mutator returns from the DB.
|
||||
cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws);
|
||||
cache.store.set(CacheKey.WORKSPACE_BY_HOST(hostname as string), ws);
|
||||
|
||||
await repo.updateSharingSettings(ws.id, 'allowInvite', true);
|
||||
|
||||
expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false);
|
||||
expect(cache.store.has(CacheKey.WORKSPACE_BY_HOST(hostname as string))).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -81,10 +81,6 @@ const HOST_CONTRACT_METHODS = [
|
||||
"insertImage",
|
||||
"replaceImage",
|
||||
"insertFootnote",
|
||||
// draw.io diagrams (#423, stage 1) — read + create + optimistic-locked update
|
||||
"drawioGet",
|
||||
"drawioCreate",
|
||||
"drawioUpdate",
|
||||
// write (comment)
|
||||
"createComment",
|
||||
"resolveComment",
|
||||
|
||||
Reference in New Issue
Block a user