From 5903592a2341330714ba823943c586c0779a6805 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Wed, 2 Oct 2024 08:12:38 +0200 Subject: [PATCH] fix(core): Fix new partial execution flow not working with workflows with multiple disabled nodes in a row (no-changelog) (#11024) --- .../PartialExecutionUtils/DirectedGraph.ts | 57 +++++ .../__tests__/DirectedGraph.test.ts | 200 ++++++++++++++++++ .../__tests__/findSubgraph.test.ts | 42 +++- .../src/PartialExecutionUtils/findSubgraph.ts | 27 +-- .../recreateNodeExecutionStack.ts | 4 +- 5 files changed, 307 insertions(+), 23 deletions(-) diff --git a/packages/core/src/PartialExecutionUtils/DirectedGraph.ts b/packages/core/src/PartialExecutionUtils/DirectedGraph.ts index bd6cf81a2..33cc11469 100644 --- a/packages/core/src/PartialExecutionUtils/DirectedGraph.ts +++ b/packages/core/src/PartialExecutionUtils/DirectedGraph.ts @@ -67,6 +67,63 @@ export class DirectedGraph { return this; } + /** + * Removes a node from the graph. + * + * By default it will also remove all connections that use that node and + * return nothing. + * + * If you pass `{ reconnectConnections: true }` it will rewire all + * connections making sure all parent nodes are connected to all child nodes + * and return the new connections. + */ + removeNode(node: INode, options?: { reconnectConnections: true }): GraphConnection[]; + removeNode(node: INode, options?: { reconnectConnections: false }): undefined; + removeNode(node: INode, { reconnectConnections = false } = {}): undefined | GraphConnection[] { + if (reconnectConnections) { + const incomingConnections = this.getDirectParents(node); + const outgoingConnections = this.getDirectChildren(node); + + const newConnections: GraphConnection[] = []; + + for (const incomingConnection of incomingConnections) { + for (const outgoingConnection of outgoingConnections) { + const newConnection = { + ...incomingConnection, + to: outgoingConnection.to, + inputIndex: outgoingConnection.inputIndex, + }; + + newConnections.push(newConnection); + } + } + + for (const [key, connection] of this.connections.entries()) { + if (connection.to === node || connection.from === node) { + this.connections.delete(key); + } + } + + for (const newConnection of newConnections) { + this.connections.set(this.makeKey(newConnection), newConnection); + } + + this.nodes.delete(node.name); + + return newConnections; + } else { + for (const [key, connection] of this.connections.entries()) { + if (connection.to === node || connection.from === node) { + this.connections.delete(key); + } + } + + this.nodes.delete(node.name); + + return; + } + } + addConnection(connectionInput: { from: INode; to: INode; diff --git a/packages/core/src/PartialExecutionUtils/__tests__/DirectedGraph.test.ts b/packages/core/src/PartialExecutionUtils/__tests__/DirectedGraph.test.ts index b383b5939..426a5405c 100644 --- a/packages/core/src/PartialExecutionUtils/__tests__/DirectedGraph.test.ts +++ b/packages/core/src/PartialExecutionUtils/__tests__/DirectedGraph.test.ts @@ -9,6 +9,8 @@ // XX denotes that the node is disabled // PD denotes that the node has pinned data +import { NodeConnectionType } from 'n8n-workflow'; + import { createNodeData, defaultWorkflowParameter } from './helpers'; import { DirectedGraph } from '../DirectedGraph'; @@ -86,4 +88,202 @@ describe('DirectedGraph', () => { expect(children).toEqual(new Set([node1, node2, node3])); }); }); + + describe('removeNode', () => { + // XX + // ┌─────┐ ┌─────┐ ┌─────┐ + // │node0├───►│node1├──►│node2│ + // └─────┘ └─────┘ └─────┘ + // turns into + // ┌─────┐ ┌─────┐ + // │node0│ │node2│ + // └─────┘ └─────┘ + test('remove node and all connections', () => { + // ARRANGE + const node0 = createNodeData({ name: 'node0' }); + const node1 = createNodeData({ name: 'node1' }); + const node2 = createNodeData({ name: 'node2' }); + const graph = new DirectedGraph() + .addNodes(node0, node1, node2) + .addConnections({ from: node0, to: node1 }, { from: node0, to: node2 }); + + // ACT + graph.removeNode(node1); + + // ASSERT + expect(graph).toEqual( + new DirectedGraph().addNodes(node0, node2).addConnections({ from: node0, to: node2 }), + ); + }); + + // XX + // ┌─────┐ ┌─────┐ ┌─────┐ + // │node0├───►│node1├──►│node2│ + // └─────┘ └─────┘ └─────┘ + // turns into + // ┌─────┐ ┌─────┐ + // │node0├──►│node2│ + // └─────┘ └─────┘ + test('remove node, but reconnect connections', () => { + // ARRANGE + const node0 = createNodeData({ name: 'node0' }); + const node1 = createNodeData({ name: 'node1' }); + const node2 = createNodeData({ name: 'node2' }); + const graph = new DirectedGraph() + .addNodes(node0, node1, node2) + .addConnections({ from: node0, to: node1 }, { from: node1, to: node2 }); + + // ACT + const newConnections = graph.removeNode(node1, { reconnectConnections: true }); + + // ASSERT + expect(newConnections).toHaveLength(1); + expect(newConnections[0]).toEqual({ + from: node0, + outputIndex: 0, + type: NodeConnectionType.Main, + inputIndex: 0, + to: node2, + }); + expect(graph).toEqual( + new DirectedGraph().addNodes(node0, node2).addConnections({ from: node0, to: node2 }), + ); + }); + + // XX + // ┌─────┐ ┌─────┐ ┌─────┐ + // │ │o o│ │o o│ │ + // │ │o─┐ o│ │o o│ │ + // │node0│o └►o│node1│o o│node2│ + // │ │o o│ │o─┐ o│ │ + // │ │o o│ │o └►o│ │ + // └─────┘ └─────┘ └─────┘ + // turns into + // ┌─────┐ ┌─────┐ + // │ │o o│ │ + // │ │o───────┐ o│ │ + // │node0│o │ o│node2│ + // │ │o │ o│ │ + // │ │o └──────►o│ │ + // └─────┘ └─────┘ + test('remove node, reconnect connections and retaining the input indexes', () => { + // ARRANGE + const node0 = createNodeData({ name: 'node0' }); + const node1 = createNodeData({ name: 'node1' }); + const node2 = createNodeData({ name: 'node2' }); + const graph = new DirectedGraph() + .addNodes(node0, node1, node2) + .addConnections( + { from: node0, outputIndex: 1, inputIndex: 2, to: node1 }, + { from: node1, outputIndex: 3, inputIndex: 4, to: node2 }, + ); + + // ACT + const newConnections = graph.removeNode(node1, { reconnectConnections: true }); + + // ASSERT + expect(newConnections).toHaveLength(1); + expect(newConnections[0]).toEqual({ + from: node0, + outputIndex: 1, + type: NodeConnectionType.Main, + inputIndex: 4, + to: node2, + }); + expect(graph).toEqual( + new DirectedGraph() + .addNodes(node0, node2) + .addConnections({ from: node0, outputIndex: 1, inputIndex: 4, to: node2 }), + ); + }); + + // XX + // ┌─────┐ ┌─────┐ ┌─────┐ + // │ │o o│ │o │ │ + // │ │o─┐ o│ │o │ │ + // │node0│ └►o│node1│o ┌►o│node2│ + // │ │ │ │o─┘ │ │ + // │ │ │ │ │ │ + // └─────┘ └─────┘ └─────┘ + // turns into + // ┌─────┐ ┌─────┐ + // │ │o │ │ + // │ │o───────┐ │ │ + // │node0│ └──────►o│node2│ + // │ │ │ │ + // │ │ │ │ + // └─────┘ └─────┘ + test('remove node, reconnect connections and retaining the input indexes, even if the child has less inputs than the than the removed node had', () => { + // ARRANGE + const node0 = createNodeData({ name: 'node0' }); + const node1 = createNodeData({ name: 'node1' }); + const node2 = createNodeData({ name: 'node2' }); + const graph = new DirectedGraph() + .addNodes(node0, node1, node2) + .addConnections( + { from: node0, outputIndex: 1, inputIndex: 2, to: node1 }, + { from: node1, outputIndex: 3, inputIndex: 0, to: node2 }, + ); + + // ACT + const newConnections = graph.removeNode(node1, { reconnectConnections: true }); + + // ASSERT + const expectedGraph = new DirectedGraph() + .addNodes(node0, node2) + .addConnections({ from: node0, outputIndex: 1, inputIndex: 0, to: node2 }); + expect(newConnections).toHaveLength(1); + expect(newConnections).toEqual(expectedGraph.getConnections()); + expect(graph).toEqual(expectedGraph); + }); + + // ┌─────┐ ┌──────┐ + // │left0├─┐ XX ┌►│right0│ + // └─────┘ │ ┌──────┐ │ └──────┘ + // ├─►│center├──┤ + // ┌─────┐ │ └──────┘ │ ┌──────┐ + // │left1├─┘ └►│right1│ + // └─────┘ └──────┘ + // turns into + // + // ┌─────┐ ┌──────┐ + // │left0├─┐ ┌─►│right0│ + // └─────┘ │ │ └──────┘ + // ├───────────┤ + // ┌─────┐ │ │ ┌──────┐ + // │left1├─┘ └─►│right1│ + // └─────┘ └──────┘ + test('remove node, reconnect connections and multiplexes them', () => { + // ARRANGE + const left0 = createNodeData({ name: 'left0' }); + const left1 = createNodeData({ name: 'left1' }); + const center = createNodeData({ name: 'center' }); + const right0 = createNodeData({ name: 'right0' }); + const right1 = createNodeData({ name: 'right1' }); + const graph = new DirectedGraph() + .addNodes(left0, left1, center, right0, right1) + .addConnections( + { from: left0, to: center }, + { from: left1, to: center }, + { from: center, to: right0 }, + { from: center, to: right1 }, + ); + + // ACT + const newConnections = graph.removeNode(center, { reconnectConnections: true }); + + // ASSERT + const expectedGraph = new DirectedGraph() + .addNodes(left0, left1, right0, right1) + .addConnections( + { from: left0, to: right0 }, + { from: left0, to: right1 }, + { from: left1, to: right0 }, + { from: left1, to: right1 }, + ); + expect(newConnections).toHaveLength(4); + expect(newConnections).toEqual(expectedGraph.getConnections()); + expect(graph).toEqual(expectedGraph); + }); + }); }); diff --git a/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts b/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts index 40ff50df3..a5187eacf 100644 --- a/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts +++ b/packages/core/src/PartialExecutionUtils/__tests__/findSubgraph.test.ts @@ -13,7 +13,7 @@ import { createNodeData } from './helpers'; import { DirectedGraph } from '../DirectedGraph'; import { findSubgraph } from '../findSubgraph'; -describe('findSubgraph2', () => { +describe('findSubgraph', () => { // ►► // ┌───────┐ ┌───────────┐ // │trigger├────►│destination│ @@ -83,6 +83,12 @@ describe('findSubgraph2', () => { // │trigger│ │disabled├─────►│destination│ // │ ├────────►│ │ └───────────┘ // └───────┘ └────────┘ + // turns into + // ┌───────┐ ►► + // │ │ ┌───────────┐ + // │trigger├─────►│destination│ + // │ │ └───────────┘ + // └───────┘ test('skip disabled nodes', () => { const trigger = createNodeData({ name: 'trigger' }); const disabled = createNodeData({ name: 'disabled', disabled: true }); @@ -101,6 +107,40 @@ describe('findSubgraph2', () => { ); }); + // XX XX + // ┌───────┐ ┌─────┐ ┌─────┐ ┌───────────┐ + // │trigger├────►│node1├────►│node2├────►│destination│ + // └───────┘ └─────┘ └─────┘ └───────────┘ + // turns into + // ┌───────┐ ┌───────────┐ + // │trigger├────►│destination│ + // └───────┘ └───────────┘ + test('skip multiple disabled nodes', () => { + // ARRANGE + const trigger = createNodeData({ name: 'trigger' }); + const disabledNode1 = createNodeData({ name: 'disabledNode1', disabled: true }); + const disabledNode2 = createNodeData({ name: 'disabledNode2', disabled: true }); + const destination = createNodeData({ name: 'destination' }); + + const graph = new DirectedGraph() + .addNodes(trigger, disabledNode1, disabledNode2, destination) + .addConnections( + { from: trigger, to: disabledNode1 }, + { from: disabledNode1, to: disabledNode2 }, + { from: disabledNode2, to: destination }, + ); + + // ACT + const subgraph = findSubgraph(graph, destination, trigger); + + // ASSERT + expect(subgraph).toEqual( + new DirectedGraph() + .addNodes(trigger, destination) + .addConnections({ from: trigger, to: destination }), + ); + }); + // ►► // ┌───────┐ ┌─────┐ ┌─────┐ // │Trigger├───┬──►│Node1├───┬─►│Node2│ diff --git a/packages/core/src/PartialExecutionUtils/findSubgraph.ts b/packages/core/src/PartialExecutionUtils/findSubgraph.ts index 4564d15ed..ea1df9184 100644 --- a/packages/core/src/PartialExecutionUtils/findSubgraph.ts +++ b/packages/core/src/PartialExecutionUtils/findSubgraph.ts @@ -51,27 +51,14 @@ function findSubgraphRecursive( // Take every incoming connection and connect it to every node that is // connected to the current node’s first output if (current.disabled) { - const incomingConnections = graph.getDirectParents(current); - const outgoingConnections = graph - .getDirectChildren(current) - // NOTE: When a node is disabled only the first output gets data - .filter((connection) => connection.outputIndex === 0); + // The last segment on the current branch is still pointing to the removed + // node, so let's remove it. + currentBranch.pop(); - parentConnections = []; - - for (const incomingConnection of incomingConnections) { - for (const outgoingConnection of outgoingConnections) { - const newConnection = { - ...incomingConnection, - to: outgoingConnection.to, - inputIndex: outgoingConnection.inputIndex, - }; - - parentConnections.push(newConnection); - currentBranch.pop(); - currentBranch.push(newConnection); - } - } + // The node is replaced by a set of new connections, connecting the parents + // and children of it directly. In the recursive call below we'll follow + // them further. + parentConnections = graph.removeNode(current, { reconnectConnections: true }); } // Recurse on each parent. diff --git a/packages/core/src/PartialExecutionUtils/recreateNodeExecutionStack.ts b/packages/core/src/PartialExecutionUtils/recreateNodeExecutionStack.ts index 897838d51..f2f1f4af6 100644 --- a/packages/core/src/PartialExecutionUtils/recreateNodeExecutionStack.ts +++ b/packages/core/src/PartialExecutionUtils/recreateNodeExecutionStack.ts @@ -44,12 +44,12 @@ export function recreateNodeExecutionStack( // Validate invariants. // The graph needs to be free of disabled nodes. If it's not it hasn't been - // passed through findSubgraph2. + // passed through findSubgraph. for (const node of graph.getNodes().values()) { a.notEqual( node.disabled, true, - `Graph contains disabled nodes. This is not supported. Make sure to pass the graph through "findSubgraph2" before calling "recreateNodeExecutionStack". The node in question is "${node.name}"`, + `Graph contains disabled nodes. This is not supported. Make sure to pass the graph through "findSubgraph" before calling "recreateNodeExecutionStack". The node in question is "${node.name}"`, ); }