Skip to content

chore: removed redundant page DB calls from js action creation flow #37668

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

Merged
sneha122 merged 6 commits into from
Dec 3, 2024
Merged

chore: removed redundant page DB calls from js action creation flow #37668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9297770
chore: removed redundant page DB calls from js action creation flow
Nov 25, 2024
2f2d957
meta paradigm refactor added and junit test cases
Nov 26, 2024
2041665
Merge branch 'release' into fix/remove-js-action-creation-redundant-p…
Nov 26, 2024
e349e8d
Merge branch 'release' into fix/remove-js-action-creation-redundant-p…
Nov 29, 2024
71f0d8e
code review comments addressed
Nov 29, 2024
2cd1a84
Merge branch 'release' into fix/remove-js-action-creation-redundant-p…
Nov 29, 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
18 changes: 18 additions & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/CreateActionMetaDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.appsmith.server.dtos;

import com.appsmith.external.helpers.AppsmithEventContext;
import com.appsmith.server.domains.NewPage;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@AllArgsConstructor
@NoArgsConstructor
@Builder(toBuilder = true)
public class CreateActionMetaDTO {
Boolean isJsAction;
AppsmithEventContext eventContext;
NewPage newPage;
}
13 changes: 9 additions & 4 deletions ...rver/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.appsmith.server.domains.Workspace;
import com.appsmith.server.dtos.ActionCollectionDTO;
import com.appsmith.server.dtos.ApplicationImportDTO;
import com.appsmith.server.dtos.CreateActionMetaDTO;
import com.appsmith.server.dtos.ForkingMetaDTO;
import com.appsmith.server.dtos.PageDTO;
import com.appsmith.server.exceptions.AppsmithError;
Expand Down Expand Up @@ -309,12 +310,16 @@ public Mono<List<String>> forkApplications(
actionDTO.setSource(
ActionCreationSourceTypeEnum
.FORK_APPLICATION);
return layoutActionService.createAction(
actionDTO,

CreateActionMetaDTO createActionMetaDTO =
new CreateActionMetaDTO();
createActionMetaDTO.setIsJsAction(Boolean.FALSE);
createActionMetaDTO.setEventContext(
new AppsmithEventContext(
AppsmithEventContextType
.CLONE_PAGE),
Boolean.FALSE);
.CLONE_PAGE));
return layoutActionService.createAction(
actionDTO, createActionMetaDTO);
})
.map(ActionDTO::getId),
Mono.justOrEmpty(originalActionId));
Expand Down
6 changes: 5 additions & 1 deletion .../src/main/java/com/appsmith/server/newactions/clonepage/ActionClonePageServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.appsmith.server.clonepage.ClonePageServiceCE;
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.dtos.ClonePageMetaDTO;
import com.appsmith.server.dtos.CreateActionMetaDTO;
import com.appsmith.server.newactions.base.NewActionService;
import com.appsmith.server.services.LayoutActionService;
import com.appsmith.server.solutions.ActionPermission;
Expand Down Expand Up @@ -54,7 +55,10 @@ public Mono<Void> cloneEntities(ClonePageMetaDTO clonePageMetaDTO) {
// Indicates that source of action creation is clone page action
cloneActionDTO.setSource(ActionCreationSourceTypeEnum.CLONE_PAGE);
copyNestedNonNullProperties(actionDTO, cloneActionDTO);
return layoutActionService.createAction(cloneActionDTO, eventContext, isJsAction);
CreateActionMetaDTO createActionMetaDTO = new CreateActionMetaDTO();
createActionMetaDTO.setIsJsAction(isJsAction);
createActionMetaDTO.setEventContext(eventContext);
return layoutActionService.createAction(cloneActionDTO, createActionMetaDTO);
})
.then();
}
Expand Down
4 changes: 2 additions & 2 deletions .../appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.appsmith.server.services.ce;

