From 7306967d0b4e15e7c59a124cce0bc7e0447e8ecb Mon Sep 17 00:00:00 2001 From: Sumit Kumar Date: Tue, 11 Jan 2022 21:43:54 +0530 Subject: [PATCH] fix: fix page load action execution order (#10252) * This PR fixes the page load action execution order when the actions have been set to run on page load explicitly via the settings tab by the user and its data has not been referenced in any other widget or action. e.g. - create action1 and action2. - make action2 dependent on action1 by adding {{action1.data}} in action2's body. - set both action1 and action2 to run on page load via settings tab. Do not reference action1 and action2 data in any other widget or action. --- .../services/ce/NewActionServiceCEImpl.java | 2 +- .../ce/PageLoadActionsUtilCEImpl.java | 30 +++- .../services/LayoutActionServiceTest.java | 134 +++++++++++++++++- 3 files changed, 157 insertions(+), 9 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java index 654f12d638..1237930661 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java @@ -210,7 +210,7 @@ public class NewActionServiceCEImpl extends BaseService actionNames = tuple.getT1(); DirectedAcyclicGraph graph = tuple.getT2(); - return computeOnPageLoadActionsSchedulingOrder(graph, onPageLoadActionSet, actionNames); + return computeOnPageLoadActionsSchedulingOrder(graph, onPageLoadActionSet, actionNames, + explicitUserSetOnLoadActions); }) .map(onPageLoadActionsSchedulingOrder -> { // Find all explicitly turned on actions which haven't found their way into the scheduling order @@ -517,13 +518,15 @@ public class PageLoadActionsUtilCEImpl implements PageLoadActionsUtilCE { * Breadth First level by level traversal is used to compute each set of such independent actions. * * @param dag : The DAG graph containing all the edges representing dependencies between appsmith entities in the page. + * @param pageLoadActionSet * @param onPageLoadActionSet * @param actionNames : All the action names for the page * @return */ private List> computeOnPageLoadActionsSchedulingOrder(DirectedAcyclicGraph dag, Set onPageLoadActionSet, - Set actionNames) { + Set actionNames, + Set explicitUserSetOnLoadActions) { Map pageLoadActionAndLevelMap = new HashMap<>(); List> onPageLoadActions = new ArrayList<>(); @@ -544,7 +547,8 @@ public class PageLoadActionsUtilCEImpl implements PageLoadActionsUtilCE { onPageLoadActions.add(new HashSet<>()); } - Set actionsFromBinding = actionCandidatesForPageLoadFromBinding(actionNames, vertex, pageLoadActionAndLevelMap, onPageLoadActions); + Set actionsFromBinding = actionCandidatesForPageLoadFromBinding(actionNames, vertex, + pageLoadActionAndLevelMap, onPageLoadActions, explicitUserSetOnLoadActions); onPageLoadActions.get(level).addAll(actionsFromBinding); for (String action : actionsFromBinding) { pageLoadActionAndLevelMap.put(action, level); @@ -908,7 +912,8 @@ public class PageLoadActionsUtilCEImpl implements PageLoadActionsUtilCE { private Set actionCandidatesForPageLoadFromBinding(Set allActionNames, String vertex, Map pageLoadActionsLevelMap, - List> existingPageLoadActions) { + List> existingPageLoadActions, + Set explicitUserSetOnLoadActions) { Set onPageLoadCandidates = new HashSet<>(); @@ -921,10 +926,21 @@ public class PageLoadActionsUtilCEImpl implements PageLoadActionsUtilCE { Boolean isCandidateForPageLoad = TRUE; - // Only add it for page load if it is not a function call. Aka the data - // of this call is being referred to in the binding. + /** + * Add action for page load if: + * o it has been explicitly set to run on page load by the user (even if its data is not + * referenced in any widget or action) + * o or, it is not a function call i.e. the data of this call is being referred to in the binding. + */ + + String validBinding; + if (explicitUserSetOnLoadActions.contains(entity)) { + validBinding = entity + "." + "actionConfiguration"; + } + else { + validBinding = entity + "." + "data"; + } - String validBinding = entity + "." + "data"; if (!vertex.contains(validBinding)) { isCandidateForPageLoad = FALSE; } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java index 67b3c35512..11fa07073d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java @@ -48,10 +48,12 @@ import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.boot.test.mock.mockito.SpyBean; import org.springframework.http.HttpMethod; import org.springframework.security.test.context.support.WithUserDetails; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringRunner; +import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import reactor.util.function.Tuple2; @@ -77,7 +79,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; @Slf4j @DirtiesContext public class LayoutActionServiceTest { - @Autowired + @SpyBean NewActionService newActionService; @Autowired @@ -1384,4 +1386,134 @@ public class LayoutActionServiceTest { }) .verifyComplete(); } + + /** + * This method tests the following case: + * o create action1 and action2. + * o make action2 dependent on action1 by adding {{action1.data}} in action2's body. + * o set both action1 and action2 to run on page load via settings tab. Do not reference action1 and action2 data + * in any other widget or action. + * @throws JsonProcessingException + */ + @Test + @WithUserDetails(value = "api_user") + public void testExecuteOnPageLoadOrderWhenAllActionsAreOnlyExplicitlySetToExecute() throws JsonProcessingException { + Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor())); + + // Configure action1 + ActionDTO action1 = new ActionDTO(); + action1.setName("firstAction"); + action1.setPageId(testPage.getId()); + ActionConfiguration actionConfiguration1 = new ActionConfiguration(); + actionConfiguration1.setHttpMethod(HttpMethod.GET); + action1.setActionConfiguration(actionConfiguration1); + action1.setExecuteOnLoad(true); + action1.setDatasource(datasource); + + // Configure action2 + ActionDTO action2 = new ActionDTO(); + action2.setName("secondAction"); + action2.setPageId(testPage.getId()); + ActionConfiguration actionConfiguration2 = new ActionConfiguration(); + actionConfiguration2.setHttpMethod(HttpMethod.GET); + actionConfiguration2.setBody("{{ firstAction.data }}"); // make action2 dependent on action1 + action2.setActionConfiguration(actionConfiguration2); + action2.setDynamicBindingPathList(List.of(new Property("body", null))); + action2.setExecuteOnLoad(true); + action2.setDatasource(datasource); + + JSONObject parentDsl = new JSONObject(objectMapper.readValue(DEFAULT_PAGE_LAYOUT, new TypeReference>() { + })); + Layout layout = testPage.getLayouts().get(0); + layout.setDsl(parentDsl); + + ActionDTO createdAction1 = layoutActionService.createSingleAction(action1).block(); // create action1 + createdAction1.setExecuteOnLoad(true); // this can only be set to true post action creation. + NewAction newAction1 = new NewAction(); + newAction1.setUnpublishedAction(createdAction1); + newAction1.setDefaultResources(createdAction1.getDefaultResources()); + + ActionDTO createdAction2 = layoutActionService.createSingleAction(action2).block(); // create action2 + createdAction2.setExecuteOnLoad(true); // this can only be set to true post action creation. + NewAction newAction2 = new NewAction(); + newAction2.setUnpublishedAction(createdAction2); + newAction2.setDefaultResources(createdAction2.getDefaultResources()); + + NewAction[] newActionArray = new NewAction[2]; + newActionArray[0] = newAction1; + newActionArray[1] = newAction2; + Flux newActionFlux = Flux.fromArray(newActionArray); + Mockito.when(newActionService.findUnpublishedOnLoadActionsExplicitSetByUserInPage(Mockito.any())).thenReturn(newActionFlux); + + Mono updateLayoutMono = layoutActionService.updateLayout(testPage.getId(), layout.getId(), layout); + + StepVerifier.create(updateLayoutMono) + .assertNext(updatedLayout -> { + + assertThat(updatedLayout.getLayoutOnLoadActions().size()).isEqualTo(2); + + // Assert that both the actions don't belong to the same set. They should be run iteratively. + DslActionDTO actionDTO1 = updatedLayout.getLayoutOnLoadActions().get(0).iterator().next(); + assertThat(actionDTO1.getName()).isEqualTo("firstAction"); + + DslActionDTO actionDTO2 = updatedLayout.getLayoutOnLoadActions().get(1).iterator().next(); + assertThat(actionDTO2.getName()).isEqualTo("secondAction"); + + }) + .verifyComplete(); + } + + /** + * This method tests the following scenario: + * o create `action1`. + * o set `action1` to run on page load via settings tab. + * o bind `{{action1.data}}` in one of the widget fields. + */ + @Test + @WithUserDetails(value = "api_user") + public void testPageLoadActionWhenSetBothWaysExplicitlyAndImplicitlyViaWidget() { + Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor())); + + ActionDTO action1 = new ActionDTO(); + action1.setName("firstAction"); + action1.setPageId(testPage.getId()); + ActionConfiguration actionConfiguration1 = new ActionConfiguration(); + actionConfiguration1.setHttpMethod(HttpMethod.GET); + action1.setActionConfiguration(actionConfiguration1); + action1.setDatasource(datasource); + + JSONObject dsl = new JSONObject(); + dsl.put("widgetName", "firstWidget"); + JSONArray temp = new JSONArray(); + temp.addAll(List.of(new JSONObject(Map.of("key", "testField")))); + dsl.put("dynamicBindingPathList", temp); + dsl.put("testField", "{{ firstAction.data }}"); + + Layout layout = testPage.getLayouts().get(0); + layout.setDsl(dsl); + + ActionDTO createdAction1 = layoutActionService.createSingleAction(action1).block(); + createdAction1.setExecuteOnLoad(true); // this can only be set to true post action creation. + createdAction1.setUserSetOnLoad(true); + NewAction newAction1 = new NewAction(); + newAction1.setUnpublishedAction(createdAction1); + newAction1.setDefaultResources(createdAction1.getDefaultResources()); + + NewAction[] newActionArray = new NewAction[1]; + newActionArray[0] = newAction1; + Flux newActionFlux = Flux.fromArray(newActionArray); + Mockito.when(newActionService.findUnpublishedOnLoadActionsExplicitSetByUserInPage(Mockito.any())).thenReturn(newActionFlux); + + Mono updateLayoutMono = layoutActionService.updateLayout(testPage.getId(), layout.getId(), layout); + + StepVerifier.create(updateLayoutMono) + .assertNext(updatedLayout -> { + assertThat(updatedLayout.getLayoutOnLoadActions().size()).isEqualTo(1); + + // Assert that both the actions don't belong to the same set. They should be run iteratively. + DslActionDTO actionDTO1 = updatedLayout.getLayoutOnLoadActions().get(0).iterator().next(); + assertThat(actionDTO1.getName()).isEqualTo("firstAction"); + }) + .verifyComplete(); + } }