Protei/PromucFlow_constructor
2
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix: Concurrent modification exception during the datasource import flow (#34818)

Browse Source 
## 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: cf63fab15f7e3717b566306f8ea598662f18aea9
> <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
This commit is contained in:
Abhijeet 2024-07-10 08:37:41 +05:30 committed by GitHub
parent 013869fae3
commit 55b53a75e3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 40 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java
View File

@ -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<Datasource> createWithoutPermissions(Datasource datasource);
Mono<Datasource> createWithoutPermissions(
Datasource datasource, Map<String, List<DatasourceStorage>> datasourceStorageDryRunQueries);
Datasource datasource, Map<DBOpsType, List<DatasourceStorage>> datasourceStorageDryRunQueries);
Mono<Datasource> updateDatasourceStorage(
DatasourceStorageDTO datasourceStorageDTO, String activeEnvironmentId, Boolean IsUserRefreshedUpdate);

15
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
View File

@ -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<Datasource> createWithoutPermissions(
Datasource datasource, Map<String, List<DatasourceStorage>> datasourceStorageDryRunQueries) {
Datasource datasource, Map<DBOpsType, List<DatasourceStorage>> datasourceStorageDryRunQueries) {
return createEx(datasource, null, true, datasourceStorageDryRunQueries);
}
@ -160,7 +161,7 @@ public class DatasourceServiceCEImpl implements DatasourceServiceCE {
@NotNull Datasource datasource,
AclPermission permission,
boolean isDryOps,
Map<String, List<DatasourceStorage>> datasourceStorageDryRunQueries) {
Map<DBOpsType, List<DatasourceStorage>> 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<DatasourceStorage> datasourceStorages =
datasourceStorageDryRunQueries.get(SAVE);
if (datasourceStorages == null) {
datasourceStorages = new ArrayList<>();
}
datasourceStorages.add(datasourceStorage1);
datasourceStorageDryRunQueries.put(SAVE, datasourceStorages);
}
return datasourceStorage1;
});

8
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
View File

@ -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<Da
}
private void addDryOpsForEntity(
DBOpsType queryType, Map<String, List<Datasource>> dryRunOpsMap, Datasource createdDatasource) {
dryRunOpsMap.computeIfAbsent(queryType.name(), k -> new ArrayList<>()).add(createdDatasource);
DBOpsType queryType, @NonNull Map<DBOpsType, List<Datasource>> dryRunOpsMap, Datasource createdDatasource) {
List<Datasource> datasourceList = dryRunOpsMap.get(queryType);
datasourceList = datasourceList == null ? new ArrayList<>() : datasourceList;
datasourceList.add(createdDatasource);
dryRunOpsMap.put(queryType, datasourceList);
}
}

22
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java
View File

@ -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<String, Object> resourceStoreFromArtifactExchangeJson = new HashMap<>();
// Dry ops queries
Map<String, List<Datasource>> datasourceDryRunQueries = new HashMap<>();
Map<DBOpsType, List<Datasource>> datasourceDryRunQueries = new HashMap<>();
Map<String, List<DatasourceStorage>> datasourceStorageDryRunQueries = new HashMap<>();
Map<DBOpsType, List<DatasourceStorage>> datasourceStorageDryRunQueries = new HashMap<>();
Map<String, List<CustomJSLib>> 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<>()));
}
}
}

2
app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java
View File

@ -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");