fixed ttl inconsistency between usage of ioredis and node-redis clients#28
fixed ttl inconsistency between usage of ioredis and node-redis clients#28SerMedvid wants to merge 2 commits intomrjasonroy:mainfrom
Conversation
Summary of ChangesHello @SerMedvid, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an inconsistency in how Time-To-Live (TTL) is applied to Redis cache entries. The previous implementation used Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully fixes a bug where the TTL for Redis cache entries was not being set correctly. The change from ioredis-style arguments to node-redis object options for the set command is correct and addresses the issue. While the code changes are good, I've noted that related documentation and examples in the repository have not been updated to reflect the switch from ioredis to node-redis. An update is needed to maintain consistency and avoid confusion.
| export interface RedisDataCacheHandlerOptions { | ||
| /** | ||
| * Redis client instance (ioredis compatible) | ||
| * Redis client instance (node-redis) |
There was a problem hiding this comment.
This change correctly specifies that a node-redis client instance is expected. However, other parts of the repository still refer to ioredis. To ensure consistency and prevent confusion for users of this package, it's important to update all related documentation and examples to reflect this switch.
Specifically:
docs/redis.mdstill contains instructions and examples forioredis.apps/redis-example/data-cache-handler.mjsalso usesioredisfor its client setup.
These should be updated to use and refer to node-redis.
mrjasonroy
left a comment
There was a problem hiding this comment.
Thanks for identifying this inconsistency, @SerMedvid! The intent is correct — the RedisClient interface and docs target node-redis, so the set() call should use node-redis-style { EX: seconds }.
However, this change will silently break TTL when used through createCacheHandler() (the recommended/primary path for most users).
The issue is in packages/cache-handler/src/data-cache/factory.ts — the ioredis adapter only handles the old calling convention:
// factory.ts:32-37
set: (key, value, exFlag?, ttl?) => {
if (exFlag === "EX" && typeof ttl === "number") {
return redis.set(key, value, "EX", ttl);
}
return redis.set(key, value); // <-- TTL silently dropped!
},After this PR, the call becomes redis.set(key, value, { EX: seconds }). The adapter receives { EX: 120 } as exFlag, the === "EX" check fails, and it falls through to redis.set(key, value) — TTL is silently discarded.
To make this work, the factory adapter needs to be updated to handle the node-redis-style object arg:
set: (key, value, ...args) => {
const opts = args[0];
if (opts && typeof opts === "object" && typeof (opts as Record<string, unknown>).EX === "number") {
return redis.set(key, value, "EX", (opts as Record<string, unknown>).EX as number);
}
if (args[0] === "EX" && typeof args[1] === "number") {
return redis.set(key, value, "EX", args[1]);
}
return redis.set(key, value);
},Plus a test covering the factory path with TTL.
Would you be open to updating the PR to include the factory adapter change? Happy to help if needed.
The RedisClient interface now uses node-redis style SET options
({ EX: seconds }). The factory adapter must translate these to
ioredis's positional format ("EX", seconds).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
33c09f6 to
ad841ce
Compare
|
Rebased onto main and added a commit to update the factory adapter ( All 95 tests pass. Ready to merge once CI is green. |
Description
The data cache Redis implementation is designed to use a node-redis instance (as indicated by the examples and Redis interface). However, the TTL is currently being set using ioredis syntax, which is incompatible with node-redis. As a result, the TTL on redis entry is not being set at all.
Type of Change
Testing
pnpm test)pnpm test:e2e)Checklist
pnpm lintpasses)pnpm formatto format my codepnpm typecheck)Related Issues
Additional Context