diff --git a/packages/rpc/package.json b/packages/rpc/package.json index 415a7be0c..e4f7d64cc 100644 --- a/packages/rpc/package.json +++ b/packages/rpc/package.json @@ -7,7 +7,7 @@ "types": "./src/index.ts", "scripts": { "check-types": "tsc --noEmit", - "test": "bun test src/routers src/lib/analytics-utils.integration.test.ts src/procedures/*.test.ts src/services/insight-schedule.test.ts src/services/uptime-lifecycle.test.ts src/services/uptime-scheduler.test.ts src/utils/*.test.ts", + "test": "bun test src/routers src/lib/analytics-utils.integration.test.ts src/lib/funnels-cache.test.ts src/procedures/*.test.ts src/services/insight-schedule.test.ts src/services/uptime-lifecycle.test.ts src/services/uptime-scheduler.test.ts src/utils/*.test.ts", "test:integration": "bun test src/services/uptime-scheduler.integration.test.ts" }, "exports": { diff --git a/packages/rpc/src/lib/funnels-cache.test.ts b/packages/rpc/src/lib/funnels-cache.test.ts new file mode 100644 index 000000000..56bec8c13 --- /dev/null +++ b/packages/rpc/src/lib/funnels-cache.test.ts @@ -0,0 +1,111 @@ +import { beforeEach, describe, expect, it, mock } from "bun:test"; +import { createDrizzleCache } from "@databuddy/redis/drizzle-cache"; + +const kv = new Map(); +const sets = new Map>(); + +const redis = { + get: mock(async (key: string) => kv.get(key) ?? null), + setex: mock(async (key: string, _ttl: number, value: string) => { + kv.set(key, value); + return "OK" as const; + }), + sadd: mock(async (key: string, ...members: string[]) => { + const set = sets.get(key) ?? new Set(); + for (const member of members) { + set.add(member); + } + sets.set(key, set); + return members.length; + }), + smembers: mock(async (key: string) => Array.from(sets.get(key) ?? [])), + srem: mock(async (key: string, member: string) => { + sets.get(key)?.delete(member); + return 1; + }), + unlink: mock(async (...keys: string[]) => { + for (const key of keys) { + kv.delete(key); + sets.delete(key); + } + return keys.length; + }), + sunion: mock(async (...keys: string[]) => { + const out = new Set(); + for (const key of keys) { + for (const member of sets.get(key) ?? []) { + out.add(member); + } + } + return Array.from(out); + }), +}; + +mock.module("@databuddy/redis", () => ({ + createDrizzleCache, + invalidateAgentContextSnapshotsForWebsite: mock(async () => undefined), + redis, +})); + +const { funnelCache, invalidateFunnelsCache } = await import("./funnels-cache"); + +beforeEach(() => { + kv.clear(); + sets.clear(); +}); + +describe("invalidateFunnelsCache", () => { + it("invalidates the exact key a cached getById lookup is stored under", async () => { + const funnelId = "funnel-1"; + let calls = 0; + + const readFunnel = () => + funnelCache.withCache({ + key: `byId:${funnelId}`, + queryFn: async () => { + calls += 1; + return { id: funnelId, name: `v${calls}` }; + }, + tables: ["funnelDefinitions"], + ttl: 300, + }); + + expect(await readFunnel()).toEqual({ id: funnelId, name: "v1" }); + // Cache hit: queryFn must not run again. + expect(await readFunnel()).toEqual({ id: funnelId, name: "v1" }); + expect(calls).toBe(1); + + await invalidateFunnelsCache("website-1", funnelId); + + // Regression guard: invalidateFunnelsCache must target the same key + // format the router actually caches under (`byId:`), not a + // key that was never written (e.g. `byId::`). + expect(await readFunnel()).toEqual({ id: funnelId, name: "v2" }); + expect(calls).toBe(2); + }); + + it("invalidates the exact key a cached list lookup is stored under", async () => { + const websiteId = "website-2"; + let calls = 0; + + const readList = () => + funnelCache.withCache({ + key: `list:${websiteId}`, + queryFn: async () => { + calls += 1; + return [{ id: `funnel-${calls}` }]; + }, + tables: ["funnelDefinitions"], + ttl: 300, + }); + + await readList(); + await readList(); + expect(calls).toBe(1); + + await invalidateFunnelsCache(websiteId); + + await readList(); + expect(calls).toBe(2); + }); +}); diff --git a/packages/rpc/src/lib/funnels-cache.ts b/packages/rpc/src/lib/funnels-cache.ts index 83b9be36f..444d509e3 100644 --- a/packages/rpc/src/lib/funnels-cache.ts +++ b/packages/rpc/src/lib/funnels-cache.ts @@ -20,7 +20,7 @@ export async function invalidateFunnelsCache( ): Promise { const keys = [`list:${websiteId}`]; if (funnelId) { - keys.push(`byId:${funnelId}:${websiteId}`); + keys.push(`byId:${funnelId}`); } const operations: Promise[] = keys.map((key) => diff --git a/packages/rpc/src/routers/funnels.ts b/packages/rpc/src/routers/funnels.ts index 4ec5d0b64..31702dcae 100644 --- a/packages/rpc/src/routers/funnels.ts +++ b/packages/rpc/src/routers/funnels.ts @@ -184,7 +184,6 @@ export const funnelsRouter = { return cache.withCache({ key: `list:${input.websiteId}`, - disabled: true, // TODO: Remove this once we have a way to invalidate the cache ttl: CACHE_TTL, tables: ["funnelDefinitions"], queryFn: () => @@ -223,10 +222,33 @@ export const funnelsRouter = { }) .input(z.object({ id: z.string() })) .output(funnelOutputSchema) - .handler(({ context, input }) => - cache.withCache({ + .handler(async ({ context, input }) => { + // Resolve the owning website and authorize on every request, not just + // on a cache miss: `queryFn` below is skipped entirely on a cache hit, + // so any permission check placed inside it would be bypassed for + // anyone who requests an id already cached by another caller. + const [funnelRef] = await context.db + .select({ websiteId: funnelDefinitions.websiteId }) + .from(funnelDefinitions) + .where( + and( + eq(funnelDefinitions.id, input.id), + isNull(funnelDefinitions.deletedAt) + ) + ) + .limit(1); + + if (!funnelRef) { + throw rpcError.notFound("funnel", input.id); + } + + await withWorkspace(context, { + websiteId: funnelRef.websiteId, + permissions: ["read"], + }); + + return cache.withCache({ key: `byId:${input.id}`, - disabled: true, // TODO: Remove this once we have a way to invalidate the cache ttl: CACHE_TTL, tables: ["funnelDefinitions"], queryFn: async () => { @@ -245,15 +267,10 @@ export const funnelsRouter = { throw rpcError.notFound("funnel", input.id); } - await withWorkspace(context, { - websiteId: funnel.websiteId, - permissions: ["read"], - }); - return funnel; }, - }) - ), + }); + }), create: trackedProcedure .route({ @@ -650,7 +667,6 @@ export const funnelsRouter = { return cache.withCache({ key: cacheKey, - disabled: true, // TODO: Remove this once we have a way to invalidate the cache ttl: ANALYTICS_CACHE_TTL, tables: ["funnelDefinitions"], tag: `funnel:${input.funnelId}`,