Entity cache optmisation#1501
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR introduces organization-scoped caching and data filtering, centralizes cache invalidation logic, adds Kafka-based user caching, and refactors entity type handling. Changes thread organization_code through controllers, services, and database queries, introduce a new formVersions cache module, restructure cache eviction with internal/Redis support, and optimize user data retrieval with cached lookups. Changes
Sequence Diagram(s)sequenceDiagram
participant KafkaConsumer as Kafka Consumer<br/>(readUser)
participant Promise as Promise.all
participant MentorHelper as MentorsHelper
participant MenteeHelper as MenteesHelper
participant DB as Database
participant Log as Logger
KafkaConsumer->>KafkaConsumer: Receive message with users array
KafkaConsumer->>Promise: Map users to parallel tasks
par User Processing
Promise->>MentorHelper: Read (user_id, org_code, tenant_code)<br/>where is_mentor=true
MentorHelper->>DB: Fetch mentor details
DB-->>MentorHelper: Mentor data
MentorHelper-->>Promise: {userId, is_mentor, success}
and
Promise->>MenteeHelper: Read (user_id, org_code, tenant_code)<br/>where is_mentor=false
MenteeHelper->>DB: Fetch mentee details
DB-->>MenteeHelper: Mentee data
MenteeHelper-->>Promise: {userId, is_mentor, success}
end
Promise-->>KafkaConsumer: All results
KafkaConsumer->>KafkaConsumer: Aggregate {results,<br/>successCount, totalCount}
KafkaConsumer->>Log: "processing of read users<br/>complete: X/Y processed"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/database/queries/entity.js (1)
45-55:options.wherecan override tenant scoping infindAllEntities/updateOneEntityBecause
...optionsis spread afterwhere, anyoptions.wherefrom callers can overwrite the method’s ownwhere(includingtenant_code), which risks bypassing tenant isolation.To harden this, merge
options.whereexplicitly and always applytenant_codelast, instead of allowing callers to replacewherewholesale. For example:static async findAllEntities(filter, tenantCode, options = {}) { try { - if (tenantCode) { - filter.tenant_code = tenantCode - } - return await Entity.findAll({ - where: filter, - ...options, - raw: true, - }) + const { where: optionsWhere = {}, ...restOptions } = options || {} + + const where = { + ...filter, + ...optionsWhere, + ...(tenantCode ? { tenant_code: tenantCode } : {}), + } + + return await Entity.findAll({ + where, + ...restOptions, + raw: true, + }) } catch (error) { throw error } } static async updateOneEntity(whereClause, tenantCode, update, options = {}) { try { - // MANDATORY: Include tenant_code in whereClause - const where = { ...(whereClause || {}), tenant_code: tenantCode } - const sanitized = { ...update } - delete sanitized.tenant_code - return await Entity.update(sanitized, { - where, - ...options, - }) + const { where: optionsWhere = {}, ...restOptions } = options || {} + + const where = { + ...(whereClause || {}), + ...optionsWhere, + ...(tenantCode ? { tenant_code: tenantCode } : {}), + } + + const sanitized = { ...update } + delete sanitized.tenant_code + + return await Entity.update(sanitized, { + where, + ...restOptions, + }) } catch (error) { throw error } }This follows the multi‑tenant pattern from earlier work (ensure
tenant_codeis enforced after merging caller options). Based on learnings, this should be treated as a must‑fix for tenant isolation.Also applies to: 60-70
src/database/queries/form.js (1)
16-30: Fix unsafe spread ofoptions.where(can throw at runtime)In
findOne,findFormsByFilter, andupdateOneFormyou destructureoptions.wherewithout a default:const { where: optionsWhere, ...otherOptions } = options ... where: { ...optionsWhere, ...filterOrWhereClause, }If
options.whereis omitted (oroptionsis{}),optionsWhereisundefinedand...optionsWherewill throwTypeError: Cannot convert undefined or null to object.Recommend defaulting
optionsWhereto an empty object (and guardingoptions):- const { where: optionsWhere, ...otherOptions } = options + const { where: optionsWhere = {}, ...otherOptions } = options || {}Apply this in all three methods so the “safe merge” pattern actually remains safe. Based on learnings, tenant scoping remains enforced while allowing caller filters.
Also applies to: 36-53, 59-76
src/services/sessions.js (1)
1713-1757: Use model‑preserving dedupe helper when combining session and accessor entity typesHere you build
entityTypesby mergingsessionEntityTypesandaccessorEntityTypesand then prune withremoveDefaultOrgEntityTypes(entityTypes, sessionAccessorDetails.organization_code)before passing intoprocessDbResponsefor mentor designation and custom entity text (Lines 1717‑1750).removeDefaultOrgEntityTypesdeduplicates purely byvalue, ignoringmodel_names, so when the same value exists for multiple models (e.g. Session vs UserExtension), one model’s entity type can be dropped. That defeats the purpose described in_removeDefaultOrgEntityTypesPreservingModels, whose JSDoc explicitly says it exists “to prevent removing UserExtension entity types needed for designation processing when Session model has entity types with same values” (Lines 1815‑1833).This is likely to surface as missing/incorrect mentor designations for orgs where Session and UserExtension share some entity type values.
Consider switching the combined-path pruning to
_removeDefaultOrgEntityTypesPreservingModelswhile keeping the per‑model call onsessionEntityTypesas‑is. For example:- let entityTypes = [] + let entityTypes = [] ... - // Combine both sets - entityTypes = [...(sessionEntityTypes || []), ...(accessorEntityTypes || [])] + // Combine both sets + entityTypes = [...(sessionEntityTypes || []), ...(accessorEntityTypes || [])] ... - if (mentorExtension?.user_id) { - const validationData = removeDefaultOrgEntityTypes( - entityTypes, - sessionAccessorDetails.organization_code - ) + if (mentorExtension?.user_id) { + const validationData = this._removeDefaultOrgEntityTypesPreservingModels( + entityTypes, + sessionAccessorDetails.organization_code + )This keeps distinct entity types per
value + model_namescombination and still prefers the current org over defaults.Also applies to: 1815-1833
src/generics/cacheHelper.js (1)
1108-1156: PassorganizationCodewhen calling_sendToKafkaBackgroundfor mentor cache warming
mentor._sendToKafkaBackgroundnow expects(userMappingData, tenantCode, organizationCode)and includesorganizationCodein the Kafka payload:const batchMessage = { tenantCode: tenantCode, organizationCode: organizationCode, userBatches: userBatches, }But in
getMentorKafkayou call it as:const userMappingData = Object.fromEntries(userMentorEntries) this._sendToKafkaBackground(userMappingData, tenantCode)So
organizationCodeis alwaysundefinedin the batch message, which may break consumers expecting a valid org code for routing or logging.Forward the
organizationCodeargument you already have ingetMentorKafka:- const userMappingData = Object.fromEntries(userMentorEntries) - this._sendToKafkaBackground(userMappingData, tenantCode) + const userMappingData = Object.fromEntries(userMentorEntries) + this._sendToKafkaBackground(userMappingData, tenantCode, organizationCode)Also applies to: 1166-1227
🧹 Nitpick comments (10)
src/services/connections.js (1)
267-283: Switch to cached user detail retrieval looks fine; consider minor optimizationUsing
userRequests.getUserDetailedListUsingCache(userIds, tenantCode)in both pending and connected-users flows keeps the existing.resultmapping logic intact while enabling cache hits, so behavior should be preserved as long as the new helper returns the same structure.Optionally, to reduce payload size and cache/HTTP overhead, you could deduplicate IDs before the call:
- const userIds = connectionsWithDetails.map((item) => item.friend_id) + const userIds = [...new Set(connectionsWithDetails.map((item) => item.friend_id))]and similarly for the connected-users list.
Also applies to: 495-507
src/generics/utils.js (1)
1289-1300:removeDefaultOrgDatacorrectly preserves tenant overrides over defaultsThe new helper:
const orgData = new Set(data.filter((f) => f.organization_code !== defaultOrgCode).map((f) => f[key])) return data.filter((f) => !(f.organization_code === defaultOrgCode && orgData.has(f[key])))does the intended thing:
- Keeps all non‑default‑org rows.
- Drops default‑org rows only when there’s at least one non‑default row with the same
key(e.g.,type), so tenant/org-specific overrides win.- Retains default records when no override exists.
Two minor robustness tweaks you could consider (optional):
- Guard against non-array input:
if (!Array.isArray(data)) return data || [].- Document expected shape in a JSDoc comment (required fields:
organization_codeand the providedkey).Otherwise this utility and its export look solid for the form-versions cache usage.
Also applies to: 1359-1359
src/services/users.js (1)
171-227: Cache invalidation here is redundant with#updateUserand could be centralizedInside
#createOrUpdateUserAndOrgyou now have:const isAMentor = userExtensionData.roles.some((role) => role.title == common.MENTOR_ROLE) try { if (!isNewUser && isAMentor) { await cacheHelper.mentor.delete(userDetails.data.result.tenant_code, userId) } else if (!isNewUser) { await cacheHelper.mentee.delete(userDetails.data.result.tenant_code, userId) } } catch (error) { // not require to clear the chache }But for the non‑new‑user path you always call
#updateUser, which already:
- Invalidates both mentee and mentor caches on role change, and
- Invalidates the appropriate cache after a plain update.
So this block mostly results in a second, redundant delete against the same keys. It’s harmless functionally but adds extra cache calls.
Consider either:
- Removing this new block and relying solely on
#updateUserfor user-cache invalidation, or- Moving all user cache invalidation into
#createOrUpdateUserAndOrgand stripping it from#updateUser, so there’s a single well-defined place handling it.That would keep behavior the same while simplifying the cache story.
src/requests/user.js (1)
874-885: Kafka publishing errors could break the main request flow.The
pushUncachedUsersToKafkacall is awaited in the main execution path without try-catch. If Kafka publishing fails, it will propagate the error and potentially cause the entiregetUserDetailedListUsingCachecall to fail, even though the user data was successfully retrieved.Consider wrapping this in a fire-and-forget pattern since caching is an optimization, not a critical path:
if (cacheHelper._internal.ENABLE_CACHE) { let unCachedUsers = [] for (const userDetail of usersFromDb) { unCachedUsers.push({ user_id: userDetail.user_id, is_mentor: userDetail.is_mentor, tenant_code: userDetail.tenant_code, organization_code: userDetail.organization_code, }) } - await kafkaCommunication.pushUncachedUsersToKafka(unCachedUsers) + // Fire-and-forget: Don't block main flow for background cache population + kafkaCommunication.pushUncachedUsersToKafka(unCachedUsers).catch((err) => { + console.error('Failed to push uncached users to Kafka:', err.message) + }) }src/helpers/entityTypeCache.js (1)
490-494:getModelNamecalls are not wrapped in safeRun and could throw.The
Promise.allfor fetching model names (lines 490-494) is outside thesafeRunwrapper. If any of these queries fail, the entire function will throw and no cache clearing will happen. Consider wrapping this in try-catch with fallback values if model names are not critical for partial cache clearing.- const [menteeModel, mentorModel, sessionModel] = await Promise.all([ - menteeExtensionQueries.getModelName(), - mentorExtensionQueries.getModelName(), - sessionQueries.getModelName(), - ]) + let menteeModel, mentorModel, sessionModel + try { + [menteeModel, mentorModel, sessionModel] = await Promise.all([ + menteeExtensionQueries.getModelName(), + mentorExtensionQueries.getModelName(), + sessionQueries.getModelName(), + ]) + } catch (modelError) { + console.error('Failed to get model names for cache clearing:', modelError.message) + // Use common constants as fallback + menteeModel = common.userExtensionModelName + mentorModel = common.mentorExtensionModelName + sessionModel = common.sessionModelName + }src/services/entity-type.js (2)
119-127: Pass fullmodel_namesarray to cache invalidation helperIn
update, you call:await entityTypeCache.clearUserCachesForEntityType( orgCode, tenantCode, updatedEntity.model_names ? updatedEntity.model_names[0] : null, updatedEntity.value )If an entity type is associated with multiple models (i.e.
model_namesis an array), this only clears caches for the first model, potentially leaving stale cache entries for the others.Safer to pass the whole array (letting the helper handle string/array gracefully):
- updatedEntity.model_names ? updatedEntity.model_names[0] : null, + updatedEntity.model_names || null,
293-317: Avoid double‑callingclearUserCachesForEntityTypein delete pathIn
delete, you callentityTypeCache.clearUserCachesForEntityTypetwice:
- Before deletion with
entityToDelete.model_names(array).- After deletion with just
entityToDelete.model_names[0].Given the helper already takes
model_namesandvalue, the second, narrower call is redundant and may be confusing to future maintainers.Either drop the post‑delete call or keep a single call (pre‑ or post‑delete) using the full
model_namesarray:- // Clear user caches since entity types affect user profiles - await entityTypeCache.clearUserCachesForEntityType( - organizationCode, - tenantCode, - entityToDelete.model_names ? entityToDelete.model_names[0] : null, - entityToDelete.value - )src/services/entity.js (2)
36-49: Entity–entityType cache invalidation wiring looks consistent, with a small logging nitAcross create, update, and delete you now:
- Retrieve
entityTypeDetails(fromcreateEntityWithValidationor viafindEntityTypeById), and- Call
entityTypeCache.clearUserCachesForEntityType(organization_code, tenantCode, model_names, value).This is aligned with the centralized invalidation pattern introduced in
entity-typeservice and should keep user-facing caches consistent when entities change.Only minor nit: the inner catch in
updatelogs"after entity creation"even though it’s in the update path:console.error(`❌ Failed to invalidate entityType cache after entity creation:`, cacheError)Consider updating the message to
"after entity update"for clarity; behaviour is otherwise fine.Also applies to: 118-141, 320-341
367-419: Tighten empty-result check inlist
listnow delegates toentityQueries.getAllEntitiesWithEntityTypeDetailsand checks:if (entities.rows == 0 || entities.count == 0) { ... }Since
rowsis an array, theentities.rows == 0comparison will always be false; onlycount == 0actually drives theNO_RESULTS_FOUNDresponse.For clarity and to avoid confusion, prefer an explicit length check:
- if (entities.rows == 0 || entities.count == 0) { + if (!entities.rows?.length || entities.count === 0) {src/generics/cacheHelper.js (1)
264-271: scanAndDelete dispatcher behaviour is consistent, but JSDoc is now outdatedYou’ve split the old
scanAndDeleteinto:
scanAndDeleteRedis(pattern, { batchSize, unlink }), and- A new dispatcher
scanAndDelete(pattern, { useInternal = false })that chooses internal vs Redis deletion.Internal call sites (evictNamespace, evictOrgByPattern, evictTenantByPattern, and various
evict*helpers) only pass{ useInternal }or nothing, so behaviour is consistent and safe.However, the JSDoc above still documents
scanAndDelete(pattern, opts)withopts.batchSize/opts.unlink, which are no longer accepted by the publicscanAndDeleteexport. If any external callers pass those options, they’ll silently be ignored.Consider either:
- Updating the comment to reflect the new dispatcher semantics, or
- Extending the dispatcher signature to forward
batchSize/unlinkthrough toscanAndDeleteRediswhenuseInternalis false.Also applies to: 281-311
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/configs/kafka.jsis excluded by!src/configs/**
📒 Files selected for processing (21)
src/constants/common.js(1 hunks)src/controllers/v1/entity.js(1 hunks)src/controllers/v1/form.js(2 hunks)src/database/queries/entity.js(1 hunks)src/database/queries/form.js(1 hunks)src/generics/cacheHelper.js(12 hunks)src/generics/kafka-communication.js(1 hunks)src/generics/kafka/consumers/readUser.js(1 hunks)src/generics/utils.js(3 hunks)src/helpers/entityTypeCache.js(2 hunks)src/requests/user.js(2 hunks)src/services/connections.js(2 hunks)src/services/entity-type.js(5 hunks)src/services/entity.js(6 hunks)src/services/form.js(3 hunks)src/services/mentees.js(2 hunks)src/services/mentors.js(1 hunks)src/services/notifications.js(1 hunks)src/services/requestSessions.js(2 hunks)src/services/sessions.js(8 hunks)src/services/users.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/notifications.jssrc/services/requestSessions.jssrc/services/mentees.jssrc/services/users.jssrc/services/mentors.jssrc/services/connections.jssrc/services/sessions.jssrc/services/form.jssrc/services/entity.jssrc/services/entity-type.js
src/database/queries/**
⚙️ CodeRabbit configuration file
Review database queries for performance. Check for N+1 problems and ensure indexes can be used.
Files:
src/database/queries/entity.jssrc/database/queries/form.js
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/entity.jssrc/controllers/v1/form.js
🧠 Learnings (8)
📚 Learning: 2025-11-06T06:25:57.830Z
Learnt from: sumanvpacewisdom
Repo: ELEVATE-Project/mentoring PR: 1426
File: src/database/migrations/20251020081719-add-orgEntity-type.js:10-12
Timestamp: 2025-11-06T06:25:57.830Z
Learning: In the ELEVATE-Project/mentoring repository, entity type migrations (like 20251020081719-add-orgEntity-type.js) run before tenant-specific migrations. Therefore, down migrations for these entity types do not need to filter by organization_code and tenant_code, as multi-tenant data does not exist at the time these migrations execute.
Applied to files:
src/generics/utils.jssrc/services/requestSessions.jssrc/database/queries/entity.jssrc/database/queries/form.jssrc/services/mentees.jssrc/services/mentors.jssrc/helpers/entityTypeCache.jssrc/services/sessions.jssrc/controllers/v1/entity.jssrc/generics/cacheHelper.jssrc/services/entity.jssrc/services/entity-type.js
📚 Learning: 2025-11-04T10:36:50.155Z
Learnt from: rakeshSgr
Repo: ELEVATE-Project/mentoring PR: 1442
File: src/services/sessions.js:1822-1831
Timestamp: 2025-11-04T10:36:50.155Z
Learning: In the ELEVATE-Project/mentoring codebase, when `mentorId` or `session.mentor_id` exist in the sessions service, the `mentorExtensionQueries.getMentorExtension()` call is guaranteed to return a valid object with a `name` property. Defensive null checks like `mentorDetails?.name` are not necessary when the mentor ID guard condition is present.
Applied to files:
src/services/notifications.jssrc/services/mentees.jssrc/services/mentors.jssrc/services/sessions.js
📚 Learning: 2025-09-16T11:03:05.949Z
Learnt from: sumanvpacewisdom
Repo: ELEVATE-Project/mentoring PR: 1334
File: src/controllers/v1/sessions.js:502-524
Timestamp: 2025-09-16T11:03:05.949Z
Learning: The removeAllSessions endpoint in src/controllers/v1/sessions.js is an internal API that intentionally accepts user_id, organization_code, and tenant_code from the request body rather than deriving them from req.decodedToken, as it's designed for service-to-service communication where the calling service specifies the tenant context.
Applied to files:
src/services/requestSessions.jssrc/services/sessions.jssrc/controllers/v1/entity.jssrc/generics/cacheHelper.js
📚 Learning: 2025-09-16T11:06:06.180Z
Learnt from: sumanvpacewisdom
Repo: ELEVATE-Project/mentoring PR: 1334
File: src/controllers/v1/entity-type.js:42-44
Timestamp: 2025-09-16T11:06:06.180Z
Learning: The entityTypeService.update method in src/services/entity-type.js has the signature: update(bodyData, id, loggedInUserId, orgCode, tenantCode, roles) - it includes the loggedInUserId parameter between id and orgCode.
Applied to files:
src/services/requestSessions.jssrc/database/queries/entity.jssrc/services/mentees.jssrc/services/mentors.jssrc/helpers/entityTypeCache.jssrc/services/sessions.jssrc/controllers/v1/entity.jssrc/services/entity.jssrc/services/entity-type.jssrc/controllers/v1/form.js
📚 Learning: 2025-08-17T08:50:22.819Z
Learnt from: sumanvpacewisdom
Repo: ELEVATE-Project/mentoring PR: 1319
File: src/database/queries/reportTypes.js:13-19
Timestamp: 2025-08-17T08:50:22.819Z
Learning: In multi-tenant database query methods, when accepting an options parameter that gets spread into Sequelize queries, ensure that critical fields like tenant_code are applied after spreading options to prevent bypassing tenant isolation. Use pattern: `const { where: optionsWhere = {}, ...rest } = options || {}; const where = { ...optionsWhere, tenant_code: tenantCode }` to safely merge caller options while enforcing tenant scoping.
Applied to files:
src/database/queries/entity.jssrc/database/queries/form.js
📚 Learning: 2025-12-03T08:35:14.163Z
Learnt from: rakeshSgr
Repo: ELEVATE-Project/mentoring PR: 1481
File: src/database/queries/sessionAttendees.js:304-330
Timestamp: 2025-12-03T08:35:14.163Z
Learning: In Sequelize queries with LEFT JOINs via include, tenant_code filtering on the main table's where clause does NOT automatically apply to the joined table. To enforce tenant isolation across joins, add a where clause with tenant_code to each include block, especially when joining tables that have tenant_code fields. The where clause on an include becomes part of the JOIN's ON condition, properly scoping the join itself.
Applied to files:
src/database/queries/entity.js
📚 Learning: 2025-07-31T09:34:29.508Z
Learnt from: sumanvpacewisdom
Repo: ELEVATE-Project/mentoring PR: 1281
File: src/controllers/v1/connections.js:16-17
Timestamp: 2025-07-31T09:34:29.508Z
Learning: In the ELEVATE-Project/mentoring repository, validation for req.decodedToken fields (including tenant_code, organization_id, id, etc.) is not required in controller methods. The project handles token validation at a different layer and does not need additional validation checks in individual controller methods.
Applied to files:
src/controllers/v1/entity.js
📚 Learning: 2025-09-16T10:57:49.662Z
Learnt from: sumanvpacewisdom
Repo: ELEVATE-Project/mentoring PR: 1334
File: src/controllers/v1/org-admin.js:117-121
Timestamp: 2025-09-16T10:57:49.662Z
Learning: In the ELEVATE-Project/mentoring repository, internal APIs that are called by other services (like user service) may intentionally accept tenant_code from the request body rather than using req.decodedToken.tenant_code, as this allows the calling service to specify the tenant context for the operation. This is different from external APIs where token-based tenant context is preferred.
Applied to files:
src/generics/cacheHelper.js
🧬 Code graph analysis (12)
src/generics/kafka-communication.js (2)
src/services/sessions.js (4)
payload(538-556)payload(2214-2231)payload(2361-2376)payload(3605-3655)src/helpers/getDefaultOrgId.js (1)
process(11-11)
src/generics/utils.js (4)
src/services/users.js (1)
data(230-237)src/database/queries/organisationExtension.js (1)
data(56-56)src/scripts/viewsScript.js (1)
defaultOrgCode(12-12)src/services/admin.js (1)
orgData(1812-1812)
src/requests/user.js (1)
src/generics/cacheHelper.js (3)
kafkaCommunication(17-17)usersFromDb(1201-1201)usersFromDb(1456-1461)
src/services/requestSessions.js (2)
src/generics/utils.js (1)
removeDefaultOrgEntityTypes(473-485)src/controllers/v1/platform.js (1)
organizationCode(14-14)
src/generics/kafka/consumers/readUser.js (1)
src/generics/kafka/consumers/readSession.js (5)
message(9-9)message(61-61)results(17-17)successCount(52-52)totalCount(53-53)
src/services/mentees.js (2)
src/services/mentors.js (10)
validationData(86-95)validationData(464-464)validationData(571-571)validationData(989-989)validationData(1324-1329)entityTypes(454-458)entityTypes(563-567)entityTypes(979-983)mentorExtension(775-780)mentorExtension(914-914)src/generics/utils.js (4)
removeDefaultOrgEntityTypes(473-485)entityTypes(516-516)entityTypes(578-578)entityTypes(1234-1251)
src/services/users.js (2)
src/generics/utils.js (1)
isAMentor(125-127)src/services/feedback.js (1)
isAMentor(151-151)
src/services/mentors.js (1)
src/generics/utils.js (4)
removeDefaultOrgEntityTypes(473-485)entityTypes(516-516)entityTypes(578-578)entityTypes(1234-1251)
src/helpers/entityTypeCache.js (1)
src/services/entity-type.js (1)
menteeExtensionQueries(4-4)
src/services/connections.js (2)
src/requests/user.js (6)
userDetails(186-186)userDetails(243-243)userDetails(364-364)userDetails(456-467)userDetails(588-588)userDetails(617-617)src/services/users.js (3)
userDetails(32-32)userDetails(172-172)userRequests(4-4)
src/services/sessions.js (1)
src/generics/utils.js (1)
removeDefaultOrgEntityTypes(473-485)
src/controllers/v1/entity.js (1)
src/controllers/v1/admin.js (2)
req(177-177)req(217-217)
🪛 Biome (2.1.2)
src/services/form.js
[error] 243-243: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 257-257: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (18)
src/database/queries/entity.js (1)
139-149: Addingtenant_codeto selected attributes looks goodIncluding
tenant_codein the projection forgetAllEntitiesWithEntityTypeDetailskeeps the query scoped as before while exposing tenant context to callers/cache logic. No functional or performance concerns here.src/database/queries/form.js (1)
88-97: Expanded attributes for form versions look correctIncluding
tenant_codeandorganization_codealongsideid,type, andversioninfindAllTypeFormVersionmatches the multi-tenant use case (e.g., for downstreamremoveDefaultOrgDatalogic) and keeps the filter ontenant_code/organization_codeunchanged.src/services/requestSessions.js (1)
131-140: Using actual org code for entity-type filtering is an improvementIn both
createandgetInfo, passing the contextual org code toremoveDefaultOrgEntityTypes:
organizationCodeincreateuserDetails.organization_codeingetInfoensures that, when both default-org and org-specific entity types exist for the same value, the org-specific variants are preferred, which matches the documented behavior of the helper.
Just make sure all call sites to
createandgetInforeliably provide a non-empty organization code; otherwise the behavior degenerates to “first entity wins” with no org preference.Also applies to: 684-691
src/services/notifications.js (1)
133-165: Cached mentor lookup is consistent with other flowsReplacing
getUserDetailedListwithgetUserDetailedListUsingCache(mentorIds, tenantCode, false, true)aligns this path with the rest of the service layer’s cache usage while keeping the downstream logic (userAccounts.result[0]) unchanged.If
getUserDetailedListUsingCachecan ever return a non-success response or a shape without.result, you may want a quick guard before accessinguserAccounts.result.lengthto avoid runtime errors, but the change itself is coherent.src/controllers/v1/entity.js (1)
103-112: Threadingorganization_codeintoentityService.listis appropriatePassing both
req.decodedToken.tenant_codeandreq.decodedToken.organization_codeintoentityService.listgives the service full tenant/org context for entity retrieval and caching, with no change to request/response handling in the controller.src/controllers/v1/form.js (2)
42-51: No behavioral change inupdateblockThe extra blank line after
formsService.update(...)has no functional impact; method behavior is unchanged.
65-72: Passing org code toreadAllFormsVersioncorrectly aligns with service expectationsCalling:
formsService.readAllFormsVersion( req.decodedToken.tenant_code, req.decodedToken.organization_code )gives the form service both tenant and organization context for the new form-versions cache and default-org filtering logic, and fits the described updated service signature.
src/services/mentors.js (1)
571-571: LGTM - Consistent with explicit orgCode handling.The change correctly uses the explicit
orgCodeparameter instead ofdefaults.orgCodefor filtering entity types during mentor extension update, aligning with the broader PR goal of organization-scoped entity type handling.src/services/mentees.js (2)
123-123: LGTM - Correctly uses the mentee's organization context for entity type filtering.Using
mentee.organization_codeensures that entity types are filtered based on the profile owner's organization, which is the correct behavior for the read endpoint.
2220-2220: No changes needed. The code is safe.At line 2148, an early return guard ensures
requestedUserExtensionis non-null before line 2216 is reached. Therefore, accessingrequestedUserExtension.organization_codeon line 2216 is safe, and the filtering at line 2220 usingmentorExtension.organization_codeis consistent with the data flow.src/constants/common.js (1)
313-318: LGTM - New cache namespace follows established patterns.The
formVersionscache namespace is correctly structured with consistent configuration (enabled, 1-day TTL, not using internal cache) matching the existing namespace definitions.src/generics/kafka/consumers/readUser.js (1)
7-66: LGTM - Well-structured parallel processing with proper error isolation.The refactored consumer correctly:
- Validates the
usersarray before processing- Uses
Promise.allfor parallel processing, improving throughput- Isolates errors per-user, preventing one failure from affecting others
- Returns comprehensive results with success/total counts for observability
The function signatures for
MentorsHelper.readandMenteesHelper.readare correctly called with appropriate default values for optional parameters.src/helpers/entityTypeCache.js (1)
469-515: All cache eviction methods referenced in the function exist in the cacheHelper implementation:evictMentee(line 1298),evictMentor(line 1054), andevictSessions(line 413) insrc/generics/cacheHelper.js.src/services/entity-type.js (1)
237-264: Pruning user entity types by org code looks correct
readUserEntityTypesnow fetches from both default and current org/tenant and then prunes viaremoveDefaultOrgEntityTypes(entityTypes, orgCode)(Line 237). This matches the intended behaviour of preferring org‑specific entity types over defaults when values collide.No issues from a correctness or caching perspective.
src/generics/cacheHelper.js (3)
701-703: EntityTypes cache: clearAll + mentor-org lookup changes look sound
entityTypes.clearAllnow resolvesuseInternalviansUseInternal('entityTypes')and passes it intoevictNamespace, so entityTypes eviction honours the configured backend (Redis vs Internal) (Lines 701‑703).getEntityTypesWithMentorOrghas been simplified to just callgetAllEntityTypesForModel(tenantCode, currentOrgCode, modelName)(Lines 805‑808). Given thatgetAllEntityTypesForModelalready merges current org and defaults, this matches the usage insessions.details, which now passessessionAccessorDetails.organization_codeascurrentOrgCode.These changes are consistent with the new centralized entityType caching strategy.
Also applies to: 805-808
413-417: New per-tenant eviction helpers for sessions/mentor/mentee are consistent with key patterns
sessions.evictSessions(tenantCode)evictstenant:${tenantCode}:sessions:*, matching the keys built viabuildKey({ tenantCode, ns: 'sessions', id })(Lines 413‑417).mentor.evictMentor(tenantCode)andmentee.evictMentee(tenantCode)evicttenant:${tenantCode}:mentor:*andtenant:${tenantCode}:mentee:*respectively (Lines 1054‑1058, 1298‑1304), consistent with the mentor/mentee key builders.These helpers provide a clean way to blow away all per-tenant session and profile caches when needed, and correctly respect the namespace-specific
useInternalconfiguration.Also applies to: 1054-1058, 1298-1304
1895-1944: formVersions cache helper is correctly wired into the public APIThe new
formVersionshelper provides get/set/delete with:
- Keys scoped as
tenant:${tenantCode}:org:${orgCode}:formVersionsviabuildKey.- TTL of 1 day on writes using
setScoped.nsUseInternal('formVersions')to respect internal vs Redis configuration on reads and deletes.Exporting
formVersionsinmodule.exports(Line 1979‑1980) makes it available alongside other namespace helpers. No issues spotted in key construction, TTL usage, or eviction behaviour.Also applies to: 1979-1980
src/services/sessions.js (1)
401-419:and
|
|
||
| const emailEncryption = require('@utils/emailEncryption') | ||
| const _ = require('lodash') | ||
| const organization = require('@validators/v1/organization') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if 'organization' validator is used anywhere in the file
rg -n 'organization' src/requests/user.js | grep -v "import\|require\|organizationId\|organizationCode\|organization_"Repository: ELEVATE-Project/mentoring
Length of output: 3058
🏁 Script executed:
cat -n src/requests/user.js | head -100Repository: ELEVATE-Project/mentoring
Length of output: 4525
🏁 Script executed:
rg -n 'organization\(' src/requests/user.jsRepository: ELEVATE-Project/mentoring
Length of output: 51
Remove unused import.
The organization validator imported on line 28 is not used anywhere in the file and should be removed.
🤖 Prompt for AI Agents
In src/requests/user.js around line 28, the organization validator is imported
but not used; remove the unused import statement "const organization =
require('@validators/v1/organization')" from the file to clean up unused code
and avoid linter errors.
|
|
||
| clearPromises.push(safeRun(cacheHelper.displayProperties.delete(tenantCode, organizationCode))) | ||
|
|
There was a problem hiding this comment.
What is this displayProperties ?
There was a problem hiding this comment.
displayProperties are driven from entity types
|
|
||
| const clearPromises = [] | ||
|
|
||
| clearPromises.push(safeRun(cacheHelper.displayProperties.delete(tenantCode, organizationCode))) |
There was a problem hiding this comment.
What is this displayProperties? Wont this break the flow?
There was a problem hiding this comment.
displayProperties are used in frontend to show or hide the entity types for the mentor details page.
we define below keys in the meta of entityType, based on these data frontend decide what do
key: 'organization',
label: 'Organization',
visible: true,
visibility: 'main',
sequence: 1,
| clearPromises.push( | ||
| safeRun(cacheHelper.entityTypes.delete(tenantCode, organizationCode, model, entityValue)) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need model names in the entity type cache? Can a same entity type code exist with multiple model names?
There was a problem hiding this comment.
within a org can't be created same entity type
| if (shouldClearProfileCaches) { | ||
| clearPromises.push(safeRun(cacheHelper.mentee.evictMentee(tenantCode))) | ||
| clearPromises.push(safeRun(cacheHelper.mentor.evictMentor(tenantCode))) | ||
| } | ||
|
|
||
| // Clear session caches | ||
| if (shouldClearSessionCaches) { | ||
| clearPromises.push(safeRun(cacheHelper.sessions.evictSessions(tenantCode))) | ||
| } |
There was a problem hiding this comment.
Do we need to clear out all the tenants' cache? Wouldn't this be only required when it's the default organisation?
There was a problem hiding this comment.
we are deleting for that tenant, not for all
| if (entityValue) { | ||
| for (const model of modelNames) { | ||
| clearPromises.push( | ||
| safeRun(cacheHelper.entityTypes.delete(tenantCode, organizationCode, model, entityValue)) |
There was a problem hiding this comment.
If the default organisation's entity/entity type is getting updated, shouldn't we clear out all other orgs' caches as well?
There was a problem hiding this comment.
No, currently its not done. we have API's to clear , we can use that if any any default organisation entity/ forms/ notifications etc create / updated
|
Comments related to cache clearing added in entity type might be applicable to entities and forms, or any place where default org data is shared with other orgs. Please check that as well. |
2 participants
Release Notes: Entity Cache Optimization
Key Features & Improvements
formVersionscache configuration with 24-hour TTL for organization-aware form version cachinguseInternalparameterreadAllFormsVersion()with automatic default organization data filtering viaremoveDefaultOrgData()utilitypushUncachedUsersToKafka()to push newly cached user details for cross-service consistencyclearUserCachesForEntityType()helper for consistent mentor, mentee, session, and display properties cache handlingreadUserconsumer for parallel user processing with per-user success tracking and comprehensive error reportinggetUserDetailedListUsingCache()across connections, notifications, and session servicesFiles Modified (21 files)