From 269075164e1f83da8c881ecf6c8fb31a1b14bdf0 Mon Sep 17 00:00:00 2001 From: Nayan Date: Wed, 25 Oct 2023 19:32:22 +0600 Subject: [PATCH] =?UTF-8?q?fix:=20Import=20from=20file=20fails=20when=20wo?= =?UTF-8?q?rkspace=20has=20a=20git=20connected=20app=20wi=E2=80=A6=20(#282?= =?UTF-8?q?99)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Fixes the bug when import app from file fails when target workspace has existing app with same name and multiple branches. #### PR fixes following issue(s) Fixes #28122 #### Media https://www.loom.com/share/5d31f58574f5422ba078e4153e39bf8c #### Type of change - Bug fix (non-breaking change which fixes an issue) ## Testing #### How Has This Been Tested? - [x] Manual - [x] JUnit #### Test Plan #### Issues raised during DP testing ## Checklist: #### Dev activity - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed --- .../ce/CustomApplicationRepositoryCE.java | 2 +- .../ce/CustomApplicationRepositoryCEImpl.java | 6 +-- .../ce/ApplicationPageServiceCEImpl.java | 28 ++++++------- .../services/ce/ApplicationServiceCE.java | 2 +- .../services/ce/ApplicationServiceCEImpl.java | 7 ++-- .../services/ApplicationPageServiceTest.java | 39 +++++++++++++++++++ 6 files changed, 63 insertions(+), 21 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCE.java index 76fb4b2edd..960f34e453 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCE.java @@ -79,7 +79,7 @@ public interface CustomApplicationRepositoryCE extends AppsmithRepository findByNameAndWorkspaceId(String applicationName, String workspaceId, AclPermission permission); + Mono countByNameAndWorkspaceId(String applicationName, String workspaceId, AclPermission permission); Flux getAllApplicationIdsInWorkspaceAccessibleToARoleWithPermission( String workspaceId, AclPermission permission, String permissionGroupId); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCEImpl.java index d70cc25f58..da482078c4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCEImpl.java @@ -318,13 +318,13 @@ public class CustomApplicationRepositoryCEImpl extends BaseAppsmithRepositoryImp } @Override - public Mono findByNameAndWorkspaceId( - String applicationName, String workspaceId, AclPermission permission) { + public Mono countByNameAndWorkspaceId(String applicationName, String workspaceId, AclPermission permission) { Criteria workspaceIdCriteria = where(fieldName(QApplication.application.workspaceId)).is(workspaceId); Criteria applicationNameCriteria = where(fieldName(QApplication.application.name)).is(applicationName); - return queryOne(List.of(workspaceIdCriteria, applicationNameCriteria), permission); + + return count(List.of(workspaceIdCriteria, applicationNameCriteria), permission); } @Override diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java index bc34c133db..c144f1a41a 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java @@ -1363,23 +1363,25 @@ public class ApplicationPageServiceCEImpl implements ApplicationPageServiceCE { Mono userMono = sessionUserService.getCurrentUser().cache(); Mono applicationWithPoliciesMono = this.setApplicationPolicies(userMono, application.getWorkspaceId(), application); - Mono applicationMono = applicationService.findByNameAndWorkspaceId( + Mono applicationNameTakenMono = applicationService.isApplicationNameTaken( actualName, application.getWorkspaceId(), MANAGE_APPLICATIONS); // We are taking pessimistic approach as this flow is used in import application where we are using transactions // which creates problem if we hit duplicate key exception - return applicationMono - .flatMap(application1 -> this.createOrUpdateSuffixedApplication(application, name, 1 + suffix)) - .switchIfEmpty(Mono.defer( - () -> applicationWithPoliciesMono.zipWith(userMono).flatMap(tuple -> { - Application application1 = tuple.getT1(); - application1.setModifiedBy( - tuple.getT2().getUsername()); // setting modified by to current user - // We can't use create or createApplication method here as we are expecting update operation - // if the - // _id is available with application object - return applicationService.save(application); - }))); + return applicationNameTakenMono.flatMap(isNameTaken -> { + if (isNameTaken) { + return this.createOrUpdateSuffixedApplication(application, name, 1 + suffix); + } else { + return applicationWithPoliciesMono.zipWith(userMono).flatMap(tuple -> { + Application application1 = tuple.getT1(); + application1.setModifiedBy(tuple.getT2().getUsername()); // setting modified by to current user + // We can't use create or createApplication method here as we are expecting update operation + // if the + // _id is available with application object + return applicationService.save(application); + }); + } + }); } /** diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCE.java index 513b3e3063..69d3e08f94 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCE.java @@ -98,7 +98,7 @@ public interface ApplicationServiceCE extends CrudService { public Mono deleteAppNavigationLogo(String branchName, String applicationId); - Mono findByNameAndWorkspaceId(String applicationName, String workspaceId, AclPermission permission); + Mono isApplicationNameTaken(String applicationName, String workspaceId, AclPermission permission); Mono isApplicationConnectedToGit(String applicationId); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCEImpl.java index e85d5b912a..438b9aaa85 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationServiceCEImpl.java @@ -980,9 +980,10 @@ public class ApplicationServiceCEImpl extends BaseService findByNameAndWorkspaceId( - String applicationName, String workspaceId, AclPermission permission) { - return repository.findByNameAndWorkspaceId(applicationName, workspaceId, permission); + public Mono isApplicationNameTaken(String applicationName, String workspaceId, AclPermission permission) { + return repository + .countByNameAndWorkspaceId(applicationName, workspaceId, permission) + .map(count -> count > 0); } @Override diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java index 2ccaca6aa7..f4298e668d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java @@ -3,6 +3,7 @@ package com.appsmith.server.services; import com.appsmith.git.constants.CommonConstants; import com.appsmith.server.acl.AclPermission; import com.appsmith.server.domains.Application; +import com.appsmith.server.domains.GitApplicationMetadata; import com.appsmith.server.domains.Layout; import com.appsmith.server.domains.NewPage; import com.appsmith.server.domains.Workspace; @@ -287,4 +288,42 @@ public class ApplicationPageServiceTest { }) .verifyComplete(); } + + @Test + @WithUserDetails("api_user") + public void createOrUpdateSuffixedApplication_GitConnectedAppExistsWithSameName_AppCreatedWithSuffixedName() { + // create a Git connected application with two branches + final String appName = "app" + UUID.randomUUID(); + Application application = new Application(); + application.setName(appName); + GitApplicationMetadata gitApplicationMetadata = new GitApplicationMetadata(); + gitApplicationMetadata.setBranchName("branch1"); + application.setGitApplicationMetadata(gitApplicationMetadata); + + Mono importAppMono = applicationPageService + .createApplication(application, workspace.getId()) + .flatMap(createdApp -> { + createdApp.getGitApplicationMetadata().setDefaultApplicationId(createdApp.getId()); + return applicationService.save(createdApp); + }) + .flatMap(createdApp -> { + createdApp.setId(null); + createdApp.getGitApplicationMetadata().setBranchName("branch2"); + // just duplicate the app, we're not considering the pages, they remain same in both apps + return applicationRepository.save(createdApp); + }) + .flatMap(createdApp -> { + Application newApplication = new Application(); + newApplication.setName(appName); + newApplication.setWorkspaceId(workspace.getId()); + return applicationPageService.createOrUpdateSuffixedApplication( + newApplication, newApplication.getName(), 0); + }); + + StepVerifier.create(importAppMono) + .assertNext(application1 -> { + assertThat(application1.getName()).isEqualTo(appName + " (1)"); + }) + .verifyComplete(); + } }