From 53f3a3d497c8ea7adac2af54cca54c07ce9d7adc Mon Sep 17 00:00:00 2001 From: Abhijeet Date: Tue, 9 Jul 2024 15:20:34 +0530 Subject: [PATCH 1/2] fix: Concurrent modification exception during the datasource import flow --- .../datasources/base/DatasourceServiceCEImpl.java | 12 ++++++++---- .../DatasourceImportableServiceCEImpl.java | 8 ++++++-- .../server/imports/internal/ImportServiceTests.java | 2 ++ 3 files changed, 16 insertions(+), 6 deletions(-) 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 c8dc4f898196..38e0884e2e06 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 @@ -19,7 +19,6 @@ import com.appsmith.server.domains.Plugin; import com.appsmith.server.domains.User; import com.appsmith.server.domains.Workspace; -import com.appsmith.server.dtos.DBOpsType; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.PluginExecutorHelper; @@ -64,6 +63,7 @@ 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; @@ -232,9 +232,13 @@ private Mono createEx( .create(datasourceStorage, isDryOps) .map(datasourceStorage1 -> { if (datasourceStorageDryRunQueries != null && isDryOps) { - datasourceStorageDryRunQueries - .computeIfAbsent(DBOpsType.SAVE.name(), k -> new ArrayList<>()) - .add(datasourceStorage1); + List datasourceStorages = + datasourceStorageDryRunQueries.get(SAVE.name()); + if (datasourceStorages == null) { + datasourceStorages = new ArrayList<>(); + } + datasourceStorages.add(datasourceStorage1); + datasourceStorageDryRunQueries.put(SAVE.name(), 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 69e6d7a7b25e..33672481c79a 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.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 Flux getEntitiesPresentInWorkspace(String workspaceId) { } private void addDryOpsForEntity( - DBOpsType queryType, Map> dryRunOpsMap, Datasource createdDatasource) { - dryRunOpsMap.computeIfAbsent(queryType.name(), k -> new ArrayList<>()).add(createdDatasource); + DBOpsType queryType, @NonNull Map> dryRunOpsMap, Datasource createdDatasource) { + List datasourceList = dryRunOpsMap.get(queryType.name()); + datasourceList = datasourceList == null ? new ArrayList<>() : datasourceList; + datasourceList.add(createdDatasource); + dryRunOpsMap.put(queryType.name(), datasourceList); } } 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 02a37f9f3431..e826f4d5803c 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 void importApplication_withUnConfiguredDatasources_Success() { .verifyComplete(); } + @Test + @WithUserDetails(value = "api_user") public void importArtifactIntoWorkspace_pageRemovedAndUpdatedDefaultPageNameInBranchApplication_Success() { Application testApplication = new Application(); testApplication.setName("importApplicationIntoWorkspace_pageRemovedInBranchApplication_Success"); From ed7457ac3334eba84b17fd809596309946ee4ae7 Mon Sep 17 00:00:00 2001 From: Abhijeet Date: Tue, 9 Jul 2024 19:43:49 +0530 Subject: [PATCH 2/2] fix: Start using synchronizedList to avoid concurrent exceptions --- .../datasources/base/DatasourceServiceCE.java | 3 ++- .../base/DatasourceServiceCEImpl.java | 9 ++++---- .../DatasourceImportableServiceCEImpl.java | 6 ++--- .../ce/MappedImportableResourcesCE_DTO.java | 22 +++++++++++++++++-- 4 files changed, 30 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 b2f2953e4dce..7f88310d3185 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.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 38e0884e2e06..52134effaa9e 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 @@ -19,6 +19,7 @@ import com.appsmith.server.domains.Plugin; import com.appsmith.server.domains.User; import com.appsmith.server.domains.Workspace; +import com.appsmith.server.dtos.DBOpsType; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.PluginExecutorHelper; @@ -147,7 +148,7 @@ public Mono create(Datasource datasource) { // 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 @@ private Mono createEx( @NotNull Datasource datasource, AclPermission permission, boolean isDryOps, - Map> datasourceStorageDryRunQueries) { + Map> datasourceStorageDryRunQueries) { // Validate incoming request String workspaceId = datasource.getWorkspaceId(); if (!hasText(workspaceId)) { @@ -233,12 +234,12 @@ private Mono createEx( .map(datasourceStorage1 -> { if (datasourceStorageDryRunQueries != null && isDryOps) { List datasourceStorages = - datasourceStorageDryRunQueries.get(SAVE.name()); + datasourceStorageDryRunQueries.get(SAVE); if (datasourceStorages == null) { datasourceStorages = new ArrayList<>(); } datasourceStorages.add(datasourceStorage1); - datasourceStorageDryRunQueries.put(SAVE.name(), datasourceStorages); + 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 33672481c79a..83489d22a359 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 @@ -409,10 +409,10 @@ public Flux getEntitiesPresentInWorkspace(String workspaceId) { } private void addDryOpsForEntity( - DBOpsType queryType, @NonNull Map> dryRunOpsMap, Datasource createdDatasource) { - List datasourceList = dryRunOpsMap.get(queryType.name()); + DBOpsType queryType, @NonNull Map> dryRunOpsMap, Datasource createdDatasource) { + List datasourceList = dryRunOpsMap.get(queryType); datasourceList = datasourceList == null ? new ArrayList<>() : datasourceList; datasourceList.add(createdDatasource); - dryRunOpsMap.put(queryType.name(), datasourceList); + 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 58363cdf2ef5..5a0a0bc62f1f 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 @@ -4,12 +4,15 @@ import com.appsmith.external.models.DatasourceStorage; import com.appsmith.server.domains.Context; 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; @@ -47,7 +50,22 @@ 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<>(); + + { + 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<>())); + } + } }