From c5d53fc07d8b3b4c9744b336dd5e05f8069934e5 Mon Sep 17 00:00:00 2001 From: blessme247 Date: Wed, 29 Jul 2026 07:19:31 +0100 Subject: [PATCH] fix(search): correct cache TTL to 30s and make cache key unique by limit - Express TTL in milliseconds via SEARCH_CACHE_TTL_MS = 30_000 constant (cache-manager v7 interprets TTL as ms; the previous value of 30 expired entries after 30ms, yielding no cache hits) - Include limit in the cache key so requests for different page sizes no longer share a single cache entry - Replace JSON.stringify(filters) with a canonical serializer that recursively sorts object keys, so filter argument order does not fragment the cache - Add tests verifying limit differentiation, canonical filter serialization, and the 30s TTL --- src/search/search.service.spec.ts | 93 ++++++++++++++++++++++++++++++- src/search/search.service.ts | 50 ++++++++++++++++- 2 files changed, 139 insertions(+), 4 deletions(-) diff --git a/src/search/search.service.spec.ts b/src/search/search.service.spec.ts index 5921e3ca..842c04f1 100644 --- a/src/search/search.service.spec.ts +++ b/src/search/search.service.spec.ts @@ -1,4 +1,4 @@ -import { SearchService } from './search.service'; +import { SearchService, SEARCH_CACHE_TTL_MS } from './search.service'; import { Repository } from 'typeorm'; import { ElasticsearchService } from '@nestjs/elasticsearch'; @@ -177,6 +177,7 @@ describe('SearchService (Issue #814 full-text search)', () => { describe('Issue #889 — Tenant isolation enforcement on Elasticsearch queries', () => { it('tenant_a_search_excludes_tenant_b_content', async () => { + await service.onModuleInit(); // Set ES as available isolationService.getTenantId.mockReturnValue('tenant-a'); const mockEsDocs = [ @@ -205,6 +206,7 @@ describe('SearchService (Issue #814 full-text search)', () => { }); it('tenant_filter_applied_with_empty_query', async () => { + await service.onModuleInit(); // Set ES as available isolationService.getTenantId.mockReturnValue('tenant-a'); elasticsearch.search.mockResolvedValueOnce({ @@ -244,4 +246,93 @@ describe('SearchService (Issue #814 full-text search)', () => { ); }); }); + + describe('Issue #917 — cache key uniqueness and TTL', () => { + let cacheStore: Map; + let cacheManager: { get: jest.Mock; set: jest.Mock }; + let serviceWithCache: SearchService; + + beforeEach(() => { + cacheStore = new Map(); + cacheManager = { + get: jest.fn(async (key: string) => cacheStore.get(key)?.value), + set: jest.fn(async (key: string, value: any, ttl: number) => { + cacheStore.set(key, { value, ttl }); + }), + }; + + serviceWithCache = new SearchService( + courseRepository as any, + elasticsearch as any, + metricsService as any, + isolationService as any, + cacheManager as any, + ); + }); + + it('produces different cache keys and page sizes for requests differing only in limit', async () => { + // Make ES unavailable so the DB path is taken for both calls. + elasticsearch.search.mockRejectedValue(new Error('ES down')); + + const qb5 = makeQb({ + rows: [{ id: 'c1' }, { id: 'c2' }, { id: 'c3' }, { id: 'c4' }, { id: 'c5' }], + total: 50, + }); + const qb100 = makeQb({ + rows: Array.from({ length: 100 }, (_, i) => ({ id: `c${i}` })), + total: 500, + }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb5).mockReturnValueOnce(qb100); + + const result5 = await serviceWithCache.search('react', undefined, undefined, 1, 5); + const result100 = await serviceWithCache.search('react', undefined, undefined, 1, 100); + + // Different page sizes in the returned results + expect(result5.limit).toBe(5); + expect(result100.limit).toBe(100); + + // Two distinct cache keys were written + const setKeys = cacheManager.set.mock.calls.map((c) => c[0]); + expect(setKeys).toHaveLength(2); + expect(setKeys[0]).not.toBe(setKeys[1]); + + // The keys differ specifically in the limit segment (last component) + expect(setKeys[0].endsWith(':5')).toBe(true); + expect(setKeys[1].endsWith(':100')).toBe(true); + }); + + it('uses the same cache entry for filter objects with identical content but different key order', async () => { + elasticsearch.search.mockRejectedValue(new Error('ES down')); + + const qb = makeQb({ rows: [{ id: 'c1', title: 'React' }], total: 1 }); + courseRepository.createQueryBuilder.mockReturnValue(qb); + + // First call with filters in one key order + const filtersA = { price: { gte: 10, lte: 100 }, category: 'programming' }; + await serviceWithCache.search('react', filtersA, undefined, 1, 20); + + // Second call with the same filters in a different key order + const filtersB = { category: 'programming', price: { lte: 100, gte: 10 } }; + const result = await serviceWithCache.search('react', filtersB, undefined, 1, 20); + + // The second call should have hit the cache — no new DB query + expect(courseRepository.createQueryBuilder).toHaveBeenCalledTimes(1); + expect(result.limit).toBe(20); + }); + + it('sets the cache TTL to SEARCH_CACHE_TTL_MS (30_000 ms / 30 seconds)', async () => { + elasticsearch.search.mockRejectedValue(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await serviceWithCache.search('react', undefined, undefined, 1, 20); + + expect(cacheManager.set).toHaveBeenCalledWith( + expect.any(String), + expect.anything(), + SEARCH_CACHE_TTL_MS, + ); + expect(SEARCH_CACHE_TTL_MS).toBe(30_000); + }); + }); }); diff --git a/src/search/search.service.ts b/src/search/search.service.ts index c359a0f0..f6dd36aa 100644 --- a/src/search/search.service.ts +++ b/src/search/search.service.ts @@ -9,6 +9,12 @@ import { LRUCache } from 'lru-cache'; import { IsolationService } from '../tenancy/isolation/isolation.service'; import { MetricsService } from '../utils/masking/metrics.service'; +/** + * TTL for cached search results, expressed in milliseconds to match + * cache-manager v7's TTL semantics. 30_000 ms == 30 seconds. + */ +export const SEARCH_CACHE_TTL_MS = 30_000; + export interface SearchFilters { category?: string | string[]; level?: string | string[]; @@ -98,7 +104,7 @@ export class SearchService implements OnModuleInit { limit: number = 20, ): Promise { const safeQuery = query?.trim() ?? ''; - const cacheKey = `search:${safeQuery}:${JSON.stringify(filters)}:${sort}:${page}`; + const cacheKey = this.buildSearchCacheKey(safeQuery, filters, sort, page, limit); if (this.cacheManager) { const cached = await this.cacheManager.get(cacheKey); @@ -108,7 +114,8 @@ export class SearchService implements OnModuleInit { if (this.isElasticsearchAvailable) { const esResults = await this.tryElasticsearch(safeQuery, filters, page, limit, sort); if (esResults) { - if (this.cacheManager) await this.cacheManager.set(cacheKey, esResults, 30); + if (this.cacheManager) + await this.cacheManager.set(cacheKey, esResults, SEARCH_CACHE_TTL_MS); return esResults; } // If tryElasticsearch returned null due to an intermittent error, we fall through and record the fallback metric. @@ -168,7 +175,7 @@ export class SearchService implements OnModuleInit { const [results, total] = await qb.getManyAndCount(); const result = { results, total, page, limit, query: safeQuery }; - if (this.cacheManager) await this.cacheManager.set(cacheKey, result, 30); + if (this.cacheManager) await this.cacheManager.set(cacheKey, result, SEARCH_CACHE_TTL_MS); return result; } catch (err) { this.logger.error(`Search failed: ${(err as Error).message}`); @@ -224,6 +231,43 @@ export class SearchService implements OnModuleInit { // ── Private helpers ──────────────────────────────────────────────────────── + /** + * Build a cache key that uniquely identifies every parameter that affects + * the search result: query, filters, sort, page, and limit. Filters are + * serialized canonically (keys sorted recursively) so that semantically + * identical filter objects produce the same key regardless of argument + * order. + */ + private buildSearchCacheKey( + query: string, + filters: SearchFilters | undefined, + sort: string | undefined, + page: number, + limit: number, + ): string { + return `search:${query}:${this.canonicalize(filters)}:${sort ?? ''}:${page}:${limit}`; + } + + /** + * Canonical serializer that recursively sorts object keys so the resulting + * string is independent of property order. Arrays preserve their order + * (order is semantically meaningful for filters like `category`). + */ + private canonicalize(value: unknown): string { + if (value === undefined || value === null) return ''; + if (Array.isArray(value)) { + return `[${value.map((v) => this.canonicalize(v)).join(',')}]`; + } + if (typeof value === 'object') { + const sortedKeys = Object.keys(value as Record).sort(); + const entries = sortedKeys.map( + (k) => `${k}:${this.canonicalize((value as Record)[k])}`, + ); + return `{${entries.join(',')}}`; + } + return JSON.stringify(value); + } + /** * Attempt the Elasticsearch path; returns `null` if ES isn't available / * throws so the caller falls back to the DB. We never throw ES errors up