Conversation
github-actions
bot
added
the
skip-changelog
WalkthroughThis update enhances the tenant management and cache eviction capabilities within the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java (1)
89-94: Replaceassertwith proper error handling in production code.The use of
assertin production code, such as inMigration059PolicySetToPolicyMap.java, is risky because assertions can be disabled, leading to potential issues if the condition fails. Consider using exceptions or logging to handle such cases more reliably. Here are some locations to review:
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java(lines 89-94)Ensure that all critical checks in production code use robust error handling to maintain application stability.
Analysis chain
Ensure robust error handling for tenant eviction.
The logic for evicting the default tenant from the cache is clear and necessary. However, ensure that the
assertstatement is appropriate for your production environment, as it can be disabled. Consider using a more robust error handling mechanism if needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of assert statements in the codebase to ensure they are not used inappropriately in production code. # Test: Search for assert statements. Expect: No inappropriate usage in production code. rg --type java --word-regexp 'assert 'Length of output: 8670
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java (4 hunks)
Additional comments not posted (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java (2)
3-4: Good use of imports for cache management.The addition of
TenantandCacheableRepositoryHelperimports is appropriate for the new cache eviction logic.
41-41: Introduction ofcacheableRepositoryHelperis well-placed.This field is essential for managing cache eviction related to tenant data.
AnaghHegde
left a comment
AnaghHegde
left a comment
There was a problem hiding this comment.
Minor comment rest look good to me
| // Evict the default tenant from the cache to ensure that the updated tenant object is fetched from the database | ||
| Query tenantQuery = new Query(); | ||
| tenantQuery.addCriteria(where(Tenant.Fields.slug).is(DEFAULT)); | ||
| Tenant defaultTenant = mongoTemplate.findOne(tenantQuery, Tenant.class).block(); |
There was a problem hiding this comment.
Why is this a block call?
There was a problem hiding this comment.
@AnaghHegde here the MongoTemplate that is injected is a reactive one, in other cases we inject the non-reactive template. The blocking is required just to make sure the eviction happens after the migration is completed. This can be done using .then but thought it won't make any difference as the earlier call is also blocking in nature.
2 participants
Description
Observation: Even after successful execution of the migration where we are replacing policies to policyMap for tenant, we end up in a state where the policies has the required values but policyMap was still end up in a empty state.
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10348884858
Commit: 539a767
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 12 Aug 2024 09:16:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes