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

[Bug Fix] : Clone Application creates corrupted clone when interrupted. (#1800)

Browse Source 
* 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.
This commit is contained in:
Trisha Anand 2020-11-20 10:55:59 +05:30 committed by GitHub
parent b03e815952
commit 63fe27fae1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 173 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java
View File

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

13
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java
View File

@ -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<ApplicationPage> getPages() {
return viewMode ? publishedPages : pages;
}

1
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ApplicationRepository.java
View File

@ -13,4 +13,5 @@ public interface ApplicationRepository extends BaseRepository<Application, Strin
Flux<Application> findByOrganizationId(String organizationId);
Flux<Application> findByClonedFromApplicationId(String clonedFromApplicationId);
}

82
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java
View File

@ -310,7 +310,7 @@ public class ApplicationPageServiceImpl implements ApplicationPageService {
}
private Mono<PageDTO> 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<PageDTO> 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<NewAction> 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<Application> 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<User> 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())
);
}
/**

9
app/server/appsmith-server/src/main/java/com/appsmith/server/services/NewPageServiceImpl.java
View File

@ -60,7 +60,7 @@ public class NewPageServiceImpl extends BaseService<NewPageRepository, NewPage,
} else {
// We are trying to fetch published page but it doesnt exist because the page hasn't been published yet
return Mono.empty();
page = new PageDTO();
}
} else {
if (newPage.getUnpublishedPage() != null) {
@ -78,7 +78,7 @@ public class NewPageServiceImpl extends BaseService<NewPageRepository, NewPage,
}
// We shouldn't reach here.
return Mono.empty();
return Mono.error(new AppsmithException(AppsmithError.INTERNAL_SERVER_ERROR));
}
@ -155,7 +155,7 @@ public class NewPageServiceImpl extends BaseService<NewPageRepository, NewPage,
@Override
public Mono<ApplicationPagesDTO> findNamesByApplicationIdAndViewMode(String applicationId, Boolean view) {
Mono<Application> 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<List<PageNameIdDTO>> pagesListMono = applicationMono
@ -196,8 +196,9 @@ public class NewPageServiceImpl extends BaseService<NewPageRepository, NewPage,
private Flux<PageNameIdDTO> findNamesByApplication(Application application, Boolean viewMode) {
List<ApplicationPage> 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());

108
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java
View File

@ -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<ApplicationPage> pages = application.getPages();
Set<String> pageIdsFromApplication = pages.stream().map(page -> page.getId()).collect(Collectors.toSet());
Set<String> 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<Application> 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<List<NewAction>> actionsMono = clonedAppFromDbMono
.flatMap(clonedAppFromDb -> newActionService
.findAllByApplicationIdAndViewMode(clonedAppFromDb.getId(), false, READ_ACTIONS, null)
.collectList()
);
Mono<List<PageDTO>> 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<NewAction> actions = tuple.getT2();
List<PageDTO> pages = tuple.getT3();
assertThat(cloneApp).isNotNull();
assertThat(pages.get(0).getId()).isNotEqualTo(pageId);
assertThat(actions.size()).isEqualTo(3);
Set<String> 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();
}
}