From 4276fd9b592fae8ce2a2a3b5ae276db0fba421c9 Mon Sep 17 00:00:00 2001 From: rishabhrathod01 Date: Fri, 18 Oct 2024 03:28:26 +0530 Subject: [PATCH 1/6] fix: resolve redundant JSObject action saving --- .../server/services/ce/LayoutCollectionServiceCEImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java index d4cc9fcbdf65..5eaa0ae89924 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java @@ -317,6 +317,7 @@ public Mono updateUnpublishedActionCollection( final Mono> newValidActionIdsMono = branchedActionCollectionMono.flatMap( branchedActionCollection -> Flux.fromIterable(actionCollectionDTO.getActions()) + .distinct(actionDTO -> actionCollectionDTO.getName() + "." + actionDTO.getName()) .flatMap(actionDTO -> { actionDTO.setDeletedAt(null); setContextId(branchedActionCollection, actionDTO); From d89f9d7a3eddad6e2e21cb209265ad795992baf2 Mon Sep 17 00:00:00 2001 From: rishabhrathod01 Date: Sun, 20 Oct 2024 13:25:21 +0530 Subject: [PATCH 2/6] avoid overriding action body if it already exist --- app/client/src/ce/entities/DataTree/dataTreeJSAction.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts b/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts index e7bc3ec36bdd..450aed4857f0 100644 --- a/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts +++ b/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts @@ -55,6 +55,9 @@ export const generateDataTreeJSAction = ( for (let i = 0; i < actions.length; i++) { const action = actions[i]; + // If action already exists, skip adding it to the data tree + if (actionsData[action.name]) continue; + meta[action.name] = { arguments: action.actionConfiguration?.jsArguments || [], confirmBeforeExecute: !!action.confirmBeforeExecute, From c8be91e3281e913fafc3cadf2ac24218ea021d22 Mon Sep 17 00:00:00 2001 From: rishabhrathod01 Date: Mon, 21 Oct 2024 14:11:13 +0530 Subject: [PATCH 3/6] avoid function getting overridden by the duplicate one --- .../ce/entities/DataTree/dataTreeJSAction.ts | 3 --- .../src/workers/Evaluation/JSObject/index.ts | 26 ++++++++++--------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts b/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts index 450aed4857f0..e7bc3ec36bdd 100644 --- a/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts +++ b/app/client/src/ce/entities/DataTree/dataTreeJSAction.ts @@ -55,9 +55,6 @@ export const generateDataTreeJSAction = ( for (let i = 0; i < actions.length; i++) { const action = actions[i]; - // If action already exists, skip adding it to the data tree - if (actionsData[action.name]) continue; - meta[action.name] = { arguments: action.actionConfiguration?.jsArguments || [], confirmBeforeExecute: !!action.confirmBeforeExecute, diff --git a/app/client/src/workers/Evaluation/JSObject/index.ts b/app/client/src/workers/Evaluation/JSObject/index.ts index 4c9c6b35f785..286ee16df3fe 100644 --- a/app/client/src/workers/Evaluation/JSObject/index.ts +++ b/app/client/src/workers/Evaluation/JSObject/index.ts @@ -21,6 +21,9 @@ import JSObjectCollection from "./Collection"; import ExecutionMetaData from "../fns/utils/ExecutionMetaData"; import { jsPropertiesState } from "./jsPropertiesState"; import { getFixedTimeDifference } from "workers/common/DataTreeEvaluator/utils"; +interface ParseJSAction extends ParsedJSSubAction { + parsedFunction: unknown; +} /** * Here we update our unEvalTree according to the change in JSObject's body @@ -114,18 +117,17 @@ export function saveResolvedFunctionsAndJSUpdates( JSObjectName: entityName, JSObjectASTParseTime, }); - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const actions: any = []; - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const variables: any = []; + + const actionsMap: Record = {}; + const variablesMap: Record = {}; if (success) { if (!!parsedObject) { jsPropertiesState.update(entityName, parsedObject); parsedObject.forEach((parsedElement) => { if (isJSFunctionProperty(parsedElement)) { + if (actionsMap[parsedElement.key]) return; + try { ExecutionMetaData.setExecutionMetaData({ enableJSVarUpdateTracking: false, @@ -164,12 +166,12 @@ export function saveResolvedFunctionsAndJSUpdates( `${entityName}.${parsedElement.key}`, functionString, ); - actions.push({ + actionsMap[parsedElement.key] = { name: parsedElement.key, body: functionString, arguments: params, parsedFunction: result, - }); + }; } } catch { // in case we need to handle error state @@ -184,10 +186,10 @@ export function saveResolvedFunctionsAndJSUpdates( ? parsedElement.key.slice(1, -1) : parsedElement.key; - variables.push({ + variablesMap[parsedKey] = { name: parsedKey, value: parsedElement.value, - }); + }; JSObjectCollection.updateUnEvalState( `${entityName}.${parsedElement.key}`, parsedElement.value, @@ -196,8 +198,8 @@ export function saveResolvedFunctionsAndJSUpdates( }); const parsedBody = { body: entity.body, - actions: actions, - variables, + actions: Object.values(actionsMap), + variables: Object.values(variablesMap), }; set(jsUpdates, `${entityName}`, { From d5c1e64808f68e49e1ad1bf9f61649081af95f9c Mon Sep 17 00:00:00 2001 From: rishabhrathod01 Date: Tue, 22 Oct 2024 15:13:28 +0530 Subject: [PATCH 4/6] add unit test --- .../ActionCollectionServiceImplTest.java | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java index 43b0d91a0850..3029799f49d0 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java @@ -411,6 +411,57 @@ public void testUpdateUnpublishedActionCollection_withInvalidId_throwsError() th .verify(); } + @Test + public void testUpdateUnpublishedActionCollection_withDuplicateActions() throws IOException { + ActionCollectionDTO actionCollectionDTO = new ActionCollectionDTO(); + actionCollectionDTO.setId("testId"); + actionCollectionDTO.setPageId("testPageId"); + actionCollectionDTO.setApplicationId("testApplicationId"); + actionCollectionDTO.setWorkspaceId("testWorkspaceId"); + actionCollectionDTO.setPluginId("testPluginId"); + actionCollectionDTO.setPluginType(PluginType.JS); + + ObjectMapper objectMapper = new ObjectMapper(); + final JsonNode jsonNode = objectMapper.readValue(mockObjects, JsonNode.class); + final NewPage newPage = objectMapper.convertValue(jsonNode.get("newPage"), NewPage.class); + + Mockito.when(actionCollectionRepository.findById(Mockito.anyString(), Mockito.any())) + .thenReturn(Mono.empty()); + + Mockito.when(newPageService.findByBranchNameAndBasePageId( + Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())) + .thenReturn(Mono.just(newPage)); + + Mockito.when(newPageService.findById(Mockito.any(), Mockito.any())) + .thenReturn(Mono.just(newPage)); + + Mockito.when(newActionService.findByCollectionIdAndViewMode( + Mockito.anyString(), Mockito.anyBoolean(), Mockito.any())) + .thenReturn(Flux.empty()); + + ActionDTO action = new ActionDTO(); + action.setName("testAction"); + action.setClientSideExecution(true); + actionCollectionDTO.setActions(List.of(action)); + action.setName("testAction"); + action.setClientSideExecution(true); + actionCollectionDTO.setActions(List.of(action)); + + final Mono actionCollectionDTOMono = + layoutCollectionService.updateUnpublishedActionCollection("testId", actionCollectionDTO); + + // verify that actionCollectionDTOMono has only one action and the duplicate action was ignored + StepVerifier.create(actionCollectionDTOMono) + .assertNext(actionCollectionDTO1 -> { + assertEquals(1, actionCollectionDTO1.getActions().size()); + final ActionDTO actionDTO = + actionCollectionDTO1.getActions().get(0); + assertEquals("testAction", actionDTO.getName()); + assertTrue(actionDTO.getClientSideExecution()); + }) + .verifyComplete(); + } + @Test public void testDeleteUnpublishedActionCollection_withInvalidId_throwsError() { Mockito.when(actionCollectionRepository.findById(Mockito.any(), Mockito.any())) From cb60ee662c0fdb4c5bd9f22900b5de99b4c27fbc Mon Sep 17 00:00:00 2001 From: Rishabh Rathod Date: Wed, 23 Oct 2024 15:59:24 +0530 Subject: [PATCH 5/6] undo the server side changes --- .../ce/LayoutCollectionServiceCEImpl.java | 1 - .../ActionCollectionServiceImplTest.java | 51 ------------------- 2 files changed, 52 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java index 5eaa0ae89924..d4cc9fcbdf65 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java @@ -317,7 +317,6 @@ public Mono updateUnpublishedActionCollection( final Mono> newValidActionIdsMono = branchedActionCollectionMono.flatMap( branchedActionCollection -> Flux.fromIterable(actionCollectionDTO.getActions()) - .distinct(actionDTO -> actionCollectionDTO.getName() + "." + actionDTO.getName()) .flatMap(actionDTO -> { actionDTO.setDeletedAt(null); setContextId(branchedActionCollection, actionDTO); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java index 3029799f49d0..43b0d91a0850 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java @@ -411,57 +411,6 @@ public void testUpdateUnpublishedActionCollection_withInvalidId_throwsError() th .verify(); } - @Test - public void testUpdateUnpublishedActionCollection_withDuplicateActions() throws IOException { - ActionCollectionDTO actionCollectionDTO = new ActionCollectionDTO(); - actionCollectionDTO.setId("testId"); - actionCollectionDTO.setPageId("testPageId"); - actionCollectionDTO.setApplicationId("testApplicationId"); - actionCollectionDTO.setWorkspaceId("testWorkspaceId"); - actionCollectionDTO.setPluginId("testPluginId"); - actionCollectionDTO.setPluginType(PluginType.JS); - - ObjectMapper objectMapper = new ObjectMapper(); - final JsonNode jsonNode = objectMapper.readValue(mockObjects, JsonNode.class); - final NewPage newPage = objectMapper.convertValue(jsonNode.get("newPage"), NewPage.class); - - Mockito.when(actionCollectionRepository.findById(Mockito.anyString(), Mockito.any())) - .thenReturn(Mono.empty()); - - Mockito.when(newPageService.findByBranchNameAndBasePageId( - Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())) - .thenReturn(Mono.just(newPage)); - - Mockito.when(newPageService.findById(Mockito.any(), Mockito.any())) - .thenReturn(Mono.just(newPage)); - - Mockito.when(newActionService.findByCollectionIdAndViewMode( - Mockito.anyString(), Mockito.anyBoolean(), Mockito.any())) - .thenReturn(Flux.empty()); - - ActionDTO action = new ActionDTO(); - action.setName("testAction"); - action.setClientSideExecution(true); - actionCollectionDTO.setActions(List.of(action)); - action.setName("testAction"); - action.setClientSideExecution(true); - actionCollectionDTO.setActions(List.of(action)); - - final Mono actionCollectionDTOMono = - layoutCollectionService.updateUnpublishedActionCollection("testId", actionCollectionDTO); - - // verify that actionCollectionDTOMono has only one action and the duplicate action was ignored - StepVerifier.create(actionCollectionDTOMono) - .assertNext(actionCollectionDTO1 -> { - assertEquals(1, actionCollectionDTO1.getActions().size()); - final ActionDTO actionDTO = - actionCollectionDTO1.getActions().get(0); - assertEquals("testAction", actionDTO.getName()); - assertTrue(actionDTO.getClientSideExecution()); - }) - .verifyComplete(); - } - @Test public void testDeleteUnpublishedActionCollection_withInvalidId_throwsError() { Mockito.when(actionCollectionRepository.findById(Mockito.any(), Mockito.any())) From c12ee7007d8f860aa3bbf2cc1889bd4b762785a6 Mon Sep 17 00:00:00 2001 From: rishabhrathod01 Date: Wed, 23 Oct 2024 23:08:24 +0530 Subject: [PATCH 6/6] added unit test and removed parsedFunction as it is not used --- .../src/workers/Evaluation/JSObject/index.ts | 8 +- .../src/workers/Evaluation/JSObject/test.ts | 127 +++++++++++++++++- 2 files changed, 127 insertions(+), 8 deletions(-) diff --git a/app/client/src/workers/Evaluation/JSObject/index.ts b/app/client/src/workers/Evaluation/JSObject/index.ts index 286ee16df3fe..79dce10b39bb 100644 --- a/app/client/src/workers/Evaluation/JSObject/index.ts +++ b/app/client/src/workers/Evaluation/JSObject/index.ts @@ -21,9 +21,6 @@ import JSObjectCollection from "./Collection"; import ExecutionMetaData from "../fns/utils/ExecutionMetaData"; import { jsPropertiesState } from "./jsPropertiesState"; import { getFixedTimeDifference } from "workers/common/DataTreeEvaluator/utils"; -interface ParseJSAction extends ParsedJSSubAction { - parsedFunction: unknown; -} /** * Here we update our unEvalTree according to the change in JSObject's body @@ -118,7 +115,7 @@ export function saveResolvedFunctionsAndJSUpdates( JSObjectASTParseTime, }); - const actionsMap: Record = {}; + const actionsMap: Record = {}; const variablesMap: Record = {}; if (success) { @@ -170,7 +167,6 @@ export function saveResolvedFunctionsAndJSUpdates( name: parsedElement.key, body: functionString, arguments: params, - parsedFunction: result, }; } } catch { @@ -314,8 +310,6 @@ export function parseJSActions( parsedBody.actions = parsedBody.actions.map((action) => { return { ...action, - // parsedFunction - used only to determine if function is async - parsedFunction: undefined, } as ParsedJSSubAction; }); }); diff --git a/app/client/src/workers/Evaluation/JSObject/test.ts b/app/client/src/workers/Evaluation/JSObject/test.ts index 3a4cebe482d0..ccfb1c6c969c 100644 --- a/app/client/src/workers/Evaluation/JSObject/test.ts +++ b/app/client/src/workers/Evaluation/JSObject/test.ts @@ -1,5 +1,14 @@ import type { ConfigTree, UnEvalTree } from "entities/DataTree/dataTreeTypes"; -import { getUpdatedLocalUnEvalTreeAfterJSUpdates } from "."; +import { + getUpdatedLocalUnEvalTreeAfterJSUpdates, + saveResolvedFunctionsAndJSUpdates, +} from "."; +import type { JSUpdate } from "utils/JSPaneUtils"; +import type { + JSActionEntity, + JSActionEntityConfig, +} from "ee/entities/DataTree/types"; +import DataTreeEvaluator from "workers/common/DataTreeEvaluator"; describe("updateJSCollectionInUnEvalTree", function () { it("updates async value of jsAction", () => { @@ -137,3 +146,119 @@ describe("updateJSCollectionInUnEvalTree", function () { expect(expectedResult).toStrictEqual(actualResult); }); }); + +describe("saveResolvedFunctionsAndJSUpdates", function () { + it("parses JSObject with duplicate actions, variables and updates jsUpdates correctly", () => { + const dataTreeEvalRef = new DataTreeEvaluator({}); + const entity: JSActionEntity = { + actionId: "64013546b956c26882acc587", + body: "export default {\n\tmyVar1: [],\n\tmyVar1: [],\n\tmyVar2: {},\n\tmyFun1: () => {\n\t\t//write code here\n\t\t\n\t},\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}\n,\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}}", + ENTITY_TYPE: "JSACTION", + name: "JSObject1", + pluginType: "JS", + }; + const jsUpdates: Record = {}; + const unEvalDataTree: UnEvalTree = { + JSObject1: { + myVar1: "[]", + myVar2: "{}", + myFun1: new String("() => {}"), + myFun2: new String("async () => {\n yeso;\n}"), + body: "export default {\n\tmyVar1: [],\n\tmyVar1: [],\n\tmyVar2: {},\n\tmyFun1: () => {\n\t\t//write code here\n\t\t\n\t},\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}\n,\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}}", + ENTITY_TYPE: "JSACTION", + meta: { + myFun1: { + arguments: [], + confirmBeforeExecute: false, + }, + myFun2: { + arguments: [], + confirmBeforeExecute: false, + }, + }, + dependencyMap: { + body: ["myFun1", "myFun2"], + }, + dynamicBindingPathList: [ + { + key: "body", + }, + { + key: "myVar1", + }, + { + key: "myVar2", + }, + { + key: "myFun1", + }, + { + key: "myFun2", + }, + { + key: "myFun2", + }, + ], + bindingPaths: { + body: "SMART_SUBSTITUTE", + myVar1: "SMART_SUBSTITUTE", + myVar2: "SMART_SUBSTITUTE", + myFun1: "SMART_SUBSTITUTE", + myFun2: "SMART_SUBSTITUTE", + }, + reactivePaths: { + body: "SMART_SUBSTITUTE", + myVar1: "SMART_SUBSTITUTE", + myVar2: "SMART_SUBSTITUTE", + myFun1: "SMART_SUBSTITUTE", + myFun2: "SMART_SUBSTITUTE", + }, + pluginType: "JS", + name: "JSObject1", + actionId: "64013546b956c26882acc587", + } as JSActionEntityConfig, + }; + const entityName = "JSObject1"; + + const result = saveResolvedFunctionsAndJSUpdates( + dataTreeEvalRef, + entity, + jsUpdates, + unEvalDataTree, + entityName, + ); + + const expectedJSUpdates = { + JSObject1: { + parsedBody: { + body: "export default {\n\tmyVar1: [],\n\tmyVar1: [],\n\tmyVar2: {},\n\tmyFun1: () => {\n\t\t//write code here\n\t\t\n\t},\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}\n,\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}}", + actions: [ + { + name: "myFun1", + body: "() => {}", + arguments: [], + }, + { + name: "myFun2", + body: "() => {\n yeso;\n}", + arguments: [], + }, + ], + variables: [ + { + name: "myVar1", + value: "[]", + }, + { + name: "myVar2", + value: "{}", + }, + ], + }, + id: "64013546b956c26882acc587", + }, + }; + + expect(result).toStrictEqual(expectedJSUpdates); + }); +});