From 55b53a75e3d5ae6c1b44650559c940a4e252d152 Mon Sep 17 00:00:00 2001 From: Abhijeet <41686026+abhvsn@users.noreply.github.com> Date: Wed, 10 Jul 2024 08:37:41 +0530 Subject: [PATCH] fix: Concurrent modification exception during the datasource import flow (#34818) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: cf63fab15f7e3717b566306f8ea598662f18aea9 > Cypress dashboard. > Tags: `@tag.ImportExport, @tag.Sanity, @tag.Git` > Spec: >
Tue, 09 Jul 2024 18:05:03 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No --- .../datasources/base/DatasourceServiceCE.java | 3 ++- .../base/DatasourceServiceCEImpl.java | 15 ++++++++----- .../DatasourceImportableServiceCEImpl.java | 8 +++++-- .../ce/MappedImportableResourcesCE_DTO.java | 22 +++++++++++++++++-- .../imports/internal/ImportServiceTests.java | 2 ++ 5 files changed, 40 insertions(+), 10 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java index b2f2953e4d..7f88310d31 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java @@ -6,6 +6,7 @@ import com.appsmith.external.models.DatasourceStorageDTO; import com.appsmith.external.models.DatasourceTestResult; import com.appsmith.external.models.MustacheBindingToken; import com.appsmith.server.acl.AclPermission; +import com.appsmith.server.dtos.DBOpsType; import org.springframework.util.MultiValueMap; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -67,7 +68,7 @@ public interface DatasourceServiceCE { Mono createWithoutPermissions(Datasource datasource); Mono createWithoutPermissions( - Datasource datasource, Map> datasourceStorageDryRunQueries); + Datasource datasource, Map> datasourceStorageDryRunQueries); Mono updateDatasourceStorage( DatasourceStorageDTO datasourceStorageDTO, String activeEnvironmentId, Boolean IsUserRefreshedUpdate); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java index c8dc4f8981..52134effaa 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java @@ -64,6 +64,7 @@ import java.util.UUID; import static com.appsmith.external.constants.spans.DatasourceSpan.FETCH_ALL_DATASOURCES_WITH_STORAGES; import static com.appsmith.external.constants.spans.DatasourceSpan.FETCH_ALL_PLUGINS_IN_WORKSPACE; import static com.appsmith.external.helpers.AppsmithBeanUtils.copyNestedNonNullProperties; +import static com.appsmith.server.dtos.DBOpsType.SAVE; import static com.appsmith.server.helpers.CollectionUtils.isNullOrEmpty; import static com.appsmith.server.helpers.DatasourceAnalyticsUtils.getAnalyticsProperties; import static com.appsmith.server.helpers.DatasourceAnalyticsUtils.getAnalyticsPropertiesForTestEventStatus; @@ -147,7 +148,7 @@ public class DatasourceServiceCEImpl implements DatasourceServiceCE { // TODO: Check usage @Override public Mono createWithoutPermissions( - Datasource datasource, Map> datasourceStorageDryRunQueries) { + Datasource datasource, Map> datasourceStorageDryRunQueries) { return createEx(datasource, null, true, datasourceStorageDryRunQueries); } @@ -160,7 +161,7 @@ public class DatasourceServiceCEImpl implements DatasourceServiceCE { @NotNull Datasource datasource, AclPermission permission, boolean isDryOps, - Map> datasourceStorageDryRunQueries) { + Map> datasourceStorageDryRunQueries) { // Validate incoming request String workspaceId = datasource.getWorkspaceId(); if (!hasText(workspaceId)) { @@ -232,9 +233,13 @@ public class DatasourceServiceCEImpl implements DatasourceServiceCE { .create(datasourceStorage, isDryOps) .map(datasourceStorage1 -> { if (datasourceStorageDryRunQueries != null && isDryOps) { - datasourceStorageDryRunQueries - .computeIfAbsent(DBOpsType.SAVE.name(), k -> new ArrayList<>()) - .add(datasourceStorage1); + List datasourceStorages = + datasourceStorageDryRunQueries.get(SAVE); + if (datasourceStorages == null) { + datasourceStorages = new ArrayList<>(); + } + datasourceStorages.add(datasourceStorage1); + datasourceStorageDryRunQueries.put(SAVE, datasourceStorages); } return datasourceStorage1; }); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java index 69e6d7a7b2..83489d22a3 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java @@ -25,6 +25,7 @@ import com.appsmith.server.imports.importable.ImportableServiceCE; import com.appsmith.server.imports.importable.artifactbased.ArtifactBasedImportableService; import com.appsmith.server.services.SequenceService; import com.appsmith.server.services.WorkspaceService; +import lombok.NonNull; import lombok.extern.slf4j.Slf4j; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.lang3.StringUtils; @@ -408,7 +409,10 @@ public class DatasourceImportableServiceCEImpl implements ImportableServiceCE> dryRunOpsMap, Datasource createdDatasource) { - dryRunOpsMap.computeIfAbsent(queryType.name(), k -> new ArrayList<>()).add(createdDatasource); + DBOpsType queryType, @NonNull Map> dryRunOpsMap, Datasource createdDatasource) { + List datasourceList = dryRunOpsMap.get(queryType); + datasourceList = datasourceList == null ? new ArrayList<>() : datasourceList; + datasourceList.add(createdDatasource); + dryRunOpsMap.put(queryType, datasourceList); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java index 2424ec26dd..74c54d640d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java @@ -5,12 +5,15 @@ import com.appsmith.external.models.DatasourceStorage; import com.appsmith.server.domains.Context; import com.appsmith.server.domains.CustomJSLib; import com.appsmith.server.dtos.CustomJSLibContextDTO; +import com.appsmith.server.dtos.DBOpsType; import com.appsmith.server.dtos.ImportActionCollectionResultDTO; import com.appsmith.server.dtos.ImportActionResultDTO; import com.appsmith.server.dtos.ImportedActionAndCollectionMapsDTO; import lombok.Data; import lombok.NoArgsConstructor; +import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -48,9 +51,24 @@ public class MappedImportableResourcesCE_DTO { Map resourceStoreFromArtifactExchangeJson = new HashMap<>(); // Dry ops queries - Map> datasourceDryRunQueries = new HashMap<>(); + Map> datasourceDryRunQueries = new HashMap<>(); - Map> datasourceStorageDryRunQueries = new HashMap<>(); + Map> datasourceStorageDryRunQueries = new HashMap<>(); Map> customJSLibsDryOps = new HashMap<>(); + + { + for (DBOpsType dbOpsType : DBOpsType.values()) { + /** + * Using Collections.synchronizedList() ensures that the list itself is synchronized, meaning that + * individual operations on the list are thread-safe. This includes operations such as adding, removing, + * or accessing elements. However, this does not protect against compound operations, such as checking if + * an element exists and then adding it based on that check. For these compound operations, we would + * need additional synchronization using synchronized blocks. + * Ref: https://blog.codingblocks.com/2019/keeping-arraylists-safe-across-threads-in-java/ + */ + datasourceStorageDryRunQueries.put(dbOpsType, Collections.synchronizedList(new ArrayList<>())); + datasourceDryRunQueries.put(dbOpsType, Collections.synchronizedList(new ArrayList<>())); + } + } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java index 02a37f9f34..e826f4d580 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java @@ -1569,6 +1569,8 @@ public class ImportServiceTests { .verifyComplete(); } + @Test + @WithUserDetails(value = "api_user") public void importArtifactIntoWorkspace_pageRemovedAndUpdatedDefaultPageNameInBranchApplication_Success() { Application testApplication = new Application(); testApplication.setName("importApplicationIntoWorkspace_pageRemovedInBranchApplication_Success");