From 7612e4f14d6add48a9b1a42e51ccfe34daaf9ed8 Mon Sep 17 00:00:00 2001 From: sneha122 Date: Tue, 19 Nov 2024 17:04:38 +0530 Subject: [PATCH] chore: perf optimisation for js action creation phase 1 (#37391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description [During analysis of action creation flow metrics](https://github.com/appsmithorg/appsmith/issues/37151#issuecomment-2468354426), we observed that RefactoringService.isNameAllowed is taking 80-90% of the total JS object action time. This PR optimises this part in a way that for any jsobject action, instead of fetching all actions from DB and comparing it to see if current action name is allowed, we simply do that check in memory where for current action collection, if any action names are being duplicated, we throw the error. We could make this change easily because recently we merged a [PR](https://github.com/appsmithorg/appsmith/pull/36958) which removes the actions with duplicate name from client payload whenever Js object update API is called, with this change, we can guarantee that for any JS object update call, all actions inside it will always have unique names. This PR makes the similar check on backend where if any action has duplicate name within collection, we throw an error and don't store that action in the DB. We may need to consider following test case in both before and after implementation of this approach. This can be covered during PR testing: What happens if the client sends multiple requests to add a new function in an existing collection. That is, as a result of the debounce logic, if the server receives 2 consecutive requests with a populated collection but without actionId associated to either request. Relevant thread: https://theappsmith.slack.com/archives/C040LHZN03V/p1731571364933089 Fixes #37365 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.JS" ### :mag: Cypress test results > [!TIP] > ๐ŸŸข ๐ŸŸข ๐ŸŸข All cypress tests have passed! ๐ŸŽ‰ ๐ŸŽ‰ ๐ŸŽ‰ > Workflow run: > Commit: d5c75edd301e75b2432b642f366bc80c6fea6b89 > Cypress dashboard. > Tags: `@tag.JS` > Spec: >
Tue, 19 Nov 2024 11:14:16 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **New Features** - Enhanced validation to prevent the creation of actions with duplicate names in action collections. - Simplified handling of JavaScript actions, allowing them to bypass certain validation checks. - **Bug Fixes** - Improved error handling during action updates and collection modifications to ensure better logging and management of failures. - **Tests** - Added tests to verify that duplicate action names trigger appropriate error messages, enhancing the robustness of the action collection feature. --------- Co-authored-by: โ€œsneha122โ€ <โ€œsneha@appsmith.comโ€> --- .../ce/LayoutActionServiceCEImpl.java | 12 +- .../ce/LayoutCollectionServiceCEImpl.java | 24 +++- .../services/ActionCollectionServiceTest.java | 122 ++++++++++++++++++ 3 files changed, 150 insertions(+), 8 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 0efe0e9e4f..9ddaad682f 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 @@ public class LayoutActionServiceCEImpl implements LayoutActionServiceCE { 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 aa0d73de13..e644b9f7ea 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,14 @@ public class LayoutCollectionServiceCEImpl implements LayoutCollectionServiceCE final Set baseActionIds = new HashSet<>(); baseActionIds.addAll(validBaseActionIds); + // 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( branchedActionCollection -> Flux.fromIterable(actionCollectionDTO.getActions()) .flatMap(actionDTO -> { @@ -335,11 +343,19 @@ public class LayoutCollectionServiceCEImpl implements LayoutCollectionServiceCE actionDTO.setPluginType(actionCollectionDTO.getPluginType()); actionDTO.setPluginId(actionCollectionDTO.getPluginId()); actionDTO.setBranchName(branchedActionCollection.getBranchName()); + // actionCollectionService is a new action, we need to create one - return layoutActionService - .createSingleAction(actionDTO, Boolean.TRUE) - .name(CREATE_ACTION) - .tap(Micrometer.observation(observationRegistry)); + if (duplicateNames.contains(actionDTO.getName())) { + return Mono.error(new AppsmithException( + AppsmithError.DUPLICATE_KEY_USER_ERROR, + actionDTO.getName(), + 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 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 c21a0f4958..8c0471e7e3 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.PluginWorkspaceDTO; 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 com.appsmith.server.constants.FieldName.VIEWER; 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,124 @@ public class ActionCollectionServiceTest { }) .verifyComplete(); } + + @Test + @WithUserDetails(value = "api_user") + public void + 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<>()))); + + 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("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 = "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()); + }); + } }