This repository was archived by the owner on May 12, 2026. It is now read-only.
feat(BE-012): implement core redis service#8
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a global RedisModule and RedisService (ioredis client lifecycle and wrappers), registers the module in AppModule, adds REDIS_MESSAGES, includes Jest tests with mocked ioredis, and updates README commands to use Yarn. ChangesRedis Module Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
elijaharhinful
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/redis/services/redis.service.ts`:
- Line 57: The error logs in Redis service methods currently include raw keys
(e.g., the this.logger.error call that logs `GET failed [key=${key}]`) which can
leak sensitive identifiers; create and use a helper function (e.g.,
redactRedisKey or hashRedisKey) inside the RedisService to return redacted
metadata (namespace/prefix and a deterministic short hash) and replace all
direct key interpolations in error logs (references: the GET logging line and
analogous logger.error usages in the same file — methods like get, set, del, and
any other methods around the mentioned lines) to log the redacted value instead
of the raw key; ensure the helper is deterministic and non-reversible and update
all logger.error invocations listed in the review (lines referenced) to use that
helper.
- Around line 62-68: The set method currently uses a truthy check on ttl which
treats 0 as "no TTL"; update RedisService.async set(key: string, value: string,
ttl?: number) to explicitly validate ttl (e.g., check ttl !== undefined && ttl
!== null and optionally Number.isInteger(ttl) && ttl >= 0) before calling
this.client.set with the 'EX' argument so a ttl of 0 is handled intentionally
(either set as immediate expiry or treated as a valid numeric TTL per your
intended semantics).
- Around line 100-113: The delByPattern implementation (async delByPattern)
currently accumulates all keys into keysToDelete before calling this.client.del
which risks high memory and oversized commands; change the SCAN loop to delete
in batches (e.g., collect up to a safe BATCH_SIZE or delete keys returned from
each scan immediately) instead of pushing every key into keysToDelete, call
this.client.del with each batch when non-empty, and update the this.logger.log
to report per-batch deletions or a running total; ensure you still iterate until
cursor === '0' and avoid calling del with zero arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b2dd438-0031-4840-a96f-38723b11b4d7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.env.examplesrc/app.module.tssrc/modules/redis/redis.module.tssrc/modules/redis/services/redis.service.tssrc/modules/redis/tests/redis.service.spec.ts
Comment on lines
+62
to
+68
| async set(key: string, value: string, ttl?: number): Promise<void> { | ||
| try { | ||
| if (ttl) { | ||
| await this.client.set(key, value, 'EX', ttl); | ||
| } else { | ||
| await this.client.set(key, value); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate TTL explicitly instead of truthy-checking it.
if (ttl) on Line 64 treats 0 as “no TTL” and silently writes a persistent key. That can break expiry-based flows (rate limits/session revocation windows).
Proposed fix
async set(key: string, value: string, ttl?: number): Promise<void> {
try {
- if (ttl) {
+ if (ttl !== undefined) {
+ if (!Number.isInteger(ttl) || ttl <= 0) {
+ throw new Error(`Invalid TTL for key ${key}: ${ttl}`);
+ }
await this.client.set(key, value, 'EX', ttl);
} else {
await this.client.set(key, value);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async set(key: string, value: string, ttl?: number): Promise<void> { | |
| try { | |
| if (ttl) { | |
| await this.client.set(key, value, 'EX', ttl); | |
| } else { | |
| await this.client.set(key, value); | |
| } | |
| async set(key: string, value: string, ttl?: number): Promise<void> { | |
| try { | |
| if (ttl !== undefined) { | |
| if (!Number.isInteger(ttl) || ttl <= 0) { | |
| throw new Error(`Invalid TTL for key ${key}: ${ttl}`); | |
| } | |
| await this.client.set(key, value, 'EX', ttl); | |
| } else { | |
| await this.client.set(key, value); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/services/redis.service.ts` around lines 62 - 68, The set
method currently uses a truthy check on ttl which treats 0 as "no TTL"; update
RedisService.async set(key: string, value: string, ttl?: number) to explicitly
validate ttl (e.g., check ttl !== undefined && ttl !== null and optionally
Number.isInteger(ttl) && ttl >= 0) before calling this.client.set with the 'EX'
argument so a ttl of 0 is handled intentionally (either set as immediate expiry
or treated as a valid numeric TTL per your intended semantics).
Comment on lines
+100
to
+113
| async delByPattern(pattern: string): Promise<void> { | ||
| try { | ||
| let cursor = '0'; | ||
| const keysToDelete: string[] = []; | ||
|
|
||
| do { | ||
| const [nextCursor, keys] = await this.client.scan(cursor, 'MATCH', pattern, 'COUNT', 100); | ||
| cursor = nextCursor; | ||
| keysToDelete.push(...keys); | ||
| } while (cursor !== '0'); | ||
|
|
||
| if (keysToDelete.length > 0) { | ||
| await this.client.del(...keysToDelete); | ||
| this.logger.log(`delByPattern: deleted ${keysToDelete.length} keys matching [${pattern}]`); |
Contributor
There was a problem hiding this comment.
Avoid accumulating all matched keys before deleting.
Current logic stores every matched key in memory, then sends one large DEL. On large datasets this can cause memory pressure and oversized commands.
Proposed fix
async delByPattern(pattern: string): Promise<void> {
try {
let cursor = '0';
- const keysToDelete: string[] = [];
+ let totalDeleted = 0;
do {
const [nextCursor, keys] = await this.client.scan(cursor, 'MATCH', pattern, 'COUNT', 100);
cursor = nextCursor;
- keysToDelete.push(...keys);
+ if (keys.length > 0) {
+ await this.client.del(...keys);
+ totalDeleted += keys.length;
+ }
} while (cursor !== '0');
- if (keysToDelete.length > 0) {
- await this.client.del(...keysToDelete);
- this.logger.log(`delByPattern: deleted ${keysToDelete.length} keys matching [${pattern}]`);
+ if (totalDeleted > 0) {
+ this.logger.log(`delByPattern: deleted ${totalDeleted} keys matching [${pattern}]`);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/services/redis.service.ts` around lines 100 - 113, The
delByPattern implementation (async delByPattern) currently accumulates all keys
into keysToDelete before calling this.client.del which risks high memory and
oversized commands; change the SCAN loop to delete in batches (e.g., collect up
to a safe BATCH_SIZE or delete keys returned from each scan immediately) instead
of pushing every key into keysToDelete, call this.client.del with each batch
when non-empty, and update the this.logger.log to report per-batch deletions or
a running total; ensure you still iterate until cursor === '0' and avoid calling
del with zero arguments.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a global Redis module and service using the ioredis library, providing standard operations like GET, SET, and pattern-based deletion. The feedback identifies several opportunities to improve the service's robustness, specifically by batching deletions in delByPattern to prevent memory exhaustion, ensuring safe parsing of the Redis port configuration, and implementing a more graceful shutdown using quit() instead of disconnect().
Comment on lines
+102
to
+114
| let cursor = '0'; | ||
| const keysToDelete: string[] = []; | ||
|
|
||
| do { | ||
| const [nextCursor, keys] = await this.client.scan(cursor, 'MATCH', pattern, 'COUNT', 100); | ||
| cursor = nextCursor; | ||
| keysToDelete.push(...keys); | ||
| } while (cursor !== '0'); | ||
|
|
||
| if (keysToDelete.length > 0) { | ||
| await this.client.del(...keysToDelete); | ||
| this.logger.log(`delByPattern: deleted ${keysToDelete.length} keys matching [${pattern}]`); | ||
| } |
There was a problem hiding this comment.
Accumulating all keys in the keysToDelete array before calling del can lead to high memory consumption and potential process crashes (e.g., Maximum call stack size exceeded when spreading the array) if the pattern matches a large number of keys. It is more efficient and safer to delete keys in batches within the SCAN loop.
let cursor = '0';
let totalDeleted = 0;
do {
const [nextCursor, keys] = await this.client.scan(cursor, 'MATCH', pattern, 'COUNT', 100);
cursor = nextCursor;
if (keys.length > 0) {
await this.client.del(...keys);
totalDeleted += keys.length;
}
} while (cursor !== '0');
if (totalDeleted > 0) {
this.logger.log(`delByPattern: deleted ${totalDeleted} keys matching [${pattern}]`);
}|
|
||
| const options: RedisOptions = { | ||
| host, | ||
| port: +port, |
There was a problem hiding this comment.
The unary plus operator (+port) will result in NaN if the port variable is undefined or an invalid number string. It is safer to provide a default value or ensure the input is valid.
Suggested change
| port: +port, | |
| port: port ? Number(port) : 6379, |
| } | ||
|
|
||
| onModuleDestroy() { | ||
| this.client?.disconnect(); |
There was a problem hiding this comment.
Using disconnect() abruptly closes the connection. For a graceful shutdown during module destruction, quit() is preferred as it allows Redis to finish processing pending commands before closing.
await this.client?.quit();
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/redis/services/redis.service.ts`:
- Around line 24-25: The logger is currently receiving the function object
instead of the generated message; call the message factory with the retry count
and pass its result to the logger. Replace the call to
this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED) with
this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED(times)) (using the
existing times variable in the retry logic inside the Redis service method) so
the rendered string is logged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 438a0cda-1f75-46dc-9abf-083f2ecd15ff
📒 Files selected for processing (2)
src/modules/redis/services/redis.service.tssrc/shared/constants/SystemMessages.ts
Comment on lines
+24
to
+25
| this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED); | ||
| return null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify message factory usage in RedisService
rg -n "RETRY_LIMIT_REACHED" src/modules/redis/services/redis.service.ts src/shared/constants/SystemMessages.tsRepository: hngprojects/flowbrand-be
Length of output: 327
🏁 Script executed:
sed -n '10,35p' src/modules/redis/services/redis.service.tsRepository: hngprojects/flowbrand-be
Length of output: 878
Invoke RETRY_LIMIT_REACHED with times to render the message.
Line 24 passes the function object to the logger instead of calling it with the times parameter. The RETRY_LIMIT_REACHED is a message factory function that requires the times argument to generate the retry limit error text.
Suggested fix
- this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED);
+ this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED(times));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED); | |
| return null; | |
| this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED(times)); | |
| return null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/services/redis.service.ts` around lines 24 - 25, The logger
is currently receiving the function object instead of the generated message;
call the message factory with the retry count and pass its result to the logger.
Replace the call to
this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED) with
this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED(times)) (using the
existing times variable in the retry logic inside the Redis service method) so
the rendered string is logged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
src/modules/redis/services/redis.service.ts (3)
100-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete keys in batches to avoid memory pressure and oversized commands.
The current implementation accumulates all matching keys into
keysToDeletebefore issuing a singleDEL. On large pattern matches (e.g., thousands of session keys), this risks memory pressure and can exceed Redis max-arguments or request size limits.♻️ Proposed fix to delete per-batch
async delByPattern(pattern: string): Promise<void> { try { let cursor = '0'; - const keysToDelete: string[] = []; + let totalDeleted = 0; do { const [nextCursor, keys] = await this.client.scan(cursor, 'MATCH', pattern, 'COUNT', 100); cursor = nextCursor; - keysToDelete.push(...keys); + if (keys.length > 0) { + await this.client.del(...keys); + totalDeleted += keys.length; + } } while (cursor !== '0'); - if (keysToDelete.length > 0) { - await this.client.del(...keysToDelete); - this.logger.log(SYS_MSG.REDIS_MESSAGES.PATTERN_DELETE_SUCCESS(keysToDelete.length, pattern)); + if (totalDeleted > 0) { + this.logger.log(SYS_MSG.REDIS_MESSAGES.PATTERN_DELETE_SUCCESS(totalDeleted, pattern)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/redis/services/redis.service.ts` around lines 100 - 116, The delByPattern method currently accumulates all matching keys into keysToDelete then calls a single DEL, which can cause memory pressure or exceed Redis argument limits; modify delByPattern (in src/modules/redis/services/redis.service.ts) to delete each scan batch as you receive it: inside the do/while, after receiving keys from this.client.scan, if keys.length > 0 call this.client.del(...keys) (or pipeline/transaction if supported) and update a running deletedCount, avoiding pushing all keys into keysToDelete; after the loop log SYS_MSG.REDIS_MESSAGES.PATTERN_DELETE_SUCCESS(deletedCount, pattern) and keep the existing catch handling.
62-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate TTL explicitly to handle zero correctly.
The truthy check
if (ttl)treatsttl=0as "no TTL" and silently persists the key. For rate-limiting or session windows, callers might legitimately pass0expecting immediate expiry or a different semantic. This can break expiry-based flows.🔧 Proposed fix
async set(key: string, value: string, ttl?: number): Promise<void> { try { - if (ttl) { + if (ttl !== undefined) { + if (!Number.isInteger(ttl) || ttl < 0) { + throw new Error(`Invalid TTL: ${ttl}`); + } await this.client.set(key, value, 'EX', ttl); } else { await this.client.set(key, value); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/redis/services/redis.service.ts` around lines 62 - 66, The current truthy check `if (ttl)` in the Redis service treats 0 as "no TTL" and causes keys to be persisted; update the check to explicitly detect presence of a TTL (e.g., `ttl !== undefined && ttl !== null` or `Number.isFinite(ttl)`) so that zero is honored, and call `this.client.set(key, value, 'EX', ttl)` when a TTL is provided (including 0); adjust the code around the `ttl` variable in the Redis service method that calls `this.client.set` accordingly.
24-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall the message factory function with the retry count.
RETRY_LIMIT_REACHEDappears to be a message factory that requires thetimesparameter to render the retry limit error text, but Line 24 passes the function object instead of invoking it.🔧 Proposed fix
- this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED); + this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED(times));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/redis/services/redis.service.ts` at line 24, The logger call is passing the message factory function rather than invoking it; change the call to invoke SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED with the retry count from the surrounding scope (e.g., this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED(times)) using the actual retry variable name such as retryCount or times) so the rendered message string is logged instead of the function object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 273-277: The README has duplicated numbered steps causing
confusion: the "In Postman:" block currently uses "1." but an earlier step
("Access the Swagger JSON endpoint") is also numbered "1."; update the numbering
so the "In Postman:" step becomes "2." (or the correct sequential number) in the
README to maintain a proper ordered list; locate the "In Postman:" heading and
its four bullet substeps and change its leading list marker from "1." to "2." to
match the surrounding sequence.
- Line 49: The heading "If you don't have git on your machine, [install
it](https://docs.github.com/en/get-started/quickstart/set-up-git)" is using an
h4 (####) while skipping h3; change it to h3 (###) or add an appropriate h3
parent above it so the Markdown heading hierarchy is correct; locate the heading
text in README.md and update the markup to restore proper heading levels.
In `@src/modules/redis/services/redis.service.ts`:
- Around line 60-70: The set method currently swallows errors and returns void,
preventing callers from knowing whether the write succeeded; change
RedisService.set to return Promise<boolean>, return true after successful await
this.client.set(...) and return false from the catch block (while logging the
full error via this.logger.error with context and the error object), and update
any callers to handle the boolean result so critical flows can react to write
failures.
- Line 16: The Redis config currently uses a falsy check for the port value
("port ? Number(port) : 6379") which incorrectly overrides explicit values like
0; change the ternary to an explicit undefined check so the config uses the
provided port when set (e.g. use "port !== undefined ? Number(port) : 6379"),
updating the expression where the port property is set in the Redis
client/config creation (the "port" variable used in the redis config in
redis.service.ts).
- Around line 46-48: The current this.client.connect().catch(...) in
onModuleInit swallows initial Redis connection failures; either make
onModuleInit async and await this.client.connect() and re-throw/log the error so
module init fails fast (update onModuleInit signature and keep the logger call
using SYS_MSG.REDIS_MESSAGES.INITIAL_CONNECTION_FAILED and err), or implement
connection state tracking (add a boolean like isRedisConnected on the
RedisService, set it true/false in connect()/on('error') handlers and expose a
getter used by health checks) so startup succeeds but health endpoints can
report Redis availability; pick one approach and update usages of
this.client.connect(), onModuleInit, and any health/DI consumers accordingly.
In `@src/modules/redis/tests/redis.service.spec.ts`:
- Around line 89-97: Add a test that verifies delByPattern correctly handles
multi-page SCAN results: mock mockRedisInstance.scan to first return a non-'0'
cursor (e.g., ['123', ['key1','key2']]) and then ['0', ['key3']], mock
mockRedisInstance.del to resolve, call service.delByPattern('prefix:*'), and
assert that scan was called twice and that del was invoked with the expected
keys (either as a single aggregated call with 'key1','key2','key3' or as
multiple calls covering all keys depending on the delByPattern implementation);
reference the delByPattern method and the
mockRedisInstance.scan/mockRedisInstance.del mocks when adding the test.
- Around line 51-55: Add a test for the TTL=0 edge case that calls
service.set('temp:key','value',0) with mockRedisInstance.set mocked to resolve
and then asserts mockRedisInstance.set was called with the EX argument and 0
(e.g.
expect(mockRedisInstance.set).toHaveBeenCalledWith('temp:key','value','EX',0));
name the test "set with TTL=0 should handle zero explicitly" to cover the
truthy-check bug in the set implementation and ensure service.set handles zero
TTL correctly.
- Around line 45-97: Add error-path unit tests for service.set, service.del,
service.exists, and service.incr: for each test spy on service['logger'].error,
mock the corresponding mockRedisInstance method (set/del/exists/incr) to reject
with new Error('Connection lost'), call the service method and assert it does
not throw, that logger.error was called with a short identifying message (e.g.
'SET failed', 'DEL failed', 'EXISTS failed', 'INCR failed') and the error
message, and that the service returns the safe default (set: completes without
throw, del: resolves to 0, exists: resolves to false, incr: resolves to 0) to
match existing error-handling behavior in the RedisService methods.
---
Duplicate comments:
In `@src/modules/redis/services/redis.service.ts`:
- Around line 100-116: The delByPattern method currently accumulates all
matching keys into keysToDelete then calls a single DEL, which can cause memory
pressure or exceed Redis argument limits; modify delByPattern (in
src/modules/redis/services/redis.service.ts) to delete each scan batch as you
receive it: inside the do/while, after receiving keys from this.client.scan, if
keys.length > 0 call this.client.del(...keys) (or pipeline/transaction if
supported) and update a running deletedCount, avoiding pushing all keys into
keysToDelete; after the loop log
SYS_MSG.REDIS_MESSAGES.PATTERN_DELETE_SUCCESS(deletedCount, pattern) and keep
the existing catch handling.
- Around line 62-66: The current truthy check `if (ttl)` in the Redis service
treats 0 as "no TTL" and causes keys to be persisted; update the check to
explicitly detect presence of a TTL (e.g., `ttl !== undefined && ttl !== null`
or `Number.isFinite(ttl)`) so that zero is honored, and call
`this.client.set(key, value, 'EX', ttl)` when a TTL is provided (including 0);
adjust the code around the `ttl` variable in the Redis service method that calls
`this.client.set` accordingly.
- Line 24: The logger call is passing the message factory function rather than
invoking it; change the call to invoke
SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED with the retry count from the
surrounding scope (e.g.,
this.logger.error(SYS_MSG.REDIS_MESSAGES.RETRY_LIMIT_REACHED(times)) using the
actual retry variable name such as retryCount or times) so the rendered message
string is logged instead of the function object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ce9bd42-fff4-4c7c-9448-2e1ab22e33b4
📒 Files selected for processing (4)
README.mdsrc/app.module.tssrc/modules/redis/services/redis.service.tssrc/modules/redis/tests/redis.service.spec.ts
| ## Getting Started | ||
|
|
||
| #### If you don't have git on your machine, [install it](https://docs.github.com/en/get-started/quickstart/set-up-git). | ||
| #### If you don't have git on your machine, [install it](https://docs.github.com/en/get-started/quickstart/set-up-git) |
Contributor
There was a problem hiding this comment.
Fix heading level hierarchy.
This h4 heading skips h3, violating markdown structure conventions. Change to h3 or introduce an intermediate h3 parent.
📝 Proposed fix
-#### If you don't have git on your machine, [install it](https://docs.github.com/en/get-started/quickstart/set-up-git)
+### If you don't have git on your machine, [install it](https://docs.github.com/en/get-started/quickstart/set-up-git)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #### If you don't have git on your machine, [install it](https://docs.github.com/en/get-started/quickstart/set-up-git) | |
| ### If you don't have git on your machine, [install it](https://docs.github.com/en/get-started/quickstart/set-up-git) |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 49-49: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 49, The heading "If you don't have git on your machine,
[install it](https://docs.github.com/en/get-started/quickstart/set-up-git)" is
using an h4 (####) while skipping h3; change it to h3 (###) or add an
appropriate h3 parent above it so the Markdown heading hierarchy is correct;
locate the heading text in README.md and update the markup to restore proper
heading levels.
Comment on lines
+273
to
277
| 1. In Postman: | ||
| - Click on "Import" in the top left corner | ||
| - Select "Link" tab | ||
| - Paste the Swagger JSON URL (http://localhost:3000/api/docs-json) | ||
| - Paste the Swagger JSON URL (<http://localhost:3000/api/docs-json>) | ||
| - Click "Continue" and then "Import" |
Contributor
There was a problem hiding this comment.
Fix duplicate step numbering in Postman instructions.
Both step 267 ("Access the Swagger JSON endpoint") and step 273 ("In Postman:") are numbered "1.", causing confusion.
📝 Proposed fix
-1. Access the Swagger JSON endpoint:
+1. Access the Swagger JSON endpoint:
```http
http://localhost:3000/api/docs-json-1. In Postman:
+2. In Postman:
- Click on "Import" in the top left corner
- Select "Link" tab
- Paste the Swagger JSON URL (http://localhost:3000/api/docs-json)
- Click "Continue" and then "Import"
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
2. In Postman:
- Click on "Import" in the top left corner
- Select "Link" tab
- Paste the Swagger JSON URL (<http://localhost:3000/api/docs-json>)
- Click "Continue" and then "Import"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 273 - 277, The README has duplicated numbered steps
causing confusion: the "In Postman:" block currently uses "1." but an earlier
step ("Access the Swagger JSON endpoint") is also numbered "1."; update the
numbering so the "In Postman:" step becomes "2." (or the correct sequential
number) in the README to maintain a proper ordered list; locate the "In
Postman:" heading and its four bullet substeps and change its leading list
marker from "1." to "2." to match the surrounding sequence.
|
|
||
| const options: RedisOptions = { | ||
| host, | ||
| port: port ? Number(port) : 6379, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use explicit undefined check for port configuration.
port ? Number(port) : 6379 will default to 6379 if port is 0, '', or other falsy values. If the config explicitly sets port to 0 (e.g., to disable or signal a special case), this silently overrides it.
🔧 Proposed fix
- port: port ? Number(port) : 6379,
+ port: port !== undefined ? Number(port) : 6379,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/services/redis.service.ts` at line 16, The Redis config
currently uses a falsy check for the port value ("port ? Number(port) : 6379")
which incorrectly overrides explicit values like 0; change the ternary to an
explicit undefined check so the config uses the provided port when set (e.g. use
"port !== undefined ? Number(port) : 6379"), updating the expression where the
port property is set in the Redis client/config creation (the "port" variable
used in the redis config in redis.service.ts).
Comment on lines
+46
to
+48
| this.client.connect().catch(err => { | ||
| this.logger.error(SYS_MSG.REDIS_MESSAGES.INITIAL_CONNECTION_FAILED, err.message); | ||
| }); |
Contributor
There was a problem hiding this comment.
Consider surfacing initial connection failures to prevent silent degradation.
The .catch() on Line 46 logs the error but allows the service to initialize successfully even if Redis is unreachable. Subsequent calls will fail silently (returning null or false), which may hide critical infrastructure issues from health checks or startup validation.
Consider either:
- Re-throwing the error to fail fast during module init, or
- Tracking connection state and exposing it via a health check endpoint
Option 1: Fail fast
- this.client.connect().catch(err => {
- this.logger.error(SYS_MSG.REDIS_MESSAGES.INITIAL_CONNECTION_FAILED, err.message);
- });
+ await this.client.connect();Then mark onModuleInit as async:
- onModuleInit() {
+ async onModuleInit() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/services/redis.service.ts` around lines 46 - 48, The
current this.client.connect().catch(...) in onModuleInit swallows initial Redis
connection failures; either make onModuleInit async and await
this.client.connect() and re-throw/log the error so module init fails fast
(update onModuleInit signature and keep the logger call using
SYS_MSG.REDIS_MESSAGES.INITIAL_CONNECTION_FAILED and err), or implement
connection state tracking (add a boolean like isRedisConnected on the
RedisService, set it true/false in connect()/on('error') handlers and expose a
getter used by health checks) so startup succeeds but health endpoints can
report Redis availability; pick one approach and update usages of
this.client.connect(), onModuleInit, and any health/DI consumers accordingly.
Comment on lines
+60
to
+70
| async set(key: string, value: string, ttl?: number): Promise<void> { | ||
| try { | ||
| if (ttl) { | ||
| await this.client.set(key, value, 'EX', ttl); | ||
| } else { | ||
| await this.client.set(key, value); | ||
| } | ||
| } catch (err) { | ||
| this.logger.error(`SET failed`, (err as Error).message); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Consider returning success/failure status from set.
The method swallows all errors and returns void, making it impossible for callers to detect write failures (e.g., OOM, network partition). For critical flows like session creation or OTP storage, the caller may need to know if the write succeeded.
Example: Return boolean
- async set(key: string, value: string, ttl?: number): Promise<void> {
+ async set(key: string, value: string, ttl?: number): Promise<boolean> {
try {
if (ttl !== undefined) {
if (!Number.isInteger(ttl) || ttl < 0) {
throw new Error(`Invalid TTL: ${ttl}`);
}
await this.client.set(key, value, 'EX', ttl);
} else {
await this.client.set(key, value);
}
+ return true;
} catch (err) {
this.logger.error(`SET failed`, (err as Error).message);
+ return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async set(key: string, value: string, ttl?: number): Promise<void> { | |
| try { | |
| if (ttl) { | |
| await this.client.set(key, value, 'EX', ttl); | |
| } else { | |
| await this.client.set(key, value); | |
| } | |
| } catch (err) { | |
| this.logger.error(`SET failed`, (err as Error).message); | |
| } | |
| } | |
| async set(key: string, value: string, ttl?: number): Promise<boolean> { | |
| try { | |
| if (ttl !== undefined) { | |
| if (!Number.isInteger(ttl) || ttl < 0) { | |
| throw new Error(`Invalid TTL: ${ttl}`); | |
| } | |
| await this.client.set(key, value, 'EX', ttl); | |
| } else { | |
| await this.client.set(key, value); | |
| } | |
| return true; | |
| } catch (err) { | |
| this.logger.error(`SET failed`, (err as Error).message); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/services/redis.service.ts` around lines 60 - 70, The set
method currently swallows errors and returns void, preventing callers from
knowing whether the write succeeded; change RedisService.set to return
Promise<boolean>, return true after successful await this.client.set(...) and
return false from the catch block (while logging the full error via
this.logger.error with context and the error object), and update any callers to
handle the boolean result so critical flows can react to write failures.
Comment on lines
+45
to
+97
| it('set stores value without TTL', async () => { | ||
| mockRedisInstance.set.mockResolvedValue('OK'); | ||
| await service.set('sess:user1:abc', '{"role":"user"}'); | ||
| expect(mockRedisInstance.set).toHaveBeenCalledWith('sess:user1:abc', '{"role":"user"}'); | ||
| }); | ||
|
|
||
| it('set stores value with TTL using EX', async () => { | ||
| mockRedisInstance.set.mockResolvedValue('OK'); | ||
| await service.set('otp:user1:login', '123456', 300); | ||
| expect(mockRedisInstance.set).toHaveBeenCalledWith('otp:user1:login', '123456', 'EX', 300); | ||
| }); | ||
|
|
||
| it('get returns value for existing key', async () => { | ||
| mockRedisInstance.get.mockResolvedValue('{"role":"user"}'); | ||
| expect(await service.get('sess:user1:abc')).toBe('{"role":"user"}'); | ||
| }); | ||
|
|
||
| it('get returns null for missing key', async () => { | ||
| mockRedisInstance.get.mockResolvedValue(null); | ||
| expect(await service.get('sess:ghost')).toBeNull(); | ||
| }); | ||
|
|
||
| it('del calls delete on correct key', async () => { | ||
| mockRedisInstance.del.mockResolvedValue(1); | ||
| await service.del('sess:user1:abc'); | ||
| expect(mockRedisInstance.del).toHaveBeenCalledWith('sess:user1:abc'); | ||
| }); | ||
|
|
||
| it('exists returns true when key present', async () => { | ||
| mockRedisInstance.exists.mockResolvedValue(1); | ||
| expect(await service.exists('otp:user1:login')).toBe(true); | ||
| }); | ||
|
|
||
| it('exists returns false when key absent', async () => { | ||
| mockRedisInstance.exists.mockResolvedValue(0); | ||
| expect(await service.exists('otp:ghost')).toBe(false); | ||
| }); | ||
|
|
||
| it('incr increments atomically', async () => { | ||
| mockRedisInstance.incr.mockResolvedValueOnce(1).mockResolvedValueOnce(2); | ||
| expect(await service.incr('rl:user1:/api/login')).toBe(1); | ||
| expect(await service.incr('rl:user1:/api/login')).toBe(2); | ||
| }); | ||
|
|
||
| it('delByPattern scans and deletes matching keys only', async () => { | ||
| mockRedisInstance.scan.mockResolvedValueOnce(['0', ['sess:user99:s1', 'sess:user99:s2']]); | ||
| mockRedisInstance.del.mockResolvedValue(2); | ||
|
|
||
| await service.delByPattern('sess:user99:*'); | ||
|
|
||
| expect(mockRedisInstance.scan).toHaveBeenCalledWith('0', 'MATCH', 'sess:user99:*', 'COUNT', 100); | ||
| expect(mockRedisInstance.del).toHaveBeenCalledWith('sess:user99:s1', 'sess:user99:s2'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding error-path tests for remaining methods.
Only get has an error-handling test (Line 99-102). Adding similar tests for set, del, exists, and incr would verify that errors are logged and safe defaults returned.
💚 Example: set error test
it('set logs error and completes when Redis fails', async () => {
const loggerSpy = jest.spyOn(service['logger'], 'error');
mockRedisInstance.set.mockRejectedValue(new Error('Connection lost'));
await service.set('key', 'value'); // Should not throw
expect(loggerSpy).toHaveBeenCalledWith(
'SET failed',
'Connection lost'
);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/tests/redis.service.spec.ts` around lines 45 - 97, Add
error-path unit tests for service.set, service.del, service.exists, and
service.incr: for each test spy on service['logger'].error, mock the
corresponding mockRedisInstance method (set/del/exists/incr) to reject with new
Error('Connection lost'), call the service method and assert it does not throw,
that logger.error was called with a short identifying message (e.g. 'SET
failed', 'DEL failed', 'EXISTS failed', 'INCR failed') and the error message,
and that the service returns the safe default (set: completes without throw,
del: resolves to 0, exists: resolves to false, incr: resolves to 0) to match
existing error-handling behavior in the RedisService methods.
Comment on lines
+51
to
+55
| it('set stores value with TTL using EX', async () => { | ||
| mockRedisInstance.set.mockResolvedValue('OK'); | ||
| await service.set('otp:user1:login', '123456', 300); | ||
| expect(mockRedisInstance.set).toHaveBeenCalledWith('otp:user1:login', '123456', 'EX', 300); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for TTL edge cases.
The test suite doesn't verify behavior when ttl=0 is passed to set. Given the truthy-check bug in the implementation (Line 62 of redis.service.ts), a test case would catch regressions if the fix is applied.
💚 Suggested test case
it('set with TTL=0 should handle zero explicitly', async () => {
mockRedisInstance.set.mockResolvedValue('OK');
await service.set('temp:key', 'value', 0);
// After fixing the implementation, this should call set with EX 0 or throw
expect(mockRedisInstance.set).toHaveBeenCalledWith('temp:key', 'value', 'EX', 0);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/tests/redis.service.spec.ts` around lines 51 - 55, Add a
test for the TTL=0 edge case that calls service.set('temp:key','value',0) with
mockRedisInstance.set mocked to resolve and then asserts mockRedisInstance.set
was called with the EX argument and 0 (e.g.
expect(mockRedisInstance.set).toHaveBeenCalledWith('temp:key','value','EX',0));
name the test "set with TTL=0 should handle zero explicitly" to cover the
truthy-check bug in the set implementation and ensure service.set handles zero
TTL correctly.
Comment on lines
+89
to
+97
| it('delByPattern scans and deletes matching keys only', async () => { | ||
| mockRedisInstance.scan.mockResolvedValueOnce(['0', ['sess:user99:s1', 'sess:user99:s2']]); | ||
| mockRedisInstance.del.mockResolvedValue(2); | ||
|
|
||
| await service.delByPattern('sess:user99:*'); | ||
|
|
||
| expect(mockRedisInstance.scan).toHaveBeenCalledWith('0', 'MATCH', 'sess:user99:*', 'COUNT', 100); | ||
| expect(mockRedisInstance.del).toHaveBeenCalledWith('sess:user99:s1', 'sess:user99:s2'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add test for delByPattern with multiple scan iterations.
The current test mocks a single-scan scenario (cursor returns '0' immediately). A test with cursor !== '0' would verify the loop correctly exhausts all batches.
💚 Suggested test case
it('delByPattern handles multi-page scan results', async () => {
mockRedisInstance.scan
.mockResolvedValueOnce(['123', ['key1', 'key2']])
.mockResolvedValueOnce(['0', ['key3']]);
mockRedisInstance.del.mockResolvedValue(3);
await service.delByPattern('prefix:*');
expect(mockRedisInstance.scan).toHaveBeenCalledTimes(2);
expect(mockRedisInstance.del).toHaveBeenCalledTimes(1);
expect(mockRedisInstance.del).toHaveBeenCalledWith('key1', 'key2', 'key3');
});Note: After fixing the batch-delete issue in the implementation, the assertion should verify multiple del calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/redis/tests/redis.service.spec.ts` around lines 89 - 97, Add a
test that verifies delByPattern correctly handles multi-page SCAN results: mock
mockRedisInstance.scan to first return a non-'0' cursor (e.g., ['123',
['key1','key2']]) and then ['0', ['key3']], mock mockRedisInstance.del to
resolve, call service.delByPattern('prefix:*'), and assert that scan was called
twice and that del was invoked with the expected keys (either as a single
aggregated call with 'key1','key2','key3' or as multiple calls covering all keys
depending on the delByPattern implementation); reference the delByPattern method
and the mockRedisInstance.scan/mockRedisInstance.del mocks when adding the test.
This was referenced May 9, 2026
Merged
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Pull Request
Description
Implements a centralized, globally available Redis service (
RedisService) to manage session states, rate limiting, and OTP hashes consistently across the application.Issue #(BE-012)
Type of Change
How Has This Been Tested?
Test Evidence
Use cases
get(key)Retrieves a stored value by key. Returns
nullif the key does not exist or Redis is unreachable.Use cases:
AuthGuardreadssess:{userId}:{sessionId}to verify a session is still active before granting access to a protected route.OtpServicereadsotp:{userId}:{action}to retrieve a stored OTP for comparison against the user-submitted code during verification.set(key, value, ttl?)Stores a value under a key with an optional TTL in seconds. Redis automatically evicts the key when the TTL expires .
Use cases:
AuthServicestores a session object after login:sess:{userId}:{sessionId}with TTL matching the JWT expiry.OtpServicestores a generated OTP code:otp:{userId}:{action}with a 5-minute TTL so codes expire automatically.del(key)Removes a single key immediately, regardless of its TTL.
Use cases:
AuthServicedeletessess:{userId}:{sessionId}on logout so the session is immediately revoked even if the JWT has not yet expired.OtpServicedeletesotp:{userId}:{action}after a code has been successfully verified so it cannot be reused.exists(key)Returns
trueif a key exists,falseotherwise. Avoids the overhead of fetching and parsing a full value just to check presence.Use cases:
AuthGuardperforms a lightweight session presence check before parsing the full session payload.OtpServicechecks whether an unexpired OTP already exists before generating and sending a new one, preventing OTP flooding.incr(key)Atomically increments a numeric counter stored at a key by 1. If the key does not exist, Redis initialises it at 0 before incrementing, returning 1. Atomicity is guaranteed by Redis's single-threaded command execution. No race conditions under concurrent requests.
Use cases:
RateLimitGuardtracks the number of requests from a user to a specific endpoint within a time window. If the count exceeds the threshold, the request is rejected with429 Too Many Requests.delByPattern(pattern)Deletes all keys matching a glob pattern. Uses
SCANin batches of 100 instead ofKEYSto avoid blocking the Redis event in production under high key volume.Use cases:
AuthServiceinvalidates all active sessions for a user during a security event (password reset, account compromise, forced logout from all devices) by deleting everysess:{userId}:*key at once.RateLimitServiceclears all rate limit counters for a user after an admin override or penalty period reset.Checklist
Additional Notes
Summary by CodeRabbit