From 8e6ddfe02877508130e775274b6d9cc920a594b9 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 10 Oct 2024 12:52:10 +0200 Subject: [PATCH] refactor(core): Use options object for functions with lots of arguments to make the call site more readable and avoid errors (#11199) --- .../__tests__/findStartNodes.test.ts | 75 ++++++++++++------- .../__tests__/findSubgraph.test.ts | 22 +++--- .../recreateNodeExecutionStack.test.ts | 2 +- .../PartialExecutionUtils/findStartNodes.ts | 20 +++-- .../src/PartialExecutionUtils/findSubgraph.ts | 15 ++-- packages/core/src/WorkflowExecute.ts | 18 ++--- 6 files changed, 93 insertions(+), 59 deletions(-) diff --git a/packages/core/src/PartialExecutionUtils/__tests__/findStartNodes.test.ts b/packages/core/src/PartialExecutionUtils/__tests__/findStartNodes.test.ts index 0ea2e4f611..57022d862c 100644 --- a/packages/core/src/PartialExecutionUtils/__tests__/findStartNodes.test.ts +++ b/packages/core/src/PartialExecutionUtils/__tests__/findStartNodes.test.ts @@ -46,7 +46,7 @@ describe('findStartNodes', () => { const node = createNodeData({ name: 'Basic Node' }); const graph = new DirectedGraph().addNode(node); - const startNodes = findStartNodes(graph, node, node); + const startNodes = findStartNodes({ graph, trigger: node, destination: node }); expect(startNodes).toHaveLength(1); expect(startNodes[0]).toEqual(node); @@ -65,7 +65,7 @@ describe('findStartNodes', () => { // if the trigger has no run data { - const startNodes = findStartNodes(graph, trigger, destination); + const startNodes = findStartNodes({ graph, trigger, destination }); expect(startNodes).toHaveLength(1); expect(startNodes[0]).toEqual(trigger); @@ -77,7 +77,7 @@ describe('findStartNodes', () => { [trigger.name]: [toITaskData([{ data: { value: 1 } }])], }; - const startNodes = findStartNodes(graph, trigger, destination, runData); + const startNodes = findStartNodes({ graph, trigger, destination, runData }); expect(startNodes).toHaveLength(1); expect(startNodes[0]).toEqual(destination); @@ -112,7 +112,7 @@ describe('findStartNodes', () => { }; // ACT - const startNodes = findStartNodes(graph, trigger, node, runData); + const startNodes = findStartNodes({ graph, trigger, destination: node, runData }); // ASSERT expect(startNodes).toHaveLength(1); @@ -153,7 +153,7 @@ describe('findStartNodes', () => { { // ACT - const startNodes = findStartNodes(graph, trigger, node4); + const startNodes = findStartNodes({ graph, trigger, destination: node4 }); // ASSERT expect(startNodes).toHaveLength(1); @@ -172,7 +172,7 @@ describe('findStartNodes', () => { }; // ACT - const startNodes = findStartNodes(graph, trigger, node4, runData); + const startNodes = findStartNodes({ graph, trigger, destination: node4, runData }); // ASSERT expect(startNodes).toHaveLength(1); @@ -201,8 +201,13 @@ describe('findStartNodes', () => { ); // ACT - const startNodes = findStartNodes(graph, trigger, node, { - [trigger.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], + const startNodes = findStartNodes({ + graph, + trigger, + destination: node, + runData: { + [trigger.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], + }, }); // ASSERT @@ -231,8 +236,13 @@ describe('findStartNodes', () => { ); // ACT - const startNodes = findStartNodes(graph, trigger, node, { - [trigger.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 1 }])], + const startNodes = findStartNodes({ + graph, + trigger, + destination: node, + runData: { + [trigger.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 1 }])], + }, }); // ASSERT @@ -261,13 +271,18 @@ describe('findStartNodes', () => { ); // ACT - const startNodes = findStartNodes(graph, trigger, node, { - [trigger.name]: [ - toITaskData([ - { data: { value: 1 }, outputIndex: 0 }, - { data: { value: 1 }, outputIndex: 1 }, - ]), - ], + const startNodes = findStartNodes({ + graph, + trigger, + destination: node, + runData: { + [trigger.name]: [ + toITaskData([ + { data: { value: 1 }, outputIndex: 0 }, + { data: { value: 1 }, outputIndex: 1 }, + ]), + ], + }, }); // ASSERT @@ -297,10 +312,15 @@ describe('findStartNodes', () => { ); // ACT - const startNodes = findStartNodes(graph, trigger, node3, { - [trigger.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], - [node1.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], - [node2.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], + const startNodes = findStartNodes({ + graph, + trigger, + destination: node3, + runData: { + [trigger.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], + [node1.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], + [node2.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 0 }])], + }, }); // ASSERT @@ -329,9 +349,14 @@ describe('findStartNodes', () => { ); // ACT - const startNodes = findStartNodes(graph, node1, node2, { - [trigger.name]: [toITaskData([{ data: { value: 1 } }])], - [node1.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 1 }])], + const startNodes = findStartNodes({ + graph, + trigger: node1, + destination: node2, + runData: { + [trigger.name]: [toITaskData([{ data: { value: 1 } }])], + [node1.name]: [toITaskData([{ data: { value: 1 }, outputIndex: 1 }])], + }, }); // ASSERT @@ -364,7 +389,7 @@ describe('findStartNodes', () => { const pinData: IPinData = {}; // ACT - const startNodes = findStartNodes(graph, trigger, node2, runData, pinData); + const startNodes = findStartNodes({ graph, trigger, destination: node2, runData, pinData }); // ASSERT expect(startNodes).toHaveLength(1); diff --git a/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts b/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts index 76a1afcc69..fceb22da06 100644 --- a/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts +++ b/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts @@ -28,7 +28,7 @@ describe('findSubgraph', () => { .addNodes(trigger, destination) .addConnections({ from: trigger, to: destination }); - const subgraph = findSubgraph(graph, destination, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); expect(subgraph).toEqual(graph); }); @@ -50,7 +50,7 @@ describe('findSubgraph', () => { { from: ifNode, to: noOp, outputIndex: 1 }, ); - const subgraph = findSubgraph(graph, noOp, ifNode); + const subgraph = findSubgraph({ graph, destination: noOp, trigger: ifNode }); expect(subgraph).toEqual(graph); }); @@ -70,7 +70,7 @@ describe('findSubgraph', () => { .addNodes(trigger, destination, node) .addConnections({ from: trigger, to: destination }, { from: destination, to: node }); - const subgraph = findSubgraph(graph, destination, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); expect(subgraph).toEqual( new DirectedGraph() @@ -100,7 +100,7 @@ describe('findSubgraph', () => { .addNodes(trigger, disabled, destination) .addConnections({ from: trigger, to: disabled }, { from: disabled, to: destination }); - const subgraph = findSubgraph(graph, destination, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); expect(subgraph).toEqual( new DirectedGraph() @@ -133,7 +133,7 @@ describe('findSubgraph', () => { ); // ACT - const subgraph = findSubgraph(graph, destination, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); // ASSERT expect(subgraph).toEqual( @@ -163,7 +163,7 @@ describe('findSubgraph', () => { ); // ACT - const subgraph = findSubgraph(graph, node2, trigger); + const subgraph = findSubgraph({ graph, destination: node2, trigger }); // ASSERT expect(subgraph).toEqual(graph); @@ -187,7 +187,7 @@ describe('findSubgraph', () => { .addConnections({ from: trigger, to: node1 }, { from: node2, to: node1 }); // ACT - const subgraph = findSubgraph(graph, node1, trigger); + const subgraph = findSubgraph({ graph, destination: node1, trigger }); // ASSERT expect(subgraph).toEqual( @@ -215,7 +215,7 @@ describe('findSubgraph', () => { ); // ACT - const subgraph = findSubgraph(graph, destination, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); // ASSERT expect(subgraph).toEqual( @@ -248,7 +248,7 @@ describe('findSubgraph', () => { ); // ACT - const subgraph = findSubgraph(graph, destination, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); // ASSERT expect(subgraph).toEqual(graph); @@ -284,7 +284,7 @@ describe('findSubgraph', () => { ); // ACT - const subgraph = findSubgraph(graph, destination, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); // ASSERT expect(subgraph.getConnections()).toHaveLength(0); @@ -320,7 +320,7 @@ describe('findSubgraph', () => { ); // ACT - const subgraph = findSubgraph(graph, root, trigger); + const subgraph = findSubgraph({ graph, destination: root, trigger }); // ASSERT expect(subgraph).toEqual( diff --git a/packages/core/src/PartialExecutionUtils/__tests__/recreateNodeExecutionStack.test.ts b/packages/core/src/PartialExecutionUtils/__tests__/recreateNodeExecutionStack.test.ts index d0cc934b13..a4bcac23a5 100644 --- a/packages/core/src/PartialExecutionUtils/__tests__/recreateNodeExecutionStack.test.ts +++ b/packages/core/src/PartialExecutionUtils/__tests__/recreateNodeExecutionStack.test.ts @@ -32,7 +32,7 @@ describe('recreateNodeExecutionStack', () => { .addNodes(trigger, node) .addConnections({ from: trigger, to: node }); - const workflow = findSubgraph(graph, node, trigger); + const workflow = findSubgraph({ graph, destination: node, trigger }); const startNodes = [node]; const runData: IRunData = { [trigger.name]: [toITaskData([{ data: { value: 1 } }])], diff --git a/packages/core/src/PartialExecutionUtils/findStartNodes.ts b/packages/core/src/PartialExecutionUtils/findStartNodes.ts index 28772bfc9a..a6165f6564 100644 --- a/packages/core/src/PartialExecutionUtils/findStartNodes.ts +++ b/packages/core/src/PartialExecutionUtils/findStartNodes.ts @@ -131,13 +131,19 @@ function findStartNodesRecursive( * - stop following the branch, there is no start node on this branch * 4. Recurse with every direct child that is part of the sub graph */ -export function findStartNodes( - graph: DirectedGraph, - trigger: INode, - destination: INode, - runData: IRunData = {}, - pinData: IPinData = {}, -): INode[] { +export function findStartNodes(options: { + graph: DirectedGraph; + trigger: INode; + destination: INode; + runData?: IRunData; + pinData?: IPinData; +}): INode[] { + const graph = options.graph; + const trigger = options.trigger; + const destination = options.destination; + const runData = options.runData ?? {}; + const pinData = options.pinData ?? {}; + const startNodes = findStartNodesRecursive( graph, trigger, diff --git a/packages/core/src/PartialExecutionUtils/findSubgraph.ts b/packages/core/src/PartialExecutionUtils/findSubgraph.ts index d05561e31a..4d3bc4cc3f 100644 --- a/packages/core/src/PartialExecutionUtils/findSubgraph.ts +++ b/packages/core/src/PartialExecutionUtils/findSubgraph.ts @@ -105,14 +105,17 @@ function findSubgraphRecursive( * dataflow in the graph they are utility nodes, like the AI model used in a * lang chain node. */ -export function findSubgraph( - graph: DirectedGraph, - destinationNode: INode, - trigger: INode, -): DirectedGraph { +export function findSubgraph(options: { + graph: DirectedGraph; + destination: INode; + trigger: INode; +}): DirectedGraph { + const graph = options.graph; + const destination = options.destination; + const trigger = options.trigger; const subgraph = new DirectedGraph(); - findSubgraphRecursive(graph, destinationNode, destinationNode, trigger, subgraph, []); + findSubgraphRecursive(graph, destination, destination, trigger, subgraph, []); // For each node in the subgraph, if it has parent connections of a type that // is not `Main` in the input graph, add the connections and the nodes diff --git a/packages/core/src/WorkflowExecute.ts b/packages/core/src/WorkflowExecute.ts index 46de2472fc..ec5963a54b 100644 --- a/packages/core/src/WorkflowExecute.ts +++ b/packages/core/src/WorkflowExecute.ts @@ -87,7 +87,7 @@ export class WorkflowExecute { * Executes the given workflow. * * @param {Workflow} workflow The workflow to execute - * @param {INode[]} [startNodes] Node to start execution from + * @param {INode[]} [startNode] Node to start execution from * @param {string} [destinationNode] Node to stop execution at */ // IMPORTANT: Do not add "async" to this function, it will then convert the @@ -332,9 +332,9 @@ export class WorkflowExecute { 'a destinationNodeName is required for the new partial execution flow', ); - const destinationNode = workflow.getNode(destinationNodeName); + const destination = workflow.getNode(destinationNodeName); assert.ok( - destinationNode, + destination, `Could not find a node with the name ${destinationNodeName} in the workflow.`, ); @@ -348,11 +348,11 @@ export class WorkflowExecute { // 2. Find the Subgraph const graph = DirectedGraph.fromWorkflow(workflow); - const subgraph = findSubgraph(graph, destinationNode, trigger); + const subgraph = findSubgraph({ graph, destination, trigger }); const filteredNodes = subgraph.getNodes(); // 3. Find the Start Nodes - const startNodes = findStartNodes(subgraph, trigger, destinationNode, runData); + const startNodes = findStartNodes({ graph: subgraph, trigger, destination, runData }); // 4. Detect Cycles const cycles = findCycles(workflow); @@ -367,7 +367,7 @@ export class WorkflowExecute { // 7. Recreate Execution Stack const { nodeExecutionStack, waitingExecution, waitingExecutionSource } = - recreateNodeExecutionStack(subgraph, startNodes, destinationNode, runData, pinData ?? {}); + recreateNodeExecutionStack(subgraph, startNodes, destination, runData, pinData ?? {}); // 8. Execute this.status = 'running'; @@ -1058,7 +1058,7 @@ export class WorkflowExecute { this.runExecutionData.startData!.runNodeFilter.indexOf(executionNode.name) === -1 ) { // If filter is set and node is not on filter skip it, that avoids the problem that it executes - // leafs that are parallel to a selected destinationNode. Normally it would execute them because + // leaves that are parallel to a selected destinationNode. Normally it would execute them because // they have the same parent and it executes all child nodes. continue; } @@ -1759,7 +1759,7 @@ export class WorkflowExecute { continue; } } else { - // A certain amout of inputs are required (amount of inputs) + // A certain amount of inputs are required (amount of inputs) if (inputsWithData.length < requiredInputs) { continue; } @@ -1817,7 +1817,7 @@ export class WorkflowExecute { // Node to add did not get found, rather an empty one removed so continue with search waitingNodes = Object.keys(this.runExecutionData.executionData!.waitingExecution); // Set counter to start again from the beginning. Set it to -1 as it auto increments - // after run. So only like that will we end up again ot 0. + // after run. So only like that will we end up again at 0. i = -1; } }