Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: perf optimisation for js action creation phase 1 #37391

Open
wants to merge 5 commits into
base: release
Choose a base branch
from
Open

chore: perf optimisation for js action creation phase 1 #37391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22ea811
chore: perf optimisation for js action creation phase 1
Nov 14, 2024
e45bdb2
updated comment
Nov 14, 2024
e4b5882
fix: test case issue fixed
Nov 14, 2024
a3c25b6
fix: updated logic to check current action name with existing actions…
Nov 14, 2024
d5c75ed
fix: comparing action names with new actions as well
Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ...smith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,10 +436,14 @@ protected Mono<NewAction> 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
Expand Down
24 changes: 20 additions & 4 deletions ...h-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,14 @@ public Mono<ActionCollectionDTO> updateUnpublishedActionCollection(
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 -> {
Expand All @@ -335,11 +343,19 @@ public Mono<ActionCollectionDTO> updateUnpublishedActionCollection(
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
Expand Down
122 changes: 122 additions & 0 deletions ...psmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -804,4 +806,124 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle
})
.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