From 7097d1c174baeea44fb125b08095545e9c8776a0 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Mon, 7 Sep 2026 03:12:55 +0200 Subject: [PATCH] fix: resolve final review findings (wiki cache tenant isolation, meeting search pagination, ambient test) --- src/client/api-client.ts | 7 + src/services/meetings.ts | 47 +++++-- src/services/openapi.ts | 2 +- src/services/wikis.ts | 16 +-- tests/client.test.ts | 20 +++ tests/meetings.test.ts | 65 ++++++++++ tests/wikis.test.ts | 177 ++++++++++++++++++++++++++ tests/work-package-activities.test.ts | 40 ++++++ 8 files changed, 353 insertions(+), 21 deletions(-) diff --git a/src/client/api-client.ts b/src/client/api-client.ts index 74bf363..2279c87 100644 --- a/src/client/api-client.ts +++ b/src/client/api-client.ts @@ -145,6 +145,13 @@ export class OpenProjectClient { this.authHeader = `Basic ${btoa(`apikey:${this.apiKey}`)}`; } + /** + * Returns a unique cache key scoped to both baseUrl and apiKey for multi-tenant cache isolation. + */ + getCacheKey(): string { + return `${this.baseUrl}#${this.apiKey}`; + } + /** * Performs an HTTP GET request to OpenProject API v3. */ diff --git a/src/services/meetings.ts b/src/services/meetings.ts index 58e3359..8ba8d9c 100644 --- a/src/services/meetings.ts +++ b/src/services/meetings.ts @@ -378,21 +378,44 @@ export async function searchMeetings( const opClient = resolveClient(client); const needle = params.query.toLowerCase().trim(); - // Fetch candidate meetings scoped to project if specified - const candidateBatchSize = Math.max(params.pageSize ?? 50, 50); - const meetingsResult = await listMeetings( - { - projectId: params.projectId, - offset: 1, - pageSize: candidateBatchSize, - }, - opClient - ); + // Fetch candidate meetings scoped to project if specified, across multiple pages up to cap + const maxCandidates = 250; + const candidateBatchSize = Math.min(maxCandidates, Math.max(params.pageSize ?? 50, 50)); + const candidateMeetings: MeetingSummary[] = []; + let candidateOffset = 1; - const candidateMeetings = meetingsResult.elements; + while (candidateMeetings.length < maxCandidates) { + const meetingsResult = await listMeetings( + { + projectId: params.projectId, + offset: candidateOffset, + pageSize: candidateBatchSize, + }, + opClient + ); + + const elements = meetingsResult.elements; + if (elements.length === 0) { + break; + } + + candidateMeetings.push(...elements); + + // Stop if all meetings fetched or fewer elements returned than requested page size + if ( + candidateMeetings.length >= meetingsResult.total || + elements.length < candidateBatchSize + ) { + break; + } + + candidateOffset += 1; + } + + const scopedCandidates = candidateMeetings.slice(0, maxCandidates); // Process candidate meetings concurrently - const matchPromises = candidateMeetings.map(async (meeting) => { + const matchPromises = scopedCandidates.map(async (meeting) => { const titleMatches = meeting.title.toLowerCase().includes(needle); const locationMatches = Boolean( meeting.location && meeting.location.toLowerCase().includes(needle) diff --git a/src/services/openapi.ts b/src/services/openapi.ts index 649fb43..3d10069 100644 --- a/src/services/openapi.ts +++ b/src/services/openapi.ts @@ -76,7 +76,7 @@ export async function getOpenApiSpec( client?: OpenProjectClient ): Promise { const activeClient = resolveClient(client); - const cacheKey = activeClient.baseUrl; + const cacheKey = activeClient.getCacheKey?.() ?? activeClient.baseUrl; if (options?.refresh) { openApiCache.delete(cacheKey); diff --git a/src/services/wikis.ts b/src/services/wikis.ts index 6962f96..940b237 100644 --- a/src/services/wikis.ts +++ b/src/services/wikis.ts @@ -63,16 +63,16 @@ export interface SearchWikiPagesParams { refreshCache?: boolean; } -// In-memory cache for discovered wiki pages keyed by client baseUrl +// In-memory cache for discovered wiki pages keyed by client cache key const wikiPagesCache = new Map>(); -const discoveredBaseUrls = new Set(); +const discoveredCacheKeys = new Set(); /** * Clears the in-memory wiki discovery cache. */ export function clearWikiCache(): void { wikiPagesCache.clear(); - discoveredBaseUrls.clear(); + discoveredCacheKeys.clear(); } /** @@ -209,8 +209,8 @@ export async function getWikiPage( const detail = normalizeWikiPageDetail(pageResource, attachments); - // Update in-memory cache for this client baseUrl if initialized - const cacheKey = opClient.baseUrl || "default"; + // Update in-memory cache for this client cache key if initialized + const cacheKey = opClient.getCacheKey?.() ?? (opClient.baseUrl || "default"); let cacheMap = wikiPagesCache.get(cacheKey); if (!cacheMap) { cacheMap = new Map(); @@ -259,10 +259,10 @@ export async function searchWikiPages( client?: OpenProjectClient ): Promise { const opClient = resolveClient(client); - const cacheKey = opClient.baseUrl || "default"; + const cacheKey = opClient.getCacheKey?.() ?? (opClient.baseUrl || "default"); let cacheMap = wikiPagesCache.get(cacheKey); - const needsDiscovery = !cacheMap || !discoveredBaseUrls.has(cacheKey) || params?.refreshCache; + const needsDiscovery = !cacheMap || !discoveredCacheKeys.has(cacheKey) || params?.refreshCache; if (params?.refreshCache && cacheMap) { cacheMap.clear(); @@ -355,7 +355,7 @@ export async function searchWikiPages( } } - discoveredBaseUrls.add(cacheKey); + discoveredCacheKeys.add(cacheKey); } // 3. Resolve target project ID if provided diff --git a/tests/client.test.ts b/tests/client.test.ts index 0ea8ccb..26bb208 100644 --- a/tests/client.test.ts +++ b/tests/client.test.ts @@ -442,6 +442,26 @@ describe("OpenProjectClient (Mocked Unit Tests)", () => { expect(capturedHeaders?.get("Accept")).toContain("application/hal+json"); }); + test("getCacheKey returns unique key scoped to both baseUrl and apiKey", () => { + const clientA = new OpenProjectClient({ + baseUrl: "https://openproject.example.com", + apiKey: "user-key-a", + }); + const clientB = new OpenProjectClient({ + baseUrl: "https://openproject.example.com", + apiKey: "user-key-b", + }); + const clientC = new OpenProjectClient({ + baseUrl: "https://other.example.com", + apiKey: "user-key-a", + }); + + expect(clientA.getCacheKey()).toBe("https://openproject.example.com#user-key-a"); + expect(clientB.getCacheKey()).toBe("https://openproject.example.com#user-key-b"); + expect(clientC.getCacheKey()).toBe("https://other.example.com#user-key-a"); + expect(clientA.getCacheKey()).not.toBe(clientB.getCacheKey()); + }); + test("sanitizes API credentials from error messages", async () => { const secretKey = "super-secret-key-xyz-999"; const mockFetch = async (): Promise => { diff --git a/tests/meetings.test.ts b/tests/meetings.test.ts index d5d4f49..ab24389 100644 --- a/tests/meetings.test.ts +++ b/tests/meetings.test.ts @@ -252,6 +252,71 @@ describe("Meetings Service", () => { expect(res.elements[0]!.meeting.id).toBe(2); }); + test("searchMeetings iterates candidate pages when total meetings exceed single page size", async () => { + const requestedOffsets: number[] = []; + const mockClient = { + get: async (path: string) => { + if (path.startsWith("/api/v3/meetings?")) { + const url = new URL(`http://dummy${path}`); + const offset = Number(url.searchParams.get("offset") ?? "1"); + requestedOffsets.push(offset); + + if (offset === 1) { + // Page 1: 50 meetings, none matching query + const page1Elements = Array.from({ length: 50 }, (_, i) => ({ + _type: "Meeting", + id: i + 1, + title: `Daily Routine ${i + 1}`, + state: "open", + startTime: "2026-09-01T10:00:00Z", + endTime: "2026-09-01T10:30:00Z", + _links: { project: { href: "/api/v3/projects/1", title: "Demo" } }, + })); + return { + _type: "Collection", + total: 55, + count: 50, + pageSize: 50, + offset: 1, + _embedded: { elements: page1Elements }, + }; + } + + if (offset === 2) { + // Page 2: 5 meetings, meeting #52 has matching title + const page2Elements = Array.from({ length: 5 }, (_, i) => ({ + _type: "Meeting", + id: 51 + i, + title: i === 1 ? "Special Architecture Sync" : `Daily Routine ${51 + i}`, + state: "open", + startTime: "2026-09-02T10:00:00Z", + endTime: "2026-09-02T10:30:00Z", + _links: { project: { href: "/api/v3/projects/1", title: "Demo" } }, + })); + return { + _type: "Collection", + total: 55, + count: 5, + pageSize: 50, + offset: 2, + _embedded: { elements: page2Elements }, + }; + } + } + if (path.includes("/agenda_items")) { + return { _type: "Collection", total: 0, _embedded: { elements: [] } }; + } + throw new Error(`Unexpected path: ${path}`); + }, + } as unknown as OpenProjectClient; + + const res = await searchMeetings({ query: "Architecture" }, mockClient); + expect(requestedOffsets).toEqual([1, 2]); + expect(res.total).toBe(1); + expect(res.elements[0]!.meeting.id).toBe(52); + expect(res.elements[0]!.meeting.title).toBe("Special Architecture Sync"); + }); + test("listMeetings supports time filter (upcoming and past)", async () => { let capturedPath = ""; const mockClient = { diff --git a/tests/wikis.test.ts b/tests/wikis.test.ts index 129b567..c2263b6 100644 --- a/tests/wikis.test.ts +++ b/tests/wikis.test.ts @@ -478,4 +478,181 @@ describe("Wikis Service", () => { expect(refreshedResults).toHaveLength(1); expect(refreshedResults[0]!.id).toBe(1); }); + + test("maintains isolated wiki page cache across different clients with same baseUrl but different API keys", async () => { + let clientACalls = 0; + let clientBCalls = 0; + + const mockFetchA = async (input: string | URL | Request) => { + clientACalls++; + const url = String(input); + if (url.includes("/api/v3/wiki_pages/1/attachments")) { + return new Response( + JSON.stringify({ _type: "Collection", total: 0, _embedded: { elements: [] } }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + return new Response( + JSON.stringify({ + _type: "WikiPage", + id: 1, + title: "User A Confidential Wiki", + _links: { project: { href: "/api/v3/projects/1", title: "Project A" } }, + }), + { headers: { "Content-Type": "application/hal+json" } } + ); + }; + + const mockFetchB = async (input: string | URL | Request) => { + clientBCalls++; + const url = String(input); + if (url.includes("/api/v3/wiki_pages/1/attachments")) { + return new Response( + JSON.stringify({ _type: "Collection", total: 0, _embedded: { elements: [] } }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + return new Response( + JSON.stringify({ + _type: "WikiPage", + id: 1, + title: "User B Confidential Wiki", + _links: { project: { href: "/api/v3/projects/2", title: "Project B" } }, + }), + { headers: { "Content-Type": "application/hal+json" } } + ); + }; + + const clientA = new OpenProjectClient({ + baseUrl: "https://openproject.example.com", + apiKey: "api-key-tenant-a", + fetchFn: mockFetchA, + }); + + const clientB = new OpenProjectClient({ + baseUrl: "https://openproject.example.com", + apiKey: "api-key-tenant-b", + fetchFn: mockFetchB, + }); + + // Client A fetches wiki page 1 -> cached in client A's bucket + const pageA = await getWikiPage(1, clientA); + expect(pageA.title).toBe("User A Confidential Wiki"); + expect(clientACalls).toBe(2); + + // Client B fetches wiki page 1 -> must NOT use client A's cache + const pageB = await getWikiPage(1, clientB); + expect(pageB.title).toBe("User B Confidential Wiki"); + expect(clientBCalls).toBe(2); + + // Repeat fetch with client A -> returns User A data without refetching from server + const pageA2 = await getWikiPage(1, clientA); + expect(pageA2.title).toBe("User A Confidential Wiki"); + }); + + test("maintains isolated wiki search discovery cache across different API keys", async () => { + const mockFetchA = async (input: string | URL | Request) => { + const url = String(input); + if (url.includes("/api/v3/wiki_page_links")) { + return new Response( + JSON.stringify({ + _type: "Collection", + total: 1, + _embedded: { + elements: [ + { + id: 101, + _links: { wikiPage: { href: "/api/v3/wiki_pages/10" } }, + }, + ], + }, + }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + if (url.includes("/api/v3/wiki_pages/10/attachments")) { + return new Response( + JSON.stringify({ _type: "Collection", total: 0, _embedded: { elements: [] } }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + if (url.includes("/api/v3/wiki_pages/10")) { + return new Response( + JSON.stringify({ + _type: "WikiPage", + id: 10, + title: "Tenant A Page", + _links: { project: { href: "/api/v3/projects/1", title: "Project A" } }, + }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + return new Response(JSON.stringify({ message: "Not found" }), { + status: 404, + headers: { "Content-Type": "application/json" }, + }); + }; + + const mockFetchB = async (input: string | URL | Request) => { + const url = String(input); + if (url.includes("/api/v3/wiki_page_links")) { + return new Response( + JSON.stringify({ + _type: "Collection", + total: 1, + _embedded: { + elements: [ + { + id: 201, + _links: { wikiPage: { href: "/api/v3/wiki_pages/20" } }, + }, + ], + }, + }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + if (url.includes("/api/v3/wiki_pages/20/attachments")) { + return new Response( + JSON.stringify({ _type: "Collection", total: 0, _embedded: { elements: [] } }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + if (url.includes("/api/v3/wiki_pages/20")) { + return new Response( + JSON.stringify({ + _type: "WikiPage", + id: 20, + title: "Tenant B Page", + _links: { project: { href: "/api/v3/projects/2", title: "Project B" } }, + }), + { headers: { "Content-Type": "application/hal+json" } } + ); + } + return new Response(JSON.stringify({ message: "Not found" }), { + status: 404, + headers: { "Content-Type": "application/json" }, + }); + }; + + const clientA = new OpenProjectClient({ + baseUrl: "https://openproject.example.com", + apiKey: "api-key-tenant-a", + fetchFn: mockFetchA, + }); + + const clientB = new OpenProjectClient({ + baseUrl: "https://openproject.example.com", + apiKey: "api-key-tenant-b", + fetchFn: mockFetchB, + }); + + const searchResultsA = await searchWikiPages({}, clientA); + expect(searchResultsA).toHaveLength(1); + expect(searchResultsA[0]!.title).toBe("Tenant A Page"); + + const searchResultsB = await searchWikiPages({}, clientB); + expect(searchResultsB).toHaveLength(1); + expect(searchResultsB[0]!.title).toBe("Tenant B Page"); + }); }); diff --git a/tests/work-package-activities.test.ts b/tests/work-package-activities.test.ts index df38bd7..50cdf23 100644 --- a/tests/work-package-activities.test.ts +++ b/tests/work-package-activities.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { OpenProjectClient } from "../src/client/api-client.ts"; +import { runWithContext, type RequestContext } from "../src/context.ts"; import { listWorkPackageActivities } from "../src/services/work-packages.ts"; describe("Work Package Activities Service", () => { @@ -97,4 +98,43 @@ describe("Work Package Activities Service", () => { const res = await listWorkPackageActivities({ workPackageId: 10 }, mockClient); expect(res).toEqual([]); }); + + test("resolves ambient RequestContext client when client parameter is omitted", async () => { + let calledPath = ""; + const ambientClient = { + get: async (path: string) => { + calledPath = path; + return { + _type: "Collection", + total: 1, + count: 1, + _embedded: { + elements: [ + { + id: 101, + version: 1, + createdAt: "2026-09-07T00:00:00Z", + comment: { raw: "Ambient resolution test comment" }, + details: [], + }, + ], + }, + }; + }, + } as unknown as OpenProjectClient; + + const context: RequestContext = { + client: ambientClient, + isReadOnly: false, + }; + + await runWithContext(context, async () => { + const res = await listWorkPackageActivities({ workPackageId: 77 }); + expect(calledPath).toBe("/api/v3/work_packages/77/activities"); + expect(res).toHaveLength(1); + expect(res[0]!.id).toBe(101); + expect(res[0]!.comment).toBe("Ambient resolution test comment"); + expect(res[0]!.isComment).toBe(true); + }); + }); });