Centralized cache helper with tenant/org scoping, update signatures #823
Conversation
…d update user-role service to use the corrected constant
…type read in user login process
…t and eviction methods
…e, update, and delete operations
…nization in constants
…idate cache on role updates and deletions
…paces in admin service
…cache invalidation for user organization deletions
… cache management
…ity types and profiles
…er organization ID
WalkthroughIntroduces a centralized cache helper, replaces direct Redis usage, adds tenant-/org-scoped caching and invalidations across services, updates method signatures for tenant awareness, fixes a language constant typo, adjusts controllers to pass new context, refines DB queries for tenant scoping, and updates dependencies and localization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Controller
participant S as Service
participant CH as CacheHelper
participant DB as Database
Note over C,S: Read user/profile (tenant- & org-scoped)
C->>S: read(id, headers, lang, tenantCode, orgCode)
S->>CH: getOrSet({ ns: profile, tenantCode, orgId: orgCode, id })
alt cache hit
CH-->>S: cached profile
S-->>C: profile
else cache miss
CH-->>S: miss
S->>DB: fetch user + roles + entities
DB-->>S: data
S->>CH: setScoped({ ns: profile, tenantCode, orgId: orgCode, id, value })
S-->>C: profile
end
sequenceDiagram
autonumber
participant C as Controller
participant Org as OrganizationService
participant Q as OrgQueries
participant CH as CacheHelper
Note over C,Org: Add/remove related organizations (tenant-scoped)
C->>Org: addRelatedOrg(id, relatedOrgs, tenantCode)
Org->>Q: validate target orgs by tenant_code
Q-->>Org: existing orgs (filtered)
Org->>Q: update related_orgs within tenant
Q-->>Org: updated rows
par Invalidate caches (non-blocking)
Org->>CH: invalidateNamespace({ ns: organization, tenantCode, orgId })
Org->>CH: invalidateNamespace({ ns: profile, tenantCode, orgId })
end
Org-->>C: result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/database/queries/userOrganization.js (2)
15-19: Critical: same-org move deletes the user_organization row. Guard when newOrgCode === oldOrgCode.As written, upserting the “new” org then destroying the “old” org will remove the just-upserted row when both codes are equal.
Apply this minimal guard before starting the transaction:
-exports.changeUserOrganization = async ({ userId, tenantCode, oldOrgCode, newOrgCode }) => { - const transaction = await sequelize.transaction() +exports.changeUserOrganization = async ({ userId, tenantCode, oldOrgCode, newOrgCode }) => { + if (oldOrgCode === newOrgCode) { + return { + success: true, + message: `No-op: user ${userId} already in organization ${newOrgCode}`, + } + } + const transaction = await sequelize.transaction()
74-78: Role copy: avoid duplicate insert violations.If the user already has some roles in the target org, bulkCreate can fail. Use ignoreDuplicates (or updateOnDuplicate if needed).
if (newRoles.length > 0) { - await UserOrganizationRole.bulkCreate(newRoles, { - transaction, - }) + await UserOrganizationRole.bulkCreate(newRoles, { + transaction, + ignoreDuplicates: true, + validate: true, + }) }src/controllers/v1/user.js (1)
72-86: Potential cross-tenant read via query param. Use token’s tenant_code instead.profileById reads tenant_code from req.query; per codebase convention and auth guarantees, prefer req.decodedToken.tenant_code to avoid spoofing.
-const userDetails = await userService.profileById(param, req.query.tenant_code) +const userDetails = await userService.profileById(param, req.decodedToken.tenant_code)src/controllers/v1/organization.js (1)
69-88: Admin + OrgAdmin role check can block admins unintentionally.If a user has both roles, the first condition triggers and denies cross-org updates. Gate the org-admin branch by !isAdmin.
- if (req.params.id != req.decodedToken.organization_id && isOrgAdmin) { + if (!isAdmin && isOrgAdmin && req.params.id != req.decodedToken.organization_id) {Optional: normalize IDs for strict compare to avoid type coercion issues.
const pathOrgId = Number(req.params.id) const tokenOrgId = Number(req.decodedToken.organization_id) if (!isAdmin && isOrgAdmin && pathOrgId !== tokenOrgId) { ... }src/services/user.js (1)
73-76: removeDefaultOrgEntityTypes called without orgId.Without orgId, dedup may keep default org values instead of org-specific ones.
- const prunedEntities = removeDefaultOrgEntityTypes(validationData) + const org = await organizationQueries.findOne( + { code: orgCode, tenant_code: tenantCode }, + { attributes: ['id'] } + ) + const prunedEntities = removeDefaultOrgEntityTypes(validationData, org?.id)And in setLanguagePreference:
- const prunedEntities = removeDefaultOrgEntityTypes(dataValidation) + const org = await organizationQueries.findOne( + { code: organizationCode, tenant_code: tenantCode }, + { attributes: ['id'] } + ) + const prunedEntities = removeDefaultOrgEntityTypes(dataValidation, org?.id)Also applies to: 561-565
src/services/account.js (2)
2013-2016: Undefined constant ERROR_MESSAGES.This will throw at runtime. Use a defined source or inline message.
- message: ERROR_MESSAGES.USER_DOESNOT_EXISTS, + message: 'USER_DOESNOT_EXISTS',
1016-1023: Likely wrong identifier when updating user session token.updateUserSession query uses id: decodedToken.data.id (user id) instead of session id. Should use session_id.
- let check = await userSessionsService.updateUserSession( - { - id: decodedToken.data.id, - }, + let check = await userSessionsService.updateUserSession( + { + id: decodedToken.data.session_id, + }, { token: accessToken, } )src/services/user-role.js (1)
148-156: Update roleService.delete callers to pass tenantCodesrc/controllers/v1/user-role.js calls roleService.delete with three arguments (lines 84–87); the service signature now requires four (id, userOrganizationId, userOrganizationCode, tenantCode) — add tenantCode (from the request/decoded token) as the 4th argument and update any other callers/tests.
🧹 Nitpick comments (20)
src/database/queries/userOrganization.js (1)
31-37: Don’t set created_at on upsert updates.Setting created_at in an upsert can overwrite creation timestamps on conflict. Let the DB/model defaults handle it.
{ user_id: userId, tenant_code: tenantCode, organization_code: newOrgCode, - created_at: new Date(), updated_at: new Date(), },src/constants/common.js (1)
116-129: CACHE_CONFIG: freeze and externalize TTLs.
- Prevent accidental runtime mutation with Object.freeze (shallow or deep).
- Consider env-driven TTLs to tune without redeploy; ensure units (seconds) match elevate-node-cache expectations consistently.
- CACHE_CONFIG: { + CACHE_CONFIG: Object.freeze({ enableCache: true, enableTracking: true, shards: 32, versionDefault: 0, namespaces: { - profile: { name: 'profile', enabled: true, defaultTtl: 3600, useInternal: false }, + profile: { name: 'profile', enabled: true, defaultTtl: Number(process.env.PROFILE_TTL || 3600), useInternal: false }, entity_types: { name: 'entity_types', enabled: true, defaultTtl: 86400, useInternal: false }, tenant: { name: 'tenant', enabled: true, defaultTtl: 21600, useInternal: false }, branding: { name: 'branding', enabled: true, defaultTtl: 43200, useInternal: false }, organization: { name: 'organization', enabled: true, defaultTtl: 21600, useInternal: false }, }, - }, + }),src/services/tenant.js (3)
442-445: Invalidate tenant namespace after update: good. Consider logging failures.Swallowing errors is fine for UX, but add a debug log/metric to observe cache invalidation failures.
- await cacheClient - .invalidateTenantVersion({ tenantCode, ns: common.CACHE_CONFIG.namespaces.tenant.name }) - .catch(() => {}) + await cacheClient + .invalidateTenantVersion({ tenantCode, ns: common.CACHE_CONFIG.namespaces.tenant.name }) + .catch((e) => console.debug('cache:invalidateTenantVersion failed', { tenantCode, err: e?.message }))
526-529: Invalidate after addDomain: LGTM. Consider same lightweight logging on failure.- .invalidateTenantVersion({ tenantCode, ns: common.CACHE_CONFIG.namespaces.tenant.name }) - .catch(() => {}) + .invalidateTenantVersion({ tenantCode, ns: common.CACHE_CONFIG.namespaces.tenant.name }) + .catch((e) => console.debug('cache:invalidateTenantVersion failed', { tenantCode, err: e?.message }))
646-652: Wrong success message for removal.Returns TENANT_DOMAINS_ADDED_SUCCESSFULLY after removal. Use a dedicated “removed successfully” message and add the i18n key.
- return responses.successResponse({ - statusCode: httpStatusCode.accepted, - message: 'TENANT_DOMAINS_ADDED_SUCCESSFULLY', - result: { - removed_domains: domainsToRemove, - }, - }) + return responses.successResponse({ + statusCode: httpStatusCode.accepted, + message: 'TENANT_DOMAINS_REMOVED_SUCCESSFULLY', + result: { removed_domains: domainsToRemove }, + })Also add to src/locales/en.json:
"TENANT_DOMAINS_REMOVED_SUCCESSFULLY": "Tenant domains removed successfully",src/helpers/userHelper.js (1)
7-7: Avoid helper→service dependency for session teardown (per prior learning)Importing @services/user-sessions in a helper risks circular deps. Extract session teardown primitives into a lower-level utility (e.g., helpers/sessionCleanup.js) and consume it here and in services.
I can draft a shared sessionCleanup helper with find-active, Redis deletes, and DB end updates if you want.
Also applies to: 89-101
src/services/user.js (1)
344-351: Guard access to organizations array.Directly indexing user.organizations[0] can throw if empty.
- const roles = user.organizations[0].roles + const primaryOrg = user.organizations && user.organizations[0] + if (!primaryOrg?.roles) { + return responses.failureResponse({ message: 'ROLE_NOT_FOUND', statusCode: httpStatusCode.not_acceptable, responseCode: 'CLIENT_ERROR' }) + } + const roles = primaryOrg.rolessrc/services/user-role.js (2)
94-112: Cache invalidation OK; combine calls to reduce RTT.Use Promise.allSettled([org, profile]) with shared handler.
- await cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: userOrganizationCode, ns: common.CACHE_CONFIG.namespaces.organization.name }).catch(console.error) - await cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: userOrganizationCode, ns: common.CACHE_CONFIG.namespaces.profile.name }).catch(console.error) + const nss = [common.CACHE_CONFIG.namespaces.organization.name, common.CACHE_CONFIG.namespaces.profile.name] + await Promise.allSettled( + nss.map((ns) => cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: userOrganizationCode, ns })) + ).then((res) => res.forEach((r, i) => r.status === 'rejected' && console.error(`invalidate ${nss[i]} failed`, r.reason)))
161-179: Repeat invalidation: apply same batching refactor as update().src/services/entityType.js (4)
112-138: getOrSet options: redundant ‘key’ prop.getOrSet elsewhere uses { tenantCode, orgId, ns, id, fetchFn }. Drop ambiguous key to avoid future API drift.
- const prunedEntities = await cacheClient.getOrSet({ - key: cacheId, + const prunedEntities = await cacheClient.getOrSet({ tenantCode, orgId: organizationCode, ns, id: cacheId, fetchFn, })
139-146: Empty set as “not found” may be too strict.Consider returning empty list with 200 to reflect “no entity types” vs an error.
157-184: Same ‘key’ redundancy here; also ensure stable cacheId.cacheId includes body.value; OK. Remove extra key prop.
- const result = await cacheClient.getOrSet({ - key: cacheId, + const result = await cacheClient.getOrSet({ tenantCode, orgId: organizationCode, ns, id: cacheId, fetchFn, })
13-39: OK — org-level eviction pattern matches namespaced/versioned keys; tenant-level namespaced keys are not covered.evictTenantByPattern prefixes
tenant:${tenantCode}:. buildVersionedKey produces keys liketenant:<tenantCode>:org:<orgId>:<ns>:v<ver>:<id>, soorg:*:<ns>:*will match org-scoped keys (the*coversv<ver>:<id>). If you also need to evict tenant-level namespaced keys (noorg), add a second pattern (e.g.<ns>:*) or call evictTenantByPattern for that suffix.See src/generics/cacheHelper.js — namespacedKey, buildVersionedKey, evictTenantByPattern
src/services/organization.js (1)
196-213: Duplicate namespace invalidations; batch them.- await cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: orgDetailsBeforeUpdate.code, ns: common.CACHE_CONFIG.namespaces.organization.name }).catch(console.error) - await cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: orgDetailsBeforeUpdate.code, ns: common.CACHE_CONFIG.namespaces.profile.name }).catch(console.error) + const nss = [common.CACHE_CONFIG.namespaces.organization.name, common.CACHE_CONFIG.namespaces.profile.name] + await Promise.allSettled( + nss.map((ns) => cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: orgDetailsBeforeUpdate.code, ns })) + ).then((res) => res.forEach((r, i) => r.status === 'rejected' && console.error(`invalidate ${nss[i]} failed`, r.reason)))src/services/account.js (2)
561-566: Fire-and-forget invalidation; add await + catch for consistency.- cacheClient.invalidateOrgNamespaceVersion({ + await cacheClient.invalidateOrgNamespaceVersion({ tenantCode: tenantDetail.code, orgId: organization.code, ns: common.CACHE_CONFIG.namespaces.organization.name, - }) + }).catch((e) => console.error('invalidate org ns failed', e))
842-856: Guard orgCodes[0] access in caching path.If user has no organizations, this will throw.
- orgId: tokenPayload.data.organization_codes[0], - id: `${tokenPayload.data.organization_codes[0]}:${modelName}`, + const primaryOrgCode = tokenPayload.data.organization_codes?.[0] + if (!primaryOrgCode) return responses.failureResponse({ message: 'ORGANIZATION_NOT_FOUND', statusCode: httpStatusCode.not_acceptable, responseCode: 'CLIENT_ERROR' }) + orgId: primaryOrgCode, + id: `${primaryOrgCode}:${modelName}`,src/generics/cacheHelper.js (4)
224-229: Apply TTL to InternalCache when supported (avoid stale in-memory entries).InternalCache writes ignore ttlSeconds. If InternalCache supports TTL, pass it; otherwise fall back to current behavior.
Apply this diff:
- if (useInternal && InternalCache && InternalCache.setKey) { - try { - InternalCache.setKey(key, value) - } catch (e) {} - } + if (useInternal && InternalCache && typeof InternalCache.setKey === 'function') { + try { + if (ttlSeconds && InternalCache.setKey.length >= 3) { + InternalCache.setKey(key, value, ttlSeconds) + } else { + InternalCache.setKey(key, value) + } + } catch (e) {} + }Please confirm whether elevate-node-cache’s InternalCache.setKey accepts a TTL as the 3rd parameter.
153-171: Non-atomic fallback in bumpVersion can lose increments under concurrency.If native INCR is unavailable, the read+set path is subject to races. Prefer a Lua script (EVAL) or WATCH/MULTI/EXEC when native client is present.
Suggested approach (pseudocode):
// if (redis.eval) const script = ` local v = redis.call("GET", KEYS[1]) if not v then v = 0 else v = tonumber(v) end v = v + 1 redis.call("SET", KEYS[1], v) return v ` const newVer = await redis.eval(script, 1, vKey)Fallback remains acceptable, but document the caveat and consider gating by config.
324-349: Cluster-wide eviction caveat for SCAN.SCAN against a single node client won’t cover all hash slots in a Redis Cluster. Ensure RedisCache.native() returns a cluster-aware client and that scan/unlink/del operate cluster-wide, or implement per-node scans.
Add a note or guard: detect cluster mode and either fan out SCANs to all nodes or use keyset tracking per shard.
4-4: Remove unused code and dependency (md5, shardOf, SHARD_RETENTION_DAYS).md5 and shardOf aren’t used; SHARD_RETENTION_DAYS is unused. Drop them to reduce bundle size and install surface.
Apply this diff:
-const md5 = require('md5') @@ -const SHARD_RETENTION_DAYS = toInt(CACHE_CONFIG.shardRetentionDays, 7) +// const SHARD_RETENTION_DAYS = toInt(CACHE_CONFIG.shardRetentionDays, 7) // reserved for future use @@ -function shardOf(key) { - const h = md5(key) - const asInt = parseInt(h.slice(0, 8), 16) - return (asInt >>> 0) % SHARDS -} +// function shardOf(key) { /* reserved for future use */ }If sharding is planned soon, keep but add a TODO with intended usage.
Also applies to: 20-21, 83-87
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/configs/redis.jsis excluded by!src/configs/**
📒 Files selected for processing (20)
src/constants/common.js(2 hunks)src/controllers/v1/organization.js(3 hunks)src/controllers/v1/user-role.js(2 hunks)src/controllers/v1/user.js(1 hunks)src/database/queries/organization.js(3 hunks)src/database/queries/userOrganization.js(1 hunks)src/generics/cacheHelper.js(1 hunks)src/generics/redis-communication.js(0 hunks)src/helpers/userHelper.js(3 hunks)src/locales/en.json(1 hunks)src/package.json(1 hunks)src/services/account.js(10 hunks)src/services/admin.js(3 hunks)src/services/entities.js(4 hunks)src/services/entityType.js(6 hunks)src/services/organization.js(6 hunks)src/services/public.js(3 hunks)src/services/tenant.js(4 hunks)src/services/user-role.js(6 hunks)src/services/user.js(4 hunks)
💤 Files with no reviewable changes (1)
- src/generics/redis-communication.js
🧰 Additional context used
📓 Path-based instructions (3)
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/userOrganization.jssrc/database/queries/organization.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/user-role.jssrc/controllers/v1/organization.jssrc/controllers/v1/user.js
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/entityType.jssrc/services/tenant.jssrc/services/account.jssrc/services/user.jssrc/services/public.jssrc/services/organization.jssrc/services/admin.jssrc/services/user-role.jssrc/services/entities.js
🧠 Learnings (8)
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/database/queries/userOrganization.jssrc/database/queries/organization.jssrc/helpers/userHelper.jssrc/services/user-role.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In the ELEVATE-Project/user codebase, when using organizationQueries.update with { returning: true, raw: true } options, the method consistently returns a structured result with updatedRows property when successful. The initial check for orgRowsAffected === 0 already handles failed updates, so additional null checking for updatedRows is redundant when the code execution reaches the event emission logic.
Applied to files:
src/database/queries/userOrganization.jssrc/database/queries/organization.jssrc/helpers/userHelper.jssrc/services/organization.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/database/queries/userOrganization.jssrc/database/queries/organization.jssrc/helpers/userHelper.jssrc/services/user-role.js
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/controllers/v1/user-role.jssrc/controllers/v1/organization.jssrc/services/organization.jssrc/controllers/v1/user.js
📚 Learning: 2025-07-31T08:43:35.971Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
Applied to files:
src/controllers/v1/organization.jssrc/services/organization.js
📚 Learning: 2025-08-08T13:06:32.911Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: In ELEVATE-Project/user, avoid potential circular dependencies between src/helpers/userHelper.js and src/services/user-sessions by extracting shared session cleanup logic (finding active sessions, deleting Redis keys, ending sessions) into a dedicated utility (e.g., helpers/sessionCleanup.js) that both layers can consume.
Applied to files:
src/services/user.jssrc/helpers/userHelper.js
📚 Learning: 2025-08-08T15:12:44.423Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:117-156
Timestamp: 2025-08-08T15:12:44.423Z
Learning: In ELEVATE-Project/user, prefer extracting shared session teardown (find active sessions, delete Redis keys, mark ended_at) into helpers/sessionCleanup.js so both helpers and services use it, preventing upward (helper→service) dependencies and avoiding circular imports.
Applied to files:
src/helpers/userHelper.js
📚 Learning: 2025-08-08T13:06:32.911Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: When session management is shared across helpers and services in ELEVATE-Project/user, prefer extracting the primitives (query active sessions, delete Redis keys, mark ended) into a lower-level generics utility to avoid helper↔service circular dependencies and keep Redis key handling consistent.
Applied to files:
src/helpers/userHelper.js
🧬 Code graph analysis (14)
src/database/queries/userOrganization.js (3)
src/services/account.js (2)
options(1633-1639)options(2115-2115)src/services/organization.js (2)
options(275-277)options(318-320)src/services/user.js (2)
options(330-332)options(450-454)
src/controllers/v1/organization.js (1)
src/services/organization.js (2)
updatedOrg(567-579)updatedOrg(679-683)
src/services/entityType.js (5)
src/services/account.js (15)
common(16-16)entityTypeQueries(24-24)cacheClient(42-42)organizationCode(194-194)tenantCode(705-705)ns(2043-2043)prunedEntities(366-366)prunedEntities(842-856)prunedEntities(1549-1549)result(568-568)result(866-866)result(1572-1572)result(1677-1677)result(1803-1803)result(2083-2083)src/services/organization.js (9)
common(2-2)common(597-597)common(698-698)cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)tenantCode(272-272)filter(430-430)filter(498-501)src/services/user.js (18)
common(10-10)entityTypeQueries(15-15)cacheClient(28-28)ns(146-146)ns(310-310)ns(587-587)attributes(252-252)attributes(278-278)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)filter(65-72)filter(242-245)filter(277-277)filter(308-308)filter(445-445)filter(554-560)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/database/queries/entityType.js (3)
entities(141-145)result(53-59)result(147-153)
src/database/queries/organization.js (2)
src/controllers/v1/organization.js (8)
result(124-124)result(143-143)result(159-163)result(180-185)result(194-198)result(206-210)result(251-255)result(296-300)src/database/models/organization.js (1)
Organization(3-67)
src/services/tenant.js (7)
src/services/account.js (6)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)common(16-16)src/services/entities.js (2)
cacheClient(7-7)common(6-6)src/services/entityType.js (1)
cacheClient(10-10)src/services/public.js (3)
cacheClient(15-15)require(11-11)common(14-14)src/services/organization.js (3)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)src/services/user.js (1)
cacheClient(28-28)src/generics/cacheHelper.js (1)
common(5-5)
src/services/account.js (6)
src/services/entityType.js (5)
cacheClient(10-10)prunedEntities(130-137)entityTypeQueries(5-5)ns(115-115)ns(160-160)src/services/public.js (9)
cacheClient(15-15)tenantDetail(37-42)tenantDetail(94-99)common(14-14)tenantQueries(2-2)prunedEntities(165-165)modelName(151-151)entityTypeQueries(8-8)utils(10-10)src/services/organization.js (10)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)common(2-2)common(597-597)common(698-698)tenantCode(272-272)utils(5-5)cacheKey(135-135)cacheKey(243-243)src/services/user.js (19)
cacheClient(28-28)common(10-10)user(50-53)user(335-335)user(458-458)user(508-508)user(543-543)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)modelName(363-363)entityTypeQueries(15-15)utils(12-12)ns(146-146)ns(310-310)ns(587-587)cacheKey(147-152)cacheKey(588-593)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/generics/cacheHelper.js (1)
raw(118-118)
src/services/user.js (6)
src/services/account.js (28)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)ns(2043-2043)common(16-16)cacheKey(2044-2049)tenantCode(705-705)organizationCode(194-194)utils(25-25)options(1633-1639)options(2115-2115)user(136-145)user(757-757)user(899-899)user(949-949)user(1102-1102)user(1255-1264)user(1436-1444)user(1784-1784)user(2007-2010)user(2122-2122)roles(1922-1924)modelName(349-349)modelName(836-836)modelName(1538-1538)entityTypeQueries(24-24)src/services/entityType.js (5)
cacheClient(10-10)ns(115-115)ns(160-160)common(4-4)entityTypeQueries(5-5)src/services/admin.js (12)
cacheClient(40-40)require(11-11)require(27-27)require(34-34)require(36-36)common(15-15)cacheKey(610-615)utils(17-17)user(55-55)user(201-204)user(339-339)user(450-450)src/generics/utils.js (7)
require(12-12)require(16-16)require(21-21)common(20-20)removeDefaultOrgEntityTypes(835-842)value(892-892)value(914-914)src/controllers/v1/user.js (2)
userDetails(48-54)userDetails(85-85)src/database/queries/users.js (1)
user(225-270)
src/services/public.js (2)
src/services/account.js (13)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)tenantDetail(74-77)tenantDetail(706-712)tenantDetail(1057-1059)tenantDetail(1189-1192)tenantDetail(1387-1389)common(16-16)tenantQueries(34-34)organizationQueries(18-18)src/generics/cacheHelper.js (2)
require(3-3)common(5-5)
src/helpers/userHelper.js (3)
src/services/account.js (17)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)user(136-145)user(757-757)user(899-899)user(949-949)user(1102-1102)user(1255-1264)user(1436-1444)user(1784-1784)user(2007-2010)user(2122-2122)ns(2043-2043)common(16-16)src/database/queries/userOrganization.js (1)
require(3-3)src/database/queries/users.js (2)
require(4-4)user(225-270)
src/services/organization.js (2)
src/database/queries/organization.js (6)
require(2-2)require(3-3)common(4-4)organization(70-74)organization(241-241)t(7-7)src/controllers/v1/organization.js (2)
common(10-10)updatedOrg(95-100)
src/generics/cacheHelper.js (1)
src/services/account.js (3)
common(16-16)ns(2043-2043)_(11-11)
src/services/admin.js (4)
src/services/account.js (19)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)common(16-16)cacheKey(2044-2049)user(136-145)user(757-757)user(899-899)user(949-949)user(1102-1102)user(1255-1264)user(1436-1444)user(1784-1784)user(2007-2010)user(2122-2122)ns(2043-2043)organizationCode(194-194)src/services/organization.js (15)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)require(12-12)require(14-14)require(15-15)require(18-18)common(2-2)common(597-597)common(698-698)cacheKey(135-135)cacheKey(243-243)results(609-609)results(710-710)i(307-307)src/helpers/userHelper.js (2)
cacheClient(2-2)common(1-1)src/services/user.js (11)
cacheClient(28-28)require(17-17)common(10-10)cacheKey(147-152)cacheKey(588-593)user(50-53)user(335-335)user(458-458)ns(146-146)ns(310-310)ns(587-587)
src/services/user-role.js (3)
src/services/organization.js (6)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)common(2-2)common(597-597)common(698-698)src/services/user.js (3)
cacheClient(28-28)userOrganizationCode(474-474)common(10-10)src/controllers/v1/user-role.js (1)
updateRole(60-66)
src/services/entities.js (3)
src/services/account.js (7)
common(16-16)require(26-26)require(27-27)require(37-37)require(41-41)cacheClient(42-42)tenantCode(705-705)src/services/entityType.js (5)
common(4-4)require(6-6)require(7-7)require(8-8)cacheClient(10-10)src/services/organization.js (7)
common(2-2)common(597-597)common(698-698)cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)tenantCode(272-272)
🔇 Additional comments (31)
src/database/queries/userOrganization.js (2)
109-119: Optional chaining on organizationAttributes: LGTM.Prevents runtime errors when the caller omits organizationAttributes.
120-125: Verified — callers supply tenant_code; no change required.
Both call sites of userOrganizationQueries.findAll include tenant_code: src/helpers/userHelper.js (lines 74–76) and src/helpers/userInvite.js (lines 667–674).src/services/tenant.js (2)
33-34: Cache client import: LGTM.Keep cache coupling centralized via cacheHelper.
642-645: Invalidate after removeDomain: LGTM.src/controllers/v1/organization.js (3)
95-101: Tenant-aware update call — confirm service signatureFile: src/controllers/v1/organization.js (lines 95–101). Controller calls orgService.update(req.params.id, req.body, req.decodedToken.id, req.decodedToken.tenant_code); confirm src/services/organization.js exports update(id, body, userId, tenantCode) or update the service/controller to match.
204-210: Confirm tenant-scoping in orgService.removeRelatedOrgController passes tenant_code (req.decodedToken.tenant_code) at src/controllers/v1/organization.js:204-210, but the rg search returned no matches for exports.removeRelatedOrg in src/services/organization.js — verify the service implementation exists and enforces tenant scoping (or provide its file location).
192-199: addRelatedOrg now tenant-scoped: LGTM.src/controllers/v1/user.js (1)
48-54: Verify userService.read signature matches forwarded args.rg returned no matches for exports.read in src/services/user.js — confirm the service implements read(id, headers, language, tenant_code, organization_code) (or equivalent ordering) and uses tenant_code + organization_code for multi-tenant scoping/cache-keying. Check src/services/user.js or the actual user service file.
src/constants/common.js (1)
104-104: Constant rename — no legacy ENGLISH_LANGUGE_CODE usages found.
Search returned no matches for ENGLISH_LANGUGE_CODE; code defines ENGLISH_LANGUAGE_CODE in src/constants/common.js and references it in src/services/user-role.js and src/services/user.js.src/controllers/v1/user-role.js (2)
84-89: delete signature verified — no action required.Controller invokes roleService.delete(id, organization_id, organization_code, tenant_code) and the service defines static async delete(id, userOrganizationId, userOrganizationCode, tenantCode) — parameters match (src/controllers/v1/user-role.js → src/services/user-role.js).
60-66: Added organization_code to update — OK (service signature aligned).
src/services/user-role.js defines update(id, bodyData, userOrganizationId, userOrganizationCode, tenantCode) and src/controllers/v1/user-role.js passes (id, body, organization_id, organization_code, tenant_code); no other roleService.update callers found.src/package.json (1)
46-46: Upgrade to elevate-node-cache v2: verify breaking changes; dedupe @shelf/jest-mongodb
- src/package.json contains @shelf/jest-mongodb in dependencies (^3.0.1) and devDependencies (^3.0.2). Remove from dependencies, keep it only in devDependencies, and align to a single version.
- Repo search didn't locate elevate-node-cache usages (rg skipped files); confirm v2 API/engine compatibility by finding usages and checking release notes/engines. Run:
- rg -nP --hidden -uu -S 'invalidateTenantVersion|cacheHelper|CACHE_CONFIG|elevate-node-cache'
- rg -nP --hidden -uu -S "from\s+['"]elevate-node-cache['"]|require(['"]elevate-node-cache['"])"
- npm view elevate-node-cache@2 engines version
Or allow a web lookup of v2 release notes and engine requirements before merging.src/services/entities.js (1)
55-57: Good placement: invalidate after successful mutationsCache invalidation is correctly sequenced after create/update/delete. LGTM.
Also applies to: 115-117, 197-199
src/helpers/userHelper.js (2)
73-79: Good: capture org codes before delete for targeted cache clearsGrabbing organization_code before removing joins is correct and avoids a second query.
102-116: Parallelize cache deletions; prefer namespace-version invalidation when available
- Parallelize per-org key deletes to reduce latency — use Promise.allSettled over orgCodes.
- Prefer bumping/invalidation of the profile namespace per org (if a helper like invalidateOrgNamespaceVersion or a cacheClient namespace-version API exists) to decouple from key shape; if unavailable, keep per-key deletes.
- Note: orgCodes here are organization_code strings (not numeric IDs).
- for (const orgId of orgCodes) { - const fullKey = await cacheClient.versionedKey({ - tenantCode: user.tenant_code, - orgId, - ns, - id: userId, - }) - await cacheClient.del(fullKey) - } + await Promise.allSettled( + orgCodes.map(async (orgId) => { + const fullKey = await cacheClient.versionedKey({ + tenantCode: user.tenant_code, + orgId, + ns, + id: userId, + }) + return cacheClient.del(fullKey) + }) + )src/services/public.js (2)
37-42: Good use of getOrSet for tenant detailsKeying by tenant namespace with id=tenantCode is correct.
62-62: Status code change to 200 OK is appropriateReturning OK for a fetch endpoint is consistent.
src/database/queries/organization.js (2)
114-133: Incorrect assumption — call sites pass arrays, not a single relatedOrgCall sites show arrays: appendRelatedOrg is invoked with newRelatedOrgs (src/services/organization.js:582, 686) and removeRelatedOrg uses relatedOrgs = [] (src/services/organization.js:637) and is called from src/controllers/v1/organization.js:204–206. Either make appendRelatedOrg accept arrays or change callers to pass a single scalar.
Likely an incorrect or invalid review comment.
152-175: Incorrect — not a bug for current callsiteremoveRelatedOrg is invoked only from src/services/organization.js:686 with the current org id (scalar) as the first arg, so removedOrgIds is a scalar and sequelize.fn('array_remove', ..., removedOrgIds) correctly removes that value from the other orgs. The proposed nested array_remove is unnecessary for current usage; apply it only as an optional defensive change if you expect removedOrgIds to be an array in other call sites.
Likely an incorrect or invalid review comment.
src/services/user-role.js (3)
118-124: Success payload: label addition LGTM.
244-246: Language constant fix LGTM.
82-92: Signature extended — verify all callers updated.New signature: update(id, bodyData, userOrganizationId, userOrganizationCode, tenantCode). Confirm all call sites pass the two added params and update related type defs/tests. Search for call sites returned no matches; unable to confirm callers updated.
Location: src/services/user-role.js:82-92.
src/services/entityType.js (1)
196-209: Delete + invalidation LGTM.src/services/organization.js (4)
534-559: Input normalization and tenant scoping LGTM.
600-617: Multi-org invalidation via Promise.allSettled LGTM.
691-717: Invalidation batching LGTM.
181-181: Signature changed — ensure all callers pass tenantCodeupdate(id, bodyData, loggedInUserId, tenantCode) now uses tenantCode (cache invalidation). Repository scan did not locate any .update(...) call sites; verify every controller/service that invokes this method passes tenantCode or update call sites accordingly.
Location: src/services/organization.js — update function (approx. lines 150–200).
src/services/account.js (2)
705-713: Tenant caching LGTM.
2043-2051: Profile cache key invalidation LGTM.src/services/user.js (1)
306-306: Signature change ripple-check — ensure callers pass organizationCodesrc/services/user.js:306 — static async read(id, header = null, language, tenantCode, organizationCode). Verify all call sites (controllers, services, helpers, tests) pass the new 5th arg; automated scan returned no matches — run a repo-wide search for "userService.read(" / "User.read(" and update any callers missing organizationCode.
src/generics/cacheHelper.js (1)
257-275: Fix namespace defaulting in getOrSet (TTL/internal-cache mismatch when only id is provided).When ns is undefined but id is provided, the key builder defaults ns to 'ns' while nsTtl/nsUseInternal still see undefined, causing wrong TTL/useInternal selection.
Apply this diff:
-async function getOrSet({ key, tenantCode, ttl = undefined, fetchFn, orgId, ns, id, useInternal = undefined }) { - if (!namespaceEnabled(ns)) return await fetchFn() - - const resolvedUseInternal = nsUseInternal(ns, useInternal) - // build versioned key (keeps previous behaviour but adds version token) - const fullKey = - ns || id - ? await buildVersionedKey({ tenantCode, orgId, ns: ns || 'ns', id: id || key }) - : await buildVersionedKey({ tenantCode, key }) +async function getOrSet({ key, tenantCode, ttl = undefined, fetchFn, orgId, ns, id, useInternal = undefined }) { + const effNs = ns || (id ? 'ns' : undefined) + if (!namespaceEnabled(effNs)) return await fetchFn() + + const resolvedUseInternal = nsUseInternal(effNs, useInternal) + // build versioned key (keeps previous behaviour but adds version token) + const fullKey = + effNs + ? await buildVersionedKey({ tenantCode, orgId, ns: effNs, id: id || key }) + : await buildVersionedKey({ tenantCode, key }) const cached = await get(fullKey, { useInternal: resolvedUseInternal }) if (cached !== null && cached !== undefined) return cached const value = await fetchFn() if (value !== undefined) { - await set(fullKey, value, nsTtl(ns, ttl), { useInternal: resolvedUseInternal }) + await set(fullKey, value, nsTtl(effNs, ttl), { useInternal: resolvedUseInternal }) } return value }No matches found for id-only callers with the supplied search; verify callers that pass id without ns.
| async function invalidateTenantVersion({ tenantCode, ns } = {}) { | ||
| if (!tenantCode) return | ||
| return bumpVersion({ tenantCode, ns }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Tenant-level invalidation bug: invalidateTenantVersion bumps tenant+ns, but tenant-level keys use tenant-only version.
Tenant-level keys (no ns) are built using getVersion({ tenantCode }), so bumping with ns won't invalidate them.
Apply this diff:
-async function invalidateTenantVersion({ tenantCode, ns } = {}) {
- if (!tenantCode) return
- return bumpVersion({ tenantCode, ns })
-}
+async function invalidateTenantVersion({ tenantCode } = {}) {
+ if (!tenantCode) return
+ // Tenant-level keys use tenant-only version (no namespace)
+ return bumpVersion({ tenantCode })
+}List usages to confirm none depend on the old (incorrect) signature:
🏁 Script executed:
#!/bin/bash
rg -nP --type=js --type=ts -C2 '\binvalidateTenantVersion\s*\('Length of output: 1213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cacheHelper.js (1-520) ==="
sed -n '1,520p' src/generics/cacheHelper.js || true
echo
echo "=== cacheHelper.js (520-1000) ==="
sed -n '520,1000p' src/generics/cacheHelper.js || true
echo
echo "=== src/services/tenant.js (420-660) ==="
sed -n '420,660p' src/services/tenant.js || true
echo
echo "=== find function defs (getVersion|bumpVersion|invalidateTenantVersion) ==="
rg -nP --type=js -C3 'function\s+(getVersion|bumpVersion|invalidateTenantVersion)\b' src || true
echo
echo "=== getVersion usages ==="
rg -nP --type=js 'getVersion\s*\(' -n src || true
echo
echo "=== bumpVersion usages ==="
rg -nP --type=js 'bumpVersion\s*\(' -n src || true
echo
echo "=== show CACHE_CONFIG / namespaces references ==="
rg -n --hidden -n "CACHE_CONFIG" || true
rg -n --hidden -n "namespaces" src || trueLength of output: 31532
Invalidate tenant version must bump tenant-only version (no ns).
invalidateTenantVersion currently forwards ns to bumpVersion (bumping the namespaced __version). Tenant-level keys use getVersion({ tenantCode }) and require bumpVersion({ tenantCode }). Change the function and update callers that pass ns to use invalidateNamespaceVersion when they intend to invalidate the namespace.
Apply this diff:
-async function invalidateTenantVersion({ tenantCode, ns } = {}) {
- if (!tenantCode) return
- return bumpVersion({ tenantCode, ns })
-}
+async function invalidateTenantVersion({ tenantCode } = {}) {
+ if (!tenantCode) return
+ // Tenant-level keys use tenant-only version (no namespace)
+ return bumpVersion({ tenantCode })
+}Call sites that currently pass ns (update them to call invalidateNamespaceVersion if namespaced invalidation was intended):
- src/services/tenant.js:443
- src/services/tenant.js:527
- src/services/tenant.js:643
📝 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.
| async function invalidateTenantVersion({ tenantCode, ns } = {}) { | |
| if (!tenantCode) return | |
| return bumpVersion({ tenantCode, ns }) | |
| } | |
| async function invalidateTenantVersion({ tenantCode } = {}) { | |
| if (!tenantCode) return | |
| // Tenant-level keys use tenant-only version (no namespace) | |
| return bumpVersion({ tenantCode }) | |
| } |
🤖 Prompt for AI Agents
In src/generics/cacheHelper.js around lines 370 to 373, invalidateTenantVersion
incorrectly forwards ns to bumpVersion causing the namespaced __version to be
bumped; change invalidateTenantVersion to call bumpVersion with only {
tenantCode } (no ns) so it bumps the tenant-level version, and update callers
that currently pass ns to instead call invalidateNamespaceVersion when namespace
invalidation was intended (specifically fix call sites in src/services/tenant.js
at lines ~443, ~527, and ~643 to replace invalidateTenantVersion({ tenantCode,
ns }) with invalidateNamespaceVersion({ tenantCode, ns }) or similar).
| "RELATED_ORGANIZATIONS_NOT_FOUND": "Related organisations not found!", | ||
| "TENANT_NOT_FOUND": "Tenant not found.", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
New key added: ensure i18n naming/spelling consistency.
Key uses American spelling (ORGANIZATIONS) while value uses British (“organisations”). Align with project convention (many keys here use “Organisation…”). If standard is British, consider RENAMING the key to RELATED_ORGANISATIONS_NOT_FOUND, or update the value to “organizations.”
🏁 Script executed:
#!/bin/bash
rg -nP --type=json 'RELATED_ORGANIZATIONS_NOT_FOUND|RELATED_ORGANISATIONS_NOT_FOUND'Length of output: 95
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repository for 'organisation' (case-insensitive)..."
rg -nSi --hidden -g '!(node_modules|dist|build)' 'organisation' || true
echo
echo "Searching repository for 'organization' (case-insensitive)..."
rg -nSi --hidden -g '!(node_modules|dist|build)' 'organization' || true
echo
echo "Searching locale JSON keys for uppercase variants (ORGANISATION / ORGANIZATION)..."
rg -nP --hidden -g 'src/**' '"[A-Z0-9_]*ORGANISATION[A-Z0-9_]*"' || true
rg -nP --hidden -g 'src/**' '"[A-Z0-9_]*ORGANIZATION[A-Z0-9_]*"' || true
echo
echo "Counts summary:"
echo -n "organisation occurrences: " ; rg -nSi --hidden -g '!(node_modules|dist|build)' 'organisation' | wc -l
echo -n "organization occurrences: " ; rg -nSi --hidden -g '!(node_modules|dist|build)' 'organization' | wc -lLength of output: 1980
Use American spelling in the message value — change to "Related organizations not found!"
Update src/locales/en.json — RELATED_ORGANIZATIONS_NOT_FOUND (line 140): "Related organisations not found!" → "Related organizations not found!". Keys in this file use "ORGANIZATION" (American); keep the key as-is.
🤖 Prompt for AI Agents
In src/locales/en.json around lines 140-141, update the value for
RELATED_ORGANIZATIONS_NOT_FOUND from "Related organisations not found!" to use
American spelling: "Related organizations not found!"; keep the JSON key
unchanged and ensure the string punctuation and surrounding commas remain valid
JSON.
| cacheClient.invalidateOrgNamespaceVersion({ | ||
| tenantCode, | ||
| orgId: organizationId, | ||
| ns: common.CACHE_CONFIG.namespaces.organization.name, | ||
| }) | ||
|
|
||
| const cacheKey = cacheClient.namespacedKey({ | ||
| tenantCode, | ||
| orgId: organizationId, | ||
| ns: common.CACHE_CONFIG.namespaces.profile.name, | ||
| id: user.id, | ||
| }) | ||
| await cacheClient.del(cacheKey) | ||
|
|
There was a problem hiding this comment.
Wrong org dimension used for cache keys + missing await
- orgId should match how keys are written elsewhere (code, not numeric id). Using organizationId here likely fails to hit the right keys.
- Also add await to surface errors and preserve ordering.
Apply this diff:
- cacheClient.invalidateOrgNamespaceVersion({
- tenantCode,
- orgId: organizationId,
- ns: common.CACHE_CONFIG.namespaces.organization.name,
- })
+ await cacheClient.invalidateOrgNamespaceVersion({
+ tenantCode,
+ orgId: organization.code, // use org code
+ ns: common.CACHE_CONFIG.namespaces.organization.name,
+ })
- const cacheKey = cacheClient.namespacedKey({
- tenantCode,
- orgId: organizationId,
- ns: common.CACHE_CONFIG.namespaces.profile.name,
- id: user.id,
- })
+ const cacheKey = cacheClient.namespacedKey({
+ tenantCode,
+ orgId: organization.code, // use org code
+ ns: common.CACHE_CONFIG.namespaces.profile.name,
+ id: user.id,
+ })📝 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.
| cacheClient.invalidateOrgNamespaceVersion({ | |
| tenantCode, | |
| orgId: organizationId, | |
| ns: common.CACHE_CONFIG.namespaces.organization.name, | |
| }) | |
| const cacheKey = cacheClient.namespacedKey({ | |
| tenantCode, | |
| orgId: organizationId, | |
| ns: common.CACHE_CONFIG.namespaces.profile.name, | |
| id: user.id, | |
| }) | |
| await cacheClient.del(cacheKey) | |
| await cacheClient.invalidateOrgNamespaceVersion({ | |
| tenantCode, | |
| orgId: organization.code, // use org code | |
| ns: common.CACHE_CONFIG.namespaces.organization.name, | |
| }) | |
| const cacheKey = cacheClient.namespacedKey({ | |
| tenantCode, | |
| orgId: organization.code, // use org code | |
| ns: common.CACHE_CONFIG.namespaces.profile.name, | |
| id: user.id, | |
| }) | |
| await cacheClient.del(cacheKey) |
🤖 Prompt for AI Agents
In src/services/admin.js around lines 604 to 617, the cache operations use the
numeric organizationId and omit awaiting invalidateOrgNamespaceVersion; change
the orgId fields to use the same org dimension used elsewhere (organization
string/key rather than numeric organizationId) for both
invalidateOrgNamespaceVersion and namespacedKey, and add await before
cacheClient.invalidateOrgNamespaceVersion so the invalidation is awaited
(keeping the existing await on cacheClient.del) to surface errors and preserve
ordering.
| await Promise.allSettled( | ||
| namespaces.map((ns) => | ||
| cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns }) | ||
| ) | ||
| ).then((results) => | ||
| results.forEach((r, i) => { | ||
| if (r.status === 'rejected') { | ||
| console.error(`invalidate failed for ns=${namespaces[i]} org=${orgId}`, r.reason) | ||
| } | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Fix undefined variable in log and simplify Promise handling
- orgId is undefined; should log organizationCode (or passed orgId).
- Avoid mixing await with .then; use a single await and iterate results.
Apply this diff:
- await Promise.allSettled(
- namespaces.map((ns) =>
- cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns })
- )
- ).then((results) =>
- results.forEach((r, i) => {
- if (r.status === 'rejected') {
- console.error(`invalidate failed for ns=${namespaces[i]} org=${orgId}`, r.reason)
- }
- })
- )
+ const results = await Promise.allSettled(
+ namespaces.map((ns) =>
+ cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns })
+ )
+ )
+ results.forEach((r, i) => {
+ if (r.status === 'rejected') {
+ console.error(`invalidate failed for ns=${namespaces[i]} org=${organizationCode}`, r.reason)
+ }
+ })📝 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.
| await Promise.allSettled( | |
| namespaces.map((ns) => | |
| cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns }) | |
| ) | |
| ).then((results) => | |
| results.forEach((r, i) => { | |
| if (r.status === 'rejected') { | |
| console.error(`invalidate failed for ns=${namespaces[i]} org=${orgId}`, r.reason) | |
| } | |
| }) | |
| ) | |
| const results = await Promise.allSettled( | |
| namespaces.map((ns) => | |
| cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns }) | |
| ) | |
| ) | |
| results.forEach((r, i) => { | |
| if (r.status === 'rejected') { | |
| console.error(`invalidate failed for ns=${namespaces[i]} org=${organizationCode}`, r.reason) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/services/admin.js around lines 708 to 718, the code mixes await with
.then and logs an undefined variable orgId; change to await
Promise.allSettled(...) into a variable (e.g., const results = await
Promise.allSettled(...)), then iterate results with a simple forEach/for loop to
check for rejected outcomes; update the error log to reference organizationCode
(or the correct orgId parameter if available) instead of orgId and include
r.reason in the message.
| static async _invalidateEntityCaches({ tenantCode, organizationCode }) { | ||
| try { | ||
| await cacheClient.invalidateOrgNamespaceVersion({ | ||
| tenantCode, | ||
| orgId: organizationCode, | ||
| ns: common.CACHE_CONFIG.namespaces.entity_types.name, | ||
| }) | ||
|
|
||
| await cacheClient.invalidateOrgNamespaceVersion({ | ||
| tenantCode, | ||
| orgId: organizationCode, | ||
| ns: common.CACHE_CONFIG.namespaces.profile.name, | ||
| }) | ||
|
|
||
| if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) { | ||
| await cacheClient.evictTenantByPattern(tenantCode, { | ||
| patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.entity_types.name}:*`, | ||
| }) | ||
| await cacheClient.evictTenantByPattern(tenantCode, { | ||
| patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.profile.name}:*`, | ||
| }) | ||
| } | ||
| } catch (err) { | ||
| console.error('Entity cache invalidation failed', err) | ||
| // Do not throw. Cache failures should not block DB ops. | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure orgId semantics + parallelize invalidations
- Confirm orgId is consistently the organization code across the codebase; passing a numeric ID here would miss cache entries keyed by code. If mixed, normalize to code.
- The two invalidations are independent; run them in parallel and guard with Promise.allSettled so one failure doesn’t block the other.
Apply this diff to parallelize and harden:
static async _invalidateEntityCaches({ tenantCode, organizationCode }) {
try {
- await cacheClient.invalidateOrgNamespaceVersion({
- tenantCode,
- orgId: organizationCode,
- ns: common.CACHE_CONFIG.namespaces.entity_types.name,
- })
-
- await cacheClient.invalidateOrgNamespaceVersion({
- tenantCode,
- orgId: organizationCode,
- ns: common.CACHE_CONFIG.namespaces.profile.name,
- })
+ const { entity_types, profile } = common.CACHE_CONFIG.namespaces
+ await Promise.allSettled([
+ cacheClient.invalidateOrgNamespaceVersion({
+ tenantCode,
+ orgId: organizationCode,
+ ns: entity_types.name,
+ }),
+ cacheClient.invalidateOrgNamespaceVersion({
+ tenantCode,
+ orgId: organizationCode,
+ ns: profile.name,
+ }),
+ ])
- if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) {
- await cacheClient.evictTenantByPattern(tenantCode, {
- patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.entity_types.name}:*`,
- })
- await cacheClient.evictTenantByPattern(tenantCode, {
- patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.profile.name}:*`,
- })
- }
+ if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) {
+ await Promise.allSettled([
+ cacheClient.evictTenantByPattern(tenantCode, {
+ patternSuffix: `org:*:${entity_types.name}:*`,
+ }),
+ cacheClient.evictTenantByPattern(tenantCode, {
+ patternSuffix: `org:*:${profile.name}:*`,
+ }),
+ ])
+ }
} catch (err) {
console.error('Entity cache invalidation failed', err)
// Do not throw. Cache failures should not block DB ops.
}
}🏁 Script executed:
#!/bin/bash
# Verify orgId dimension used for cache keys in readers/writers
rg -n -C2 "invalidateOrgNamespaceVersion\s*\(" --type=ts --type=js
rg -n -C2 "getOrSet\(\s*{[^}]*ns:\s*common\.CACHE_CONFIG\.namespaces\.(profile|entity_types|organization)\.name" --type=ts --type=js
rg -n -C2 "namespacedKey\(\s*{[^}]*orgId" --type=ts --type=jsLength of output: 5006
Ensure orgId is consistently the organization code and parallelize cache invalidations
- Confirm orgId semantics: usages are mixed — callers pass .code/code-like vars in src/services/organization.js (197-199), src/services/account.js (561-563), src/services/user-role.js (95-106,162-173), src/services/entities.js (10-20) but admin passes organizationId (src/services/admin.js:604). Normalize to a single canonical form (prefer organization.code) or centralize normalization in the cache helper to avoid cache misses.
- Parallelize independent cache ops: wrap the two invalidateOrgNamespaceVersion calls and the evictTenantByPattern calls in Promise.allSettled (admin.js already uses this pattern at src/services/admin.js:708-712). Apply the diff suggested in this review for entities.js.
🤖 Prompt for AI Agents
In src/services/entities.js around lines 10 to 36, the cache helper is called
with mixed orgId semantics and serial awaits; ensure orgId consistently uses
organizationCode (normalize incoming values or convert here so orgId is the
organization.code string) to avoid cache key mismatches, and parallelize
independent cache operations by replacing the two invalidateOrgNamespaceVersion
awaits with a single Promise.allSettled([...]) and likewise wrap the two
evictTenantByPattern calls in Promise.allSettled([...]) when
DEFAULT_ORGANISATION_CODE matches; keep the existing try/catch and log errors
but do not rethrow.
| const tenantDetail = await cacheClient.getOrSet({ | ||
| tenantCode: code, // ensures key is tenant-scoped | ||
| ns: common.CACHE_CONFIG.namespaces.tenant.name, | ||
| id: code, // unique per tenant | ||
| fetchFn: () => tenantQueries.findOne({ code: tenantDomain.tenant_code }, {}), | ||
| }) | ||
|
|
There was a problem hiding this comment.
Bug: undefined variable ‘code’ in checkUsername
‘code’ isn’t defined in this scope; should use tenantDomain.tenant_code for both tenantCode and id.
Apply this diff:
- const tenantDetail = await cacheClient.getOrSet({
- tenantCode: code, // ensures key is tenant-scoped
- ns: common.CACHE_CONFIG.namespaces.tenant.name,
- id: code, // unique per tenant
- fetchFn: () => tenantQueries.findOne({ code: tenantDomain.tenant_code }, {}),
- })
+ const tenantDetail = await cacheClient.getOrSet({
+ tenantCode: tenantDomain.tenant_code, // tenant-scoped
+ ns: common.CACHE_CONFIG.namespaces.tenant.name,
+ id: tenantDomain.tenant_code, // unique per tenant
+ fetchFn: () => tenantQueries.findOne({ code: tenantDomain.tenant_code }, {}),
+ })📝 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.
| const tenantDetail = await cacheClient.getOrSet({ | |
| tenantCode: code, // ensures key is tenant-scoped | |
| ns: common.CACHE_CONFIG.namespaces.tenant.name, | |
| id: code, // unique per tenant | |
| fetchFn: () => tenantQueries.findOne({ code: tenantDomain.tenant_code }, {}), | |
| }) | |
| const tenantDetail = await cacheClient.getOrSet({ | |
| tenantCode: tenantDomain.tenant_code, // tenant-scoped | |
| ns: common.CACHE_CONFIG.namespaces.tenant.name, | |
| id: tenantDomain.tenant_code, // unique per tenant | |
| fetchFn: () => tenantQueries.findOne({ code: tenantDomain.tenant_code }, {}), | |
| }) |
🤖 Prompt for AI Agents
In src/services/public.js around lines 94 to 100, the cacheClient.getOrSet call
uses an undefined variable `code`; replace both `tenantCode` and `id` properties
to use `tenantDomain.tenant_code` instead (so the cache key is tenant-scoped and
unique per tenant), keeping the existing fetchFn as-is that already references
tenantDomain.tenant_code.
| const ns = common.CACHE_CONFIG.namespaces.profile.name | ||
| const cacheKey = cacheClient.namespacedKey({ | ||
| tenantCode, | ||
| orgId: orgCode, | ||
| ns, | ||
| id, | ||
| }) | ||
| await cacheClient.del(cacheKey) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify del() targets versioned keys.
read() uses versionedKey + get(), write uses setScoped(), but invalidation uses del(namespacedKey). Confirm del() deletes the versioned entry created by setScoped().
🏁 Script executed:
#!/bin/bash
# Inspect cache helper to confirm `del()` behavior and key shapes.
rg -n "class|module\.exports|exports\." node_modules -g '!**' >/dev/null 2>&1 || true
fd cacheHelper --hidden
rg -nP "namespacedKey|versionedKey|setScoped|getOrSet|del\s*\(" -n src/generics/cacheHelper.jsLength of output: 779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="src/generics/cacheHelper.js"
echo "== file: $f =="
nl -ba -w4 -s': ' "$f" | sed -n '1,120p'
echo "== lines 220-320 =="
nl -ba -w4 -s': ' "$f" | sed -n '220,320p'
echo "== lines 320-380 =="
nl -ba -w4 -s': ' "$f" | sed -n '320,380p'Length of output: 80
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="src/generics/cacheHelper.js"
if [ ! -f "$f" ]; then echo "ERROR: file not found: $f" >&2; exit 1; fi
tmp=$(mktemp)
if command -v awk >/dev/null 2>&1; then
awk '{printf("%4d: %s\n", NR, $0)}' "$f" > "$tmp"
else
sed -n '1,12000p' "$f" | sed '=' | sed 'N;s/\n/: /' > "$tmp"
fi
echo "== lines 1-160 =="
sed -n '1,160p' "$tmp"
echo "== lines 161-300 =="
sed -n '161,300p' "$tmp"
echo "== lines 301-520 =="
sed -n '301,520p' "$tmp"
echo "== grep matches for targeted symbols =="
if command -v rg >/dev/null 2>&1; then
rg -nP -C2 "namespacedKey|versionedKey|setScoped|getOrSet|\\bdel\\s*\\(" "$f" || true
else
grep -nE "namespacedKey|versionedKey|setScoped|getOrSet|del\s*\(" "$f" || true
fi
rm "$tmp"Length of output: 18562
Delete the versioned key — namespacedKey != versionedKey.
namespacedKey() builds tenant:...:ns:id but setScoped/getOrSet write tenant:...:ns:v{N}:id; calling del(namespacedKey) will not remove the stored versioned entry.
File: src/services/user.js (around lines 146–154). Replace usage with one of:
- await cacheClient.delScoped({ tenantCode, orgId: orgCode, ns, id })
- or: const full = await cacheClient.versionedKey({ tenantCode, orgId: orgCode, ns, id }); await cacheClient.del(full)
If the intention is broad eviction, use cacheClient.evictNamespace(...) or cacheClient.invalidateNamespaceVersion(...).
🤖 Prompt for AI Agents
In src/services/user.js around lines 146 to 154, the code calls cacheClient.del
with a namespacedKey which builds tenant:...:ns:id but the stored entries are
versioned (tenant:...:ns:v{N}:id), so del(namespacedKey) won't remove the cached
item; replace it with either await cacheClient.delScoped({ tenantCode, orgId:
orgCode, ns, id }) or compute the full key first via await
cacheClient.versionedKey({ tenantCode, orgId: orgCode, ns, id }) and then await
cacheClient.del(full), or if you meant to evict the whole namespace use
cacheClient.evictNamespace(...) or cacheClient.invalidateNamespaceVersion(...)
accordingly.
| const filter = utils.isNumeric(id) ? { id, tenant_code: tenantCode } : { share_link: id } | ||
|
|
There was a problem hiding this comment.
Share-link read is not tenant-scoped (potential data leak).
When id is a share_link, tenant_code isn’t applied. If share links aren’t globally unique across tenants, users could read cross-tenant profiles.
Apply:
- const filter = utils.isNumeric(id) ? { id, tenant_code: tenantCode } : { share_link: id }
+ const filter = utils.isNumeric(id)
+ ? { id, tenant_code: tenantCode }
+ : { share_link: id, tenant_code: tenantCode }📝 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.
| const filter = utils.isNumeric(id) ? { id, tenant_code: tenantCode } : { share_link: id } | |
| const filter = utils.isNumeric(id) | |
| ? { id, tenant_code: tenantCode } | |
| : { share_link: id, tenant_code: tenantCode } |
🤖 Prompt for AI Agents
In src/services/user.js around lines 308 to 309, the lookup filter sets
tenant_code only for numeric ids and omits it for share_link which can allow
cross-tenant reads; update the non-numeric branch to include tenant_code as well
(e.g. make the filter for share_link include both share_link: id and
tenant_code: tenantCode) so all reads are tenant-scoped, and ensure any
downstream DB/query code uses this combined filter so share links are validated
within the tenant.
| if (userDetails) { | ||
| if (userDetails.image) userDetails.image = await utils.getDownloadableUrl(userDetails.image) | ||
| return responses.successResponse({ | ||
| statusCode: httpStatusCode.ok, | ||
| message: 'PROFILE_FETCHED_SUCCESSFULLY', | ||
| result: userDetails, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Inconsistent response: cached path omits image_cloud_path.
DB path returns image_cloud_path while cached path doesn’t, causing shape drift.
- if (userDetails) {
- if (userDetails.image) userDetails.image = await utils.getDownloadableUrl(userDetails.image)
+ if (userDetails) {
+ if (userDetails.image) {
+ userDetails.image_cloud_path = userDetails.image
+ userDetails.image = await utils.getDownloadableUrl(userDetails.image)
+ }
return responses.successResponse({
...
})
}Optionally cache canonical value:
- await cacheClient.setScoped({ tenantCode, orgId: organizationCode, ns, id, value: processDbResponse })
+ const toCache = processDbResponse && processDbResponse.image
+ ? { ...processDbResponse, image_cloud_path: processDbResponse.image }
+ : processDbResponse
+ await cacheClient.setScoped({ tenantCode, orgId: organizationCode, ns, id, value: toCache })Also applies to: 406-410
| const prunedEntities = await cacheClient.getOrSet({ | ||
| tenantCode, | ||
| orgId: organizationCode, | ||
| ns: common.CACHE_CONFIG.namespaces.entity_types.name, | ||
| id: `${userOrg}:${modelName}`, // per-org stable id | ||
| fetchFn: async () => { | ||
| const raw = await entityTypeQueries.findUserEntityTypesAndEntities({ | ||
| status: 'ACTIVE', | ||
| organization_code: { [Op.in]: [userOrg, defaultOrganizationCode] }, | ||
| tenant_code: tenantCode, | ||
| model_names: { [Op.contains]: [modelName] }, | ||
| }) | ||
| } | ||
| return removeDefaultOrgEntityTypes(raw, user.organizations[0].id) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Cache org scope mismatch for entity types.
getOrSet is scoped with orgId: organizationCode while id embeds userOrg. If these ever differ, invalidations may miss. Prefer orgId: userOrg.
- const prunedEntities = await cacheClient.getOrSet({
- tenantCode,
- orgId: organizationCode,
+ const prunedEntities = await cacheClient.getOrSet({
+ tenantCode,
+ orgId: userOrg,
ns: common.CACHE_CONFIG.namespaces.entity_types.name,
id: `${userOrg}:${modelName}`,
fetchFn: async () => {
const raw = await entityTypeQueries.findUserEntityTypesAndEntities({
status: 'ACTIVE',
- organization_code: { [Op.in]: [userOrg, defaultOrganizationCode] },
+ organization_code: { [Op.in]: [userOrg, defaultOrganizationCode] },
tenant_code: tenantCode,
model_names: { [Op.contains]: [modelName] },
})
return removeDefaultOrgEntityTypes(raw, user.organizations[0].id)
},
})📝 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.
| const prunedEntities = await cacheClient.getOrSet({ | |
| tenantCode, | |
| orgId: organizationCode, | |
| ns: common.CACHE_CONFIG.namespaces.entity_types.name, | |
| id: `${userOrg}:${modelName}`, // per-org stable id | |
| fetchFn: async () => { | |
| const raw = await entityTypeQueries.findUserEntityTypesAndEntities({ | |
| status: 'ACTIVE', | |
| organization_code: { [Op.in]: [userOrg, defaultOrganizationCode] }, | |
| tenant_code: tenantCode, | |
| model_names: { [Op.contains]: [modelName] }, | |
| }) | |
| } | |
| return removeDefaultOrgEntityTypes(raw, user.organizations[0].id) | |
| }, | |
| }) | |
| const prunedEntities = await cacheClient.getOrSet({ | |
| tenantCode, | |
| orgId: userOrg, | |
| ns: common.CACHE_CONFIG.namespaces.entity_types.name, | |
| id: `${userOrg}:${modelName}`, // per-org stable id | |
| fetchFn: async () => { | |
| const raw = await entityTypeQueries.findUserEntityTypesAndEntities({ | |
| status: 'ACTIVE', | |
| organization_code: { [Op.in]: [userOrg, defaultOrganizationCode] }, | |
| tenant_code: tenantCode, | |
| model_names: { [Op.contains]: [modelName] }, | |
| }) | |
| return removeDefaultOrgEntityTypes(raw, user.organizations[0].id) | |
| }, | |
| }) |
🤖 Prompt for AI Agents
In src/services/user.js around lines 365 to 379, the cache entry is created with
orgId: organizationCode while the id uses userOrg, causing a cache scope
mismatch that can prevent proper invalidation; change the orgId to use userOrg
(so orgId and the id namespace are consistent), and ensure any surrounding cache
get/set or invalidation calls use the same userOrg value rather than
organizationCode.
Summary by CodeRabbit