From 63fe27fae1d22c848cb2e6fa0114bd569899f431 Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Fri, 20 Nov 2020 10:55:59 +0530 Subject: [PATCH] [Bug Fix] : Clone Application creates corrupted clone when interrupted. (#1800) * Doing a deep copy during clone application instead of updating the original application which may have been causing a concurrency bug. * Ensuring that once the clone application flow is triggered, the flow completes eventually even if the client cancels the request before completion. * Cloned application would not be public. * Added parametrized Application constructor * Removed lombok all args constructor * Optimized import * Incorporated review comments : 1. Updated the constructor for creating the application 2. Added a test case to assert that if during cloning of an application the flow gets cancelled, the cloning would still complete and ensure that the application created is sane. --- .../appsmith/server/constants/FieldName.java | 1 + .../appsmith/server/domains/Application.java | 13 +++ .../repositories/ApplicationRepository.java | 1 + .../services/ApplicationPageServiceImpl.java | 82 +++++++------ .../server/services/NewPageServiceImpl.java | 9 +- .../services/ApplicationServiceTest.java | 108 ++++++++++++++++++ 6 files changed, 173 insertions(+), 41 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java index 08efddb658..02ad28d737 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java @@ -57,4 +57,5 @@ public class FieldName { public static String USERNAMES = "usernames"; public static String ACTION = "action"; public static String ASSET = "asset"; + public static String APPLICATION = "application"; } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java index cc1b64ae47..c7da9ece8d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java @@ -12,6 +12,7 @@ import org.springframework.data.annotation.Transient; import org.springframework.data.mongodb.core.mapping.Document; import javax.validation.constraints.NotNull; +import java.util.ArrayList; import java.util.List; @Getter @@ -49,6 +50,18 @@ public class Application extends BaseDomain { String icon; + // This constructor is used during clone application. It only deeply copies selected fields. The rest are either + // initialized newly or is left up to the calling function to set. + public Application(Application application) { + super(); + this.organizationId = application.getOrganizationId(); + this.pages = new ArrayList<>(); + this.publishedPages = new ArrayList<>(); + this.clonedFromApplicationId = application.getId(); + this.color = application.getColor(); + this.icon = application.getIcon(); + } + public List getPages() { return viewMode ? publishedPages : pages; } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ApplicationRepository.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ApplicationRepository.java index 5423c593c2..c9811f7f80 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ApplicationRepository.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ApplicationRepository.java @@ -13,4 +13,5 @@ public interface ApplicationRepository extends BaseRepository findByOrganizationId(String organizationId); + Flux findByClonedFromApplicationId(String clonedFromApplicationId); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java index 9da96447e3..c20190c7ea 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java @@ -310,7 +310,7 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { } private Mono clonePageGivenApplicationId(String pageId, String applicationId, - @Nullable String newPageNameSuffix) { + @Nullable String newPageNameSuffix) { // Find the source page and then prune the page layout fields to only contain the required fields that should be // copied. Mono sourcePageMono = newPageService.findPageById(pageId, MANAGE_PAGES, false) @@ -328,9 +328,9 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { .map(layouts -> { page.setLayouts(layouts); return page; - })); + }) + ); - // This call is without Flux sourceActionFlux = newActionService.findByPageId(pageId, MANAGE_ACTIONS) // In case there are no actions in the page being cloned, return empty .switchIfEmpty(Flux.empty()); @@ -367,8 +367,8 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { return newPageService.createDefault(page); }); }) - .flatMap(page -> { - String newPageId = page.getId(); + .flatMap(clonedPage -> { + String newPageId = clonedPage.getId(); return sourceActionFlux .flatMap(action -> { // Set new page id in the actionDTO @@ -378,7 +378,7 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { return newActionService.createAction(action.getUnpublishedAction()); }) .collectList() - .thenReturn(page); + .thenReturn(clonedPage); }) // Calculate the onload actions for this page now that the page and actions have been created .flatMap(savedPage -> { @@ -420,7 +420,6 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { .map(application1 -> application1.getName()) .collect(Collectors.toSet()) .map(appNames -> { - log.debug("app names for this organization are : {}", appNames); String newAppName = application.getName() + " Copy"; int i = 0; String name = newAppName; @@ -431,43 +430,52 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { return name; })); - return Mono.zip(applicationMono, newAppNameMono) + Mono clonedResultMono = Mono.zip(applicationMono, newAppNameMono) .flatMap(tuple -> { Application sourceApplication = tuple.getT1(); String newName = tuple.getT2(); - sourceApplication.setId(null); - sourceApplication.setIsPublic(false); - sourceApplication.setName(newName); - + // Create a new clone application object without the pages using the parametrized Application constructor + Application newApplication = new Application(sourceApplication); + newApplication.setName(newName); + Mono userMono = sessionUserService.getCurrentUser().cache(); // First set the correct policies for the new cloned application - return setApplicationPolicies(userMono, sourceApplication.getOrganizationId(), sourceApplication) - // Create the cloned application with the new name and policies before proceeding further. - .flatMap(applicationService::createDefault) - // Now fetch the pages of the source application, clone and add them to this new application - .flatMap(savedApplication -> applicationMono - .flatMap(application -> Flux.fromIterable(application.getPages()) - .flatMap(applicationPage -> { - String pageId = applicationPage.getId(); - Boolean isDefault = applicationPage.getIsDefault(); - return this.clonePageGivenApplicationId(pageId, savedApplication.getId()) - .map(page -> { - ApplicationPage newApplicationPage = new ApplicationPage(); - newApplicationPage.setId(page.getId()); - newApplicationPage.setIsDefault(isDefault); - return newApplicationPage; - }); - }) - .collectList() - ) - // Set the cloned pages into the cloned application and save. - .flatMap(clonedPages -> { - savedApplication.setPages(clonedPages); - return applicationService.save(savedApplication); - }) - ); + return setApplicationPolicies(userMono, sourceApplication.getOrganizationId(), newApplication) + // Create the cloned application with the new name and policies before proceeding further. + .flatMap(applicationService::createDefault) + // Now fetch the pages of the source application, clone and add them to this new application + .flatMap(savedApplication -> Flux.fromIterable(sourceApplication.getPages()) + .flatMap(applicationPage -> { + String pageId = applicationPage.getId(); + Boolean isDefault = applicationPage.getIsDefault(); + return this.clonePageGivenApplicationId(pageId, savedApplication.getId()) + .map(clonedPage -> { + ApplicationPage newApplicationPage = new ApplicationPage(); + newApplicationPage.setId(clonedPage.getId()); + newApplicationPage.setIsDefault(isDefault); + return newApplicationPage; + }); + }) + .collectList() + // Set the cloned pages into the cloned application and save. + .flatMap(clonedPages -> { + savedApplication.setPages(clonedPages); + return applicationService.save(savedApplication); + }) + ); }); + + // Clone Application is currently a slow API because it needs to create application, clone all the pages, and then + // clone all the actions. This process may take time and the client may cancel the request. This leads to the flow + // getting stopped mid way producing corrupted clones. The following ensures that even though the client may have + // cancelled the flow, the cloning of the application should proceed uninterrupted and whenever the user refreshes + // the page, the cloned application is available and is in sane state. + // To achieve this, we use a synchronous sink which does not take subscription cancellations into account. This + // means that even if the subscriber has cancelled its subscription, the create method still generates its event. + return Mono.create(sink -> clonedResultMono + .subscribe(sink::success, sink::error, null, sink.currentContext()) + ); } /** diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/NewPageServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/NewPageServiceImpl.java index c45c4bf54d..0d50584bc0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/NewPageServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/NewPageServiceImpl.java @@ -60,7 +60,7 @@ public class NewPageServiceImpl extends BaseService findNamesByApplicationIdAndViewMode(String applicationId, Boolean view) { Mono applicationMono = applicationService.findById(applicationId, AclPermission.READ_APPLICATIONS) - .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.PAGE + "by application id", applicationId))) + .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.APPLICATION, applicationId))) .cache(); Mono> pagesListMono = applicationMono @@ -196,8 +196,9 @@ public class NewPageServiceImpl extends BaseService findNamesByApplication(Application application, Boolean viewMode) { List pages = application.getPages(); + return findByApplicationId(application.getId(), AclPermission.READ_PAGES, viewMode) - .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.PAGE + "by application name", application.getName()))) + .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.PAGE + " by application id", application.getId()))) .map(page -> { PageNameIdDTO pageNameIdDTO = new PageNameIdDTO(); pageNameIdDTO.setId(page.getId()); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java index 2a33f59bae..d981c8e18d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java @@ -22,6 +22,7 @@ import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.MockPluginExecutor; import com.appsmith.server.helpers.PluginExecutorHelper; +import com.appsmith.server.repositories.ApplicationRepository; import com.appsmith.server.repositories.NewPageRepository; import com.appsmith.server.solutions.ApplicationFetcher; import lombok.extern.slf4j.Slf4j; @@ -42,9 +43,11 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Set; +import java.util.stream.Collectors; import static com.appsmith.server.acl.AclPermission.EXECUTE_ACTIONS; import static com.appsmith.server.acl.AclPermission.EXECUTE_DATASOURCES; @@ -97,6 +100,9 @@ public class ApplicationServiceTest { @Autowired NewPageRepository newPageRepository; + @Autowired + ApplicationRepository applicationRepository; + String orgId; @Before @@ -613,6 +619,11 @@ public class ApplicationServiceTest { assertThat(application.getName().equals("ApplicationServiceTest Clone Source TestApp Copy")); assertThat(application.getPolicies()).containsAll(Set.of(manageAppPolicy, readAppPolicy)); assertThat(application.getOrganizationId().equals(orgId)); + List pages = application.getPages(); + Set pageIdsFromApplication = pages.stream().map(page -> page.getId()).collect(Collectors.toSet()); + Set pageIdsFromDb = pageList.stream().map(page -> page.getId()).collect(Collectors.toSet()); + + assertThat(pageIdsFromApplication.containsAll(pageIdsFromDb)); assertThat(pageList).isNotEmpty(); for (PageDTO page : pageList) { @@ -852,4 +863,101 @@ public class ApplicationServiceTest { }) .verifyComplete(); } + + @Test + @WithUserDetails(value = "api_user") + public void validCloneApplicationWhenCancelledMidWay() { + Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor())); + + Application testApplication = new Application(); + String appName = "ApplicationServiceTest Clone Application Midway Cancellation"; + testApplication.setName(appName); + + Application originalApplication = applicationPageService.createApplication(testApplication, orgId) + .block(); + + String pageId = originalApplication.getPages().get(0).getId(); + + Plugin plugin = pluginService.findByName("Installed Plugin Name").block(); + Datasource datasource = new Datasource(); + datasource.setName("Cloned App Test"); + datasource.setPluginId(plugin.getId()); + DatasourceConfiguration datasourceConfiguration = new DatasourceConfiguration(); + datasourceConfiguration.setUrl("http://test.com"); + datasource.setDatasourceConfiguration(datasourceConfiguration); + datasource.setOrganizationId(orgId); + + Datasource savedDatasource = datasourceService.create(datasource).block(); + + ActionDTO action1 = new ActionDTO(); + action1.setName("Clone App Test action1"); + action1.setPageId(pageId); + action1.setDatasource(savedDatasource); + ActionConfiguration actionConfiguration = new ActionConfiguration(); + actionConfiguration.setHttpMethod(HttpMethod.GET); + action1.setActionConfiguration(actionConfiguration); + + ActionDTO savedAction1 = newActionService.createAction(action1).block(); + + ActionDTO action2 = new ActionDTO(); + action2.setName("Clone App Test action2"); + action2.setPageId(pageId); + action2.setDatasource(savedDatasource); + action2.setActionConfiguration(actionConfiguration); + + ActionDTO savedAction2 = newActionService.createAction(action2).block(); + + ActionDTO action3 = new ActionDTO(); + action3.setName("Clone App Test action3"); + action3.setPageId(pageId); + action3.setDatasource(savedDatasource); + action3.setActionConfiguration(actionConfiguration); + + ActionDTO savedAction3 = newActionService.createAction(action3).block(); + + + // Trigger the clone of application now. + applicationPageService.cloneApplication(originalApplication.getId()) + .timeout(Duration.ofMillis(10)) + .subscribe(); + + Mono clonedAppFromDbMono = Mono.just(originalApplication) + .flatMap(originalApp -> { + try { + // Before fetching the cloned application, sleep for 5 seconds to ensure that the cloning finishes + Thread.sleep(5000); + } catch (InterruptedException e) { + e.printStackTrace(); + } + return applicationRepository.findByClonedFromApplicationId(originalApp.getId()).next(); + }) + .cache(); + + Mono> actionsMono = clonedAppFromDbMono + .flatMap(clonedAppFromDb -> newActionService + .findAllByApplicationIdAndViewMode(clonedAppFromDb.getId(), false, READ_ACTIONS, null) + .collectList() + ); + + Mono> pagesMono = clonedAppFromDbMono + .flatMapMany(application -> Flux.fromIterable(application.getPages())) + .flatMap(applicationPage -> newPageService.findPageById(applicationPage.getId(), READ_PAGES, false)) + .collectList(); + + StepVerifier + .create(Mono.zip(clonedAppFromDbMono, actionsMono, pagesMono)) + .assertNext(tuple -> { + Application cloneApp = tuple.getT1(); + List actions = tuple.getT2(); + List pages = tuple.getT3(); + + assertThat(cloneApp).isNotNull(); + assertThat(pages.get(0).getId()).isNotEqualTo(pageId); + assertThat(actions.size()).isEqualTo(3); + Set actionNames = actions.stream().map(action -> action.getUnpublishedAction().getName()).collect(Collectors.toSet()); + assertThat(actionNames).containsExactlyInAnyOrder("Clone App Test action1", "Clone App Test action2", "Clone App Test action3"); + }) + .verifyComplete(); + + } }