Fix/cache wrap function fixup#3169
Open
danoswaltCL wants to merge 3 commits into
Open
Conversation
… can be fetched from db, instead of being all-or-nothing
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is part of the Feature Flag optimizations work (issue #3144, item “4b”) and refactors the backend cache batching logic so callers can fetch only missing items on partial cache hits.
Changes:
- Replaces
CacheService.wrapFunctionwithCacheService.wrapMany, allowing DB fetch callbacks to receive only the missing keys and then merge cached + fetched results. - Updates
SegmentService.getSegmentByIdsto query only missing segment IDs and return results in missing-key order for alignment. - Updates unit tests to cover partial-hit behavior and ordering guarantees for
wrapMany.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/backend/src/api/services/CacheService.ts | Introduces wrapMany to batch cache reads/writes and fetch only cache misses. |
| packages/backend/src/api/services/SegmentService.ts | Switches segment batching to wrapMany and queries only missing segment IDs. |
| packages/backend/test/unit/services/CacheService.test.ts | Renames/extends tests to validate wrapMany partial-hit and ordering behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
105
to
112
| if (!keys.length) { | ||
| return []; | ||
| } | ||
|
|
||
| // find all cached values for the keys, which will return the value, or undefined if the key is missing. | ||
| const keysWithPrefix = keys.map((key) => prefix + key); | ||
| const cachedData = (await this.cache.store.mget(...keysWithPrefix)) as (T | undefined)[]; | ||
|
|
Comment on lines
+113
to
120
| // determine which keys are missing from the cache | ||
| const missingKeys = keys.filter((_, i) => !cachedData[i]); | ||
|
|
||
| // if none are missing, we're done, return the cached data | ||
| const allCachedFound = cachedData.length > 0 && cachedData.every((cached) => !!cached); | ||
| if (allCachedFound) { | ||
| return cachedData as T[]; | ||
| } |
Comment on lines
+100
to
+104
| public async wrapMany<T>( | ||
| prefix: CACHE_PREFIX, | ||
| keys: string[], | ||
| dbFetchCallback: (missingKeys: string[]) => Promise<T[]> | ||
| ): Promise<T[]> { |
Comment on lines
+169
to
+170
| // sort to match missingIds order so wrapMany can align with the full keys array | ||
| return missingIds.map((id) => result.find((data) => data.id === id)); |
|
|
||
| it('returns null and skips set when value is undefined', async () => { | ||
| const result = await service.setCache('key', undefined); | ||
| const result = await service.setCache('key', undefined as unknown as string); |
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
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.
this is 4b from #3144.
this is a lower-order thing and can go in 6.6, just putting it up now cuz i have it