import com.appsmith.external.helpers.AppsmithEventContext;
import com.appsmith.external.models.ActionDTO;
import com.appsmith.server.dtos.ActionMoveDTO;
import com.appsmith.server.dtos.CreateActionMetaDTO;
import reactor.core.publisher.Mono;

public interface LayoutActionServiceCE {
Expand All @@ -23,7 +23,7 @@ public interface LayoutActionServiceCE {

Mono<ActionDTO> createSingleAction(ActionDTO actionDTO, Boolean isJsAction);

Mono<ActionDTO> createAction(ActionDTO actionDTO, AppsmithEventContext eventContext, Boolean isJsAction);
Mono<ActionDTO> createAction(ActionDTO actionDTO, CreateActionMetaDTO actionMetaDTO);

Mono<ActionDTO> deleteUnpublishedAction(String id);
}
46 changes: 28 additions & 18 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 @@ -13,6 +13,7 @@
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.domains.NewPage;
import com.appsmith.server.dtos.ActionMoveDTO;
import com.appsmith.server.dtos.CreateActionMetaDTO;
import com.appsmith.server.dtos.PageDTO;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
Expand Down Expand Up @@ -337,25 +338,29 @@ public Mono<ActionDTO> createSingleAction(ActionDTO actionDTO, Boolean isJsActio
if (!StringUtils.hasLength(actionDTO.getPageId())) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.PAGE_ID));
}

AclPermission aclPermission =
isJsAction ? pagePermission.getReadPermission() : pagePermission.getActionCreatePermission();

return newPageService
.findById(actionDTO.getPageId(), pagePermission.getActionCreatePermission())
.findById(actionDTO.getPageId(), aclPermission)
.switchIfEmpty(Mono.error(
new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, actionDTO.getPageId())))
.flatMap(newPage -> {
actionDTO.setBranchName(newPage.getBranchName());
return createAction(actionDTO, isJsAction);
AppsmithEventContext eventContext = new AppsmithEventContext(AppsmithEventContextType.DEFAULT);
CreateActionMetaDTO createActionMetaDTO = new CreateActionMetaDTO();
createActionMetaDTO.setIsJsAction(isJsAction);
createActionMetaDTO.setNewPage(newPage);
createActionMetaDTO.setEventContext(eventContext);
return createAction(actionDTO, createActionMetaDTO);
});
}

protected Mono<ActionDTO> createAction(ActionDTO actionDTO, Boolean isJsAction) {
AppsmithEventContext eventContext = new AppsmithEventContext(AppsmithEventContextType.DEFAULT);
return createAction(actionDTO, eventContext, isJsAction);
}

@Override
public Mono<ActionDTO> createAction(ActionDTO actionDTO, AppsmithEventContext eventContext, Boolean isJsAction) {

return validateAndGenerateActionDomainBasedOnContext(actionDTO, isJsAction)
public Mono<ActionDTO> createAction(ActionDTO actionDTO, CreateActionMetaDTO actionMetaDTO) {
AppsmithEventContext eventContext = actionMetaDTO.getEventContext();
return validateAndGenerateActionDomainBasedOnContext(actionDTO, actionMetaDTO)
.name(VALIDATE_AND_GENERATE_ACTION_DOMAIN_BASED_ON_CONTEXT)
.tap(Micrometer.observation(observationRegistry))
.flatMap(newAction -> {
Expand Down Expand Up @@ -412,7 +417,10 @@ public Mono<ActionDTO> createAction(ActionDTO actionDTO, AppsmithEventContext ev
});
}

protected Mono<NewAction> validateAndGenerateActionDomainBasedOnContext(ActionDTO action, boolean isJsAction) {
protected Mono<NewAction> validateAndGenerateActionDomainBasedOnContext(
ActionDTO action, CreateActionMetaDTO actionMetaDTO) {
Boolean isJsAction = actionMetaDTO.getIsJsAction();
NewPage newPage = actionMetaDTO.getNewPage();
if (!StringUtils.hasLength(action.getPageId())) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.PAGE_ID));
}
Expand All @@ -421,13 +429,15 @@ protected Mono<NewAction> validateAndGenerateActionDomainBasedOnContext(ActionDT
AclPermission aclPermission =
isJsAction ? pagePermission.getReadPermission() : pagePermission.getActionCreatePermission();

Mono<NewPage> pageMono = newPageService
.findById(action.getPageId(), aclPermission)
.name(GET_PAGE_BY_ID)
.tap(Micrometer.observation(observationRegistry))
.switchIfEmpty(Mono.error(
new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, action.getPageId())))
.cache();
Mono<NewPage> pageMono = newPage != null
? Mono.just(newPage)
: newPageService
.findById(action.getPageId(), aclPermission)
.name(GET_PAGE_BY_ID)
.tap(Micrometer.observation(observationRegistry))
.switchIfEmpty(Mono.error(new AppsmithException(
AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, action.getPageId())))
.cache();

final NewAction newAction = newActionService.generateActionDomain(action);

Expand Down
51 changes: 50 additions & 1 deletion ...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 @@ -94,7 +94,7 @@ public class ActionCollectionServiceTest {
@Autowired
ApplicationPageService applicationPageService;

@Autowired
@SpyBean
LayoutActionService layoutActionService;

@Autowired
Expand Down Expand Up @@ -967,4 +967,53 @@ public void testLayoutOnLoadActions_withTwoWidgetsAndSameBinding_callsCorrectAct
})
.verifyComplete();
}

