From 5ee6e7605c48b8d1f08b33e76f9958e371c1aa07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 18 Jan 2024 13:54:03 +0100 Subject: [PATCH 1/2] fix(core): Adjust starter node priority for manual executions with pinned activators --- .../workflows/workflowExecution.service.ts | 64 +++++--- .../unit/workflow-execution.service.test.ts | 137 ++++++++++++++++++ 2 files changed, 178 insertions(+), 23 deletions(-) create mode 100644 packages/cli/test/unit/workflow-execution.service.test.ts diff --git a/packages/cli/src/workflows/workflowExecution.service.ts b/packages/cli/src/workflows/workflowExecution.service.ts index d18f02519d778..30a2b5e1174ab 100644 --- a/packages/cli/src/workflows/workflowExecution.service.ts +++ b/packages/cli/src/workflows/workflowExecution.service.ts @@ -47,7 +47,7 @@ export class WorkflowExecutionService { user: User, sessionId?: string, ) { - const pinnedTrigger = this.findPinnedTrigger(workflowData, startNodes, pinData); + const pinnedTrigger = this.selectPinnedActivatorStarter(workflowData, startNodes, pinData); // If webhooks nodes exist and are active we have to wait for till we receive a call if ( @@ -243,44 +243,62 @@ export class WorkflowExecutionService { } /** - * Find the pinned trigger to execute the workflow from, if any. + * Select the pinned activator node to use as starter for a manual execution. * - * - In a full execution, select the _first_ pinned trigger. - * - In a partial execution, - * - select the _first_ pinned trigger that leads to the executed node, - * - else select the executed pinned trigger. + * In a full manual execution, select the pinned activator that was first added + * to the workflow, prioritizing `n8n-nodes-base.webhook` over other activators. + * + * In a partial manual execution, if the executed node has parent nodes among the + * pinned activators, select the pinned activator that was first added to the workflow, + * prioritizing `n8n-nodes-base.webhook` over other activators. If the executed node + * has no upstream nodes and is itself is a pinned activator, select it. */ - private findPinnedTrigger(workflow: IWorkflowDb, startNodes?: string[], pinData?: IPinData) { + selectPinnedActivatorStarter(workflow: IWorkflowDb, startNodes?: string[], pinData?: IPinData) { if (!pinData || !startNodes) return null; - const isTrigger = (nodeTypeName: string) => - ['trigger', 'webhook'].some((suffix) => nodeTypeName.toLowerCase().includes(suffix)); + const allPinnedActivators = this.findAllPinnedActivators(workflow, pinData); + + const [firstPinnedActivator] = allPinnedActivators; - const pinnedTriggers = workflow.nodes.filter( - (node) => !node.disabled && pinData[node.name] && isTrigger(node.type), - ); + // full manual execution - if (pinnedTriggers.length === 0) return null; + if (startNodes?.length === 0) return firstPinnedActivator ?? null; - if (startNodes?.length === 0) return pinnedTriggers[0]; // full execution + // partial manual execution - const [startNodeName] = startNodes; + /** + * If the partial manual execution has 2+ start nodes, we search only the zeroth + * start node's parents for a pinned activator. If we had 2+ start nodes without + * a common ancestor and so if we end up finding multiple pinned activators, we + * would still need to return one to comply with existing usage. + */ + const [firstStartNodeName] = startNodes; - const parentNames = new Workflow({ + const parentNodeNames = new Workflow({ nodes: workflow.nodes, connections: workflow.connections, active: workflow.active, nodeTypes: this.nodeTypes, - }).getParentNodes(startNodeName); + }).getParentNodes(firstStartNodeName); - let checkNodeName = ''; + if (parentNodeNames.length > 0) { + const parentNodeName = parentNodeNames.find((p) => p === firstPinnedActivator.name); - if (parentNames.length === 0) { - checkNodeName = startNodeName; - } else { - checkNodeName = parentNames.find((pn) => pn === pinnedTriggers[0].name) as string; + return allPinnedActivators.find((pa) => pa.name === parentNodeName) ?? null; } - return pinnedTriggers.find((pt) => pt.name === checkNodeName) ?? null; // partial execution + return allPinnedActivators.find((pa) => pa.name === firstStartNodeName) ?? null; + } + + private findAllPinnedActivators(workflow: IWorkflowDb, pinData?: IPinData) { + return workflow.nodes + .filter( + (node) => + !node.disabled && + pinData?.[node.name] && + ['trigger', 'webhook'].some((suffix) => node.type.toLowerCase().endsWith(suffix)) && + node.type !== 'n8n-nodes-base.respondToWebhook', + ) + .sort((a) => (a.type.endsWith('webhook') ? -1 : 1)); } } diff --git a/packages/cli/test/unit/workflow-execution.service.test.ts b/packages/cli/test/unit/workflow-execution.service.test.ts new file mode 100644 index 0000000000000..fd2848a46ecfa --- /dev/null +++ b/packages/cli/test/unit/workflow-execution.service.test.ts @@ -0,0 +1,137 @@ +import type { INode } from 'n8n-workflow'; +import { WorkflowExecutionService } from '@/workflows/workflowExecution.service'; +import type { IWorkflowDb } from '@/Interfaces'; +import { mock } from 'jest-mock-extended'; + +const webhookNode: INode = { + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + id: '111f1db0-e7be-44c5-9ce9-3e35362490f0', + parameters: {}, + typeVersion: 1, + position: [0, 0], + webhookId: 'de0f8dcb-7b64-4f22-b66d-d8f74d6aefb7', +}; + +const secondWebhookNode = { + ...webhookNode, + name: 'Webhook 2', + id: '222f1db0-e7be-44c5-9ce9-3e35362490f1', +}; + +const executeWorkflowTriggerNode: INode = { + name: 'Execute Workflow Trigger', + type: 'n8n-nodes-base.executeWorkflowTrigger', + id: '78d63bca-bb6c-4568-948f-8ed9aacb1fe9', + parameters: {}, + typeVersion: 1, + position: [0, 0], +}; + +const respondToWebhookNode: INode = { + name: 'Respond to Webhook', + type: 'n8n-nodes-base.respondToWebhook', + id: '66d63bca-bb6c-4568-948f-8ed9aacb1fe9', + parameters: {}, + typeVersion: 1, + position: [0, 0], +}; + +const hackerNewsNode: INode = { + name: 'Hacker News', + type: 'n8n-nodes-base.hackerNews', + id: '55d63bca-bb6c-4568-948f-8ed9aacb1fe9', + parameters: {}, + typeVersion: 1, + position: [0, 0], +}; + +describe('WorkflowExecutionService', () => { + let workflowExecutionService: WorkflowExecutionService; + + beforeAll(() => { + workflowExecutionService = new WorkflowExecutionService( + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + ); + }); + + describe('selectPinnedActivatorStarter()', () => { + const workflow = mock({ + nodes: [], + }); + + const pinData = { + [webhookNode.name]: [{ json: { key: 'value' } }], + [executeWorkflowTriggerNode.name]: [{ json: { key: 'value' } }], + }; + + afterEach(() => { + workflow.nodes = []; + }); + + it('should return `null` if no pindata', () => { + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, []); + + expect(node).toBeNull(); + }); + + it('should return `null` if no starter nodes', () => { + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow); + + expect(node).toBeNull(); + }); + + it('should select webhook node if only choice', () => { + workflow.nodes.push(webhookNode); + + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData); + + expect(node).toEqual(webhookNode); + }); + + it('should return `null` if no choice', () => { + workflow.nodes.push(hackerNewsNode); + + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData); + + expect(node).toBeNull(); + }); + + it('should return ignore Respond to Webhook', () => { + workflow.nodes.push(respondToWebhookNode); + + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData); + + expect(node).toBeNull(); + }); + + it('should select execute workflow trigger if only choice', () => { + workflow.nodes.push(executeWorkflowTriggerNode); + + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData); + + expect(node).toEqual(executeWorkflowTriggerNode); + }); + + it('should favor webhook node over execute workflow trigger', () => { + workflow.nodes.push(webhookNode, executeWorkflowTriggerNode); + + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData); + + expect(node).toEqual(webhookNode); + }); + + it('should favor first webhook node over second webhook node', () => { + workflow.nodes.push(webhookNode, secondWebhookNode); + + const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData); + + expect(node).toEqual(webhookNode); + }); + }); +}); From f3fe46e4689b56240f78805102956d3b1fe3475e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 19 Jan 2024 08:43:29 +0100 Subject: [PATCH 2/2] Fix e2e hopefully --- packages/cli/src/workflows/workflowExecution.service.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/cli/src/workflows/workflowExecution.service.ts b/packages/cli/src/workflows/workflowExecution.service.ts index 30a2b5e1174ab..8ee41955a6200 100644 --- a/packages/cli/src/workflows/workflowExecution.service.ts +++ b/packages/cli/src/workflows/workflowExecution.service.ts @@ -258,6 +258,8 @@ export class WorkflowExecutionService { const allPinnedActivators = this.findAllPinnedActivators(workflow, pinData); + if (allPinnedActivators.length === 0) return null; + const [firstPinnedActivator] = allPinnedActivators; // full manual execution