Add CacheTagService to @cacheable/utils#1646
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the CacheTagService utility, which provides tag-based cache invalidation on top of any Keyv store using a lazy invalidation model. The feedback highlights opportunities to parallelize sequential asynchronous operations using Promise.all in setKeyTags, isKeyFresh, and invalidateTags to prevent performance bottlenecks when handling multiple tags.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the CacheTagService to provide tag-based invalidation on top of any Keyv store using a lazy invalidation model. The feedback suggests defensive checks in getTagVersions to ensure compatibility with older or custom Keyv stores that might not return an array. Additionally, it recommends deduplicating the input tags in both setKeyTags and invalidateTags to prevent redundant store queries and duplicate writes.
| private async getTagVersions(tags: string[]): Promise<number[]> { | ||
| if (tags.length === 0) { | ||
| return []; | ||
| } | ||
| const tagKeys = tags.map((tag) => this.tagKey(tag)); | ||
| const raw = await this._store.get<number>(tagKeys); | ||
| return tags.map((_, i) => { | ||
| const value = raw?.[i]; | ||
| return typeof value === "number" ? value : 0; | ||
| }); | ||
| } |
There was a problem hiding this comment.
To ensure robustness against custom or older Keyv stores/adapters that might not return an array when queried with multiple keys, we should defensively verify that the returned value is indeed an array before accessing its elements by index.
| private async getTagVersions(tags: string[]): Promise<number[]> { | |
| if (tags.length === 0) { | |
| return []; | |
| } | |
| const tagKeys = tags.map((tag) => this.tagKey(tag)); | |
| const raw = await this._store.get<number>(tagKeys); | |
| return tags.map((_, i) => { | |
| const value = raw?.[i]; | |
| return typeof value === "number" ? value : 0; | |
| }); | |
| } | |
| private async getTagVersions(tags: string[]): Promise<number[]> { | |
| if (tags.length === 0) { | |
| return []; | |
| } | |
| const tagKeys = tags.map((tag) => this.tagKey(tag)); | |
| const raw = await this._store.get<number>(tagKeys); | |
| const rawArray = Array.isArray(raw) ? raw : []; | |
| return tags.map((_, i) => { | |
| const value = rawArray[i]; | |
| return typeof value === "number" ? value : 0; | |
| }); | |
| } |
There was a problem hiding this comment.
I'm not sure I agree.
Keyv's get(string[]) returns an array. That's the contract. If a store adapter ever breaks it, I'd rather see a thrown error than a silent fallback to "all versions = 0", which would make every key look fresh forever after invalidation.
The real cases (missing keys, undefined slots) already get handled by raw?.[i] and the typeof === "number" check.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
As suggested in #1640, this PR implements
CacheTagServicein the@cacheable/utilspackage. It is a standalone service that adds tag-based invalidation on top of any Keyv store, without touching adapters or thecacheablecore.The invalidation model is lazy, in the spirit of Symfony's tag-aware cache and Next.js's
revalidateTag:invalidateTagit bumps a per-tag version counter rather than scanning and deleting keys.isKeyFreshcompares each key's stored tag snapshot to current versions. That keeps invalidation constant-time at the cost of one extra read per cache lookup.Integration into
cacheableandcache-managerare intentionally left for follow-ups so this PR stays focused on the primitive. Optimisations per-store type (like an atomic Redis fast path using Lua) are also natural follow ups.