fix(search): correct cache TTL to 30s and make cache key unique by limit

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<string, { value: any; ttl: number }>;
+    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);
+    });
+  });
 });

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<any> {
     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<any>(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<string, unknown>).sort();
+      const entries = sortedKeys.map(
+        (k) => `${k}:${this.canonicalize((value as Record<string, unknown>)[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