@Test
@WithUserDetails(value = "api_user")
public void testUpdateUnpublishedActionCollection_createSingleAction_fetchesPageFromDBOnlyOnce() {
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("export default {\n\t\n}");
actionCollectionDTO.setPluginType(PluginType.JS);

// 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 action1 = new ActionDTO();
action1.setName("testAction1");
action1.setActionConfiguration(new ActionConfiguration());
action1.getActionConfiguration().setBody("mockBody");
action1.getActionConfiguration().setIsValid(false);

actionCollectionDTO.setActions(List.of(action1));

final Mono<ActionCollectionDTO> updatedActionCollectionDTO =
layoutCollectionService.updateUnpublishedActionCollection(
createdActionCollectionId, actionCollectionDTO);

StepVerifier.create(updatedActionCollectionDTO)
.assertNext(actionCollectionDTO1 -> {
assertEquals(createdActionCollectionId, actionCollectionDTO1.getId());
Mockito.verify(layoutActionService, Mockito.times(1))
.createAction(
Mockito.any(),
Mockito.argThat(createActionMetaDTO ->
createActionMetaDTO != null && createActionMetaDTO.getNewPage() != null));
})
.verifyComplete();
}
}
6 changes: 5 additions & 1 deletion ...r/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.appsmith.server.dtos.ActionViewDTO;
import com.appsmith.server.dtos.ApplicationAccessDTO;
import com.appsmith.server.dtos.ApplicationJson;
import com.appsmith.server.dtos.CreateActionMetaDTO;
import com.appsmith.server.dtos.LayoutDTO;
import com.appsmith.server.dtos.PageDTO;
import com.appsmith.server.exceptions.AppsmithError;
Expand Down Expand Up @@ -1180,7 +1181,10 @@ public void testExecuteOnLoadParamOnActionCreateWithClonePageContext() {
action.setDatasource(datasource);

AppsmithEventContext eventContext = new AppsmithEventContext(AppsmithEventContextType.CLONE_PAGE);
Mono<ActionDTO> actionMono = layoutActionService.createAction(action, eventContext, Boolean.FALSE);
CreateActionMetaDTO createActionMetaDTO = new CreateActionMetaDTO();
createActionMetaDTO.setEventContext(eventContext);
createActionMetaDTO.setIsJsAction(Boolean.FALSE);
Mono<ActionDTO> actionMono = layoutActionService.createAction(action, createActionMetaDTO);
StepVerifier.create(actionMono)
.assertNext(createdAction -> {
// executeOnLoad is expected to be set to false in case of default context
Expand Down