Found a few bugs while reading through the source. All minimal fixes, no refactoring, no new files.#1
Open
sur-ser wants to merge 1 commit intom8a-io:mainfrom
Open
Found a few bugs while reading through the source. All minimal fixes, no refactoring, no new files.#1sur-ser wants to merge 1 commit intom8a-io:mainfrom
sur-ser wants to merge 1 commit intom8a-io:mainfrom
Conversation
1. P1: Falsy cached values (0, false, "", null) treated as cache misses
- Changed `if (cachedValue)` to `if (cachedValue !== undefined)`
2. P2: HTTP method not included in cache key
- Changed key from `request.url` to `${request.method}:${request.url}`
3. P2: Cache write failure prevents valid response from being returned
- Made cache set fire-and-forget with `.catch(() => {})`
4. P2: JSON.stringify in logger throws on circular references
- Wrapped in try/catch to prevent logging from crashing set()
5. P2: disconnect() only disconnects secondary, leaks primary connections
- Replaced manual secondary disconnect with `this.cache.disconnect()`
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
P1: Falsy cached values treated as cache misses
cacheable.interceptor.ts—if (cachedValue)fails for0,false,"". The cacheable library returnsundefinedon miss, so changed to!== undefined.P2: HTTP method not included in cache key
cacheable.interceptor.ts—GET /usersandPOST /usersproduce the same cache key, causing cross-method pollution. Added method to key.P2: Cache write failure kills valid response
cacheable.interceptor.ts— Ifset()throws (Redis down), the already-computed response is lost. Made cache write fire-and-forget so the response always returns. Note: the error is silently swallowed since the interceptor has no Logger instance — worth adding one later to catch Redis failures.P2: JSON.stringify in logger crashes on circular references
nestjs-cacheable.service.ts— Debug log withJSON.stringify(value)throws on circular objects, crashingset(). Wrapped in try/catch.P2: disconnect() leaks primary store connections
nestjs-cacheable.service.ts—disconnect()only closes secondary. UsedCacheable.disconnect()which handles both stores.No breaking changes
All fixes are backward-compatible. No new files, no refactoring, no API changes.