fix: cache hits on falsy values, method and caller in the key, and failure handling

libs/nestjs-cacheable/README.md

@@ -21,7 +21,7 @@ For static configuration, use the `register()` method.
 ```typescript
 // app.module.ts
 import { Module } from '@nestjs/common'
-import { NestjsCacheableModule } from '@m8a-io/nestjs-cacheable'
+import { NestjsCacheableModule } from '@m8a/nestjs-cacheable'
 import KeyvRedis from '@keyv/redis'
 
 @Module({
@@ -41,7 +41,7 @@ For asynchronous configuration, use the `registerAsync()` method. This is useful
 ```typescript
 // app.module.ts
 import { Module } from '@nestjs/common'
-import { NestjsCacheableModule } from '@m8a-io/nestjs-cacheable'
+import { NestjsCacheableModule } from '@m8a/nestjs-cacheable'
 import { ConfigModule, ConfigService } from '@nestjs/config'
 import KeyvRedis from '@keyv/redis'
 
@@ -66,7 +66,7 @@ The easiest way to use the cache is with the `CacheableInterceptor`. You can app
 ```typescript
 // app.controller.ts
 import { Controller, Get, UseInterceptors } from '@nestjs/common'
-import { CacheableInterceptor, CacheTTL } from '@m8a-io/nestjs-cacheable'
+import { CacheableInterceptor, CacheTTL } from '@m8a/nestjs-cacheable'
 
 @Controller()
 export class AppController {
@@ -82,14 +82,40 @@ export class AppController {
 }
 ```
 
