Cache issues related to entity_type , entity and notifications #1607
Cache issues related to entity_type , entity and notifications #1607sumanvpacewisdom wants to merge 2 commits into
Conversation
Feature/tenant phase2 temp branch -1
…ficiency. Updated cache set and delete methods in cacheHelper for session and user data. Simplified cache invalidation logic in EntityHelper and NotificationTemplateHelper by capturing old values before updates. Removed redundant cache invalidation after entity operations to streamline processing.
sumanvpacewisdom
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request refactors caching logic across multiple services. Changes include aligning cache API call signatures to remove Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/generics/cacheHelper.js (3)
1331-1333:⚠️ Potential issue | 🔴 CriticalBug: Reference to undefined variable
userIsMentorMap.Line 1333 references
userIsMentorMapwhich is never declared in thegetMenteeKafkamethod. This will throw aReferenceErrorat runtime when users are fetched from the database.🐛 Proposed fix - remove dead code
These lines appear to be leftover from a previous implementation. Since
userMentorEntriesanduserIsMentorMapare not used anywhere in thegetMenteeKafkamethod's return flow, they should be removed:if (usersFromDb && usersFromDb.length > 0) { // Cache each fetched user individually for (const user of usersFromDb) { await this.set(tenantCode, user.user_id, user) } // Add fetched users to result dbFetchedUsers.push(...usersFromDb) - // Create user ID to is_mentor mapping using map - const userMentorEntries = usersFromDb.map((user) => [user.user_id, user.is_mentor || false]) - Object.assign(userIsMentorMap, Object.fromEntries(userMentorEntries)) - console.log(`✅ [getMenteeKafka] ${usersFromDb.length} users fetched from DB and cached`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generics/cacheHelper.js` around lines 1331 - 1333, The block in getMenteeKafka that builds userMentorEntries and calls Object.assign on the undeclared userIsMentorMap should be removed; locate the lines creating userMentorEntries and the Object.assign(userIsMentorMap, ...) inside the getMenteeKafka function and delete them (they are unused and cause a ReferenceError for userIsMentorMap). Ensure no other code in getMenteeKafka depends on userMentorEntries or userIsMentorMap after removal.
1064-1082:⚠️ Potential issue | 🔴 CriticalBug: Reassignment to
constvariable causes runtime error.Line 1064 declares
const userMentorEntries = [], but line 1074 attempts to reassign it withuserMentorEntries = usersFromDb.map(...). This will throw aTypeError: Assignment to constant variableat runtime.🐛 Proposed fix
const dbFetchedUsers = [] -const userMentorEntries = [] +let userMentorEntries = [] if (missingUserIds.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generics/cacheHelper.js` around lines 1064 - 1082, The code declares userMentorEntries as a const and later reassigns it with usersFromDb.map, causing a runtime TypeError; change the declaration of userMentorEntries to a mutable binding (let) or avoid reassignment by populating the existing array (e.g., push/concat) so that the assignment at userMentorEntries = usersFromDb.map(...) is removed; update the declaration near userMentorEntries and ensure the rest of the logic that uses userMentorEntries (the mapping derived from usersFromDb) continues to work with the chosen approach.
1154-1158:⚠️ Potential issue | 🔴 CriticalFix incorrect cache invalidation call with wrong parameters.
The
cacheHelper.mentee.delete()method signature accepts 2 parameters(tenantCode, menteeId), but the call atsrc/services/admin.js:697passes 3 arguments:await cacheHelper.mentee.delete(tenantCode, menteeData.organization_code, menteeData.mentee_id)Since
organization_codeis not a field in theSessionAttendeemodel (onlymentee_idexists), this passesundefinedas thementeeIdparameter while the actualmentee_idis ignored. This results in an invalid cache key being constructed, and the mentee's cache entry is never properly invalidated.Fix: Change the call to pass only the correct parameters:
await cacheHelper.mentee.delete(tenantCode, menteeData.mentee_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generics/cacheHelper.js` around lines 1154 - 1158, The call to cacheHelper.mentee.delete is passing three args causing menteeId to be undefined; update the caller in admin (where it calls cacheHelper.mentee.delete(tenantCode, menteeData.organization_code, menteeData.mentee_id)) to pass only the correct two parameters — tenantCode and menteeData.mentee_id — so cacheHelper.mentee.delete(tenantCode, menteeData.mentee_id) constructs the right key (buildKey / nsUseInternal('mentee')) and properly invalidates the mentee cache.
🧹 Nitpick comments (1)
src/services/entity-type.js (1)
343-349: Note: Duplicate cache clearing call.
_clearUserCachesForEntityTypeChangeis called twice in the delete flow - once at line 292-297 (before deletion) and again here at lines 344-349 (after deletion). While redundant, this is defensive and not harmful. Consider consolidating to a single call after deletion for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/entity-type.js` around lines 343 - 349, Duplicate call to _clearUserCachesForEntityTypeChange is made in the delete flow; consolidate to a single call after the entity is deleted by removing the redundant pre-deletion invocation and keeping the post-deletion call that passes (organizationCode, tenantCode, entityToDelete.model_names ? entityToDelete.model_names[0] : null, entityToDelete.value); ensure the single remaining call is executed after the deletion completes so caches reflect the final state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/generics/cacheHelper.js`:
- Around line 1331-1333: The block in getMenteeKafka that builds
userMentorEntries and calls Object.assign on the undeclared userIsMentorMap
should be removed; locate the lines creating userMentorEntries and the
Object.assign(userIsMentorMap, ...) inside the getMenteeKafka function and
delete them (they are unused and cause a ReferenceError for userIsMentorMap).
Ensure no other code in getMenteeKafka depends on userMentorEntries or
userIsMentorMap after removal.
- Around line 1064-1082: The code declares userMentorEntries as a const and
later reassigns it with usersFromDb.map, causing a runtime TypeError; change the
declaration of userMentorEntries to a mutable binding (let) or avoid
reassignment by populating the existing array (e.g., push/concat) so that the
assignment at userMentorEntries = usersFromDb.map(...) is removed; update the
declaration near userMentorEntries and ensure the rest of the logic that uses
userMentorEntries (the mapping derived from usersFromDb) continues to work with
the chosen approach.
- Around line 1154-1158: The call to cacheHelper.mentee.delete is passing three
args causing menteeId to be undefined; update the caller in admin (where it
calls cacheHelper.mentee.delete(tenantCode, menteeData.organization_code,
menteeData.mentee_id)) to pass only the correct two parameters — tenantCode and
menteeData.mentee_id — so cacheHelper.mentee.delete(tenantCode,
menteeData.mentee_id) constructs the right key (buildKey /
nsUseInternal('mentee')) and properly invalidates the mentee cache.
---
Nitpick comments:
In `@src/services/entity-type.js`:
- Around line 343-349: Duplicate call to _clearUserCachesForEntityTypeChange is
made in the delete flow; consolidate to a single call after the entity is
deleted by removing the redundant pre-deletion invocation and keeping the
post-deletion call that passes (organizationCode, tenantCode,
entityToDelete.model_names ? entityToDelete.model_names[0] : null,
entityToDelete.value); ensure the single remaining call is executed after the
deletion completes so caches reflect the final state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9e54ed1-ac6e-4dfe-8046-e3f8130b39bd
📒 Files selected for processing (4)
src/generics/cacheHelper.jssrc/services/entity-type.jssrc/services/entity.jssrc/services/notification.js
| await this._clearUserCachesForEntityTypeChange( | ||
| organizationCode, | ||
| tenantCode, | ||
| entityToDelete.model_names ? entityToDelete.model_names[0] : null, |
There was a problem hiding this comment.
Why are we using just the first element from the array? What happens if there are two models for the same entity type?
nevil-mathew
left a comment
nevil-mathew
left a comment
There was a problem hiding this comment.
While you’ve fixed the issue in cacheHelper.js line 487, it still persists in 442 and 511. Shouldn’t we fix this as well? Are there any other occurrences that haven’t been updated?
|
Similar issue in line 1283, 1352 |
| value: entityToDelete.value, | ||
| modelNames: entityToDelete.model_names, | ||
| }) | ||
| await this._clearUserCachesForEntityTypeChange( |
There was a problem hiding this comment.
Why are we calling _clearUserCachesForEntityTypeChange twice(line 343)?
|
|
||
| // Create user ID to is_mentor mapping using map | ||
| const userMentorEntries = usersFromDb.map((user) => [user.user_id, user.is_mentor || false]) | ||
| Object.assign(userIsMentorMap, Object.fromEntries(userMentorEntries)) |
There was a problem hiding this comment.
Is userIsMentorMap declared in this fn?
There was a problem hiding this comment.
getMentorKafka (src/generics/cacheHelper.js:1036) still has the same .get() parameter mismatch.
It calls this.get(tenantCode, organizationCode, userIds), but mentor.get (line 884) only accepts (tenantCode, mentorId). The fallback at line 1105 has the same issue. The PR fixed this for session and mentee, but not here. No impact for now since it’s not used.
Also, in getMenteeKafka (src/generics/cacheHelper.js:1332–1333), userIsMentorMap is referenced but never defined. This throws a ReferenceError (caught at line 1337), causing errors to be logged every time DB users are found, skipping the success log at 1335, and also shadowing userMentorEntries from line 1311.
2 participants
Release Notes
tenantCode,id,datainstead of includingorganizationCodeas the second argument)Lines Changed by Author