chore: Add empty policyMap when policies are empty or null to fix NPE #36374
Conversation
WalkthroughThe changes introduce a new migration class, Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
github-actions
bot
added
the
skip-changelog
![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 (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration062AddEmptyPolicyMapForNullValues.java (1)
53-54: Consider adding a comment or removing therollbackExecutionmethod.The
rollbackExecutionmethod is currently empty, which suggests that rollback is not implemented for this migration.To improve clarity and maintainability, consider:
- Adding a comment to explain why the method is empty and whether rollback is supported for this migration.
- If rollback is not supported, consider removing the method altogether to avoid confusion.
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/Migration062AddEmptyPolicyMapForNullValues.java (1 hunks)
Additional comments not posted (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration062AddEmptyPolicyMapForNullValues.java (3)
1-100: Great job on the migration class structure! 👍The
Migration062AddEmptyPolicyMapForNullValuesclass follows the standard structure for a database migration script and is properly annotated with@ChangeUnitand@RequiredArgsConstructor. Defining the collection names as a constant set is a good practice for maintainability.A few suggestions for improvement:
- Consider adding a brief class-level Javadoc comment to describe the purpose of the migration.
- Consider adding a comment to explain why the
rollbackExecutionmethod is empty, or remove it if rollback is not supported for this migration.
57-76: Excellent work on theexecutemethod! 🌟The
executemethod follows a reactive programming model usingMonoto handle asynchronous operations efficiently. UsingMono.whenDelayErrorto run the migration in parallel across all collections is a smart approach. The method also properly handles and logs any errors that occur during the migration.A couple of suggestions for improvement:
- Consider extracting the error handling logic into a separate method to improve readability and reusability.
- Consider adding more detailed logging statements to track the progress of the migration for each collection.
78-99: Fantastic work on theaddEmptyPolicyMapForNullEntriesmethod! 🎉The method uses
ReactiveMongoTemplateto perform the database operations, which is a standard approach in Spring Data MongoDB. Constructing the query usingCriteriato identify the documents that need to be updated is a clear and concise approach. UsingUpdateDefinitionto set the policy map to an emptyHashMapis a straightforward way to add the missing field.A few suggestions for improvement:
- Consider extracting the query construction logic into a separate method to improve readability and reusability.
- Consider adding a comment to explain the purpose of the
notDeletedCriteriaand why it's included in the query.- Consider adding a log statement to indicate when no documents were modified for a collection.
|
|
||
| private final ReactiveMongoTemplate mongoTemplate; | ||
|
|
||
| private static final Set<String> CE_COLLECTION_NAMES = Set.of( |
There was a problem hiding this comment.
Is it possible to use mongoTemplate and run this migration for every collection in the db? This way we can avoid adding a EE migration and also not missing any collection.
There was a problem hiding this comment.
I was hoping to mimic the policyMap refactor to have complete control over which collections are getting migrated. Should we just add the EE collection names as well in that case?
There was a problem hiding this comment.
In that case lets stick to this approach, this will save us time in running this migration on prod. Yes we should add EE collection names and handle collection does not exists kind of errors if required.
There was a problem hiding this comment.
handle collection does not exists kind of errors if required.
This won't be required as no objects will be returned.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10903947230. |
|
|
||
| final Query query = new Query().addCriteria(policyMapNotExists).addCriteria(notDeletedCriteria); | ||
| UpdateDefinition update = new Update().set(POLICY_MAP, new HashMap<>()); | ||
| final Mono<UpdateResult> convertToMap = mongoTemplate.updateMulti(query, update, collectionName); |
There was a problem hiding this comment.
Can this result in an error state if the collection does not exist?
There was a problem hiding this comment.
As mentioned above this is equivalent to db.random_collection.find() which doesn't return any result so should not be an issue for us.
|
Deploy-Preview-URL: https://ce-36374.dp.appsmith.com |
Failed server tests
|
…appsmithorg#36374) ## Description We have quite a lot of objects in the DB for which we expect the policies should be present but that's not the case. This is causing issues when we migrated to using `policyMap` where we didn't create empty map if the policies object is empty expecting all such entries are anyway not accessible to the user. We can query mongodb to find such entries: ``` db .collectionName .find({ policies: [], deletedAt: {$exists: false} }) ``` We expect collections like plugins, customJSlibs etc to have empty values for policies and hence policyMap was not migrated for this, but we are seeing the same pattern for other collections as well like newAction, workspace etc. which is why we are seeing NPEs. With this PR we intend to remove this NPE by adding a empty map to policyMap field. With prod dump this is taking 340sec so we need to plan accordingly. /test Sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10903942123> > Commit: c1573a6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10903942123&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Tue, 17 Sep 2024 13:41:53 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Implemented a database migration to ensure all relevant documents have a defined policy map, improving data consistency. - **Bug Fixes** - Addressed issues where documents had null or missing policy maps by updating them to an empty policy map. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
We have quite a lot of objects in the DB for which we expect the policies should be present but that's not the case. This is causing issues when we migrated to using
policyMapwhere we didn't create empty map if the policies object is empty expecting all such entries are anyway not accessible to the user.We can query mongodb to find such entries:
We expect collections like plugins, customJSlibs etc to have empty values for policies and hence policyMap was not migrated for this, but we are seeing the same pattern for other collections as well like newAction, workspace etc. which is why we are seeing NPEs. With this PR we intend to remove this NPE by adding a empty map to policyMap field.
With prod dump this is taking 340sec so we need to plan accordingly.
/test Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10903942123
Commit: c1573a6
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 17 Sep 2024 13:41:53 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes