chore: perf optimisation for js action creation phase 1 (#37391)
## 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" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11911295324> > Commit: d5c75edd301e75b2432b642f366bc80c6fea6b89 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11911295324&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Tue, 19 Nov 2024 11:14:16 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
This commit is contained in:
sneha122
GitHub
parent
5ec2ff15b6
commit
7612e4f14d
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -315,6 +315,14 @@ public class LayoutCollectionServiceCEImpl implements LayoutCollectionServiceCE
|
|||
final Set<String> baseActionIds = new HashSet<>();
|
||||
baseActionIds.addAll(validBaseActionIds);
|
||||
|
||||
// create duplicate name map
|
||||
final Map<String, Long> actionNameCountMap = actionCollectionDTO.getActions().stream()
|
||||
.collect(Collectors.groupingBy(ActionDTO::getName, Collectors.counting()));
|
||||
List<String> duplicateNames = actionNameCountMap.entrySet().stream()
|
||||
.filter(entry -> entry.getValue() > 1)
|
||||
.map(Map.Entry::getKey)
|
||||
.collect(Collectors.toList());
|
||||
|
||||
final Mono<Map<String, String>> 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
|
||||
|
|
|
|||
|
|
@ -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<ActionCollectionDTO> 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<ActionCollectionDTO> 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());
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user