entities and entity type fix for cache invalidation#1618
Merged
Conversation
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds tenant-scoped bulk cache deletion helpers (sessions, entityTypes, mentor, mentee) to cacheHelper; entity-type service now decides per-update/delete whether to clear caches across all orgs (when default org) and calls tenant-wide deletions; user query computes tenant view earlier. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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.
🧹 Nitpick comments (1)
src/services/entity-type.js (1)
319-330: Inconsistent cache invalidation:deletemethod doesn't handle default org cross-org sweep.The
updatemethod now usesdeleteAcrossAllOrgsfor the default org to clear stale cache across all organizations, but thedeletemethod always uses the targeted single-org deletion. This inconsistency means deleting a default org entity type will leave stale cache entries in other organizations that cached it via fallback.Consider applying the same pattern here for consistency:
♻️ Proposed fix to align delete with update behavior
// THIRD: Remove individual entity type from cache try { // For each model this entity belonged to if (entityToDelete.model_names && Array.isArray(entityToDelete.model_names)) { + const isDefaultOrg = organizationCode === process.env.DEFAULT_ORGANISATION_CODE for (const modelName of entityToDelete.model_names) { - // Remove the specific entity type cache - await cacheHelper.entityTypes.delete( - tenantCode, - organizationCode, - modelName, - entityToDelete.value - ) + if (isDefaultOrg) { + await cacheHelper.entityTypes.deleteAcrossAllOrgs( + tenantCode, + modelName, + entityToDelete.value + ) + } else { + await cacheHelper.entityTypes.delete( + tenantCode, + organizationCode, + modelName, + entityToDelete.value + ) + } } } } catch (cacheError) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/entity-type.js` around lines 319 - 330, When removing an entity type in the delete path (the function containing the loop that calls cacheHelper.entityTypes.delete for each modelName), ensure you mirror the update behavior by using cacheHelper.entityTypes.deleteAcrossAllOrgs for the default organization to clear fallback caches across all orgs; specifically, detect the default org (organizationCode === DEFAULT_ORG or the same constant used in the update flow) and call deleteAcrossAllOrgs(tenantCode, modelName, entityToDelete.value) instead of the single-org delete, otherwise call the existing delete(tenantCode, organizationCode, modelName, entityToDelete.value).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/entity-type.js`:
- Around line 319-330: When removing an entity type in the delete path (the
function containing the loop that calls cacheHelper.entityTypes.delete for each
modelName), ensure you mirror the update behavior by using
cacheHelper.entityTypes.deleteAcrossAllOrgs for the default organization to
clear fallback caches across all orgs; specifically, detect the default org
(organizationCode === DEFAULT_ORG or the same constant used in the update flow)
and call deleteAcrossAllOrgs(tenantCode, modelName, entityToDelete.value)
instead of the single-org delete, otherwise call the existing delete(tenantCode,
organizationCode, modelName, entityToDelete.value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 146affbf-b131-4ca1-a6e5-2324ecaeeac9
📒 Files selected for processing (2)
src/generics/cacheHelper.jssrc/services/entity-type.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/entity-type.js`:
- Around line 110-122: When handling a default-org entity type update/delete,
broaden dependent-cache invalidation: instead of calling
_clearUserCachesForEntityTypeChange only for the current org, iterate over all
org codes and invoke _clearUserCachesForEntityTypeChange for each so derived
caches (displayProperties, mentor, mentee, user caches) are swept across all
orgs; keep the existing
cacheHelper.entityTypes.deleteEntityTypesAcrossAllOrgs/delete behavior but
ensure you do NOT remove the canonical default-org entity-type entry needed for
cross-org filter validation (preserve default org entityTypes entries), and
apply this change in both places where _clearUserCachesForEntityTypeChange is
currently called (the block around the
cacheHelper.entityTypes.deleteEntityTypesAcrossAllOrgs call and the other
occurrence within the 319-361 region).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d38a8851-7606-4dae-aa0b-11980edbbc76
📒 Files selected for processing (2)
src/generics/cacheHelper.jssrc/services/entity-type.js
…into fix/entity
Comment on lines
302
to
306
Collaborator
There was a problem hiding this comment.
Verify this _clearUserCachesForEntityTypeChange doesn’t accept 3rd parameter as an object.
It expects a string.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/entity-type.js`:
- Around line 135-136: The change is causing default-org entity-type updates to
trigger tenant-wide eviction of user-profile caches (mentee/mentor) by passing
isDefaultOrg into _clearUserCachesForEntityTypeChange; revert this by removing
the isDefaultOrg flag when invoking _clearUserCachesForEntityTypeChange so that
mentor/mentee invalidation remains org-scoped, and ensure cross-org invalidation
still happens via deleteEntityTypesAcrossAllOrgs for default-org changes; update
all call sites (including where _clearUserCachesForEntityTypeChange is called
around the mentions of updatedEntity.value and similar invocations) to stop
forwarding isDefaultOrg for mentor/mentee cache clears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8c87de4-141c-4f76-9588-e2ef6609d364
📒 Files selected for processing (2)
src/database/queries/userExtension.jssrc/services/entity-type.js
Contributor
Author
|
@coderabbitai review my latest changes in the PR |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/entity-type.js (2)
131-133:⚠️ Potential issue | 🟠 MajorMissing
isDefaultOrgparameter for tenant-wide user cache clearing.The call to
_clearUserCachesForEntityTypeChangedoesn't passisDefaultOrgas the 5th parameter (allOrgs), so it will default tofalse. Per the documented design decision, default-org entity type updates should trigger tenant-wide mentee/mentor cache eviction.🐛 Proposed fix
const updatedEntity = updatedEntityType[0] for (const modelName of updatedEntity.model_names) { - await this._clearUserCachesForEntityTypeChange(orgCode, tenantCode, modelName, updatedEntity.value) + await this._clearUserCachesForEntityTypeChange(orgCode, tenantCode, modelName, updatedEntity.value, isDefaultOrg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/entity-type.js` around lines 131 - 133, The loop calling this._clearUserCachesForEntityTypeChange currently passes (orgCode, tenantCode, modelName, updatedEntity.value) but omits the fifth parameter that signals tenant-wide eviction; update the call inside the for-loop to pass the default-org flag as the fifth argument (e.g., this._clearUserCachesForEntityTypeChange(orgCode, tenantCode, modelName, updatedEntity.value, updatedEntity.isDefaultOrg)) so default-org entity type updates trigger tenant-wide mentee/mentor cache clearing.
309-316:⚠️ Potential issue | 🟠 MajorMissing
isDefaultOrgparameter in delete method's user cache clearing.Same issue as in the update method — once
isDefaultOrgis defined, it should be passed to enable tenant-wide cache eviction for default-org deletions.🐛 Proposed fix
for (const modelName of entityToDelete.model_names) { await this._clearUserCachesForEntityTypeChange( organizationCode, tenantCode, modelName, - entityToDelete.value + entityToDelete.value, + isDefaultOrg ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/entity-type.js` around lines 309 - 316, The delete flow is not passing the isDefaultOrg flag to the cache eviction helper: inside the loop over entityToDelete.model_names in the delete method, call this._clearUserCachesForEntityTypeChange with the isDefaultOrg argument (same way the update method does) so tenant-wide cache eviction occurs for default-org deletions; locate the delete method's model_names loop and add the isDefaultOrg parameter to the existing call signature (organizationCode, tenantCode, modelName, entityToDelete.value, isDefaultOrg).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/entity-type.js`:
- Around line 324-337: The delete method uses isDefaultOrg but never defines it;
after you fetch the organization in the delete function (same place update
computes it), add a local isDefaultOrg variable set from the fetched
organization (use the same property name the update method uses, e.g.,
organization.isDefaultOrg or organization.isDefault) before calling
cacheHelper.entityTypes.delete or deleteEntityTypesAcrossAllOrgs so the cache
branch selection works without a ReferenceError.
---
Outside diff comments:
In `@src/services/entity-type.js`:
- Around line 131-133: The loop calling this._clearUserCachesForEntityTypeChange
currently passes (orgCode, tenantCode, modelName, updatedEntity.value) but omits
the fifth parameter that signals tenant-wide eviction; update the call inside
the for-loop to pass the default-org flag as the fifth argument (e.g.,
this._clearUserCachesForEntityTypeChange(orgCode, tenantCode, modelName,
updatedEntity.value, updatedEntity.isDefaultOrg)) so default-org entity type
updates trigger tenant-wide mentee/mentor cache clearing.
- Around line 309-316: The delete flow is not passing the isDefaultOrg flag to
the cache eviction helper: inside the loop over entityToDelete.model_names in
the delete method, call this._clearUserCachesForEntityTypeChange with the
isDefaultOrg argument (same way the update method does) so tenant-wide cache
eviction occurs for default-org deletions; locate the delete method's
model_names loop and add the isDefaultOrg parameter to the existing call
signature (organizationCode, tenantCode, modelName, entityToDelete.value,
isDefaultOrg).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7f102c4-d73f-49a0-a02a-33a18d96f00e
📒 Files selected for processing (3)
src/database/queries/userExtension.jssrc/generics/cacheHelper.jssrc/services/entity-type.js
Contributor
Author
|
@coderabbitai review my latest changes. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/entity-type.js (1)
296-304:⚠️ Potential issue | 🟡 MinorAdd defensive handling for
model_namesto prevent TypeError.The loop iterates directly over
entityToDelete.model_nameswithout defensive handling. Ifmodel_namesisnullorundefined, this will throw aTypeError: entityToDelete.model_names is not iterable.Other locations in this file (line 437) and codebase (see relevant snippet from
entity.js) use defensive|| []handling for this field.🛡️ Proposed fix
// Clear user caches after successful deletion - for (const modelName of entityToDelete.model_names) { + for (const modelName of entityToDelete.model_names || []) { await this._clearUserCachesForEntityTypeChange(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/entity-type.js` around lines 296 - 304, The loop over entityToDelete.model_names can throw if model_names is null/undefined; update the iteration in the block that calls _clearUserCachesForEntityTypeChange to defensively handle missing values (e.g., iterate over entityToDelete.model_names || [] or guard with Array.isArray(entityToDelete.model_names) ? entityToDelete.model_names : []) so the for...of never receives a non-iterable; keep the same call to _clearUserCachesForEntityTypeChange with organizationCode, tenantCode, modelName, entityToDelete.value, isDefaultOrg.
🧹 Nitpick comments (1)
src/database/queries/userExtension.js (1)
715-719: Verify necessity of reorderingviewNamecomputation.Moving
viewNamecomputation before the early return for invalidorgCodescauses unnecessary computation whenorgCodesis empty/invalid. Based on past review discussion, this change was marked for removal. If this reordering was intentional for a specific reason (e.g., needed by a future code path), please clarify; otherwise, consider restoring the original order whereviewNameis computed only after validation passes.♻️ Suggested reorder
static async getAllUsersByOrgId(orgCodes, tenantCode) { try { - const viewName = utils.getTenantViewName(tenantCode, MenteeExtension.tableName) - if (!Array.isArray(orgCodes) || orgCodes.length === 0) { return [] } + const viewName = utils.getTenantViewName(tenantCode, MenteeExtension.tableName) const query = `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/userExtension.js` around lines 715 - 719, The computation of viewName via utils.getTenantViewName(tenantCode, MenteeExtension.tableName) is being done before validating orgCodes, causing unnecessary work when orgCodes is not an array or empty; move the viewName computation so it occurs after the orgCodes check (i.e., keep the early return if (!Array.isArray(orgCodes) || orgCodes.length === 0) return [] and only call utils.getTenantViewName(...) to set viewName once validation passes), unless there is a documented reason to compute viewName earlier — if so, add a comment explaining that reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/services/entity-type.js`:
- Around line 296-304: The loop over entityToDelete.model_names can throw if
model_names is null/undefined; update the iteration in the block that calls
_clearUserCachesForEntityTypeChange to defensively handle missing values (e.g.,
iterate over entityToDelete.model_names || [] or guard with
Array.isArray(entityToDelete.model_names) ? entityToDelete.model_names : []) so
the for...of never receives a non-iterable; keep the same call to
_clearUserCachesForEntityTypeChange with organizationCode, tenantCode,
modelName, entityToDelete.value, isDefaultOrg.
---
Nitpick comments:
In `@src/database/queries/userExtension.js`:
- Around line 715-719: The computation of viewName via
utils.getTenantViewName(tenantCode, MenteeExtension.tableName) is being done
before validating orgCodes, causing unnecessary work when orgCodes is not an
array or empty; move the viewName computation so it occurs after the orgCodes
check (i.e., keep the early return if (!Array.isArray(orgCodes) ||
orgCodes.length === 0) return [] and only call utils.getTenantViewName(...) to
set viewName once validation passes), unless there is a documented reason to
compute viewName earlier — if so, add a comment explaining that reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a359e90-3399-475e-a20e-60173317f583
📒 Files selected for processing (3)
src/database/queries/userExtension.jssrc/generics/cacheHelper.jssrc/services/entity-type.js
Contributor
Author
|
@coderabbitai is my pr approved from your side ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 42192
🟠 Open Issues
Once these are addressed, I can proceed with approval. Let me know when those are fixed! 🧠 Learnings used |
Contributor
Author
|
@coderabbitai entityToDelete.model_names will never be empty or null. It is mandatory. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2154
Verified: In Noted: Both issues are now resolved. Approving the PR! 🎉 [approve] (^･o･^)ﾉ" ✏️ Learnings added
🧠 Learnings used |
Contributor
Author
|
@coderabbitai review latest changes, approve if everything is okay. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10866 🏁 Script executed: Length of output: 24696
No new issues found. Approving! 🎉 [approve] 🧠 Learnings used |
Contributor
Author
|
@coderabbitai just pushed new change, verify and approve if all good. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10891 🏁 Script executed: Length of output: 25276
Approving! 🎉 [approve] (^･o･^)ﾉ" ✏️ Learnings added
🧠 Learnings used |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release Notes
Contributions by Author