From fac3e783f9f5666689662a583d5b7bf3681ec6a2 Mon Sep 17 00:00:00 2001 From: yasser Date: Mon, 6 Apr 2026 08:55:22 +0100 Subject: [PATCH] improvement: replace KEYS with SCAN, fix @Cacheable null-return bug, clean up index exports --- .../redis-cache-store.adapter.spec.ts | 17 +++++-- src/adapters/redis-cache-store.adapter.ts | 30 +++++++---- src/decorators/cacheable.decorator.spec.ts | 34 +++++++++++-- src/decorators/cacheable.decorator.ts | 51 +++++++++++++++++-- src/index.ts | 20 -------- 5 files changed, 111 insertions(+), 41 deletions(-) diff --git a/src/adapters/redis-cache-store.adapter.spec.ts b/src/adapters/redis-cache-store.adapter.spec.ts index 35a7732..08111b9 100644 --- a/src/adapters/redis-cache-store.adapter.spec.ts +++ b/src/adapters/redis-cache-store.adapter.spec.ts @@ -5,12 +5,12 @@ * * Uses ioredis-mock so no real Redis server is required. The mock exposes the * same API surface as real ioredis, meaning all RedisCacheStore code paths - * (get, set+EX, del, keys+del, flushdb) are exercised against in-memory state. + * (get, set+EX, del, scan+del, flushdb) are exercised against in-memory state. * * Tests cover: * - Full ICacheStore contract: get, set (with and without TTL), delete, clear * - Key prefix namespacing: keys are stored and retrieved with the prefix - * - clear() with prefix: only prefixed keys are removed + * - clear() with prefix: only prefixed keys are removed via cursor-based SCAN * - clear() without prefix: full flushdb is called * - Parse-error resilience: malformed JSON stored in Redis returns null * - Constructor accepts both a URL string and an existing Redis instance @@ -154,9 +154,20 @@ describe("RedisCacheStore", () => { expect(await redis.get(`${PREFIX}:key`)).toBeNull(); }); - // ── clear (with prefix → keys + del) ──────────────────────────────── + // ── clear (with prefix → scan + del) ──────────────────────────────── describe("clear() with prefix", () => { + it("uses SCAN (not KEYS) to iterate matching keys", async () => { + // SCAN must be used instead of the blocking KEYS command + const scanSpy = jest.spyOn(redis, "scan"); + await redis.set(`${PREFIX}:x`, "1"); + + await store.clear(); + + // At least one SCAN call must have been made + expect(scanSpy).toHaveBeenCalled(); + }); + it("removes only keys matching the prefix pattern", async () => { // Populate both prefixed and non-prefixed keys await redis.set(`${PREFIX}:x`, "1"); diff --git a/src/adapters/redis-cache-store.adapter.ts b/src/adapters/redis-cache-store.adapter.ts index 36fb2c6..16e10bc 100644 --- a/src/adapters/redis-cache-store.adapter.ts +++ b/src/adapters/redis-cache-store.adapter.ts @@ -8,7 +8,8 @@ * - A parse failure (malformed JSON) returns null instead of throwing. * - An optional key prefix namespaces every key so multiple adapters can * share the same Redis database without colliding. - * - clear() only removes keys that belong to this adapter's prefix; + * - clear() only removes keys that belong to this adapter's prefix via + * cursor-based SCAN (non-blocking, safe for large key sets); * without a prefix it flushes the entire Redis database (FLUSHDB). * * Exports: @@ -137,15 +138,24 @@ export class RedisCacheStore implements ICacheStore { /** {@inheritDoc ICacheStore.clear} */ async clear(): Promise { if (this.keyPrefix) { - // Prefix mode: collect only the keys that belong to this adapter's namespace. - // NOTE: KEYS is O(N) and blocks Redis — acceptable for dev / low-traffic scenarios. - // Consider SCAN-based iteration for high-traffic production deployments. - const keys = await this.redis.keys(`${this.keyPrefix}:*`); - - // Only call DEL when there is at least one matching key - if (keys.length > 0) { - await this.redis.del(...keys); - } + // Use cursor-based SCAN instead of KEYS to avoid blocking Redis while + // iterating large key sets. SCAN is O(1) per call (amortised O(N) total) + // and yields control back to Redis between iterations. + const pattern = `${this.keyPrefix}:*`; + let cursor = "0"; + + do { + // Each SCAN call returns [nextCursor, matchedKeys]. + // COUNT is a hint to Redis — it may return more or fewer per call. + const [nextCursor, keys] = await this.redis.scan(cursor, "MATCH", pattern, "COUNT", 100); + cursor = nextCursor; + + // Delete the batch found in this iteration immediately to keep memory + // usage flat regardless of total key count. + if (keys.length > 0) { + await this.redis.del(...keys); + } + } while (cursor !== "0"); } else { // No prefix — flush every key in the currently selected Redis database await this.redis.flushdb(); diff --git a/src/decorators/cacheable.decorator.spec.ts b/src/decorators/cacheable.decorator.spec.ts index 05fecaf..3975468 100644 --- a/src/decorators/cacheable.decorator.spec.ts +++ b/src/decorators/cacheable.decorator.spec.ts @@ -11,7 +11,8 @@ * - Cache hit: original method NOT called on second invocation * - Cache miss: original method called and result persisted on first call * - Key template interpolation: "user:{0}" with arg "42" → "user:42" - * - Optional TTL forwarded to the store + * - Optional TTL forwarded to the store (value wrapped in envelope) + * - Null return value is correctly cached (not treated as a permanent miss) * - Works on async methods * - Works on sync methods */ @@ -149,8 +150,35 @@ describe("@Cacheable decorator", () => { } await new DataService().fetch(); - // The explicit 60-second TTL must have been passed to set() - expect(setSpy).toHaveBeenCalledWith("data-key", "data", 60); + // The value is wrapped in an envelope before being passed to set(); + // the explicit 60-second TTL must be forwarded unchanged. + expect(setSpy).toHaveBeenCalledWith("data-key", { __v: "data" }, 60); + }); + + // ── Null return value ────────────────────────────────────────────────────── + + it("correctly caches a null return value (does not re-call the method on the next invocation)", async () => { + // Methods that return null are a real edge case: without the internal + // envelope, null from get() looks identical to a cache miss, so the + // original method would be called on every invocation. + const impl = jest.fn().mockResolvedValue(null); + + class UserService { + @Cacheable("user:null-case") + async findDeleted() { + return impl(); + } + } + + const svc = new UserService(); + const first = await svc.findDeleted(); // miss — caches null + const second = await svc.findDeleted(); // must be a hit, NOT a miss + + // The original method should only have been called once + expect(impl).toHaveBeenCalledTimes(1); + // Both calls must return null + expect(first).toBeNull(); + expect(second).toBeNull(); }); // ── Sync method support ─────────────────────────────────────────────────── diff --git a/src/decorators/cacheable.decorator.ts b/src/decorators/cacheable.decorator.ts index 16a7839..a27ae65 100644 --- a/src/decorators/cacheable.decorator.ts +++ b/src/decorators/cacheable.decorator.ts @@ -21,6 +21,37 @@ import { CacheServiceRef } from "@utils/cache-service-ref"; import { resolveCacheKey } from "@utils/resolve-cache-key.util"; +// --------------------------------------------------------------------------- +// Internal cache envelope +// --------------------------------------------------------------------------- + +/** + * Thin wrapper stored under every cache key written by @Cacheable. + * + * Why this is needed: + * ICacheStore.get() returns null for both a cache miss AND a stored null value. + * Without an envelope, a method that legitimately returns null would never + * benefit from caching — every call would look like a miss and re-execute + * the original method. + * + * By wrapping the return value in `{ __v: result }` before writing, and + * unwrapping after reading, the decorator can tell "cached null" (envelope + * present, __v is null) from "cache miss" (get() returned null itself). + * + * This type is intentionally NOT exported — it is an internal implementation + * detail. Consumers should not read @Cacheable keys via CacheService.get() + * directly, as they would receive the raw envelope object. + */ +interface CacheEnvelope { + /** The actual cached return value — may be null or undefined */ + readonly __v: unknown; +} + +/** Type guard — true when `v` is a CacheEnvelope written by @Cacheable */ +function isEnvelope(v: unknown): v is CacheEnvelope { + return typeof v === "object" && v !== null && "__v" in v; +} + /** * Cache-aside method decorator. * @@ -46,6 +77,12 @@ import { resolveCacheKey } from "@utils/resolve-cache-key.util"; * @Cacheable("config") * async getConfig(): Promise { ... } * ``` + * + * @remarks + * Methods that return `null` are correctly cached. The decorator uses an + * internal envelope so a cached `null` is distinguishable from a cache miss. + * Do **not** read keys written by `@Cacheable` via `CacheService.get()` directly + * — you will receive the raw envelope object `{ __v: value }` instead of the value. */ export function Cacheable(key: string, ttlSeconds?: number): MethodDecorator { return ( @@ -65,17 +102,21 @@ export function Cacheable(key: string, ttlSeconds?: number): MethodDecorator { const resolvedKey = resolveCacheKey(key, args); // ── Cache hit ────────────────────────────────────────────────────── - // Return the stored value immediately without calling the original method - const cached = await cacheService.get(resolvedKey); - if (cached !== null) return cached; + // Read the envelope. null from get() means cache miss; a present envelope + // means hit — even when the stored return value was null or undefined. + const envelope = await cacheService.get(resolvedKey); + if (envelope !== null && isEnvelope(envelope)) { + return envelope.__v; + } // ── Cache miss ───────────────────────────────────────────────────── // Call the original method; wrap in Promise.resolve() to handle both // sync methods (returns a plain value) and async methods (returns a Promise) const result = await Promise.resolve(originalMethod.apply(this, args)); - // Persist the result under the resolved key for future calls - await cacheService.set(resolvedKey, result, ttlSeconds); + // Wrap in an envelope before storing so null/undefined returns are + // preserved and distinguishable from a cache miss on the next call. + await cacheService.set(resolvedKey, { __v: result }, ttlSeconds); return result; }; diff --git a/src/index.ts b/src/index.ts index 4755cd6..10be535 100644 --- a/src/index.ts +++ b/src/index.ts @@ -29,26 +29,6 @@ export { CACHE_STORE, CACHE_MODULE_OPTIONS } from "./constants"; // Inject it anywhere via constructor injection. export { CacheService } from "./services/cache.service"; -// ============================================================================ -// DTOs (Public Contracts) -// ============================================================================ -// DTOs are the public interface for your API -// Consumers depend on these, so they must be stable -export { CreateExampleDto } from "./dto/create-example.dto"; -export { UpdateExampleDto } from "./dto/update-example.dto"; - -// ============================================================================ -// GUARDS (For Route Protection) -// ============================================================================ -// Export guards so consumers can use them in their apps -export { ExampleGuard } from "./guards/example.guard"; - -// ============================================================================ -// DECORATORS (For Dependency Injection & Metadata) -// ============================================================================ -// Export decorators for use in consumer controllers/services -export { ExampleData, ExampleParam } from "./decorators/example.decorator"; - // ============================================================================ // DECORATORS // ============================================================================