Skip to content

Commit

Permalink
fix: Load workflows with unconnected Switch outputs (#12020)
Browse files Browse the repository at this point in the history
  • Loading branch information
mutdmour authored Dec 4, 2024
1 parent bd69316 commit abc851c
Show file tree
Hide file tree
Showing 24 changed files with 324 additions and 89 deletions.
9 changes: 9 additions & 0 deletions cypress/composables/workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ export function getCanvasNodes() {
);
}

export function getCanvasNodeByName(nodeName: string) {
return getCanvasNodes().filter(`:contains(${nodeName})`);
}

export function getSaveButton() {
return cy.getByTestId('workflow-save-button');
}
Expand Down Expand Up @@ -194,3 +198,8 @@ export function pasteWorkflow(workflow: object) {
export function clickZoomToFit() {
getZoomToFitButton().click();
}

export function deleteNode(name: string) {
getCanvasNodeByName(name).first().click();
cy.get('body').type('{del}');
}
17 changes: 17 additions & 0 deletions cypress/e2e/2929-ado-can-load-old-switch-node-workflows.cy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import {
deleteNode,
getCanvasNodes,
navigateToNewWorkflowPage,
pasteWorkflow,
} from '../composables/workflow';
import Workflow from '../fixtures/Switch_node_with_null_connection.json';

describe('ADO-2929 can load Switch nodes', () => {
it('can load workflows with Switch nodes with null at connection index', () => {
navigateToNewWorkflowPage();
pasteWorkflow(Workflow);
getCanvasNodes().should('have.length', 3);
deleteNode('Switch');
getCanvasNodes().should('have.length', 2);
});
});
85 changes: 85 additions & 0 deletions cypress/fixtures/Switch_node_with_null_connection.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
{
"nodes": [
{
"parameters": {},
"id": "418350b8-b402-4d3b-93ba-3794d36c1ad5",
"name": "When clicking \"Test workflow\"",
"type": "n8n-nodes-base.manualTrigger",
"typeVersion": 1,
"position": [440, 380]
},
{
"parameters": {
"rules": {
"values": [
{
"conditions": {
"options": {
"caseSensitive": true,
"leftValue": "",
"typeValidation": "strict"
},
"conditions": [
{
"leftValue": "",
"rightValue": "",
"operator": {
"type": "string",
"operation": "equals"
}
}
],
"combinator": "and"
}
},
{},
{}
]
},
"options": {}
},
"id": "b67ad46f-6b0d-4ff4-b2d2-dfbde44e287c",
"name": "Switch",
"type": "n8n-nodes-base.switch",
"typeVersion": 3,
"position": [660, 380]
},
{
"parameters": {
"options": {}
},
"id": "24731c11-e2a4-4854-81a6-277ce72e8a93",
"name": "Edit Fields",
"type": "n8n-nodes-base.set",
"typeVersion": 3.3,
"position": [840, 480]
}
],
"connections": {
"When clicking \"Test workflow\"": {
"main": [
[
{
"node": "Switch",
"type": "main",
"index": 0
}
]
]
},
"Switch": {
"main": [
null,
null,
[
{
"node": "Edit Fields",
"type": "main",
"index": 0
}
]
]
}
},
"pinData": {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class PurgeInvalidWorkflowConnections1675940580449 implements Irreversibl

// It filters out all connections that are connected to a node that cannot receive input
outputConnection.forEach((outputConnectionItem, outputConnectionItemIdx) => {
outputConnection[outputConnectionItemIdx] = outputConnectionItem.filter(
outputConnection[outputConnectionItemIdx] = (outputConnectionItem ?? []).filter(
(outgoingConnections) =>
!nodesThatCannotReceiveInput.includes(outgoingConnections.node),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export class InstanceRiskReporter implements RiskReporter {
node: WorkflowEntity['nodes'][number];
workflow: WorkflowEntity;
}) {
const childNodeNames = workflow.connections[node.name]?.main[0].map((i) => i.node);
const childNodeNames = workflow.connections[node.name]?.main[0]?.map((i) => i.node);

if (!childNodeNames) return false;

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/PartialExecutionUtils/DirectedGraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ export class DirectedGraph {

for (const [outputType, outputs] of Object.entries(iConnection)) {
for (const [outputIndex, conns] of outputs.entries()) {
for (const conn of conns) {
for (const conn of conns ?? []) {
// TODO: What's with the input type?
const { node: toNodeName, type: _inputType, index: inputIndex } = conn;
const to = workflow.getNode(toNodeName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,28 @@ describe('DirectedGraph', () => {
);
});

// ┌─────┐ ┌─────┐──► null
// │node1├───►│node2| ┌─────┐
// └─────┘ └─────┘──►│node3|
// └─────┘
//
test('linear workflow with null connections', () => {
// ARRANGE
const node1 = createNodeData({ name: 'Node1' });
const node2 = createNodeData({ name: 'Node2' });
const node3 = createNodeData({ name: 'Node3' });

// ACT
const graph = new DirectedGraph()
.addNodes(node1, node2, node3)
.addConnections({ from: node1, to: node2 }, { from: node2, to: node3, outputIndex: 1 });

// ASSERT
expect(DirectedGraph.fromWorkflow(graph.toWorkflow({ ...defaultWorkflowParameter }))).toEqual(
graph,
);
});

describe('getChildren', () => {
// ┌─────┐ ┌─────┐ ┌─────┐
// │node1├───►│node2├──►│node3│
Expand Down
19 changes: 13 additions & 6 deletions packages/core/src/WorkflowExecute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import type {
CloseFunction,
StartNodeData,
NodeExecutionHint,
NodeInputConnections,
} from 'n8n-workflow';
import {
LoggerProxy as Logger,
Expand Down Expand Up @@ -208,6 +209,9 @@ export class WorkflowExecute {
// Get the data of the incoming connections
incomingSourceData = { main: [] };
for (const connections of incomingNodeConnections.main) {
if (!connections) {
continue;
}
for (let inputIndex = 0; inputIndex < connections.length; inputIndex++) {
connection = connections[inputIndex];

Expand Down Expand Up @@ -249,6 +253,9 @@ export class WorkflowExecute {
incomingNodeConnections = workflow.connectionsByDestinationNode[destinationNode];
if (incomingNodeConnections !== undefined) {
for (const connections of incomingNodeConnections.main) {
if (!connections) {
continue;
}
for (let inputIndex = 0; inputIndex < connections.length; inputIndex++) {
connection = connections[inputIndex];

Expand Down Expand Up @@ -642,7 +649,7 @@ export class WorkflowExecute {
}
for (const connectionDataCheck of workflow.connectionsBySourceNode[parentNodeName].main[
outputIndexParent
]) {
] ?? []) {
checkOutputNodes.push(connectionDataCheck.node);
}
}
Expand All @@ -661,7 +668,7 @@ export class WorkflowExecute {
) {
for (const inputData of workflow.connectionsByDestinationNode[connectionData.node].main[
inputIndex
]) {
] ?? []) {
if (inputData.node === parentNodeName) {
// Is the node we come from so its data will be available for sure
continue;
Expand All @@ -681,7 +688,7 @@ export class WorkflowExecute {
if (
!this.incomingConnectionIsEmpty(
this.runExecutionData.resultData.runData,
workflow.connectionsByDestinationNode[inputData.node].main[0],
workflow.connectionsByDestinationNode[inputData.node].main[0] ?? [],
runIndex,
)
) {
Expand Down Expand Up @@ -770,7 +777,7 @@ export class WorkflowExecute {
} else if (
this.incomingConnectionIsEmpty(
this.runExecutionData.resultData.runData,
workflow.connectionsByDestinationNode[nodeToAdd].main[0],
workflow.connectionsByDestinationNode[nodeToAdd].main[0] ?? [],
runIndex,
)
) {
Expand Down Expand Up @@ -1066,7 +1073,7 @@ export class WorkflowExecute {
if (workflow.connectionsByDestinationNode.hasOwnProperty(executionNode.name)) {
// Check if the node has incoming connections
if (workflow.connectionsByDestinationNode[executionNode.name].hasOwnProperty('main')) {
let inputConnections: IConnection[][];
let inputConnections: NodeInputConnections;
let connectionIndex: number;

// eslint-disable-next-line prefer-const
Expand Down Expand Up @@ -1586,7 +1593,7 @@ export class WorkflowExecute {
// Iterate over all the different connections of this output
for (connectionData of workflow.connectionsBySourceNode[executionNode.name].main[
outputIndex
]) {
] ?? []) {
if (!workflow.nodes.hasOwnProperty(connectionData.node)) {
throw new ApplicationError('Destination node not found', {
extra: {
Expand Down
1 change: 1 addition & 0 deletions packages/editor-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"clean": "rimraf dist .turbo",
"build": "cross-env VUE_APP_PUBLIC_PATH=\"/{{BASE_PATH}}/\" NODE_OPTIONS=\"--max-old-space-size=8192\" vite build",
"typecheck": "vue-tsc --noEmit",
"typecheck:watch": "vue-tsc --watch --noEmit",
"dev": "pnpm serve",
"lint": "eslint src --ext .js,.ts,.vue --quiet",
"lintfix": "eslint src --ext .js,.ts,.vue --fix",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ export function useChatMessaging({
];
if (!connectedMemoryInputs) return [];

const memoryConnection = (connectedMemoryInputs ?? []).find((i) => i.length > 0)?.[0];
const memoryConnection = (connectedMemoryInputs ?? []).find((i) => (i ?? []).length > 0)?.[0];

if (!memoryConnection) return [];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ function useJsonFieldCompletions() {
if (activeNode) {
const workflow = workflowsStore.getCurrentWorkflow();
const input = workflow.connectionsByDestinationNode[activeNode.name];
return input.main[0][0].node;
return input.main[0] ? input.main[0][0].node : null;
}
} catch (e) {
console.error(e);
Expand Down
2 changes: 1 addition & 1 deletion packages/editor-ui/src/components/InputNodeSelect.vue
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function getMultipleNodesText(nodeName: string): string {
props.workflow.connectionsByDestinationNode[activeNode.value.name].main || [];
// Collect indexes of connected nodes
const connectedInputIndexes = activeNodeConnections.reduce((acc: number[], node, index) => {
if (node[0] && node[0].node === nodeName) return [...acc, index];
if (node?.[0] && node[0].node === nodeName) return [...acc, index];
return acc;
}, []);
Expand Down
4 changes: 2 additions & 2 deletions packages/editor-ui/src/composables/useCanvasMapping.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ describe('useCanvasMapping', () => {
expect(
mappedNodes.value[0]?.data?.connections[CanvasConnectionMode.Output][
NodeConnectionType.Main
][0][0],
][0]?.[0],
).toEqual(
expect.objectContaining({
node: setNode.name,
Expand All @@ -253,7 +253,7 @@ describe('useCanvasMapping', () => {
expect(
mappedNodes.value[1]?.data?.connections[CanvasConnectionMode.Input][
NodeConnectionType.Main
][0][0],
][0]?.[0],
).toEqual(
expect.objectContaining({
node: manualTriggerNode.name,
Expand Down
76 changes: 76 additions & 0 deletions packages/editor-ui/src/composables/useCanvasOperations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,82 @@ describe('useCanvasOperations', () => {
expect(workflowsStore.removeNodeExecutionDataById).toHaveBeenCalledWith(nodes[1].id);
expect(workflowsStore.removeNodeById).toHaveBeenCalledWith(nodes[1].id);
});

it('should handle nodes with null connections for unconnected indexes', () => {
const workflowsStore = mockedStore(useWorkflowsStore);
const nodeTypesStore = mockedStore(useNodeTypesStore);

nodeTypesStore.nodeTypes = {
[SET_NODE_TYPE]: { 1: mockNodeTypeDescription({ name: SET_NODE_TYPE }) },
};

const nodes = [
createTestNode({
id: 'input',
type: SET_NODE_TYPE,
position: [10, 20],
name: 'Input Node',
}),
createTestNode({
id: 'middle',
type: SET_NODE_TYPE,
position: [10, 20],
name: 'Middle Node',
}),
createTestNode({
id: 'output',
type: SET_NODE_TYPE,
position: [10, 20],
name: 'Output Node',
}),
];

workflowsStore.getNodeByName = vi
.fn()
.mockImplementation((name: string) => nodes.find((node) => node.name === name));

workflowsStore.workflow.nodes = nodes;
workflowsStore.workflow.connections = {
[nodes[0].name]: {
main: [
null,
[
{
node: nodes[1].name,
type: NodeConnectionType.Main,
index: 0,
},
],
],
},
[nodes[1].name]: {
main: [
// null here to simulate no connection at index
null,
[
{
node: nodes[2].name,
type: NodeConnectionType.Main,
index: 0,
},
],
],
},
};

const workflowObject = createTestWorkflowObject(workflowsStore.workflow);
workflowsStore.getCurrentWorkflow.mockReturnValue(workflowObject);
workflowsStore.incomingConnectionsByNodeName.mockReturnValue({});

workflowsStore.getNodeById.mockReturnValue(nodes[1]);

const { deleteNode } = useCanvasOperations({ router });
deleteNode(nodes[1].id);

expect(workflowsStore.removeNodeById).toHaveBeenCalledWith(nodes[1].id);
expect(workflowsStore.removeNodeExecutionDataById).toHaveBeenCalledWith(nodes[1].id);
expect(workflowsStore.removeNodeById).toHaveBeenCalledWith(nodes[1].id);
});
});

describe('revertDeleteNode', () => {
Expand Down
Loading

0 comments on commit abc851c

Please sign in to comment.