fix: Super user cache eviction when user is added via env variable #37785
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (2)
453-456: Consider Refactoring Method Parameters for Improved ReadabilityThe
updateSuperUsersmethod now accepts multiple parameters, which can impact readability and maintainability. Encapsulating these parameters into a configuration object or builder pattern can streamline the method signature and enhance code clarity.
499-499: Add Error Handling for Cache EvictionTo ensure robustness, consider adding error handling around the
evictPermissionCacheForUserscall. This will help catch and log any exceptions that might occur during cache eviction, improving observability and debuggability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java(3 hunks)
🧰 Additional context used
📓 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (1)
Learnt from: sharat87
PR: appsmithorg/appsmith#33602
File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java:29-29
Timestamp: 2024-11-12T08:11:36.416Z
Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts.
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (3)
24-24: LGTM!
The import statement is correctly placed and follows the package naming convention.
214-214: LGTM!
The parameter type change from PermissionGroupRepository to CacheableRepositoryHelper aligns with the new caching strategy while maintaining the method's interface compatibility.
226-228: Consider the impact of blocking operations
While the blocking call ensures immediate cache eviction, it could impact performance in high-throughput scenarios. Consider if non-blocking alternatives are feasible.
Consider returning the Mono<Void> and letting the caller decide the blocking behavior if appropriate for your use case:
- cacheableRepositoryHelper
- .evictPermissionGroupsUser(user.getEmail(), user.getTenantId())
- .block();
+ return cacheableRepositoryHelper
+ .evictPermissionGroupsUser(user.getEmail(), user.getTenantId());✅ Verification successful
Blocking operation is necessary in this migration context
The blocking call is intentional and appropriate here:
- The code is within a migration method where synchronous execution is required
- There's an explicit comment indicating the blocking is needed for cache eviction before proceeding
- The implementation of
evictPermissionGroupsUseris a lightweight cache eviction operation (returnsMono.empty()) - Other cache eviction operations in the codebase follow similar patterns, particularly in migration and test scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other blocking operations in cache eviction flows
# Look for similar patterns of cache eviction in the codebase
rg -A 2 "evictPermissionGroupsUser.*block\(\)"
# Check for any performance-related comments or issues
rg -A 2 "TODO.*performance|FIXME.*performance"
Length of output: 97
Script:
#!/bin/bash
# Let's check the implementation of evictPermissionGroupsUser and its usage context
ast-grep --pattern 'class $_ {
$$$
evictPermissionGroupsUser($$$) {
$$$
}
$$$
}'
# Check for other block() calls in similar cache eviction patterns
rg "evict.*block\(\)" -A 2
# Look for the method usage to understand the execution context
rg "evictPermissionGroupsUser" -A 2 -B 2
Length of output: 8183
github-actions
bot
added
Admin Settings Product
abhvsn
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12054814563. |
|
Deploy-Preview-URL: https://ce-37785.dp.appsmith.com |
abhvsn
changed the title
Description
PR to fix the issue with evicting the cache lines for super users added via env. Migration was getting stuck at
updateSuperUsersrepetitive migration as Mongock was not able to complete the eviction with PermissionGroupRepository which underneath have dependency on mongo driver.Fixes #37703
/test Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12054789155
Commit: fccb1b3
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 27 Nov 2024 17:23:12 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?