From 04b4b870691dc7c2919f2d6b607a3c5faa75ba15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Tue, 29 Oct 2024 18:59:33 +0530 Subject: [PATCH 1/4] fix: skipped unnecessary code for js object updates --- .../ce/LayoutActionServiceCEImpl.java | 64 ++++++++++--------- .../services/ActionCollectionServiceTest.java | 9 ++- 2 files changed, 42 insertions(+), 31 deletions(-) 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..9828f02091c 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 @@ -94,7 +94,7 @@ public class ActionCollectionServiceTest { @Autowired RefactoringService refactoringService; - @Autowired + @SpyBean NewPageService newPageService; @Autowired @@ -766,6 +766,13 @@ public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL // collection as expected Mockito.verify(updateLayoutService, Mockito.times(2)) .updatePageLayoutsByPageId(Mockito.anyString()); + + // This gets called 4 times: + // 2 times during createCollection -> a. getExistingEntityNames and b. updatePageLayoutsByPageId + // 1 time during setup of this test case + // 1 time during update action collection + Mockito.verify(newPageService, Mockito.times(4)) + .findPageById(Mockito.anyString(), Mockito.any(), Mockito.anyBoolean()); }) .verifyComplete(); } From cdd2e380b136b092b830f2b5bbf16bce13ef0b63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Wed, 30 Oct 2024 12:24:19 +0530 Subject: [PATCH 2/4] fix: updated test case name and comments --- .../server/services/ActionCollectionServiceTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 9828f02091c..5907009c756 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 @@ -708,7 +708,7 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle @Test @WithUserDetails(value = "api_user") - public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutOnlyOnce() { + public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutAndFindPageByIdOnlyOnce() { 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<>()))); @@ -770,7 +770,8 @@ public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL // This gets called 4 times: // 2 times during createCollection -> a. getExistingEntityNames and b. updatePageLayoutsByPageId // 1 time during setup of this test case - // 1 time during update action collection + // 1 time during update action collection -> As expected as find page by id should be called only + // once in updatePageLayoutsByPageId Mockito.verify(newPageService, Mockito.times(4)) .findPageById(Mockito.anyString(), Mockito.any(), Mockito.anyBoolean()); }) From 0b1976458906c5589d0943c2ba72363c18ee4085 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Tue, 5 Nov 2024 13:00:47 +0530 Subject: [PATCH 3/4] fix: test case added to assert error reports --- .../services/ActionCollectionServiceTest.java | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) 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 5907009c756..7a9e9d93725 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; @@ -94,7 +95,7 @@ public class ActionCollectionServiceTest { @Autowired RefactoringService refactoringService; - @SpyBean + @Autowired NewPageService newPageService; @Autowired @@ -708,7 +709,8 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle @Test @WithUserDetails(value = "api_user") - public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutAndFindPageByIdOnlyOnce() { + 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 +729,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 +746,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( @@ -767,13 +789,10 @@ public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL Mockito.verify(updateLayoutService, Mockito.times(2)) .updatePageLayoutsByPageId(Mockito.anyString()); - // This gets called 4 times: - // 2 times during createCollection -> a. getExistingEntityNames and b. updatePageLayoutsByPageId - // 1 time during setup of this test case - // 1 time during update action collection -> As expected as find page by id should be called only - // once in updatePageLayoutsByPageId - Mockito.verify(newPageService, Mockito.times(4)) - .findPageById(Mockito.anyString(), Mockito.any(), Mockito.anyBoolean()); + assertEquals(1, actionCollectionDTO1.getErrorReports().size()); + assertEquals( + AppsmithError.CYCLICAL_DEPENDENCY_ERROR.getAppErrorCode(), + actionCollectionDTO1.getErrorReports().get(0).getCode()); }) .verifyComplete(); } From 9fbde996545451c1a12736e0b8a9252ad4ab69bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Csneha122=E2=80=9D?= <“sneha@appsmith.com”> Date: Wed, 6 Nov 2024 11:47:49 +0530 Subject: [PATCH 4/4] fix: code review comments --- .../server/services/ActionCollectionServiceTest.java | 8 ++++++++ 1 file changed, 8 insertions(+) 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 7a9e9d93725..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 @@ -74,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 @@ -793,6 +794,13 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle 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(); }