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 a68913540f9..0efe0e9e4f3 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 @@ -249,36 +249,40 @@ protected Mono updateActionBasedOnContextType(NewAction newAction, Ac String pageId = newAction.getUnpublishedAction().getPageId(); action.setApplicationId(null); action.setPageId(null); - return updateSingleAction(newAction.getId(), action) - .name(UPDATE_SINGLE_ACTION) - .tap(Micrometer.observation(observationRegistry)) - .flatMap(updatedAction -> { - // Update page layout is skipped for JS actions here because when JSobject is updated, we first - // update all actions, action - // collection and then we update the page layout, hence updating page layout with each action update - // is not required here - if (action.getPluginType() != PluginType.JS) { - return updateLayoutService - .updatePageLayoutsByPageId(pageId) - .name(UPDATE_PAGE_LAYOUT_BY_PAGE_ID) - .tap(Micrometer.observation(observationRegistry)) - .thenReturn(updatedAction); - } - return Mono.just(updatedAction); - }) - .zipWhen(actionDTO -> newPageService.findPageById(pageId, pagePermission.getEditPermission(), false)) - .map(tuple2 -> { - ActionDTO actionDTO = tuple2.getT1(); - PageDTO pageDTO = tuple2.getT2(); - // redundant check - if (pageDTO.getLayouts().size() > 0) { - actionDTO.setErrorReports(pageDTO.getLayouts().get(0).getLayoutOnLoadActionErrors()); - } - log.debug( - "Update action based on context type completed, returning actionDTO with action id: {}", - actionDTO != null ? actionDTO.getId() : null); - return actionDTO; - }); + + // Update page layout is skipped for JS actions here because when JSobject is updated, we first + // update all actions, action + // collection and then we update the page layout, hence updating page layout with each action update + // is not required here + if (action.getPluginType() == PluginType.JS) { + return updateSingleAction(newAction.getId(), action) + .name(UPDATE_SINGLE_ACTION) + .tap(Micrometer.observation(observationRegistry)); + } else { + return updateSingleAction(newAction.getId(), action) + .name(UPDATE_SINGLE_ACTION) + .tap(Micrometer.observation(observationRegistry)) + .flatMap(updatedAction -> updateLayoutService + .updatePageLayoutsByPageId(pageId) + .name(UPDATE_PAGE_LAYOUT_BY_PAGE_ID) + .tap(Micrometer.observation(observationRegistry)) + .thenReturn(updatedAction)) + .zipWhen( + actionDTO -> newPageService.findPageById(pageId, pagePermission.getEditPermission(), false)) + .map(tuple2 -> { + ActionDTO actionDTO = tuple2.getT1(); + PageDTO pageDTO = tuple2.getT2(); + // redundant check + if (pageDTO.getLayouts().size() > 0) { + actionDTO.setErrorReports( + pageDTO.getLayouts().get(0).getLayoutOnLoadActionErrors()); + } + log.debug( + "Update action based on context type completed, returning actionDTO with action id: {}", + actionDTO != null ? actionDTO.getId() : null); + return actionDTO; + }); + } } @Override 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 f201b0a3bff..c21a0f49589 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 @@ -26,6 +26,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.helpers.MockPluginExecutor; import com.appsmith.server.helpers.PluginExecutorHelper; import com.appsmith.server.layouts.UpdateLayoutService; @@ -73,6 +74,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; @SpringBootTest @Slf4j @@ -708,7 +710,8 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle @Test @WithUserDetails(value = "api_user") - public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutOnlyOnce() { + public void + testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutOnlyOnceAndAssertCyclicDependencyError() { 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<>()))); @@ -727,7 +730,7 @@ public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL ActionDTO action1 = new ActionDTO(); action1.setName("testAction1"); action1.setActionConfiguration(new ActionConfiguration()); - action1.getActionConfiguration().setBody("mockBody"); + action1.getActionConfiguration().setBody("initial body"); action1.getActionConfiguration().setIsValid(false); ActionDTO action2 = new ActionDTO(); @@ -744,15 +747,35 @@ public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL actionCollectionDTO.setActions(List.of(action1, action2, action3)); + Layout layout = testPage.getLayouts().get(0); + ArrayList dslList = (ArrayList) layout.getDsl().get("children"); + JSONObject tableDsl = (JSONObject) dslList.get(0); + tableDsl.put("tableData", "{{testCollection1.testAction1.data}}"); + JSONArray temp2 = new JSONArray(); + temp2.add(new JSONObject(Map.of("key", "tableData"))); + tableDsl.put("dynamicBindingPathList", temp2); + JSONArray temp3 = new JSONArray(); + temp3.add(new JSONObject(Map.of("key", "tableData"))); + tableDsl.put("dynamicPropertyPathList", temp3); + layout.getDsl().put("widgetName", "MainContainer"); + + testPage.setLayouts(List.of(layout)); + PageDTO updatedPage = + newPageService.updatePage(testPage.getId(), testPage).block(); + + // Create Js object ActionCollectionDTO createdActionCollectionDTO = layoutCollectionService.createCollection(actionCollectionDTO).block(); assert createdActionCollectionDTO != null; assert createdActionCollectionDTO.getId() != null; String createdActionCollectionId = createdActionCollectionDTO.getId(); - applicationPageService.publish(testApp.getId(), true).block(); - - actionCollectionDTO.getActions().get(0).getActionConfiguration().setBody("updatedBody"); + // Update JS object to create cyclic dependency + actionCollectionDTO.getActions().stream() + .filter(action -> "testAction1".equals(action.getName())) + .findFirst() + .ifPresent(action -> + action.getActionConfiguration().setBody("function () {\n return Table1.tableData;\n}")); final Mono updatedActionCollectionDTO = layoutCollectionService.updateUnpublishedActionCollection( @@ -766,6 +789,18 @@ public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL // collection as expected Mockito.verify(updateLayoutService, Mockito.times(2)) .updatePageLayoutsByPageId(Mockito.anyString()); + + assertEquals(1, actionCollectionDTO1.getErrorReports().size()); + assertEquals( + AppsmithError.CYCLICAL_DEPENDENCY_ERROR.getAppErrorCode(), + actionCollectionDTO1.getErrorReports().get(0).getCode()); + + // Iterate over each action and assert that errorReports is null as action collection already has + // error reports + // it's not required in each action + actionCollectionDTO + .getActions() + .forEach(action -> assertNull(action.getErrorReports(), "Error reports should be null")); }) .verifyComplete(); }