fix: Concurrent modification exception during the datasource import flow #34818
Conversation
WalkthroughThe key change in this update is the adoption of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImportService
participant DatasourceService
User->>ImportService: Import Artifact Request
ImportService->>DatasourceService: Create Datasource with DryRun
DatasourceService->>DatasourceStorage: Use DBOpsType in Data Map
DatasourceService-->>ImportService: Return Datasource Info
ImportService-->>User: Import Success
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 Configration File (
|
Failed server tests
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (1)
Line range hint
1573-1606: Refactor the test method for readabilityThe test method can be refactored for better readability by breaking down the setup, import, and validation steps into separate sections or helper methods.
@Test @WithUserDetails(value = "api_user") public void importArtifactIntoWorkspace_pageRemovedAndUpdatedDefaultPageNameInBranchApplication_Success() { Application application = setupApplication(); String gitSyncIdBeforeImport = getGitSyncIdBeforeImport(application); PageDTO savedPage = createPage(application); Set<String> applicationPageIdsBeforeImport = getPageIdsBeforeImport(application); ApplicationJson applicationJson = prepareApplicationJson(gitSyncIdBeforeImport); Application importedApplication = performImport(application, applicationJson); validateImport(applicationPageIdsBeforeImport, savedPage, importedApplication); } private Application setupApplication() { // Setup application code } private String getGitSyncIdBeforeImport(Application application) { // Get Git sync ID before import } private PageDTO createPage(Application application) { // Create page code } private Set<String> getPageIdsBeforeImport(Application application) { // Get page IDs before import } private ApplicationJson prepareApplicationJson(String gitSyncIdBeforeImport) { // Prepare application JSON } private Application performImport(Application application, ApplicationJson applicationJson) { // Perform import } private void validateImport(Set<String> applicationPageIdsBeforeImport, PageDTO savedPage, Application importedApplication) { // Validate import }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (2 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (1 hunks)
Additional comments not posted (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (4)
8-8: Import forDBOpsTypelooks good.The import is necessary for the changes made in the file.
54-56: Modification of data structures to useDBOpsTypekeys is correct.The changes enhance type safety and correctness.
60-72: Thread safety measures withCollections.synchronizedList()are correctly applied.The use of
Collections.synchronizedList()ensures that individual operations on the list are thread-safe. However, ensure that compound operations are correctly synchronized.
62-69: Block comment explainingCollections.synchronizedList()is clear and informative.The comment provides valuable context and references for understanding the use of
Collections.synchronizedList().app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java (2)
9-9: Import forDBOpsTypelooks good.The import is necessary for the changes made in the file.
71-71: Update tocreateWithoutPermissionsmethod's parameter type is correct.The changes are consistent with the rest of the codebase.
Ensure that all method calls to
createWithoutPermissionsmatch the new signature.app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (2)
28-28: Import for@NonNulllooks good.The import is necessary for the changes made in the file.
412-416: Update toaddDryOpsForEntitymethod's parameter type and addition of@NonNullannotation are correct.The changes enhance type safety and robustness.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (3)
67-67: Static import forSAVEfromDBOpsTypelooks good.The import is necessary for the changes made in the file.
151-151: Update tocreateWithoutPermissionsmethod to employDBOpsTypeis correct.The changes are consistent with the rest of the codebase.
Ensure that all method calls to
createWithoutPermissionsmatch the new signature.
Line range hint
164-242: Update tocreateExmethod to handledatasourceStorageDryRunQueriesusingDBOpsTypeis correct.The changes enhance type safety and correctness.
...r/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java
Show resolved
Hide resolved
…low (appsmithorg#34818) ## Description In the import service, we are trying to import datasources using the combination of Flux and Flatmap to create the reactive chain. This translates to creating multiple threads to accomplish the task in an efficient manner. After we introduced database dry ops, where we are now sharing the map of ops type to the object for which the operation needs to be performed. This caused a `concurrentModificationException` as datasourceService layer is referring to same object via different threads. ## Automation /ok-to-test tags="@tag.ImportExport, @tag.Sanity, @tag.Git" ### 🔍 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/9861701410> > Commit: cf63fab > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9861701410&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.ImportExport, @tag.Sanity, @tag.Git` > Spec: > <hr>Tue, 09 Jul 2024 18:05:03 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
Description
In the import service, we are trying to import datasources using the combination of Flux and Flatmap to create the reactive chain. This translates to creating multiple threads to accomplish the task in an efficient manner. After we introduced database dry ops, where we are now sharing the map of ops type to the object for which the operation needs to be performed. This caused a
concurrentModificationExceptionas datasourceService layer is referring to same object via different threads.Automation
/ok-to-test tags="@tag.ImportExport, @tag.Sanity, @tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9861701410
Commit: cf63fab
Cypress dashboard.
Tags:
@tag.ImportExport, @tag.Sanity, @tag.GitSpec:
Tue, 09 Jul 2024 18:05:03 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?