+#### What the interceptor caches
+
+The default key is `METHOD:url`, and only `GET` and `HEAD` responses are cached.
+
+Requests that carry credentials (a `user` on the request, an `Authorization`
+header or a `Cookie`) are **not** cached by default, because keying them by url
+alone would serve one user's response to the next. To cache them, subclass the
+interceptor and key by the caller:
+
+```typescript
+@Injectable()
+export class UserScopedCacheInterceptor extends CacheableInterceptor {
+  protected getCacheKey(context: ExecutionContext): string | undefined {
+    const request = context.switchToHttp().getRequest()
+
+    if (request.method !== 'GET') return undefined
+
+    return `GET:${request.user.id}:${request.url}`
+  }
+}
+```
+
+Concurrent requests that miss the same key run the handler once and share the
+result. A cache backend that is unreachable is logged at debug level and the
+request is answered from the handler.
+
 ### Using the Service Directly
 
 You can also inject the `NestjsCacheableService` to interact with the cache programmatically.
 
 ```typescript
 // my.service.ts
 import { Injectable } from '@nestjs/common'
-import { NestjsCacheableService } from '@m8a-io/nestjs-cacheable'
+import { NestjsCacheableService } from '@m8a/nestjs-cacheable'
 
 @Injectable()
 export class MyService {

libs/nestjs-cacheable/package.json

@@ -16,6 +16,8 @@
   },
   "peerDependencies": {
     "@keyv/redis": "^5.0.2",
+    "@nestjs/common": "^10.0.0 || ^11.0.0",
+    "@nestjs/core": "^10.0.0 || ^11.0.0",
     "cacheable": "^1.0.0",
     "ioredis": "^5.0.0",
     "keyv": "^5.0.0",

libs/nestjs-cacheable/src/cache-ttl.decorator.ts

@@ -1,4 +1,4 @@
 import { SetMetadata } from '@nestjs/common'
 
 export const CACHE_TTL_KEY = 'cache_ttl'
-export const CacheTTL = (ttl: number) => SetMetadata(CACHE_TTL_KEY, ttl)
+export const CacheTTL = (ttl: number | string) => SetMetadata(CACHE_TTL_KEY, ttl)

libs/nestjs-cacheable/src/cacheable.interceptor.regression.spec.ts

@@ -0,0 +1,186 @@
+import { CallHandler, ExecutionContext } from '@nestjs/common'
+import { Reflector } from '@nestjs/core'
+import { Cacheable } from 'cacheable'
+import { firstValueFrom, of, toArray } from 'rxjs'
+import { CacheableInterceptor } from './cacheable.interceptor'
+import { NestjsCacheableService } from './nestjs-cacheable.service'
+
+function contextFor(request: Record<string, unknown>): ExecutionContext {
+  return {
+    switchToHttp: () => ({ getRequest: () => request }),
+    getHandler: () => function handler() {},
+  } as unknown as ExecutionContext
+}
+
+function handlerFor(value: unknown): {
+  handler: CallHandler
+  calls: () => number
+} {
+  const handle = jest.fn(() => of(value))
+  return {
+    handler: { handle } as CallHandler,
+    calls: () => handle.mock.calls.length,
+  }
+}
+
+describe('CacheableInterceptor (regressions)', () => {
+  let service: NestjsCacheableService
+  let interceptor: CacheableInterceptor
+
+  beforeEach(() => {
+    service = new NestjsCacheableService(new Cacheable())
+    interceptor = new CacheableInterceptor(service, new Reflector())
+  })
+
+  it('serves a cached falsy value instead of running the handler again', async () => {
+    const context = contextFor({ url: '/count', method: 'GET', headers: {} })
+    const first = handlerFor(0)
+
+    await firstValueFrom(await interceptor.intercept(context, first.handler))
+
+    const second = handlerFor(0)
+    const body = await firstValueFrom(
+      await interceptor.intercept(context, second.handler),
+    )
+
+    expect(body).toBe(0)
+    expect(second.calls()).toBe(0)
+  })
+
+  it('never serves a mutation response to a read of the same url', async () => {
+    const post = handlerFor({ created: true })
+    await firstValueFrom(
+      await interceptor.intercept(
+        contextFor({ url: '/items', method: 'POST', headers: {} }),
+        post.handler,
+      ),
+    )
+
+    const get = handlerFor({ items: ['real'] })
+    const body = await firstValueFrom(
+      await interceptor.intercept(
+        contextFor({ url: '/items', method: 'GET', headers: {} }),
+        get.handler,
+      ),
+    )
+
+    expect(body).toEqual({ items: ['real'] })
+  })
+
+  it('does not share a response between callers that send credentials', async () => {
+    const alice = handlerFor({ balance: 'alice' })
+    await firstValueFrom(
+      await interceptor.intercept(
+        contextFor({
+          url: '/me',
+          method: 'GET',
+          headers: { authorization: 'Bearer alice' },
+        }),
+        alice.handler,
+      ),
+    )
+
+    const bob = handlerFor({ balance: 'bob' })
+    const body = await firstValueFrom(
+      await interceptor.intercept(
+        contextFor({
+          url: '/me',
+          method: 'GET',
+          headers: { authorization: 'Bearer bob' },
+        }),
+        bob.handler,
+      ),
+    )
+
+    expect(body).toEqual({ balance: 'bob' })
+  })
+
+  it('runs the handler once for concurrent identical requests', async () => {
+    const context = contextFor({ url: '/slow', method: 'GET', headers: {} })
+    const handle = jest.fn(() => of({ ok: true }))
+    const handler = { handle } as CallHandler
+
+    await Promise.all([
+      interceptor
+        .intercept(context, handler)
+        .then((response$) => firstValueFrom(response$)),
+      interceptor
+        .intercept(context, handler)
+        .then((response$) => firstValueFrom(response$)),
+      interceptor
+        .intercept(context, handler)
+        .then((response$) => firstValueFrom(response$)),
+    ])
+
+    expect(handle).toHaveBeenCalledTimes(1)
+  })
+
+  it('passes every emission of the handler through', async () => {
+    const context = contextFor({ url: '/stream', method: 'GET', headers: {} })
+    const handle = jest.fn(() => of(1, 2, 3))
+
+    const emitted = await firstValueFrom(
+      (await interceptor.intercept(context, { handle } as CallHandler)).pipe(
+        toArray(),
+      ),
+    )
+
+    expect(emitted).toEqual([1, 2, 3])
+  })
+
+  it('answers the request when the cache fails to read', async () => {
+    const failing = {
+      get: jest.fn().mockRejectedValue(new Error('ECONNREFUSED')),
+      set: jest.fn().mockResolvedValue(true),
+    } as unknown as NestjsCacheableService
+    const local = new CacheableInterceptor(failing, new Reflector())
+    const { handler, calls } = handlerFor({ ok: true })
+
+    const body = await firstValueFrom(
+      await local.intercept(
+        contextFor({ url: '/data', method: 'GET', headers: {} }),
+        handler,
+      ),
+    )
+
+    expect(body).toEqual({ ok: true })
+    expect(calls()).toBe(1)
+  })
+
+  it('answers the request when the cache fails to write', async () => {
+    const failing = {
+      get: jest.fn().mockResolvedValue(undefined),
+      set: jest.fn().mockRejectedValue(new Error('ECONNREFUSED')),
+    } as unknown as NestjsCacheableService
+    const local = new CacheableInterceptor(failing, new Reflector())
+
+    const body = await firstValueFrom(
+      await local.intercept(
+        contextFor({ url: '/data', method: 'GET', headers: {} }),
+        handlerFor({ ok: true }).handler,
+      ),
+    )
+
+    expect(body).toEqual({ ok: true })
+  })
+})
+
+describe('NestjsCacheableService (regressions)', () => {
+  it('caches a value that cannot be serialised to JSON', async () => {
+    const service = new NestjsCacheableService(new Cacheable())
+    const circular: Record<string, unknown> = { name: 'node' }
+    circular.self = circular
+
+    await expect(service.set('circular', circular)).resolves.toBe(true)
+  })
+
+  it('disconnects the primary store as well as the secondary', async () => {
+    const cache = new Cacheable()
+    const service = new NestjsCacheableService(cache)
+    const primary = jest.spyOn(cache.primary, 'disconnect')
+
+    await service.disconnect()
+
+    expect(primary).toHaveBeenCalled()
+  })
+})

libs/nestjs-cacheable/src/cacheable.interceptor.spec.ts

@@ -23,6 +23,8 @@ describe('CacheableInterceptor', () => {
     switchToHttp: () => ({
       getRequest: () => ({
         url: '/test',
+        method: 'GET',
+        headers: {},
       }),
     }),
     getHandler: () => ({}),
@@ -61,8 +63,11 @@ describe('CacheableInterceptor', () => {
   })
 
   it('should return cached value if it exists', async () => {
-    mockCacheService.get.mockResolvedValue('cached_value')
-    const result$ = await interceptor.intercept(mockExecutionContext, mockCallHandler)
+    mockCacheService.get.mockResolvedValue({ value: 'cached_value' })
+    const result$ = await interceptor.intercept(
+      mockExecutionContext,
+      mockCallHandler,
+    )
     const result = await firstValueFrom(result$)
     expect(result).toBe('cached_value')
     expect(mockCallHandler.handle).not.toHaveBeenCalled()
@@ -71,18 +76,32 @@ describe('CacheableInterceptor', () => {
   it('should call handler, cache the result, and return it if no cached value', async () => {
     mockCacheService.get.mockResolvedValue(undefined)
     mockCallHandler.handle.mockReturnValue(of('new_value'))
-    const result$ = await interceptor.intercept(mockExecutionContext, mockCallHandler)
+    const result$ = await interceptor.intercept(
+      mockExecutionContext,
+      mockCallHandler,
+    )
     const value = await firstValueFrom(result$)
     expect(value).toBe('new_value')
-    expect(mockCacheService.set).toHaveBeenCalledWith('/test', 'new_value', undefined)
+    expect(mockCacheService.set).toHaveBeenCalledWith(
+      'GET:/test',
+      { value: 'new_value' },
+      undefined,
+    )
   })
 
   it('should use TTL from decorator if present', async () => {
     mockCacheService.get.mockResolvedValue(undefined)
     mockCallHandler.handle.mockReturnValue(of('new_value'))
     mockReflector.get.mockReturnValue(5000)
-    const result$ = await interceptor.intercept(mockExecutionContext, mockCallHandler)
+    const result$ = await interceptor.intercept(
+      mockExecutionContext,
+      mockCallHandler,
+    )
     await firstValueFrom(result$)
-    expect(mockCacheService.set).toHaveBeenCalledWith('/test', 'new_value', 5000)
+    expect(mockCacheService.set).toHaveBeenCalledWith(
+      'GET:/test',
+      { value: 'new_value' },
+      5000,
+    )
   })
 })