From 22ea8110e7a2eae4327c9712ec900f84dfa4b698 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Thu, 14 Nov 2024 17:29:46 +0530 Subject: [PATCH 1/5] chore: perf optimisation for js action creation phase 1 --- .../ce/LayoutActionServiceCEImpl.java | 12 ++-- .../ce/LayoutCollectionServiceCEImpl.java | 13 ++++ .../services/ActionCollectionServiceTest.java | 62 +++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java index 0efe0e9e4f3..9ddaad682f2 100755 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java @@ -436,10 +436,14 @@ protected Mono validateAndGenerateActionDomainBasedOnContext(ActionDT String name = action.getValidName(); CreatorContextType contextType = action.getContextType() == null ? CreatorContextType.PAGE : action.getContextType(); - return refactoringService - .isNameAllowed(page.getId(), contextType, layout.getId(), name) - .name(IS_NAME_ALLOWED) - .tap(Micrometer.observation(observationRegistry)); + + if (!isJsAction) { + return refactoringService + .isNameAllowed(page.getId(), contextType, layout.getId(), name) + .name(IS_NAME_ALLOWED) + .tap(Micrometer.observation(observationRegistry)); + } + return Mono.just(true); }) .flatMap(nameAllowed -> { // If the name is allowed, return pageMono for further processing diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java index aa0d73de132..b6ab3ddba02 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java @@ -315,6 +315,19 @@ public Mono updateUnpublishedActionCollection( final Set baseActionIds = new HashSet<>(); baseActionIds.addAll(validBaseActionIds); + // If duplicate action name exists, throw an error + final Map actionNameCountMap = actionCollectionDTO.getActions().stream() + .collect(Collectors.groupingBy(ActionDTO::getName, Collectors.counting())); + List duplicateNames = actionNameCountMap.entrySet().stream() + .filter(entry -> entry.getValue() > 1) + .map(Map.Entry::getKey) + .collect(Collectors.toList()); + + if (!duplicateNames.isEmpty()) { + return Mono.error(new AppsmithException( + AppsmithError.DUPLICATE_KEY_USER_ERROR, duplicateNames.get(0), FieldName.NAME)); + } + final Mono> newValidActionIdsMono = branchedActionCollectionMono.flatMap( branchedActionCollection -> Flux.fromIterable(actionCollectionDTO.getActions()) .flatMap(actionDTO -> { diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java index c21a0f49589..632a76d97c2 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java @@ -27,6 +27,7 @@ import com.appsmith.server.dtos.RefactorEntityNameDTO; import com.appsmith.server.dtos.WorkspacePluginStatus; 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.layouts.UpdateLayoutService; @@ -75,6 +76,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; @SpringBootTest @Slf4j @@ -804,4 +806,64 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle }) .verifyComplete(); } + + @Test + @WithUserDetails(value = "api_user") + public void + testUpdateUnpublishedActionCollection_createMultipleActionsWithSameName_returnsDuplicateActionNameError() { + Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(pluginExecutor)); + Mockito.when(pluginExecutor.getHintMessages(Mockito.any(), Mockito.any())) + .thenReturn(Mono.zip(Mono.just(new HashSet<>()), Mono.just(new HashSet<>()))); + + ActionCollectionDTO actionCollectionDTO = new ActionCollectionDTO(); + actionCollectionDTO.setName("testCollection1"); + actionCollectionDTO.setPageId(testPage.getId()); + actionCollectionDTO.setApplicationId(testApp.getId()); + actionCollectionDTO.setWorkspaceId(workspaceId); + actionCollectionDTO.setPluginId(datasource.getPluginId()); + actionCollectionDTO.setVariables(List.of(new JSValue("test", "String", "test", true))); + actionCollectionDTO.setBody("collectionBody"); + actionCollectionDTO.setPluginType(PluginType.JS); + + // Create actions + ActionDTO action1 = new ActionDTO(); + action1.setName("testAction1"); + action1.setActionConfiguration(new ActionConfiguration()); + action1.getActionConfiguration().setBody("initial body"); + action1.getActionConfiguration().setIsValid(false); + actionCollectionDTO.setActions(List.of(action1)); + + // Create Js object + ActionCollectionDTO createdActionCollectionDTO = + layoutCollectionService.createCollection(actionCollectionDTO).block(); + assert createdActionCollectionDTO != null; + assert createdActionCollectionDTO.getId() != null; + String createdActionCollectionId = createdActionCollectionDTO.getId(); + + // Update JS object to create two actions with same name + ActionDTO action2 = new ActionDTO(); + action2.setName("testAction1"); + action2.setActionConfiguration(new ActionConfiguration()); + action2.getActionConfiguration().setBody("mockBody"); + action2.getActionConfiguration().setIsValid(false); + + ActionDTO action3 = new ActionDTO(); + action3.setName("testAction2"); + action3.setActionConfiguration(new ActionConfiguration()); + action3.getActionConfiguration().setBody("mockBody"); + action3.getActionConfiguration().setIsValid(false); + + actionCollectionDTO.setActions( + List.of(createdActionCollectionDTO.getActions().get(0), action2, action3)); + + final Mono updatedActionCollectionDTO = + layoutCollectionService.updateUnpublishedActionCollection( + createdActionCollectionId, actionCollectionDTO); + + StepVerifier.create(updatedActionCollectionDTO).verifyErrorSatisfies(error -> { + assertTrue(error instanceof AppsmithException); + String expectedMessage = "testCollection1.testAction1 already exists. Please use a different name"; + assertEquals(expectedMessage, error.getMessage()); + }); + } } From e45bdb2c014935a696012996921aa71d17e83030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Thu, 14 Nov 2024 17:39:30 +0530 Subject: [PATCH 2/5] updated comment --- .../appsmith/server/services/ActionCollectionServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java index 632a76d97c2..610c6426fb2 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java @@ -840,7 +840,7 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle assert createdActionCollectionDTO.getId() != null; String createdActionCollectionId = createdActionCollectionDTO.getId(); - // Update JS object to create two actions with same name + // Update JS object to create an action with same name as previously created action ActionDTO action2 = new ActionDTO(); action2.setName("testAction1"); action2.setActionConfiguration(new ActionConfiguration()); From e4b58823c2c0c811a911fc61e8beb2c75f8c9879 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Thu, 14 Nov 2024 18:33:18 +0530 Subject: [PATCH 3/5] fix: test case issue fixed --- .../server/services/ce/LayoutCollectionServiceCEImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java index b6ab3ddba02..d4c4707801b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java @@ -317,7 +317,7 @@ public Mono updateUnpublishedActionCollection( // If duplicate action name exists, throw an error final Map actionNameCountMap = actionCollectionDTO.getActions().stream() - .collect(Collectors.groupingBy(ActionDTO::getName, Collectors.counting())); + .collect(Collectors.groupingBy(ActionDTO::getValidName, Collectors.counting())); List duplicateNames = actionNameCountMap.entrySet().stream() .filter(entry -> entry.getValue() > 1) .map(Map.Entry::getKey) From a3c25b6c8215f577f9ed24ee5067d45eef330e42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Thu, 14 Nov 2024 20:25:28 +0530 Subject: [PATCH 4/5] fix: updated logic to check current action name with existing actions to retain the previous logic we had --- .../ce/LayoutCollectionServiceCEImpl.java | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java index d4c4707801b..4e3bcdd86a1 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java @@ -315,19 +315,10 @@ public Mono updateUnpublishedActionCollection( final Set baseActionIds = new HashSet<>(); baseActionIds.addAll(validBaseActionIds); - // If duplicate action name exists, throw an error - final Map actionNameCountMap = actionCollectionDTO.getActions().stream() - .collect(Collectors.groupingBy(ActionDTO::getValidName, Collectors.counting())); - List duplicateNames = actionNameCountMap.entrySet().stream() - .filter(entry -> entry.getValue() > 1) - .map(Map.Entry::getKey) + List existingActions = actionCollectionDTO.getActions().stream() + .filter(action -> action.getId() != null) .collect(Collectors.toList()); - if (!duplicateNames.isEmpty()) { - return Mono.error(new AppsmithException( - AppsmithError.DUPLICATE_KEY_USER_ERROR, duplicateNames.get(0), FieldName.NAME)); - } - final Mono> newValidActionIdsMono = branchedActionCollectionMono.flatMap( branchedActionCollection -> Flux.fromIterable(actionCollectionDTO.getActions()) .flatMap(actionDTO -> { @@ -348,11 +339,23 @@ public Mono updateUnpublishedActionCollection( actionDTO.setPluginType(actionCollectionDTO.getPluginType()); actionDTO.setPluginId(actionCollectionDTO.getPluginId()); actionDTO.setBranchName(branchedActionCollection.getBranchName()); + + boolean isDuplicateActionName = existingActions.stream() + .anyMatch(action -> action.getValidName() != null + && action.getValidName().contains(actionDTO.getValidName())); + // actionCollectionService is a new action, we need to create one - return layoutActionService - .createSingleAction(actionDTO, Boolean.TRUE) - .name(CREATE_ACTION) - .tap(Micrometer.observation(observationRegistry)); + if (isDuplicateActionName) { + return Mono.error(new AppsmithException( + AppsmithError.DUPLICATE_KEY_USER_ERROR, + actionDTO.getValidName(), + FieldName.NAME)); + } else { + return layoutActionService + .createSingleAction(actionDTO, Boolean.TRUE) + .name(CREATE_ACTION) + .tap(Micrometer.observation(observationRegistry)); + } } else { actionDTO.setCollectionId(null); // Client only knows about the default action ID, fetch branched action id to update the From d5c75edd301e75b2432b642f366bc80c6fea6b89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Fri, 15 Nov 2024 13:38:25 +0530 Subject: [PATCH 5/5] fix: comparing action names with new actions as well --- .../ce/LayoutCollectionServiceCEImpl.java | 16 ++--- .../services/ActionCollectionServiceTest.java | 64 ++++++++++++++++++- 2 files changed, 70 insertions(+), 10 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java index 4e3bcdd86a1..e644b9f7ea6 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java @@ -315,8 +315,12 @@ public Mono updateUnpublishedActionCollection( final Set baseActionIds = new HashSet<>(); baseActionIds.addAll(validBaseActionIds); - List existingActions = actionCollectionDTO.getActions().stream() - .filter(action -> action.getId() != null) + // create duplicate name map + final Map actionNameCountMap = actionCollectionDTO.getActions().stream() + .collect(Collectors.groupingBy(ActionDTO::getName, Collectors.counting())); + List duplicateNames = actionNameCountMap.entrySet().stream() + .filter(entry -> entry.getValue() > 1) + .map(Map.Entry::getKey) .collect(Collectors.toList()); final Mono> newValidActionIdsMono = branchedActionCollectionMono.flatMap( @@ -340,15 +344,11 @@ public Mono updateUnpublishedActionCollection( actionDTO.setPluginId(actionCollectionDTO.getPluginId()); actionDTO.setBranchName(branchedActionCollection.getBranchName()); - boolean isDuplicateActionName = existingActions.stream() - .anyMatch(action -> action.getValidName() != null - && action.getValidName().contains(actionDTO.getValidName())); - // actionCollectionService is a new action, we need to create one - if (isDuplicateActionName) { + if (duplicateNames.contains(actionDTO.getName())) { return Mono.error(new AppsmithException( AppsmithError.DUPLICATE_KEY_USER_ERROR, - actionDTO.getValidName(), + actionDTO.getName(), FieldName.NAME)); } else { return layoutActionService diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java index 610c6426fb2..8c0471e7e36 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java @@ -810,7 +810,7 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle @Test @WithUserDetails(value = "api_user") public void - testUpdateUnpublishedActionCollection_createMultipleActionsWithSameName_returnsDuplicateActionNameError() { + testUpdateUnpublishedActionCollection_createNewActionsWithSameNameAsExisting_returnsDuplicateActionNameError() { Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(pluginExecutor)); Mockito.when(pluginExecutor.getHintMessages(Mockito.any(), Mockito.any())) .thenReturn(Mono.zip(Mono.just(new HashSet<>()), Mono.just(new HashSet<>()))); @@ -862,7 +862,67 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle StepVerifier.create(updatedActionCollectionDTO).verifyErrorSatisfies(error -> { assertTrue(error instanceof AppsmithException); - String expectedMessage = "testCollection1.testAction1 already exists. Please use a different name"; + String expectedMessage = "testAction1 already exists. Please use a different name"; + assertEquals(expectedMessage, error.getMessage()); + }); + } + + @Test + @WithUserDetails(value = "api_user") + public void + testUpdateUnpublishedActionCollection_createMultipleActionsWithSameName_returnsDuplicateActionNameError() { + Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(pluginExecutor)); + Mockito.when(pluginExecutor.getHintMessages(Mockito.any(), Mockito.any())) + .thenReturn(Mono.zip(Mono.just(new HashSet<>()), Mono.just(new HashSet<>()))); + + ActionCollectionDTO actionCollectionDTO = new ActionCollectionDTO(); + actionCollectionDTO.setName("testCollection1"); + actionCollectionDTO.setPageId(testPage.getId()); + actionCollectionDTO.setApplicationId(testApp.getId()); + actionCollectionDTO.setWorkspaceId(workspaceId); + actionCollectionDTO.setPluginId(datasource.getPluginId()); + actionCollectionDTO.setVariables(List.of(new JSValue("test", "String", "test", true))); + actionCollectionDTO.setBody("collectionBody"); + actionCollectionDTO.setPluginType(PluginType.JS); + + // Create actions + ActionDTO action1 = new ActionDTO(); + action1.setName("testAction1"); + action1.setActionConfiguration(new ActionConfiguration()); + action1.getActionConfiguration().setBody("initial body"); + action1.getActionConfiguration().setIsValid(false); + actionCollectionDTO.setActions(List.of(action1)); + + // Create Js object + ActionCollectionDTO createdActionCollectionDTO = + layoutCollectionService.createCollection(actionCollectionDTO).block(); + assert createdActionCollectionDTO != null; + assert createdActionCollectionDTO.getId() != null; + String createdActionCollectionId = createdActionCollectionDTO.getId(); + + // Update JS object to create an action with same name as previously created action + ActionDTO action2 = new ActionDTO(); + action2.setName("testAction2"); + action2.setActionConfiguration(new ActionConfiguration()); + action2.getActionConfiguration().setBody("mockBody"); + action2.getActionConfiguration().setIsValid(false); + + ActionDTO action3 = new ActionDTO(); + action3.setName("testAction2"); + action3.setActionConfiguration(new ActionConfiguration()); + action3.getActionConfiguration().setBody("mockBody"); + action3.getActionConfiguration().setIsValid(false); + + actionCollectionDTO.setActions( + List.of(createdActionCollectionDTO.getActions().get(0), action2, action3)); + + final Mono updatedActionCollectionDTO = + layoutCollectionService.updateUnpublishedActionCollection( + createdActionCollectionId, actionCollectionDTO); + + StepVerifier.create(updatedActionCollectionDTO).verifyErrorSatisfies(error -> { + assertTrue(error instanceof AppsmithException); + String expectedMessage = "testAction2 already exists. Please use a different name"; assertEquals(expectedMessage, error.getMessage()); }); }